# proxmox provider review: final report Branch `feat/proxmox-provider` at `7f2073f` (Phases 0 to 9), diffed against `main` (`2167819`): 123 files, 49 of them Go. The core is `internal/provider/proxmox/`; also in scope were the `nocloud` and `keyedlock` extraction from kvm2 (`0a6c862`), the config block in `internal/provider/config*.go`, registration and the deploy files. **Inputs.** [docs/proxmox-provider-spec.md](../../proxmox-provider-spec.md) (the spec the code implements) and [docs/provider-contract.md](../../provider-contract.md) (the MUST/SHOULD rules). A disagreement with either counts as a finding. **Reviewers.** Two independent passes, both read-only: 1. The `go-operator-reviewer` agent (`.claude/agents/go-operator-reviewer.md`), briefed with the spec and the contract, findings **P1 to P6** and simplifications **S1 to S3**. 2. `/code-review high` on the branch, findings **CR1 to CR10**. Where both passes found the same thing, the finding is listed once with both IDs. Every finding is **fixed** (with a regression test when it is code), **accepted** (real, not fixed here, documented as a known limit), or **rejected** (with the reason). **Toolchain.** `make test` green before and after the fixes. `make lint` panics inside staticcheck on the local Go 1.27 toolchain, as it did during kvm2, so lint ran without it, over the proxmox package and `internal/provider`, with and without the integration tag: ```bash bin/golangci-lint run --disable staticcheck --max-same-issues 0 --max-issues-per-linter 0 ./internal/provider/proxmox/... ./internal/provider/ bin/golangci-lint run --build-tags integration --disable staticcheck --max-same-issues 0 --max-issues-per-linter 0 ./internal/provider/proxmox/... ./internal/provider/ ``` Before the fixes both runs were at the Phase 8 and 9 baseline: logcheck 12, modernize (mapsloop) 2, unparam 3 in `fake_test.go`, gocyclo 1 (`TestAPITypes_decodeCaptures`), goconst on repeated strings, and the three goconst hits in `internal/provider`. After the fixes the counts are the same except goconst, one lower (224, 231 with the tag). No hit is new. The first version of the P2 test did add one: a third `"backup"` literal in the package's tests, then `"migrate"`. It now uses `"snapshot-delete"`. ## Summary | ID | Severity | Finding | Disposition | |---|---|---|---| | CR1 + P1 | medium-high | A seed delete that fails after the destroy leaks the seed for good, and the class of the error depends on the attempt | **fixed** | | CR2 | medium | A 403 on the seed delete latches a replaced Proxy whose VM is already gone | **fixed** (same change as CR1) | | CR3 + P2 | medium | The ownership check waits out any lock, even on a VM whose listed name is someone else's | **fixed** | | P3 | low-medium | Create adopts a leftover stopped VM whatever the current placement says | **accepted**, documented | | P4 | low | A crash mid-clone plus a freed earlier VMID can yield a second VM | **accepted**, documented | | P5 | low | 401/403 are Permanent, which bites during a token rotation | **fixed** in the guide | | P6 | low | Get and Delete trust the listing's `status`, which lags | **accepted**, confirmed on the host; error message fixed | | CR5 | low | `isLocked` and `isVMNotRunning` are called only from tests | **fixed** | | CR8 | low | `TestPermanentfAndTransientf` breaks the `Test_` rule | **fixed** | | CR9 | low | `get` repeats `mapState`'s "guest running" rule inline | **fixed** | | CR4 + S2 | low | The Checker repeats the cluster-wide `/version` and `/cluster/resources` per node | **rejected** | | CR6 | low | `lockName`, `permanentf` and `transientf` are copies of kvm2's | **rejected** | | CR7 | low | `readToken`, the CA load and seed staging do file I/O without a ctx | **rejected** | | CR10 | low | `New` runs `withDefaults` twice | **rejected** | | S1 | simplification | The `observe.go` decorator could be one log line in `send` | **rejected** | | S3 | simplification | `nocloud` exports `GuestAgentScript` and `SeedLabel` for tests only | **rejected** | ## Fixed ### CR1 + P1: the seed outlives a Delete that fails after the destroy `internal/provider/proxmox/delete.go`. Delete stopped the VM, destroyed it, then deleted the seed. If the seed delete failed (a 595, a task over its 1 minute budget, a 403), Delete returned an error, but the VM was already gone. The reconciler's next `Get` was `ErrNotFound`, so `reconcileDelete` released the finalizer without calling Delete again, and GC lists only VMs. The seed stayed on `isoStorage` for good. That breaks contract §2.3 ("MUST remove everything Create made ... GC sees only what ListByTag returns"). A destroy task that outlived its budget but finished in the background had the same effect. P1 adds the second half. On the finalizer path the next Delete found the VMID gone and took `deleteSeedEverywhere`, which logged the same 403 at V(1) and returned nil. The seed leaked with nothing visible. **Fix.** The seed now goes between the stop and the destroy, so the VM, which is what `Get` looks at, is the last thing removed: a failed seed delete leaves the VM stopped, `Get` keeps finding it, and the next Delete finishes both. `deleteSeedEverywhere` now fails on an `ErrPermanent` answer (a 401 or 403 every node would give) instead of skipping it. It still skips a node that is down or lacks the storage. `runTask`'s "gone" result lost its last reader and was dropped. That Proxmox deletes an ISO a stopped VM's `ide2` still references is the one assumption the fake cannot prove. From memory of the PVE source, not checked against it: the content delete runs `vdisk_free` without looking for references to `iso` volumes, and the destroy skips CD-ROM drives. Phase 0 only ever deleted the seed after the destroy. **The Phase 10 real-host run proved it** on 2026-09-24: the imgdel task on the stopped VM's seed ended `OK`, the destroy followed, and `TestProxmox_lifecycle` step 10 found neither the VM nor the seed. **Tests.** `TestDelete_seedDeleteFailureOnVMNodeFails` (403, 596 and a failed task on the VM's node) now asserts that no Destroy was sent, that `Get` still finds the VM, that a second Delete gives the same class, and that a Delete after the fault clears removes both. It failed on the old code with `Destroy calls = 1` and `Get = instance not found`. `TestDelete_seedDeleteNodeFailureSkippedWhenNodeUnknown/a refused privilege fails the call` covers the every-node 403. Updated for the new order: `TestDelete_stoppedVMSkipsStop`, `TestDelete_runningVMStoppedFirst`, `TestDelete_seedDeleteIsTaskAndIdempotent`, `TestDelete_destroyTaskFailureIsTransient` (the seed is now gone and the VM kept) and `TestDelete_goneMidwayIsSuccess` (a VM gone at the stop still gets a Destroy call, which answers `does not exist`). `TestLogging_noOpLinesAtV1`'s skip case now injects a node-level 500, since a 403 no longer skips. Docs: spec entry 11 (steps 5 to 7 reordered, the Permanent exception), the guide's Delete paragraph, `providers-existing.md`'s Delete bullet and its `delete-return` cell. ### CR2: a 403 on the seed delete latched a Proxy with no VM Same code, same fix. On the replace path (`reconcileManaged` sees a Stopped VM and calls Delete), the old order destroyed the VM, got a 403 on the seed, and returned Permanent. `providerFailure` latched it, so the Proxy sat Failed pointing at a VM that no longer existed. The next attempt would have returned nil. Now the 403 comes before the destroy: the VM stays stopped, and every attempt gives the same Permanent answer naming `Datastore.Allocate` until the privilege is granted. The guide says so. ### CR3 + P2: the lock wait on a VM that is plainly not ours `internal/provider/proxmox/create.go`, `allocate`. Any taken qemu VMID went through `inspect`, which first waits for the VM's lock to clear (clone budget, 10 minutes) and only then compares names. A VM on one of the Proxy's probe VMIDs, listed with another name and holding a long lock (a `vzdump` backup, or a stale lock a failed backup left behind), made every Create wait 10 minutes and return Transient. `vmidFor` is deterministic, so every retry landed on the same VMID. Only a clone in progress is nameless (F8), so a listed name other than ours settles the question. **Fix.** `allocate` sends a taken entry to `inspect` only when its listed name is empty or ours; any other name probes on without a config read. Spec entry 2's step 3 says so. **Tests.** `TestCreate_takenVMIDInspectedBeforeProbing` gains "foreign VM under a lock probes on without waiting" (a `snapshot-delete` lock with a 2 s clone budget; the old code spent the 2 s and failed Transient), and "foreign VM probes on without a read" now expects one config read (the template's). `TestCreate_vmidRangeExhaustedIsTransient` expects one VMConfig call instead of five. ### P5: token rotation order `docs/proxmox-provider.md`, cluster-side setup. A 401 is Permanent (the same convention as GCP), so a request made with a revoked token before the kubelet refreshes the Secret latches that Proxy. The guide now gives the order: add the new token, update the Secret, wait for the mount, then remove the old one, and the same before an `--expire` date. ### CR5: matchers used only by tests `isLocked` and `isVMNotRunning` in `errors.go` had no production caller. Both answers are Transient through `classify`'s default branch. They moved to `fake_test.go`, where `TestFakeAPI_errorsMatchCaptures`, `TestAPIError_matchers` and the fake's own checks still use them to hold the fake's wording to the captures. Spec entry 13's matching sentence and `providers-existing.md` now name only the matchers the provider uses. ### CR8: test name `TestPermanentfAndTransientf` became `TestPermanentf_wrapsClassWithMessage` and `TestTransientf_wrapsClassWithMessage`. kvm2 has the same name; it is out of scope here. ### CR9: one rule, one place `get` asked `status == "running" && qmpstatus in {running, ""}` inline to decide whether to query the agent, the same rule `mapState` applies. Both now call `guestRunning(status, qmpStatus)` in `state.go`, so a change to it cannot leave the two out of step. `TestMapState_*` and the Get tests cover it unchanged (`TestMapState_table`, `TestMapState_fromCaptures`). ## Accepted ### P3: adoption ignores a placement change `create.go:94-118`. A Create that fails Permanent after the clone (a 403 on the upload, say) leaves a stopped VM. If the user then changes `placement.image` or `zone`, the next Create adopts that VM by name, and the Proxy runs the old template on the old node. The fix the reviewer proposed (record the template in the mark, re-clone a never-started VM whose template differs) is a new mark field and a destroy path inside Create. That is too much for this MR for a two-step, user-driven sequence. Documented in the guide's Known gaps and spec entry 24: destroy the VM before changing `placement` on such a Proxy. ### P4: a second VM after a crash mid-clone The retry misses a nameless in-progress clone on a later probe VMID when the VM on the earlier VMID was removed meanwhile, and clones again. It takes a crash or cut-off during a clone of a few seconds, a foreign VM on the first probe VMID, and that VM's removal in the same window. The proposed fix (inspect every nameless `lock: clone` entry in the range first) would make every Create wait on every other Proxy's clone. The outcome is visible, not silent: the next Create fails Permanent naming both VMIDs. Documented in the guide and spec entry 24. ### P6: the listing's `status` may lag `get.go:51`, `delete.go:60`, `delete.go:90`. This is the reviewer's inference, not observed: `/cluster/resources` comes from pvestatd, which may lag a state change by seconds. Both cases heal on retry: `Get` answering Transient for `unknown` is polled through, and a destroy of a VM that started behind the listing's back fails its task as Transient and the next Delete stops it first. **Confirmed on pv1-1.** The first `Get` came 32 ms after the second Create, about 6 s after the clone, and was Transient: the listing still showed the VM as `status: unknown`. The next poll 5 s later saw it running. The behaviour is right, but the message blamed the node ("its node pv1-1 is not answering"), which was up. Get and Delete now say "its node … is not answering, or the VM is too new to be reported yet". `TestGet_unknownStatusIsTransient` asserts the wording, and spec entry 9 and the guide's error table name both causes. The Delete side (a stop skipped because the listing lags) was not met: the run's Delete came a minute after the start. ## Rejected - **CR4 + S2, Checker repeats cluster-wide calls per node.** Spec entry 16 makes each node's target self-contained, so a failure is reported against the node whose round saw it. The cost is two extra requests per extra node per round, and the configured cluster has one node. Worth revisiting with a multi-node config, not before. - **CR6, helpers copied from kvm2.** `permanentf`/`transientf` are one line each; `lockName` is five and wraps `keyedlock.Acquire`'s error in `provider.ErrTransient`. Moving that into `keyedlock` would couple a generic lock package to the provider sentinels, and putting the helpers in `internal/provider` would touch every kvm2 call site in a proxmox MR. - **CR7, file I/O without a ctx.** `readToken` and the CA load read a mounted Secret with `os.ReadFile`, which cannot be cancelled; a ctx parameter would be unused. kvm2's `os.ReadFile` of the SSH key is the precedent. Seed staging is `nocloud`'s, shared with kvm2 and unchanged by the extraction. - **CR10, `withDefaults` twice in `New`.** Startup only, idempotent, a few small maps. `newProvider` applies it because the tests build providers through it; defaulting once would need a second constructor shape. - **S1, replace the `observe.go` decorator with one log line in `send`.** The decorator is spec entries 17 and 20's `span-decorator` answer: each seam call gets a child span and a V(1)/V(2) line with node, VMID and UPID, which `send` does not know. It mirrors kvm2's decorator, and the reflection test guarantees every seam method is covered. Swapping the design in the review phase is out of proportion. - **S3, `nocloud` exports used only by tests.** `SeedLabel` is read by both kvm2's and proxmox's Create tests to recognise a real seed, and `GuestAgentScript` by proxmox's `TestBuildSeed_table`, which tests proxmox's own mapping of its config fields onto the vendor-data, not `nocloud`'s composition. The exports are the price of cross-package tests. ## Checked and fine From the reviewer agent's report, verified in code: - The kvm2 extraction is a pure move. kvm2 still wraps every seed-build error as Permanent, as before, and `keyedlock` is byte-identical to the code it replaced. - Delete stops or destroys only after the listing shows a qemu entry at the VMID and the config carries the name and the mark; a reused VMID or an unmarked VM is left alone. - A missing VM inside Create is Transient, never NotFound; locks and `status: unknown` are Transient, never NotFound. - Create resumes at every step: the VMID comes from the UID, a nameless locked entry is waited on and adopted, upload/config/resize are repeatable, and the mark is written in the clone call. - Secrets: `apiError` carries status and message only, the token never reaches a `url.Error`, the Checker keeps only the token id, and the cloud-init text appears only as `cloudInitBytes`. - Every seam call takes the ctx; `pollUntil` honours ctx and budget; `keyedlock.Acquire` gives up with ctx. - Spec entries 1, 3, 4, 9, 11, 12, 13, 15 and 16 match the code (P2 was the one disagreement, and the spec shared the flaw). - Every §11 test name in spec entry 21 exists. The agent also saw kvm2's `TestCheck_hostsCheckedConcurrently` fail once under `-race` (199.9 ms against its 200 ms floor). That file is unchanged on this branch; the flake is parked, as before.