diff --git a/api/v1alpha1/providerhealth_types.go b/api/v1alpha1/providerhealth_types.go index c12e058..87ce8ac 100644 --- a/api/v1alpha1/providerhealth_types.go +++ b/api/v1alpha1/providerhealth_types.go @@ -68,7 +68,7 @@ type ProviderTarget struct { // ProviderHealthStatus is the result of the latest check of one provider. type ProviderHealthStatus struct { // type is the provider's type from the providers config: kubernetes, - // gcp, kvm or kvm2. + // gcp, kvm, kvm2 or proxmox. // +optional Type string `json:"type,omitempty"` diff --git a/config/crd/bases/crawl.example.com_providerhealths.yaml b/config/crd/bases/crawl.example.com_providerhealths.yaml index 2f207cf..d3f8084 100644 --- a/config/crd/bases/crawl.example.com_providerhealths.yaml +++ b/config/crd/bases/crawl.example.com_providerhealths.yaml @@ -183,7 +183,7 @@ spec: type: description: |- type is the provider's type from the providers config: kubernetes, - gcp, kvm or kvm2. + gcp, kvm, kvm2 or proxmox. type: string type: object type: object diff --git a/docs/plans-executions/2026-09-18-1502-proxmox-provider.md b/docs/plans-executions/2026-09-18-1502-proxmox-provider.md index 2eac296..9f403a2 100644 --- a/docs/plans-executions/2026-09-18-1502-proxmox-provider.md +++ b/docs/plans-executions/2026-09-18-1502-proxmox-provider.md @@ -11,7 +11,7 @@ Pairs with [docs/plans/2026-09-18-1502-proxmox-provider.md](../plans/2026-09-18- - [x] Phase 4 — Seam, HTTP client, decorator, fake - [x] Phase 5 — Get, ListByTag, task helpers - [x] Phase 6 — Create -- [ ] Phase 7 — Delete, Checker, CRD comment +- [x] Phase 7 — Delete, Checker, CRD comment - [ ] Phase 8 — Contract and observability sweep, integration test, testing.md - [ ] Phase 9 — Deployment and documentation - [ ] Phase 10 — Review, real-host validation, changelog, MR @@ -789,3 +789,153 @@ Worth noting: would find the VM. This stays untested, like every cross-node path (entry 24). - Nothing in this phase talked to the host. + +## Phase 7 — Delete, Checker, CRD comment + +`delete.go` holds `Delete`: parse, the per-name lock, `destroy` (resolve, +the lock and status checks, config, ownership, stop, destroy) and the seed +delete. `check.go` holds `Check`, which fans out one `checkNode` per node +with `wg.Go` and returns the verdicts in config order, plus `checkReason` +and the four provider reasons. The Delete stub and +`TestStubs_answerTransient` are gone, and `grep -rn 'not implemented' +internal/provider/proxmox/` is empty. All four contract methods and +`Check` are real. The `status.type` comment in +`api/v1alpha1/providerhealth_types.go` now ends `kvm2 or proxmox`, and +`make manifests` changed only that description line in the ProviderHealth +CRD. + +The spec won over the plan as the Phase 7 notes said. An absent VM still +gets its seed deleted, on every configured node. A task waits on the seed +delete, and nothing matches F13's absent-volume message. A paused VM gets +the stop too. The Checker's reasons include `CertificateInvalid` and +`CredentialsUnavailable`, and pveproxy's 595 and 596 are `Unreachable`. + +Open questions decided (spec entries 11 and 16 updated): + +- **Our hibernated VM (`lock: suspended`) is Permanent in Delete:** "resume + or destroy it in Proxmox", the same answer Create's ownership check + gives. Proxmox's destroy refuses a locked VM (the fake answers with F7's + `VM is locked (…)` text; the suspended variant was never captured), and + that lock never clears on its own. Transient would retry forever. A + Permanent from Delete in `reconcileManaged`'s Stopped branch goes through + `providerFailure` and latches the Proxy with the admin action in its + status. On the finalizer path `deletionFailure` keeps retrying, and the + retries can be seen. Nothing captured shows what a hibernated VM does on + a stop or start through the API, so there was no evidence for a + self-healing path (for example, start to resume, then stop and destroy). + A hibernated VM that is not ours is left alone. The resources-entry lock + check skips `suspended`, so ownership is judged from the config, as in + Get. +- **The Checker reads `p.cfg.TokenFile` itself, through the client's + `readToken`**, before its first request, and keeps only the part before + `=`. A seam method would break the rule of one method per endpoint, and + both `TestClient_methodPathAndParams` and + `TestObserve_everySeamMethodLogsOneLine` enumerate the interface. An + unusable file answers `CredentialsUnavailable` without a request. + `readToken`'s errors never carry the content. Every row of + `TestCheck_reasonsTable` runs at V(2) over the decorator with a sentinel + secret and asserts that the secret is in no message, detail or log line. + +Other judgment calls: + +- **The seed is deleted on the VM's node only when Delete destroyed a VM + of ours; otherwise on every node.** Entry 11 said "skip to step 7" for + a VM that is foreign, unmarked, or gone by the time the config is read, + without naming a node. In each of those cases our VM's node is unknown: + the node in the listing belongs to someone else's VM, or to one that + just vanished. A container at the VMID takes the same path. +- **Stop's `does not exist` means the VM is gone**, the same as Destroy's. + The seed is then deleted on its node, and Destroy is not called. +- **`errCAFile` joins `errCredentials` in `client.go`.** A missing CA file + or one without a certificate is `CertificateInvalid` ("fix `caFile`"). + Without the sentinel it would be `APIError`. Its class is unchanged + (Transient or Permanent, as entry 14 says). +- **Any 500 from the storage status call is `StorageNotFound`**, carrying + Proxmox's text. The plan matched a 500 "does not exist", but no response + for a missing storage was captured. Matching an invented string would go + against the fixture discipline, and every 500 there (missing, not + enabled on that node, offline) is fixed on the storage anyway. A 401, + 403, 595 or 596 keeps its shared reason. +- **`checkReason` puts `*net.OpError` before the deadline check**, so a + dial that runs out of its own 10 s budget is `Unreachable`, not + `Timeout`. A request timing out on its own while the round still runs + is `Timeout`. The round's ctx ending is `Timeout` whatever the error + says, and that guard is in `checkNode`, as in kvm2. +- **The outcome line `proxmox vm deleted` carries `destroyed`**, which is + false when there was no VM of ours left to destroy. It has no `node` in + that case. Entry 20 lists the new V(1) no-op lines. +- `templates` counts catalogue entries, not unique VMIDs. + +Test names. These plan names changed with the behaviour: +`TestDelete_absentIsNoOp` → `TestDelete_absentStillDeletesSeedOnEveryNode`, +`TestDelete_foreignNameIsNoOp` → `TestDelete_foreignNameLeavesVMAlone`, +`TestDelete_unmarkedIsNoOp` → `TestDelete_unmarkedLeavesVMAlone`, and +`TestDelete_toleratesMissingISO` merged into entry 21's +`TestDelete_seedDeleteIsTaskAndIdempotent`, which runs with the seed both +present and absent and ties the absent case to +`task_status_volume_missing.json`. Every other plan and entry 21 name +exists. `TestDelete_usesPurgeAndDestroyUnreferencedDisks` runs Delete over +the real `httpAPI` against the Phase 0 destroy captures +(`cluster_resources_during_destroy`, `config_during_destroy`, +`destroy_response`, `content_delete_response`, +`task_status_volume_missing`). `TestGet_duringDeleteStillReturnsInstance` +holds the destroy task in its first poll and first checks that the +mid-destroy captures show the VM listed, stopped and without a lock. +Tests beyond both lists: `TestDelete_containerAtVMIDIsLeftAlone`, +`TestDelete_hibernatedVMIsPermanentIfOurs`, +`TestDelete_seedDeleteFailureOnVMNodeFails`, +`TestDelete_seedDeleteNodeFailureSkippedWhenNodeUnknown`, +`TestDelete_goneMidwayIsSuccess`, `TestDelete_resourcesFailureIsTransient`, +`TestDelete_waitsOnTheNameLock` (a held name lock means no call at all, +which is what makes a Delete racing a Create wait), +`TestDelete_cancelledMidWaitReturnsPromptly`, `TestDelete_outcomeLine` and +`TestCheck_reasonsOverHTTPClient`, which takes the transport reasons from +real net/http and crypto/tls errors: unknown CA, hostname, missing CA file, +a replayed 401, 596, a closed server and a cut response. The fake gained +`destroyFailsWith`. New test helpers are only the small ones in +`delete_test.go` (`putSeed`, `seedPresent`, `delete`/`deleteFails`, +`seedDeleteNodes` read from the decorator's lines, `twoNodeFixture`) and +`check_test.go` (`newCheckFixture`, `assertNoSecret`, `setStorage`). +`TestCheck_nodesCheckedConcurrently` uses a barrier, not a timer: every +node's `Version` waits until all have arrived, so checking the nodes one +at a time fails the verdicts deterministically and does not measure +elapsed time. That avoids the kind of flake kvm2's 200 ms test has. + +The mutation check applied 34 mutations: 17 in `delete.go`, 16 in +`check.go` and 1 in `client.go`. Every one compiled and every one failed at +least one test. Three did not compile on the first try (an unused `upid` +or `tokenID`) and were redone. The first run of "never stop" did fail +tests, but it also **hung** `TestDelete_concurrentSameNameSafe`, whose +wait for the first `Stop` call could not end. The wait is now bounded by +the first Delete returning, and the rerun fails fast. One reasons row +("dial cut short by the round's deadline") was added before the run, +because no existing row would have caught dropping `checkNode`'s ctx +guard. Every mutation was reverted by a script that compared each file's +hash with the original. + +```bash +go test -race -count=2 ./internal/provider/proxmox/ ./internal/providerhealth/ # ok +make manifests && git diff --stat config/ # crawl.example.com_providerhealths.yaml | 2 +- +grep -rn 'not implemented' internal/provider/proxmox/ # nothing +go build ./... && go vet ./... && go vet -tags=integration ./... +go test -short -count=1 ./... # all ok +go test -count=1 -v ./internal/provider/proxmox/ | grep -c '^=== RUN' # 519 before, 649 after +bin/golangci-lint run --disable staticcheck --max-same-issues 0 --max-issues-per-linter 0 ./internal/provider/proxmox/... +``` + +Worth noting: + +- Lint defaults hide repeats: with `--max-same-issues 0` the logcheck hits + are 4 in `create.go`, 4 in the new `delete.go`, and 2, 1 and 1 in + `get.go`, `seed.go` and `task.go`, all the ctx-plus-logger shape. + Nothing else is new except goconst on test strings. A prealloc hit in + `delete_test.go` was fixed. +- `TestCreate_concurrentSameNameSerialised` has the same unbounded + `for callCount("Clone") == 0` wait that hung the Delete test under a + mutation. It is left for the Phase 8 sweep, because this phase does not + touch Create. +- Never seen on the host, for Phase 10: what the storage status call + answers for a missing or disabled storage, what destroy says for a + hibernated VM, and a stop on a paused VM. The try-every-node seed delete + and the multi-node Checker have only met the fake (entry 24). +- Nothing in this phase talked to the host. diff --git a/docs/proxmox-provider-spec.md b/docs/proxmox-provider-spec.md index b697917..7c22a78 100644 --- a/docs/proxmox-provider-spec.md +++ b/docs/proxmox-provider-spec.md @@ -381,22 +381,35 @@ the seed ISO; the VM's disks are removed by the destroy call itself, with `purge=1&destroy-unreferenced-disks=1`. Under the per-name lock: 1. Parse the ID (`ErrPermanent` if malformed). -2. `/cluster/resources`. VMID absent: skip to step 7, trying every configured - node because the VM's node is no longer known. -3. `lock` set or `status: unknown`: `ErrTransient`. A clone or another task - still running finishes first; the reconciler calls Delete again after - `DeletionPoll`. -4. Read the config. `does not exist`: skip to step 7. A different name, or - no mark: the VM is not ours; log it and skip to step 7 without touching - it. -5. Status other than `stopped`: `POST …/status/stop`, wait (stop budget). - Stop works on a paused VM too. +2. `/cluster/resources`. VMID absent, or held by a container: skip to + step 7, trying every configured node because the VM's node is no longer + known. +3. `lock` set, other than `suspended`, or `status: unknown`: `ErrTransient`. + A clone or another task still running finishes first; the reconciler + calls Delete again after `DeletionPoll`. +4. Read the config. `does not exist`: skip to step 7 on every node. A + different name, or no mark: the VM is not ours; log it at V(1) and skip + to step 7 on every node without touching it. Our VM with a `lock` other + than `suspended` (taken since the listing): `ErrTransient`. Our VM with + `lock: suspended` (hibernated): `ErrPermanent` asking the admin to resume + or destroy it, the same answer Create's ownership check gives. The + destroy refuses a locked VM, and that lock never clears on its own, so + Transient would retry forever; Permanent puts the admin action in the + Proxy's status, where `reconcileManaged` latches it and the finalizer + path keeps retrying it visibly. A hibernated VM that is not ours is left + alone like any other. +5. Status (from the listing) other than `stopped`: `POST …/status/stop`, + wait (stop budget). Stop works on a paused VM too, which the listing + shows as `running`. `does not exist` means the VM is gone. 6. `DELETE /nodes/{node}/qemu/{vmid}?purge=1&destroy-unreferenced-disks=1`, wait (destroy budget). `does not exist` is success. 7. `DELETE /nodes/{node}/storage/{isoStorage}/content/:iso/-seed.iso`, - wait (seed-delete budget). A missing volume is still a successful task, - so no message is matched. On the try-every-node path a node whose - request fails is logged and skipped. + wait (seed-delete budget), on the VM's node when steps 5 and 6 ran on a + VM of ours, and on every configured node in config order otherwise. A + missing volume is still a successful task, so no message is matched. On + the every-node path a node whose request or task fails is logged at V(1) + and skipped, unless ctx ended, which fails the call; on the VM's node a + failure fails the call (a 403 for `Datastore.Allocate` is Permanent). 8. Return nil. Everything is gone when Delete returns nil, so `Get` afterwards is @@ -593,18 +606,31 @@ failed for a network reason, or pveproxy answered 595 or 596); `APIError` (any other failure). Provider-specific, each fixed differently by an admin: -- `CertificateInvalid`: TLS verification failed. Fix `url` or `caFile`, not - the host. +- `CertificateInvalid`: TLS verification failed, or `caFile` is missing or + holds no certificate. Fix `url` or `caFile`, not the host. - `CredentialsUnavailable`: the token file is missing or malformed. Fix the Secret, not Proxmox. - `StorageNotFound`: `isoStorage` is missing, inactive, or has no `iso` - content. Fix the storage on the node. + content. Fix the storage on the node. Any 500 from the storage status + call counts as missing, whatever its wording (it does not exist, is not + enabled on that node, or is offline; no missing-storage response was + captured, so no text is matched), and the message carries Proxmox's + text. A 401, 403, 595 or 596 there keeps its shared reason. - `TemplateNotFound`: a configured template VMID does not exist or is not a template. Fix the template or the catalogue. Details: `pveVersion`, `kernel`, `tokenID` (the part before `=`, never the secret), `isoStorage`, `isoStorageAvailGiB`, `templates`, filled as far as -the check got. Cost: four requests per node per round. The `status.type` +the check got; `isoStorage` is always there. `templates` counts catalogue +entries. The Checker reads `tokenFile` itself, before the first request, +with the client's `readToken`: the seam stays one method per endpoint, and +a missing or malformed file answers `CredentialsUnavailable` without a +request. Only the part before `=` is kept. + +`Timeout` also covers a request that runs out of its own timeout while the +round is still running. A dial or a connection cut mid-response is +`Unreachable`, even when the dial ran out of its own 10 s budget, because +the node did not answer. Cost: four requests per node per round. The `status.type` comment in `api/v1alpha1/providerhealth_types.go` gains `proxmox` and the CRD is regenerated. @@ -713,14 +739,16 @@ named `proxmox`, with `provider` on every line and `vmid`, `name` and `node` on every line about an instance. - V(1): one line per operation outcome (`proxmox vm ensured running`, - `proxmox vm fetched` with state, ip and uid, `proxmox vm deleted`, + `proxmox vm fetched` with state, ip and uid, `proxmox vm deleted` with + `destroyed` (false when no VM of ours was left to destroy), `proxmox vms listed` with counts, `proxmox node checked` with the reason), one `proxmox api call` line per seam call from the decorator (`call`, `node`, `vmid`, `storage`, `upid`, `duration`, plus `newid` and `name` on a clone, `filename` on an upload and `volid` on a volume delete, and on failure `httpStatus` and `error`), a `proxmox task failed` line with the UPID and - `exitstatus`, and the no-op lines of Delete and ListByTag (foreign VM - left alone, unreadable candidate skipped). + `exitstatus`, and the no-op lines of Delete and ListByTag (VM already + gone, foreign or unmarked VM left alone, seed delete skipped on a node, + unreadable candidate skipped). - V(2): request parameters (without `description`'s content at V(1) and never the upload body), config summaries, agent addresses, the resolved Create parameters with `cloudInitBytes`. diff --git a/internal/provider/proxmox/check.go b/internal/provider/proxmox/check.go new file mode 100644 index 0000000..a4b7836 --- /dev/null +++ b/internal/provider/proxmox/check.go @@ -0,0 +1,218 @@ +package proxmox + +import ( + "context" + "errors" + "fmt" + "io" + "maps" + "net" + "net/http" + "slices" + "strconv" + "strings" + "sync" + "time" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +var _ provider.Checker = (*Provider)(nil) + +// Reasons only this provider can tell apart (spec entry 16). Each names a +// failure an admin fixes somewhere other than where the shared reasons +// point. +const ( + // reasonCertificateInvalid: TLS verification failed or the CA file is + // unusable; fix url or caFile, not the host. + reasonCertificateInvalid = "CertificateInvalid" + // reasonCredentialsUnavailable: the token file is missing or malformed; + // fix the Secret, not Proxmox. + reasonCredentialsUnavailable = "CredentialsUnavailable" + // reasonStorageNotFound: isoStorage is missing, inactive, or holds no + // iso content on the node; fix the storage on the node. + reasonStorageNotFound = "StorageNotFound" + // reasonTemplateNotFound: a configured template VMID does not exist or + // is not a template; fix the template or the catalogue. + reasonTemplateNotFound = "TemplateNotFound" +) + +// pveproxy's own answers when it cannot reach the node behind it. +const ( + statusProxyConnect = 595 + statusProxyBroken = 596 +) + +// Check verifies every configured node concurrently and returns one verdict +// per node in config order. The requests share the provider's HTTP client, +// but each is its own request, so a round that runs out of time cancels +// only its own requests and never a reconcile's. +func (p *Provider) Check(ctx context.Context) []provider.TargetStatus { + out := make([]provider.TargetStatus, len(p.cfg.Nodes)) + var wg sync.WaitGroup + for i, node := range p.cfg.Nodes { + wg.Go(func() { out[i] = p.checkNode(ctx, node) }) + } + wg.Wait() + return out +} + +func (p *Provider) checkNode(ctx context.Context, node string) provider.TargetStatus { + start := time.Now() + log := p.logger(ctx).WithValues("node", node) + ts := provider.TargetStatus{ + Name: node, + Address: p.cfg.URL, + Details: map[string]string{"isoStorage": p.cfg.ISOStorage}, + } + err := p.probeNode(ctx, node, ts.Details) + ts.Latency = time.Since(start) + switch { + case err == nil: + ts.Ready = true + ts.Reason = provider.ReasonReachable + case ctx.Err() != nil: + // The round ran out of time, whichever request was running. + ts.Reason = provider.ReasonTimeout + ts.Message = err.Error() + default: + ts.Reason = checkReason(err) + ts.Message = err.Error() + } + log.V(1).Info("proxmox node checked", "ready", ts.Ready, "reason", ts.Reason, "latency", ts.Latency) + return ts +} + +// probeNode runs spec entry 16's four requests and fills details as facts +// arrive, so a failure late in the sequence still reports what came before. +func (p *Provider) probeNode(ctx context.Context, node string, details map[string]string) error { + // The token file is read here only for its id: the secret after "=" + // is dropped at once and never reaches details, logs or errors. + token, err := readToken(p.cfg.TokenFile) + if err != nil { + return err + } + tokenID, _, _ := strings.Cut(token, "=") + details["tokenID"] = tokenID + + v, err := p.api.Version(ctx) + if err != nil { + return fmt.Errorf("reading the API version: %w", err) + } + details["pveVersion"] = v.Version + ns, err := p.api.NodeStatus(ctx, node) + if err != nil { + return fmt.Errorf("reading the status of node %s: %w", node, err) + } + details["kernel"] = ns.CurrentKernel.Release + if err := p.checkStorage(ctx, node, details); err != nil { + return err + } + return p.checkTemplates(ctx, details) +} + +// checkStorage reads isoStorage on the node. A 500 means Proxmox cannot +// use that storage on this node (it does not exist, is not enabled here, +// or is offline), whatever its wording; the message carries Proxmox's +// text. +func (p *Provider) checkStorage(ctx context.Context, node string, details map[string]string) error { + storage := p.cfg.ISOStorage + st, err := p.api.StorageStatus(ctx, node, storage) + var ae *apiError + if errors.As(err, &ae) && ae.Status == http.StatusInternalServerError { + return &checkFailure{reason: reasonStorageNotFound, msg: fmt.Sprintf("storage %s on node %s: %s", storage, node, ae.Message)} + } + if err != nil { + return fmt.Errorf("reading storage %s on node %s: %w", storage, node, err) + } + details["isoStorageAvailGiB"] = formatGiB(st.Avail) + if st.Active == 0 { + return &checkFailure{reason: reasonStorageNotFound, msg: fmt.Sprintf("storage %s is not active on node %s", storage, node)} + } + if !slices.Contains(strings.Split(st.Content, ","), "iso") { + return &checkFailure{reason: reasonStorageNotFound, msg: fmt.Sprintf("storage %s on node %s does not hold iso content (content: %s)", storage, node, st.Content)} + } + return nil +} + +// checkTemplates looks every catalogue entry up in the cluster's resources: +// a template may sit on any node, since a clone can target another. +func (p *Provider) checkTemplates(ctx context.Context, details map[string]string) error { + rs, err := p.api.ClusterResources(ctx) + if err != nil { + return fmt.Errorf("listing cluster resources: %w", err) + } + isTemplate := make(map[int]bool) + for _, r := range rs { + if r.Type == "qemu" { + isTemplate[r.VMID] = r.Template == 1 + } + } + var problems []string + for _, key := range slices.Sorted(maps.Keys(p.cfg.Templates)) { + vmid := p.cfg.Templates[key].VMID + switch tmpl, found := isTemplate[vmid]; { + case !found: + problems = append(problems, fmt.Sprintf("template %q: VM %d does not exist", key, vmid)) + case !tmpl: + problems = append(problems, fmt.Sprintf("template %q: VM %d is not a template", key, vmid)) + } + } + details["templates"] = fmt.Sprintf("%d/%d", len(p.cfg.Templates)-len(problems), len(p.cfg.Templates)) + if len(problems) > 0 { + return &checkFailure{reason: reasonTemplateNotFound, msg: strings.Join(problems, "; ")} + } + return nil +} + +// checkFailure is a verdict the backend answered but the check refuses: the +// storage or a template is not usable. +type checkFailure struct { + reason string + msg string +} + +func (e *checkFailure) Error() string { return e.msg } + +// checkReason maps a failed probe to a reason once checkNode has ruled out +// the round's ctx ending. A failed connect or a cut connection is +// Unreachable; a request timeout with the round still running is Timeout. +func checkReason(err error) string { + var ( + cf *checkFailure + ae *apiError + op *net.OpError + ne net.Error + ) + switch { + case errors.As(err, &cf): + return cf.reason + case errors.Is(err, errCredentials): + return reasonCredentialsUnavailable + case errors.Is(err, errCAFile), isTLSVerification(err): + return reasonCertificateInvalid + case errors.As(err, &ae): + switch ae.Status { + case http.StatusUnauthorized: + return provider.ReasonUnauthorized + case http.StatusForbidden: + return provider.ReasonForbidden + case statusProxyConnect, statusProxyBroken: + return provider.ReasonUnreachable + } + return provider.ReasonAPIError + case errors.As(err, &op): + // Checked before the deadline: a dial that runs out of its own + // budget is a node that did not answer. + return provider.ReasonUnreachable + case errors.Is(err, context.DeadlineExceeded), errors.Is(err, context.Canceled): + return provider.ReasonTimeout + case errors.As(err, &ne), errors.Is(err, io.EOF), errors.Is(err, io.ErrUnexpectedEOF): + return provider.ReasonUnreachable + } + return provider.ReasonAPIError +} + +func formatGiB(bytes int64) string { + return strconv.FormatFloat(float64(bytes)/(1<<30), 'f', 1, 64) +} diff --git a/internal/provider/proxmox/check_test.go b/internal/provider/proxmox/check_test.go new file mode 100644 index 0000000..79b9609 --- /dev/null +++ b/internal/provider/proxmox/check_test.go @@ -0,0 +1,577 @@ +package proxmox + +import ( + "context" + "crypto/x509" + "errors" + "fmt" + "io" + "maps" + "net" + "net/http" + "net/url" + "path/filepath" + "slices" + "strings" + "sync" + "syscall" + "testing" + "time" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +// checkSecret is the secret half of the Checker's token file; it must never +// reach a verdict or a log line. +const checkSecret = "SENTINEL-token-secret-4c1d" + +// newCheckFixture is an observed create fixture at V(2) with a token file +// whose secret is checkSecret. +func newCheckFixture(t *testing.T) *fixture { + t.Helper() + fx := newCreateFixture(t, 2) + fx.p.api = observe(fx.fake, testProvider) + fx.p.cfg.TokenFile = filepath.Join(t.TempDir(), "token") + writeFile(t, fx.p.cfg.TokenFile, "proxy-operator@pve!operator="+checkSecret+"\n") + return fx +} + +// assertNoSecret checks every verdict and every log line for checkSecret. +func assertNoSecret(t *testing.T, fx *fixture, got []provider.TargetStatus) { + t.Helper() + for _, ts := range got { + if strings.Contains(ts.Message, checkSecret) || slices.ContainsFunc(slices.Collect(maps.Values(ts.Details)), func(v string) bool { return strings.Contains(v, checkSecret) }) { + t.Errorf("verdict carries the token secret: %+v", ts) + } + } + for _, l := range fx.logs.all() { + if strings.Contains(l, checkSecret) { + t.Errorf("log line carries the token secret: %s", l) + } + } +} + +func TestCheck_oneEntryPerNodeInConfigOrder(t *testing.T) { + t.Parallel() + fx := newCheckFixture(t) + fx.p.cfg.Nodes = []string{"pv1-3", fakeNode, "pv1-2"} + fx.fake.nodes["pv1-3"] = fx.fake.nodes[fakeNode] + got := fx.p.Check(fx.ctx) + if len(got) != len(fx.p.cfg.Nodes) { + t.Fatalf("Check() returned %d targets, want %d", len(got), len(fx.p.cfg.Nodes)) + } + for i, node := range fx.p.cfg.Nodes { + ts := got[i] + if ts.Name != node || ts.Address != fx.p.cfg.URL { + t.Errorf("target %d = %s (%s), want %s (%s)", i, ts.Name, ts.Address, node, fx.p.cfg.URL) + } + if wantReady := node != "pv1-2"; ts.Ready != wantReady { + t.Errorf("target %s ready = %v, want %v (%s: %s)", node, ts.Ready, wantReady, ts.Reason, ts.Message) + } + } + if got[2].Reason != provider.ReasonAPIError || !strings.Contains(got[2].Message, "pv1-2") { + t.Errorf("pv1-2 verdict = %s: %s, want APIError naming the node", got[2].Reason, got[2].Message) + } +} + +// TestCheck_nodesCheckedConcurrently holds every node's first request +// until all of them have arrived; checked one at a time, the first would +// give up and fail its verdict. +func TestCheck_nodesCheckedConcurrently(t *testing.T) { + t.Parallel() + fx := newCheckFixture(t) + fx.p.cfg.Nodes = []string{fakeNode, "pv1-2", "pv1-3"} + for _, n := range fx.p.cfg.Nodes[1:] { + fx.fake.nodes[n] = fx.fake.nodes[fakeNode] + } + var ( + mu sync.Mutex + arrived int + ) + all := make(chan struct{}) + fx.fake.hookOn("Version", func() error { + mu.Lock() + if arrived++; arrived == len(fx.p.cfg.Nodes) { + close(all) + } + mu.Unlock() + select { + case <-all: + return nil + case <-time.After(2 * time.Second): + return errors.New("barrier: the nodes were not checked concurrently") + } + }) + for _, ts := range fx.p.Check(fx.ctx) { + if !ts.Ready { + t.Errorf("target %s = %s: %s, want Ready", ts.Name, ts.Reason, ts.Message) + } + } +} + +func TestCheck_expiredContextReturnsPromptly(t *testing.T) { + t.Parallel() + fx := newCheckFixture(t) + fx.p.cfg.Nodes = []string{fakeNode, "pv1-2"} + fx.fake.blockOn("Version") + ctx, cancel := context.WithTimeout(fx.ctx, 0) + defer cancel() + start := time.Now() + got := fx.p.Check(ctx) + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("Check() took %v with an expired ctx, want under 1s", elapsed) + } + if len(got) != 2 { + t.Fatalf("Check() returned %d targets, want 2", len(got)) + } + for _, ts := range got { + if ts.Ready || ts.Reason != provider.ReasonTimeout || !strings.Contains(ts.Message, context.DeadlineExceeded.Error()) { + t.Errorf("target %s = ready %v reason %s (%s), want Timeout naming the deadline", ts.Name, ts.Ready, ts.Reason, ts.Message) + } + } +} + +func TestCheck_reasonsTable(t *testing.T) { + t.Parallel() + urlErr := func(err error) error { + return &url.Error{Op: "Get", URL: "https://192.0.2.8:8006/api2/json/version", Err: err} + } + tests := []struct { + name string + prep func(fx *fixture) + timeout time.Duration + cancel bool + + wantReady bool + wantReason string + wantMessage string + }{ + {name: "ok", wantReady: true, wantReason: provider.ReasonReachable}, + { + name: "token file missing", + prep: func(fx *fixture) { fx.p.cfg.TokenFile = filepath.Join(fx.t.TempDir(), "absent") }, + wantReason: reasonCredentialsUnavailable, + wantMessage: "does not exist yet", + }, + { + name: "token file malformed", + prep: func(fx *fixture) { writeFile(fx.t, fx.p.cfg.TokenFile, checkSecret+"\n") }, + wantReason: reasonCredentialsUnavailable, + wantMessage: "is not @!=", + }, + { + name: "token file gone by the time the client reads it", + prep: func(fx *fixture) { + fx.fake.failWith("NodeStatus", fmt.Errorf("%w: %w: /etc/proxy-operator/proxmox/token does not exist yet", provider.ErrTransient, errCredentials)) + }, + wantReason: reasonCredentialsUnavailable, + }, + { + name: "unknown certificate authority", + prep: func(fx *fixture) { fx.fake.failWith("Version", urlErr(x509.UnknownAuthorityError{})) }, + wantReason: reasonCertificateInvalid, + }, + { + name: "host not in the certificate", + prep: func(fx *fixture) { fx.fake.failWith("Version", urlErr(x509.HostnameError{Host: "192.168.2.8"})) }, + wantReason: reasonCertificateInvalid, + }, + { + name: "CA file unusable", + prep: func(fx *fixture) { + fx.fake.failWith("Version", fmt.Errorf("%w: %w: reading CA file: %w", provider.ErrTransient, errCAFile, syscall.ENOENT)) + }, + wantReason: reasonCertificateInvalid, + }, + { + name: "wrong secret", + prep: func(fx *fixture) { fx.fake.failWith("Version", loadAPIError(fx.t, "err_unauthorized")) }, + wantReason: provider.ReasonUnauthorized, + }, + { + name: "no credentials sent", + prep: func(fx *fixture) { fx.fake.failWith("Version", loadAPIError(fx.t, "err_no_credentials")) }, + wantReason: provider.ReasonUnauthorized, + }, + { + name: "privilege missing", + prep: func(fx *fixture) { fx.fake.failWith("NodeStatus", loadAPIError(fx.t, "err_forbidden")) }, + wantReason: provider.ReasonForbidden, + wantMessage: "Sys.Syslog", + }, + { + name: "pveproxy cannot reach the node", + prep: func(fx *fixture) { fx.fake.failWith("NodeStatus", fakeErr(595, "Connection refused")) }, + wantReason: provider.ReasonUnreachable, + }, + { + name: "pveproxy broken pipe", + prep: func(fx *fixture) { fx.fake.failWith("NodeStatus", fakeErr(596, "Broken pipe")) }, + wantReason: provider.ReasonUnreachable, + }, + { + name: "connection refused", + prep: func(fx *fixture) { + fx.fake.failWith("Version", urlErr(&net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED})) + }, + wantReason: provider.ReasonUnreachable, + }, + { + name: "node status 500", + prep: func(fx *fixture) { fx.fake.failWith("NodeStatus", fakeErr(500, "fake: pve-cluster not running")) }, + wantReason: provider.ReasonAPIError, + }, + { + name: "iso storage missing", + prep: func(fx *fixture) { + fx.fake.mu.Lock() + delete(fx.fake.storages, fakeISOStorage) + fx.fake.mu.Unlock() + }, + wantReason: reasonStorageNotFound, + wantMessage: "storage proxy-seeds on node pv1-1: fake: storage 'proxy-seeds' does not exist", + }, + { + name: "iso storage inactive", + prep: func(fx *fixture) { setStorage(fx, func(s *storageStatus) { s.Active = 0 }) }, + wantReason: reasonStorageNotFound, + wantMessage: "not active", + }, + { + name: "iso storage without iso content", + prep: func(fx *fixture) { setStorage(fx, func(s *storageStatus) { s.Content = "images,rootdir,isoish" }) }, + wantReason: reasonStorageNotFound, + wantMessage: "does not hold iso content (content: images,rootdir,isoish)", + }, + { + name: "iso storage forbidden", + prep: func(fx *fixture) { fx.fake.failWith("StorageStatus", loadAPIError(fx.t, "err_forbidden")) }, + wantReason: provider.ReasonForbidden, + }, + { + name: "template missing", + prep: func(fx *fixture) { + fx.fake.mu.Lock() + delete(fx.fake.vms, testTemplateVMID) + fx.fake.mu.Unlock() + }, + wantReason: reasonTemplateNotFound, + wantMessage: `template "debian-13": VM 9001 does not exist`, + }, + { + name: "template VMID holds a plain VM", + prep: func(fx *fixture) { fx.fake.vms[testTemplateVMID].template = false }, + wantReason: reasonTemplateNotFound, + wantMessage: `template "debian-13": VM 9001 is not a template`, + }, + { + name: "template VMID holds a container", + prep: func(fx *fixture) { fx.fake.vms[testTemplateVMID].kind = "lxc" }, + wantReason: reasonTemplateNotFound, + wantMessage: "does not exist", + }, + { + name: "resources listing fails", + prep: func(fx *fixture) { + fx.fake.failWith("ClusterResources", fakeErr(500, "fake: cluster filesystem not ready")) + }, + wantReason: provider.ReasonAPIError, + }, + { + name: "request outlives the deadline", + prep: func(fx *fixture) { fx.fake.blockOn("StorageStatus") }, + timeout: 50 * time.Millisecond, + wantReason: provider.ReasonTimeout, + }, + { + // The real client's dial sees the round's ctx end and fails as + // a dial error, which checkReason alone would call Unreachable. + name: "dial cut short by the round's deadline", + prep: func(fx *fixture) { + fx.fake.hookOn("Version", func() error { + time.Sleep(200 * time.Millisecond) + return urlErr(&net.OpError{Op: "dial", Net: "tcp", Err: context.DeadlineExceeded}) + }) + }, + timeout: 20 * time.Millisecond, + wantReason: provider.ReasonTimeout, + }, + { + name: "ctx cancelled mid check", + prep: func(fx *fixture) { fx.fake.blockOn("ClusterResources") }, + cancel: true, + wantReason: provider.ReasonTimeout, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCheckFixture(t) + if tc.prep != nil { + tc.prep(fx) + } + ctx := fx.ctx + switch { + case tc.timeout > 0: + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, tc.timeout) + defer cancel() + case tc.cancel: + var cancel context.CancelFunc + ctx, cancel = context.WithCancel(ctx) + time.AfterFunc(50*time.Millisecond, cancel) + } + got := fx.p.Check(ctx) + if len(got) != 1 { + t.Fatalf("Check() returned %d targets, want 1", len(got)) + } + ts := got[0] + if ts.Ready != tc.wantReady || ts.Reason != tc.wantReason { + t.Errorf("verdict = ready %v reason %s (%s), want ready %v reason %s", ts.Ready, ts.Reason, ts.Message, tc.wantReady, tc.wantReason) + } + if !tc.wantReady && ts.Message == "" { + t.Error("failed check has no message") + } + if tc.wantReady && ts.Message != "" { + t.Errorf("successful check has a message: %q", ts.Message) + } + if tc.wantMessage != "" && !strings.Contains(ts.Message, tc.wantMessage) { + t.Errorf("message = %q, want it to contain %q", ts.Message, tc.wantMessage) + } + if ts.Details["isoStorage"] != fakeISOStorage { + t.Errorf("details = %v, want isoStorage whatever failed", ts.Details) + } + assertNoSecret(t, fx, got) + }) + } +} + +func setStorage(fx *fixture, fn func(s *storageStatus)) { + fx.fake.mu.Lock() + defer fx.fake.mu.Unlock() + s := fx.fake.storages[fakeISOStorage] + fn(&s) + fx.fake.storages[fakeISOStorage] = s +} + +// TestCheck_reasonsOverHTTPClient takes the transport-level reasons from +// the real client, so the errors checkReason sees are the ones net/http and +// crypto/tls produce, and the Ready row replays the Phase 0 captures. +func TestCheck_reasonsOverHTTPClient(t *testing.T) { + t.Parallel() + captures := func(t *testing.T) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + path := strings.TrimPrefix(r.URL.Path, "/api2/json/") + name := map[string]string{ + "version": "version", + "nodes/pv1-1/status": "node_status", + "nodes/pv1-1/storage/local/status": "storage_status", + "cluster/resources": "cluster_resources", + }[path] + if name == "" { + t.Errorf("unexpected request %s", path) + http.NotFound(w, r) + return + } + serveJSON(w, readCapture(t, name).Body) + } + } + tests := []struct { + name string + handler func(t *testing.T) http.HandlerFunc + mutate func(t *testing.T, cfx *clientFixture) + wantReason string + }{ + {name: "captures", handler: captures, wantReason: provider.ReasonReachable}, + {name: "unknown authority", handler: captures, mutate: func(_ *testing.T, cfx *clientFixture) { cfx.cfg.CAFile = "" }, wantReason: reasonCertificateInvalid}, + { + name: "host not in the certificate", + handler: captures, + mutate: func(_ *testing.T, cfx *clientFixture) { + cfx.cfg.URL = strings.Replace(cfx.srv.URL, "127.0.0.1", "localhost", 1) + }, + wantReason: reasonCertificateInvalid, + }, + { + name: "CA file missing", + handler: captures, + mutate: func(t *testing.T, cfx *clientFixture) { cfx.cfg.CAFile = filepath.Join(t.TempDir(), "ca.pem") }, + wantReason: reasonCertificateInvalid, + }, + { + name: "401 from pveproxy", + handler: func(t *testing.T) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { serveCapture(t, w, readCapture(t, "err_unauthorized")) } + }, + wantReason: provider.ReasonUnauthorized, + }, + { + name: "596 from pveproxy", + handler: func(t *testing.T) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + serveCapture(t, w, capture{Status: 596, Reason: "Broken pipe"}) + } + }, + wantReason: provider.ReasonUnreachable, + }, + { + name: "server gone", + handler: captures, + mutate: func(_ *testing.T, cfx *clientFixture) { cfx.srv.Close() }, + wantReason: provider.ReasonUnreachable, + }, + { + name: "connection cut mid response", + handler: func(t *testing.T) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + conn, _, err := w.(http.Hijacker).Hijack() + if err != nil { + t.Errorf("hijack: %v", err) + return + } + _, _ = io.WriteString(conn, "HTTP/1.1 200 OK\r\nContent-Length: 100\r\n\r\n{\"data\":") + _ = conn.Close() + } + }, + wantReason: provider.ReasonUnreachable, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + cfx := newClientFixture(t, tc.handler(t)) + if tc.mutate != nil { + tc.mutate(t, cfx) + } + cfg := testConfig() + cfg.ISOStorage = "local" + cfg.URL, cfg.TokenFile, cfg.CAFile = cfx.cfg.URL, cfx.cfg.TokenFile, cfx.cfg.CAFile + p := newProvider(testProvider, cfg, cfx.api(t)) + got := p.Check(context.Background()) + if len(got) != 1 { + t.Fatalf("Check() returned %d targets, want 1", len(got)) + } + if ts := got[0]; ts.Reason != tc.wantReason || ts.Ready != (tc.wantReason == provider.ReasonReachable) { + t.Errorf("verdict = ready %v reason %s (%s), want %s", ts.Ready, ts.Reason, ts.Message, tc.wantReason) + } + }) + } +} + +func TestCheck_detailsKeys(t *testing.T) { + t.Parallel() + full := map[string]string{ + "tokenID": "proxy-operator@pve!operator", "isoStorage": fakeISOStorage, "pveVersion": "9.2.20", + "kernel": "7.0.14-17-pve", "isoStorageAvailGiB": "90.0", "templates": "1/1", + } + tests := []struct { + name string + prep func(fx *fixture) + want []string + }{ + {name: "success", want: slices.Sorted(maps.Keys(full))}, + {name: "token unusable", prep: func(fx *fixture) { writeFile(fx.t, fx.p.cfg.TokenFile, "junk") }, want: []string{"isoStorage"}}, + {name: "node status fails", prep: func(fx *fixture) { fx.fake.failWith("NodeStatus", fakeErr(596, "Broken pipe")) }, want: []string{"isoStorage", "pveVersion", "tokenID"}}, + {name: "storage inactive", prep: func(fx *fixture) { setStorage(fx, func(s *storageStatus) { s.Active = 0 }) }, want: []string{"isoStorage", "isoStorageAvailGiB", "kernel", "pveVersion", "tokenID"}}, + {name: "template missing", prep: func(fx *fixture) { fx.fake.vms[testTemplateVMID].template = false }, want: slices.Sorted(maps.Keys(full))}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCheckFixture(t) + if tc.prep != nil { + tc.prep(fx) + } + got := fx.p.Check(fx.ctx)[0] + if keys := slices.Sorted(maps.Keys(got.Details)); !slices.Equal(keys, tc.want) { + t.Errorf("details keys = %v, want %v", keys, tc.want) + } + for k, v := range got.Details { + if want, ok := full[k]; ok && tc.name == "success" && v != want { + t.Errorf("details[%s] = %q, want %q", k, v, want) + } + if v == "" { + t.Errorf("details[%s] is empty", k) + } + } + }) + } + t.Run("templates counts found over configured", func(t *testing.T) { + t.Parallel() + fx := newCheckFixture(t) + fx.p.cfg.Templates["ubuntu"] = provider.ProxmoxTemplate{VMID: 9002} + fx.p.cfg.Templates["debian-again"] = provider.ProxmoxTemplate{VMID: testTemplateVMID} + got := fx.p.Check(fx.ctx)[0] + if got.Details["templates"] != "2/3" || got.Reason != reasonTemplateNotFound || !strings.Contains(got.Message, `template "ubuntu": VM 9002`) { + t.Errorf("verdict = %s %q, details %v; want TemplateNotFound naming ubuntu, templates 2/3", got.Reason, got.Message, got.Details) + } + }) +} + +func TestCheck_logsVerdictPerNode(t *testing.T) { + t.Parallel() + fx := newCheckFixture(t) + fx.p.cfg.Nodes = []string{fakeNode, "pv1-2"} + fx.p.Check(fx.ctx) + checked := fx.logs.matching(`"msg"="proxmox node checked"`) + if len(checked) != 2 { + t.Fatalf("got %d verdict lines, want 2: %v", len(checked), checked) + } + for _, l := range checked { + switch { + case strings.Contains(l, `"node"="pv1-1"`): + if !strings.Contains(l, `"ready"=true`) || !strings.Contains(l, `"reason"="Reachable"`) || !strings.Contains(l, `"latency"=`) { + t.Errorf("pv1-1 verdict line = %s", l) + } + case strings.Contains(l, `"node"="pv1-2"`): + if !strings.Contains(l, `"ready"=false`) || !strings.Contains(l, `"reason"="APIError"`) { + t.Errorf("pv1-2 verdict line = %s", l) + } + default: + t.Errorf("verdict line without a node: %s", l) + } + if !strings.Contains(l, `"provider"="`+testProvider+`"`) || !strings.Contains(l, `"level"=1`) { + t.Errorf("verdict line lacks the provider or is not V(1): %s", l) + } + } +} + +func TestCheckReason_table(t *testing.T) { + t.Parallel() + dial := &url.Error{Op: "Get", URL: "https://192.0.2.8:8006/api2/json/version", Err: &net.OpError{Op: "dial", Net: "tcp", Err: context.DeadlineExceeded}} + tests := []struct { + name string + err error + want string + }{ + {name: "storage verdict", err: &checkFailure{reason: reasonStorageNotFound, msg: "x"}, want: reasonStorageNotFound}, + {name: "template verdict wrapped", err: fmt.Errorf("check: %w", &checkFailure{reason: reasonTemplateNotFound, msg: "x"}), want: reasonTemplateNotFound}, + {name: "token missing", err: fmt.Errorf("%w: %w: /t does not exist yet", provider.ErrTransient, errCredentials), want: reasonCredentialsUnavailable}, + {name: "token malformed", err: fmt.Errorf("%w: %w: /t is not a token", provider.ErrPermanent, errCredentials), want: reasonCredentialsUnavailable}, + {name: "CA file", err: fmt.Errorf("%w: %w: /ca holds no PEM certificate", provider.ErrPermanent, errCAFile), want: reasonCertificateInvalid}, + {name: "unknown authority", err: &url.Error{Err: x509.UnknownAuthorityError{}}, want: reasonCertificateInvalid}, + {name: "certificate invalid", err: &url.Error{Err: x509.CertificateInvalidError{Reason: x509.Expired}}, want: reasonCertificateInvalid}, + {name: "401", err: loadAPIError(t, "err_unauthorized"), want: provider.ReasonUnauthorized}, + {name: "403 wrapped", err: fmt.Errorf("reading: %w", loadAPIError(t, "err_forbidden_volume_delete")), want: provider.ReasonForbidden}, + {name: "595", err: fakeErr(595, "Connection refused"), want: provider.ReasonUnreachable}, + {name: "596", err: fakeErr(596, "Broken pipe"), want: provider.ReasonUnreachable}, + {name: "400", err: loadAPIError(t, "err_bad_param"), want: provider.ReasonAPIError}, + {name: "500", err: loadAPIError(t, "err_vm_missing"), want: provider.ReasonAPIError}, + {name: "dial ran out of its own budget", err: dial, want: provider.ReasonUnreachable}, + {name: "connection reset", err: &url.Error{Err: &net.OpError{Op: "read", Net: "tcp", Err: syscall.ECONNRESET}}, want: provider.ReasonUnreachable}, + {name: "request timeout", err: fmt.Errorf("reading: %w", &url.Error{Err: context.DeadlineExceeded}), want: provider.ReasonTimeout}, + {name: "cancelled", err: context.Canceled, want: provider.ReasonTimeout}, + {name: "server closed mid response", err: &url.Error{Err: io.ErrUnexpectedEOF}, want: provider.ReasonUnreachable}, + {name: "eof", err: io.EOF, want: provider.ReasonUnreachable}, + {name: "decode failure", err: errors.New("decoding GET version: invalid character '<'"), want: provider.ReasonAPIError}, + {name: "nil", err: nil, want: provider.ReasonAPIError}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := checkReason(tc.err); got != tc.want { + t.Errorf("checkReason(%v) = %s, want %s", tc.err, got, tc.want) + } + }) + } +} diff --git a/internal/provider/proxmox/client.go b/internal/provider/proxmox/client.go index c1df283..940ac2c 100644 --- a/internal/provider/proxmox/client.go +++ b/internal/provider/proxmox/client.go @@ -39,6 +39,10 @@ const ( // Checker can report CredentialsUnavailable instead of an API failure. var errCredentials = errors.New("proxmox token file unusable") +// errCAFile marks a CA file that is missing or holds no certificate, so the +// Checker can report CertificateInvalid: the admin fixes caFile. +var errCAFile = errors.New("proxmox CA file unusable") + // tokenPattern is @!= (spec entry 14). var tokenPattern = regexp.MustCompile(`^[^@\s]+@[^!\s]+![^=\s]+=\S+$`) @@ -91,11 +95,11 @@ func (a *httpAPI) httpClient() (*http.Client, error) { if a.caFile != "" && !a.insecure { pem, err := os.ReadFile(a.caFile) if err != nil { - return nil, fmt.Errorf("%w: reading CA file: %w", provider.ErrTransient, err) + return nil, fmt.Errorf("%w: %w: reading CA file: %w", provider.ErrTransient, errCAFile, err) } pool := x509.NewCertPool() if !pool.AppendCertsFromPEM(pem) { - return nil, permanentf("CA file %s holds no PEM certificate", a.caFile) + return nil, fmt.Errorf("%w: %w: %s holds no PEM certificate", provider.ErrPermanent, errCAFile, a.caFile) } tlsCfg.RootCAs = pool } diff --git a/internal/provider/proxmox/delete.go b/internal/provider/proxmox/delete.go new file mode 100644 index 0000000..309324e --- /dev/null +++ b/internal/provider/proxmox/delete.go @@ -0,0 +1,149 @@ +package proxmox + +import ( + "context" + "fmt" + "time" + + "github.com/go-logr/logr" +) + +// Delete is spec entry 11: under the per-name lock, stop and destroy the VM +// when it is ours, then delete its seed. Everything already gone is +// success, and nothing of ours is left when nil is returned, so Get +// afterwards is NotFound. +func (p *Provider) Delete(ctx context.Context, id string) error { + vmid, name, err := parseProviderID(id) + if err != nil { + return p.wrapErr("delete", id, err) + } + log := p.logger(ctx).WithValues("vmid", vmid, "name", name) + unlock, err := p.lockName(ctx, name) + if err != nil { + return p.wrapErr("delete", id, err) + } + defer unlock() + + node, err := p.destroy(ctx, log, vmid, name) + if err != nil { + return p.wrapErr("delete", id, err) + } + if node == "" { + err = p.deleteSeedEverywhere(ctx, log, name) + } else { + log = log.WithValues("node", node) + err = p.deleteSeed(ctx, log, node, name) + } + if err != nil { + return p.wrapErr("delete", id, err) + } + log.V(1).Info("proxmox vm deleted", "destroyed", node != "") + return nil +} + +// destroy is steps 2 to 6. It returns the node of the VM it destroyed, or +// "" when there was no VM of ours to destroy: then the seed's node is not +// known, and the seed is looked for on every node. +func (p *Provider) destroy(ctx context.Context, log logr.Logger, vmid int, name string) (string, error) { + r, ok, err := p.resolve(ctx, vmid) + if err != nil { + return "", err + } + if !ok { + log.V(1).Info("proxmox vm already gone") + return "", nil + } + log = log.WithValues("node", r.Node) + if r.Lock != "" && r.Lock != lockSuspended { + return "", transientf("VM %d is locked (%s)", vmid, r.Lock) + } + if r.Status == "unknown" { + return "", transientf("VM %d has status unknown: its node %s is not answering", vmid, r.Node) + } + cfg, err := p.api.VMConfig(ctx, r.Node, vmid) + if isNotExist(err) { + log.V(1).Info("proxmox vm already gone") + return "", nil + } + if err != nil { + return "", fmt.Errorf("reading config of VM %d: %w", vmid, err) + } + if cfg["name"] != name { + log.V(1).Info("proxmox vm at this vmid is not ours, left alone", "vmName", cfg["name"]) + return "", nil + } + if !parseDescription(cfg["description"]).Managed { + log.V(1).Info("proxmox vm carries no proxy-operator mark, left alone") + return "", nil + } + switch lock := cfg["lock"]; lock { + case "": + case lockSuspended: + // Destroy refuses a locked VM and the lock never clears on its own, + // so only an admin can move this on, as in Create's ownership check. + return "", permanentf("VM %d named %s is hibernated (lock suspended); resume or destroy it in Proxmox", vmid, name) + default: + return "", transientf("VM %d is locked (%s)", vmid, lock) + } + + // The listing shows a paused VM as running, and Stop ends it too. + if r.Status != "stopped" { + gone, err := p.runTask(ctx, log, "stopping", vmid, p.budgets.stop, func() (string, error) { return p.api.Stop(ctx, r.Node, vmid) }) + if err != nil || gone { + return r.Node, err + } + } + if _, err := p.runTask(ctx, log, "destroying", vmid, p.budgets.destroy, func() (string, error) { return p.api.Destroy(ctx, r.Node, vmid) }); err != nil { + return "", err + } + return r.Node, nil +} + +// runTask submits a task on a VM and waits for it. A VM that is gone by the +// time the task is submitted reports gone: it is what the task was for. +func (p *Provider) runTask(ctx context.Context, log logr.Logger, what string, vmid int, budget time.Duration, submit func() (string, error)) (bool, error) { + upid, err := submit() + if isNotExist(err) { + log.V(1).Info("proxmox vm already gone") + return true, nil + } + if err != nil { + return false, fmt.Errorf("%s VM %d: %w", what, vmid, err) + } + if err := p.waitTask(ctx, log, upid, budget); err != nil { + return false, fmt.Errorf("%s VM %d: %w", what, vmid, err) + } + return false, nil +} + +// deleteSeed is step 7 on one node. A missing volume still ends in a +// successful task (F13), so nothing is matched. +func (p *Provider) deleteSeed(ctx context.Context, log logr.Logger, node, name string) error { + volid := seedVolID(p.cfg.ISOStorage, name) + upid, err := p.api.DeleteVolume(ctx, node, p.cfg.ISOStorage, volid) + if err != nil { + return fmt.Errorf("deleting seed %s on %s: %w", volid, node, err) + } + if err := p.waitTask(ctx, log, upid, p.budgets.seedDelete); err != nil { + return fmt.Errorf("deleting seed %s on %s: %w", volid, node, err) + } + return nil +} + +// deleteSeedEverywhere is step 7 when the VM's node is not known: every +// configured node is tried, and one that fails is logged and skipped, so a +// node that is down or lacks the storage cannot hold the Delete up. An +// ended ctx fails the call instead. +func (p *Provider) deleteSeedEverywhere(ctx context.Context, log logr.Logger, name string) error { + for _, node := range p.cfg.Nodes { + err := p.deleteSeed(ctx, log, node, name) + if err == nil { + continue + } + if ctx.Err() != nil { + return err + } + log.V(1).Info("proxmox seed delete failed on a node, skipped", "node", node, "reason", err.Error()) + } + return nil +} diff --git a/internal/provider/proxmox/delete_test.go b/internal/provider/proxmox/delete_test.go new file mode 100644 index 0000000..bd621c4 --- /dev/null +++ b/internal/provider/proxmox/delete_test.go @@ -0,0 +1,717 @@ +package proxmox + +import ( + "context" + "errors" + "net/http" + "slices" + "strconv" + "strings" + "sync" + "testing" + "time" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +// putSeed uploads testName's seed to the fake's ISO storage, as Create +// leaves it. +func (fx *fixture) putSeed() { + fx.t.Helper() + upid, err := fx.fake.UploadISO(context.Background(), fakeNode, fakeISOStorage, seedISOName(testName), strings.NewReader("seed"), 4) + if err != nil { + fx.t.Fatalf("UploadISO error = %v", err) + } + pollTask(fx.t, fx.fake, upid) +} + +func (fx *fixture) seedPresent() bool { + _, ok := fx.fake.iso(fakeISOStorage, seedISOName(testName)) + return ok +} + +// deleteFails runs Delete on fx.id(), checks the error's class and returns +// its text. +func (fx *fixture) deleteFails(class error) string { + fx.t.Helper() + err := fx.p.Delete(fx.ctx, fx.id()) + assertProviderError(fx.t, err, "delete", class) + return err.Error() +} + +func (fx *fixture) delete() { + fx.t.Helper() + if err := fx.p.Delete(fx.ctx, fx.id()); err != nil { + fx.t.Fatalf("Delete error = %v", err) + } +} + +// seedDeleteNodes returns the node of every DeleteVolume call, read from +// the decorator's lines; the fixture's api must be observed. +func (fx *fixture) seedDeleteNodes() []string { + lines := fx.logs.matching(`"call"="DeleteVolume"`) + nodes := make([]string, 0, len(lines)) + for _, l := range lines { + _, rest, _ := strings.Cut(l, `"node"="`) + node, _, _ := strings.Cut(rest, `"`) + nodes = append(nodes, node) + } + return nodes +} + +// twoNodeFixture is an observed fixture at V(1) with nodes pv1-1 and pv1-2. +func twoNodeFixture(t *testing.T) *fixture { + t.Helper() + fx := newFixture(t, 1) + fx.p.api = observe(fx.fake, testProvider) + fx.p.cfg.Nodes = []string{fakeNode, "pv1-2"} + return fx +} + +func TestDelete_malformedIDIsPermanent(t *testing.T) { + t.Parallel() + for _, id := range []string{"", "vms/150042", "vms/0150042/" + testName, "vm/150042/" + testName, "vms/abc/" + testName, "vms/150042/a/b", "vms/99/" + testName} { + t.Run(id, func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + assertProviderError(t, fx.p.Delete(fx.ctx, id), "delete", provider.ErrPermanent) + if calls := fx.fake.callLog(); len(calls) != 0 { + t.Errorf("calls = %v, want none", calls) + } + }) + } +} + +func TestDelete_twiceSucceeds(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "running") + fx.putSeed() + fx.delete() + fx.delete() + if _, ok := fx.fake.vmSnapshot(testVMID); ok { + t.Error("VM still exists") + } + if fx.seedPresent() { + t.Error("seed still exists") + } + for method, want := range map[string]int{"Stop": 1, "Destroy": 1, "DeleteVolume": 2} { + if got := fx.fake.callCount(method); got != want { + t.Errorf("%s calls = %d, want %d", method, got, want) + } + } +} + +// TestDelete_absentStillDeletesSeedOnEveryNode replaces the plan's +// TestDelete_absentIsNoOp: spec entry 11 deletes the seed even when the VM +// is gone, on every node, because the VM's node is no longer known. +func TestDelete_absentStillDeletesSeedOnEveryNode(t *testing.T) { + t.Parallel() + fx := twoNodeFixture(t) + fx.putSeed() + fx.delete() + if got := fx.seedDeleteNodes(); !slices.Equal(got, []string{fakeNode, "pv1-2"}) { + t.Errorf("seed deleted on %v, want every node in config order", got) + } + if fx.seedPresent() { + t.Error("seed still exists") + } + for _, method := range []string{"VMConfig", "Stop", "Destroy"} { + if n := fx.fake.callCount(method); n != 0 { + t.Errorf("%s calls = %d, want 0 for an absent VM", method, n) + } + } + if lines := fx.logs.matching(`"msg"="proxmox vm already gone"`); len(lines) != 1 { + t.Errorf("already-gone lines = %v, want one", lines) + } +} + +func TestDelete_containerAtVMIDIsLeftAlone(t *testing.T) { + t.Parallel() + fx := twoNodeFixture(t) + fx.addOurVM(testVMID, testName, "running").kind = "lxc" + fx.delete() + if vm, ok := fx.fake.vmSnapshot(testVMID); !ok || vm.status != "running" { + t.Errorf("container = %+v, %v; want it untouched", vm, ok) + } + if got := fx.seedDeleteNodes(); len(got) != 2 { + t.Errorf("seed deleted on %v, want every node", got) + } +} + +// TestDelete_foreignNameLeavesVMAlone replaces the plan's +// TestDelete_foreignNameIsNoOp: the VM is not touched, but our seed is +// still deleted, on every node since ours is gone. +func TestDelete_foreignNameLeavesVMAlone(t *testing.T) { + t.Parallel() + fx := twoNodeFixture(t) + fx.addOurVM(testVMID, "proxy-someoneelse00000", "running") + fx.putSeed() + fx.delete() + if vm, ok := fx.fake.vmSnapshot(testVMID); !ok || vm.status != "running" { + t.Errorf("foreign VM = %+v, %v; want it running and untouched", vm, ok) + } + for _, method := range []string{"Stop", "Destroy", "SetVMConfig"} { + if n := fx.fake.callCount(method); n != 0 { + t.Errorf("%s calls = %d, want 0", method, n) + } + } + if got := fx.seedDeleteNodes(); !slices.Equal(got, []string{fakeNode, "pv1-2"}) { + t.Errorf("seed deleted on %v, want every node", got) + } + lines := fx.logs.matching(`"msg"="proxmox vm at this vmid is not ours, left alone"`) + if len(lines) != 1 || !strings.Contains(lines[0], `"vmName"="proxy-someoneelse00000"`) { + t.Errorf("no-op lines = %v, want one naming the other VM", lines) + } +} + +// TestDelete_unmarkedLeavesVMAlone replaces the plan's +// TestDelete_unmarkedIsNoOp, for the same reason. +func TestDelete_unmarkedLeavesVMAlone(t *testing.T) { + t.Parallel() + for name, desc := range map[string]string{ + "no description": "", + "managed false": "proxy-operator-managed=false\nproxy-operator-uid=" + testUID, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + fx := twoNodeFixture(t) + vm := fx.fake.addVM(testVMID, testName, "stopped") + if desc != "" { + vm.config["description"] = desc + } + fx.delete() + if _, ok := fx.fake.vmSnapshot(testVMID); !ok { + t.Error("unmarked VM was destroyed") + } + if n := fx.fake.callCount("Destroy"); n != 0 { + t.Errorf("Destroy calls = %d, want 0", n) + } + if got := fx.seedDeleteNodes(); len(got) != 2 { + t.Errorf("seed deleted on %v, want every node", got) + } + if lines := fx.logs.matching(`"msg"="proxmox vm carries no proxy-operator mark, left alone"`); len(lines) != 1 { + t.Errorf("no-op lines = %v, want one", lines) + } + }) + } +} + +// TestDelete_runningVMStoppedFirst covers a paused VM too: the listing +// shows it as running (F14), and Stop ends it. The fake refuses to destroy +// a running VM, and each task takes two polls, so a Destroy that does not +// wait for the stop fails. +func TestDelete_runningVMStoppedFirst(t *testing.T) { + t.Parallel() + for _, qmp := range []string{"running", "paused", "prelaunch"} { + t.Run(qmp, func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.fake.taskPolls = 2 + fx.addOurVM(testVMID, testName, "running").qmpstatus = qmp + fx.delete() + log := fx.fake.callLog() + stop, destroy := slices.Index(log, "Stop"), slices.Index(log, "Destroy") + if stop < 0 || destroy < stop { + t.Fatalf("calls = %v, want Stop before Destroy", log) + } + if n := callsBefore(log[stop:], "TaskStatus", "Destroy"); n != fx.fake.taskPolls+1 { + t.Errorf("stop task polled %d times before Destroy, want %d", n, fx.fake.taskPolls+1) + } + if _, ok := fx.fake.vmSnapshot(testVMID); ok { + t.Error("VM still exists") + } + }) + } +} + +func TestDelete_stoppedVMSkipsStop(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "stopped") + fx.delete() + want := []string{"ClusterResources", "VMConfig", "Destroy", "TaskStatus", "DeleteVolume", "TaskStatus"} + if got := fx.fake.callLog(); !slices.Equal(got, want) { + t.Errorf("calls = %v, want %v", got, want) + } +} + +func TestDelete_lockedIsTransient(t *testing.T) { + t.Parallel() + for _, lock := range []string{"backup", "migrate", "snapshot", "rollback", "create"} { + t.Run("resources lock "+lock, func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "running").lock = lock + fx.putSeed() + if msg := fx.deleteFails(provider.ErrTransient); !strings.Contains(msg, "locked ("+lock+")") { + t.Errorf("error = %s, want it to name the lock", msg) + } + if got := fx.fake.callLog(); !slices.Equal(got, []string{"UploadISO", "TaskStatus", "ClusterResources"}) { + t.Errorf("calls = %v, want only the listing after the seed setup", got) + } + if !fx.seedPresent() { + t.Error("seed deleted while the VM is locked") + } + }) + } + t.Run("mid-clone nameless entry", func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.fake.addTemplate(9001, "debian-13-template") + if _, err := fx.fake.Clone(fx.ctx, fakeNode, 9001, cloneParams{NewID: testVMID, Name: testName, Description: formatDescription(testUID, testCreatedAt)}); err != nil { + t.Fatalf("Clone error = %v", err) + } + fx.deleteFails(provider.ErrTransient) + if _, ok := fx.fake.vmSnapshot(testVMID); !ok { + t.Error("mid-clone VM is gone") + } + }) + t.Run("lock taken between the resources and config reads", func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + vm := fx.addOurVM(testVMID, testName, "running") + mutateOn(fx.fake, "VMConfig", func() { vm.lock = "backup" }) + fx.deleteFails(provider.ErrTransient) + if n := fx.fake.callCount("Stop"); n != 0 { + t.Errorf("Stop calls = %d, want 0", n) + } + }) +} + +func TestDelete_unknownStatusIsTransient(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "running").status = "unknown" + if msg := fx.deleteFails(provider.ErrTransient); !strings.Contains(msg, fakeNode) { + t.Errorf("error = %s, want it to name the node", msg) + } + if n := fx.fake.callCount("VMConfig"); n != 0 { + t.Errorf("VMConfig calls = %d, want 0 while the node is unknown", n) + } +} + +// TestDelete_hibernatedVMIsPermanentIfOurs matches Create's ownership +// check: Destroy refuses the suspended lock, which never clears on its own, +// so an admin must act. A hibernated VM that is not ours is left alone. +func TestDelete_hibernatedVMIsPermanentIfOurs(t *testing.T) { + t.Parallel() + t.Run("ours", func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "stopped").lock = lockSuspended + fx.putSeed() + if msg := fx.deleteFails(provider.ErrPermanent); !strings.Contains(msg, "resume or destroy it in Proxmox") { + t.Errorf("error = %s, want the admin action", msg) + } + for _, method := range []string{"Stop", "Destroy", "DeleteVolume"} { + if n := fx.fake.callCount(method); n != 0 { + t.Errorf("%s calls = %d, want 0", method, n) + } + } + if _, err := fx.fake.Destroy(fx.ctx, fakeNode, testVMID); !isLocked(err) { + t.Errorf("fake Destroy on the hibernated VM error = %v, want the lock answer", err) + } + }) + t.Run("someone else's", func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, "proxy-someoneelse00000", "stopped").lock = lockSuspended + fx.delete() + if vm, ok := fx.fake.vmSnapshot(testVMID); !ok || vm.lock != lockSuspended { + t.Errorf("foreign hibernated VM = %+v, %v; want it untouched", vm, ok) + } + }) +} + +func TestDelete_removesSeedISO(t *testing.T) { + t.Parallel() + fx := twoNodeFixture(t) + fx.addOurVM(testVMID, testName, "stopped").node = "pv1-2" + fx.putSeed() + fx.delete() + if fx.seedPresent() { + t.Error("seed still exists") + } + if got := fx.seedDeleteNodes(); !slices.Equal(got, []string{"pv1-2"}) { + t.Errorf("seed deleted on %v, want only the VM's node pv1-2", got) + } + lines := fx.logs.matching(`"call"="DeleteVolume"`) + if len(lines) != 1 || !strings.Contains(lines[0], `"volid"="`+seedVolID(fakeISOStorage, testName)+`"`) || !strings.Contains(lines[0], `"storage"="`+fakeISOStorage+`"`) { + t.Errorf("DeleteVolume lines = %v, want the seed's volid on the ISO storage", lines) + } +} + +// TestDelete_seedDeleteIsTaskAndIdempotent replaces the plan's +// TestDelete_toleratesMissingISO: Proxmox ends a delete of a missing volume +// with a successful imgdel task (F13), so nothing is matched, but the task +// is still waited on. +func TestDelete_seedDeleteIsTaskAndIdempotent(t *testing.T) { + t.Parallel() + missing := decodeData[taskStatus](t, "task_status_volume_missing") + if missing.Type != "imgdel" || missing.ExitStatus != "OK" { + t.Fatalf("capture task_status_volume_missing = %+v, want an imgdel task ending OK", missing) + } + for _, withSeed := range []bool{true, false} { + t.Run("seed present "+strconv.FormatBool(withSeed), func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "stopped") + if withSeed { + fx.putSeed() + } + fx.fake.taskPolls = 2 + fx.delete() + log := fx.fake.callLog() + last := len(log) - 1 + for log[last] != "DeleteVolume" { + last-- + } + if n := callsBefore(log[last:], "TaskStatus", ""); n != fx.fake.taskPolls+1 { + t.Errorf("seed delete task polled %d times, want %d: %v", n, fx.fake.taskPolls+1, log) + } + if fx.seedPresent() { + t.Error("seed still exists") + } + }) + } +} + +func TestDelete_seedDeleteFailureOnVMNodeFails(t *testing.T) { + t.Parallel() + tests := []struct { + name string + setup func(fx *fixture) + class error + }{ + {name: "token lacks Datastore.Allocate", setup: func(fx *fixture) { + fx.fake.failWith("DeleteVolume", loadAPIError(t, "err_forbidden_volume_delete")) + }, class: provider.ErrPermanent}, + {name: "pveproxy 596", setup: func(fx *fixture) { fx.fake.failWith("DeleteVolume", fakeErr(596, "Broken pipe")) }, class: provider.ErrTransient}, + {name: "task failed", setup: func(fx *fixture) { + fx.fake.hookOn("TaskStatus", func() error { + if fx.fake.callCount("DeleteVolume") > 0 { + return fakeErr(500, "fake: task log unreadable") + } + return nil + }) + }, class: provider.ErrTransient}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "stopped") + tc.setup(fx) + fx.deleteFails(tc.class) + if _, ok := fx.fake.vmSnapshot(testVMID); ok { + t.Error("VM still exists: the destroy should have run before the seed delete") + } + }) + } +} + +func TestDelete_seedDeleteNodeFailureSkippedWhenNodeUnknown(t *testing.T) { + t.Parallel() + t.Run("one node refuses", func(t *testing.T) { + t.Parallel() + fx := twoNodeFixture(t) + fx.putSeed() + var calls int + fx.fake.hookOn("DeleteVolume", func() error { + if calls++; calls == 1 { + return fakeErr(500, "fake: storage 'proxy-seeds' is not available on node 'pv1-1'") + } + return nil + }) + fx.delete() + if got := fx.seedDeleteNodes(); !slices.Equal(got, []string{fakeNode, "pv1-2"}) { + t.Errorf("seed deleted on %v, want both nodes tried", got) + } + lines := fx.logs.matching(`"msg"="proxmox seed delete failed on a node, skipped"`) + if len(lines) != 1 || !strings.Contains(lines[0], `"node"="pv1-1"`) || !strings.Contains(lines[0], "not available") { + t.Errorf("skip lines = %v, want one naming pv1-1 and the reason", lines) + } + if fx.seedPresent() { + t.Error("seed still exists after pv1-2's delete") + } + }) + t.Run("ended ctx fails the call", func(t *testing.T) { + t.Parallel() + fx := twoNodeFixture(t) + ctx, cancel := context.WithCancel(fx.ctx) + defer cancel() + fx.fake.hookOn("DeleteVolume", func() error { + cancel() + return context.Canceled + }) + err := fx.p.Delete(ctx, fx.id()) + assertProviderError(t, err, "delete", provider.ErrTransient) + if n := fx.fake.callCount("DeleteVolume"); n != 1 { + t.Errorf("DeleteVolume calls = %d, want 1: the ended ctx stops the loop", n) + } + }) +} + +func TestDelete_destroyTaskFailureIsTransient(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "stopped") + fx.putSeed() + fx.fake.destroyFailsWith = "fake: unable to remove disk" + if msg := fx.deleteFails(provider.ErrTransient); !strings.Contains(msg, "unable to remove disk") { + t.Errorf("error = %s, want the task's exitstatus", msg) + } + if _, ok := fx.fake.vmSnapshot(testVMID); !ok { + t.Error("VM gone after a failed destroy task") + } + if !fx.seedPresent() { + t.Error("seed deleted although the VM is still there") + } + if lines := fx.logs.matching(`"msg"="proxmox task failed"`); len(lines) != 1 { + t.Errorf("task failed lines = %v, want one", lines) + } +} + +// TestDelete_goneMidwayIsSuccess: the VM vanishing at any step counts as +// done, and F2's does-not-exist answer is what each step then sees. +func TestDelete_goneMidwayIsSuccess(t *testing.T) { + t.Parallel() + tests := []struct { + name string + status string + method string + wantDestroy int + wantNodes []string + }{ + {name: "before the config read", status: "stopped", method: "VMConfig", wantNodes: []string{fakeNode, "pv1-2"}}, + {name: "before the stop", status: "running", method: "Stop", wantNodes: []string{fakeNode}}, + {name: "before the destroy", status: "stopped", method: "Destroy", wantDestroy: 1, wantNodes: []string{fakeNode}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := twoNodeFixture(t) + fx.addOurVM(testVMID, testName, tc.status) + mutateOn(fx.fake, tc.method, func() { delete(fx.fake.vms, testVMID) }) + fx.delete() + if n := fx.fake.callCount("Destroy"); n != tc.wantDestroy { + t.Errorf("Destroy calls = %d, want %d", n, tc.wantDestroy) + } + if got := fx.seedDeleteNodes(); !slices.Equal(got, tc.wantNodes) { + t.Errorf("seed deleted on %v, want %v", got, tc.wantNodes) + } + }) + } +} + +func TestDelete_resourcesFailureIsTransient(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "running") + fx.fake.failWith("ClusterResources", fakeErr(595, "Connection refused")) + fx.deleteFails(provider.ErrTransient) + if n := fx.fake.callCount("DeleteVolume"); n != 0 { + t.Errorf("DeleteVolume calls = %d, want 0 when the VM's state is unknown", n) + } +} + +// TestDelete_usesPurgeAndDestroyUnreferencedDisks runs Delete over the real +// client against Phase 0's destroy captures: the DELETE carries both flags +// and the seed delete sends the volid as one escaped segment. +func TestDelete_usesPurgeAndDestroyUnreferencedDisks(t *testing.T) { + t.Parallel() + const ( + vmid = 199999 + name = "proxy-reconaaaaaaaaaaa" + ) + var ( + mu sync.Mutex + seen []string + ) + cfx := newClientFixture(t, func(w http.ResponseWriter, r *http.Request) { + path := strings.TrimPrefix(r.URL.EscapedPath(), "/api2/json/") + mu.Lock() + seen = append(seen, r.Method+" "+path+"?"+r.URL.RawQuery) + mu.Unlock() + capture := "" + switch { + case r.Method == http.MethodGet && path == "cluster/resources": + capture = "cluster_resources_during_destroy" + case r.Method == http.MethodGet && path == "nodes/pv1-1/qemu/199999/config": + capture = "config_during_destroy" + case r.Method == http.MethodDelete && path == "nodes/pv1-1/qemu/199999": + capture = "destroy_response" + case r.Method == http.MethodDelete && path == "nodes/pv1-1/storage/proxy-seeds/content/proxy-seeds:iso%2F"+name+"-seed.iso": + capture = "content_delete_response" + case r.Method == http.MethodGet && strings.Contains(path, ":imgdel:"): + capture = "task_status_volume_missing" + case r.Method == http.MethodGet && strings.Contains(path, ":qmdestroy:"): + capture = "task_status_stopped_ok" + default: + t.Errorf("unexpected request %s %s", r.Method, path) + http.Error(w, "unexpected", http.StatusNotImplemented) + return + } + serveJSON(w, readCapture(t, capture).Body) + }) + cfg := testConfig() + cfg.URL, cfg.TokenFile, cfg.CAFile = cfx.cfg.URL, cfx.cfg.TokenFile, cfx.cfg.CAFile + p := newProvider(testProvider, cfg, cfx.api(t)) + p.budgets = testBudgets() + if err := p.Delete(context.Background(), formatProviderID(vmid, name)); err != nil { + t.Fatalf("Delete error = %v", err) + } + mu.Lock() + defer mu.Unlock() + if !slices.Contains(seen, "DELETE nodes/pv1-1/qemu/199999?destroy-unreferenced-disks=1&purge=1") { + t.Errorf("requests = %v, want the destroy with purge=1 and destroy-unreferenced-disks=1", seen) + } + if !slices.ContainsFunc(seen, func(s string) bool { return strings.HasPrefix(s, "DELETE nodes/pv1-1/storage/proxy-seeds/content/") }) { + t.Errorf("requests = %v, want the seed delete", seen) + } + if slices.ContainsFunc(seen, func(s string) bool { return strings.Contains(s, "status/stop") }) { + t.Errorf("requests = %v, want no stop for the captured stopped VM", seen) + } +} + +func TestDelete_concurrentSameNameSafe(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "running") + release := fx.fake.blockOn("Stop") + errs := make([]error, 2) + first := make(chan struct{}) + var wg sync.WaitGroup + wg.Go(func() { + defer close(first) + errs[0] = fx.p.Delete(fx.ctx, fx.id()) + }) + for fx.fake.callCount("Stop") == 0 { + select { + case <-first: + t.Fatalf("first Delete returned %v without stopping the VM", errs[0]) + case <-time.After(time.Millisecond): + } + } + wg.Go(func() { errs[1] = fx.p.Delete(fx.ctx, fx.id()) }) + close(release) + wg.Wait() + for i, err := range errs { + if err != nil { + t.Errorf("Delete %d error = %v", i, err) + } + } + for method, want := range map[string]int{"Stop": 1, "Destroy": 1} { + if got := fx.fake.callCount(method); got != want { + t.Errorf("%s calls = %d, want %d", method, got, want) + } + } +} + +// TestDelete_waitsOnTheNameLock: Delete takes the per-name lock before any +// call, so it cannot interleave with a Create or another Delete of that +// name (spec entry 19). +func TestDelete_waitsOnTheNameLock(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "running") + release, err := fx.p.lockName(fx.ctx, testName) + if err != nil { + t.Fatalf("lockName error = %v", err) + } + defer release() + ctx, cancel := context.WithTimeout(fx.ctx, 20*time.Millisecond) + defer cancel() + err = fx.p.Delete(ctx, fx.id()) + assertProviderError(t, err, "delete", provider.ErrTransient) + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("error = %v, want it to wrap DeadlineExceeded", err) + } + if calls := fx.fake.callLog(); len(calls) != 0 { + t.Errorf("calls = %v, want none while the name is locked", calls) + } +} + +func TestDelete_cancelledMidWaitReturnsPromptly(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.p.budgets = defaultBudgets() + fx.p.budgets.poll = time.Millisecond + fx.fake.taskPolls = 1 << 30 + fx.addOurVM(testVMID, testName, "running") + ctx, cancel := context.WithCancel(fx.ctx) + defer cancel() + fx.fake.hookOn("TaskStatus", func() error { + cancel() + return nil + }) + start := time.Now() + err := fx.p.Delete(ctx, fx.id()) + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Errorf("Delete took %v after cancellation", elapsed) + } + assertProviderError(t, err, "delete", provider.ErrTransient) + if !errors.Is(err, context.Canceled) { + t.Errorf("error = %v, want it to wrap context.Canceled", err) + } +} + +func TestDelete_outcomeLine(t *testing.T) { + t.Parallel() + tests := []struct { + name string + setup func(fx *fixture) + want []string + never string + }{ + { + name: "destroyed", + setup: func(fx *fixture) { fx.addOurVM(testVMID, testName, "stopped") }, + want: []string{`"node"="` + fakeNode + `"`, `"destroyed"=true`}, + }, + {name: "already gone", setup: func(*fixture) {}, want: []string{`"destroyed"=false`}, never: `"node"=`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + tc.setup(fx) + fx.delete() + lines := fx.logs.matching(`"msg"="proxmox vm deleted"`) + if len(lines) != 1 { + t.Fatalf("outcome lines = %d, want 1:\n%s", len(lines), strings.Join(fx.logs.all(), "\n")) + } + for _, want := range append(tc.want, `"vmid"=150042`, `"name"="`+testName+`"`, `"provider"="`+testProvider+`"`) { + if !strings.Contains(lines[0], want) { + t.Errorf("outcome line lacks %s: %s", want, lines[0]) + } + } + if tc.never != "" && strings.Contains(lines[0], tc.never) { + t.Errorf("outcome line has %s: %s", tc.never, lines[0]) + } + }) + } +} + +func TestDelete_noOpLinesAreSilentAtV0(t *testing.T) { + t.Parallel() + tests := map[string]func(fx *fixture){ + "absent": func(*fixture) {}, + "foreign": func(fx *fixture) { fx.addOurVM(testVMID, "proxy-someoneelse00000", "running") }, + "unmarked": func(fx *fixture) { fx.fake.addVM(testVMID, testName, "running") }, + "ours": func(fx *fixture) { fx.addOurVM(testVMID, testName, "running") }, + } + for name, setup := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 0) + fx.p.api = observe(fx.fake, testProvider) + setup(fx) + fx.delete() + if lines := fx.logs.all(); len(lines) != 0 { + t.Errorf("V(0) lines = %v, want none", lines) + } + }) + } +} diff --git a/internal/provider/proxmox/fake_test.go b/internal/provider/proxmox/fake_test.go index 52206d0..594f569 100644 --- a/internal/provider/proxmox/fake_test.go +++ b/internal/provider/proxmox/fake_test.go @@ -39,9 +39,10 @@ type fakeAPI struct { // taskPolls is how many TaskStatus polls answer "running" before a // task stops. taskPolls int - // cloneFailsWith and startFailsWith, when set, are the exitstatus of - // every clone or start task. A failed clone removes its VM. - cloneFailsWith, startFailsWith string + // cloneFailsWith, startFailsWith and destroyFailsWith, when set, are + // the exitstatus of every clone, start or destroy task. A failed clone + // removes its VM; a failed destroy keeps it. + cloneFailsWith, startFailsWith, destroyFailsWith string // agentNotRunning makes the agent of every running VM with agent=1 // answer "QEMU guest agent is not running". agentNotRunning bool @@ -510,8 +511,12 @@ func (f *fakeAPI) Destroy(ctx context.Context, node string, vmid int) (string, e if vm.status == "running" { return "", fakeErr(500, fmt.Sprintf("fake: VM %d is running - destroy failed", vmid)) } - return f.newTask(node, "qmdestroy", strconv.Itoa(vmid), "OK", func() { - if f.vms[vmid] == vm { + exit := "OK" + if f.destroyFailsWith != "" { + exit = f.destroyFailsWith + } + return f.newTask(node, "qmdestroy", strconv.Itoa(vmid), exit, func() { + if exit == "OK" && f.vms[vmid] == vm { delete(f.vms, vmid) } }), nil diff --git a/internal/provider/proxmox/get_test.go b/internal/provider/proxmox/get_test.go index ca7de3b..39d672d 100644 --- a/internal/provider/proxmox/get_test.go +++ b/internal/provider/proxmox/get_test.go @@ -4,6 +4,7 @@ import ( "errors" "slices" "strings" + "sync" "testing" "time" @@ -474,3 +475,60 @@ func TestGet_resourcesFailureIsTransient(t *testing.T) { }) } } + +func TestGet_afterDeleteReturnsNotFound(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "running").ifaces = capturedIfaces(t) + if inst := fx.get(fx.id()); inst.State != provider.StateRunning { + t.Fatalf("Get before Delete = %+v, want Running", inst) + } + if err := fx.p.Delete(fx.ctx, fx.id()); err != nil { + t.Fatalf("Delete error = %v", err) + } + assertGetFails(t, fx, provider.ErrNotFound) +} + +// TestGet_duringDeleteStillReturnsInstance holds the destroy task running: +// destroying sets no lock and keeps the VM listed until the task ends, as +// Phase 0's mid-destroy captures show, so Get sees it Stopped, not gone. +func TestGet_duringDeleteStillReturnsInstance(t *testing.T) { + t.Parallel() + if _, locked := capturedConfig(t, "config_during_destroy")["lock"]; locked { + t.Fatal("capture config_during_destroy has a lock; the fake models none") + } + listed := decodeData[[]vmResource](t, "cluster_resources_during_destroy") + if !slices.ContainsFunc(listed, func(r vmResource) bool { return r.VMID == 199999 && r.Status == "stopped" && r.Lock == "" }) { + t.Fatal("capture cluster_resources_during_destroy does not list VM 199999 stopped and unlocked") + } + + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, "running") + inTask, release := make(chan struct{}), make(chan struct{}) + var once sync.Once + fx.fake.hookOn("TaskStatus", func() error { + if fx.fake.callCount("Destroy") > 0 { + once.Do(func() { + close(inTask) + <-release + }) + } + return nil + }) + done := make(chan error, 1) + go func() { done <- fx.p.Delete(fx.ctx, fx.id()) }() + select { + case <-inTask: + case err := <-done: + t.Fatalf("Delete returned %v before its destroy task was polled", err) + } + inst := fx.get(fx.id()) + if inst.State != provider.StateStopped || inst.UID != testUID || inst.ID != fx.id() { + t.Errorf("Get during the destroy = %+v, want the instance Stopped with its uid", inst) + } + close(release) + if err := <-done; err != nil { + t.Fatalf("Delete error = %v", err) + } + assertGetFails(t, fx, provider.ErrNotFound) +} diff --git a/internal/provider/proxmox/proxmox.go b/internal/provider/proxmox/proxmox.go index 5843857..97c6d58 100644 --- a/internal/provider/proxmox/proxmox.go +++ b/internal/provider/proxmox/proxmox.go @@ -172,9 +172,3 @@ func (p *Provider) lockName(ctx context.Context, name string) (func(), error) { } return release, nil } - -// Delete is a stub until Phase 7. It answers Transient, so a Proxy on this -// provider retries rather than latching. -func (p *Provider) Delete(_ context.Context, id string) error { - return p.wrapErr("delete", id, transientf("proxmox Delete is not implemented yet")) -} diff --git a/internal/provider/proxmox/proxmox_test.go b/internal/provider/proxmox/proxmox_test.go index 502ff9d..dd47efd 100644 --- a/internal/provider/proxmox/proxmox_test.go +++ b/internal/provider/proxmox/proxmox_test.go @@ -215,16 +215,6 @@ func TestWrapErr_carriesProviderNameOpAndID(t *testing.T) { } } -// TestStubs_answerTransient keeps a Proxy on this provider retrying, not -// latched, until Delete is implemented in Phase 7. -func TestStubs_answerTransient(t *testing.T) { - t.Parallel() - p := newProvider("proxmox-home", testConfig(), nil) - if err := p.Delete(context.Background(), "vms/150042/proxy-x"); !errors.Is(err, provider.ErrTransient) { - t.Errorf("delete error = %v, want ErrTransient", err) - } -} - func TestLockName_givesUpWithContextAsTransient(t *testing.T) { t.Parallel() p := newProvider("proxmox-home", testConfig(), nil)