egress-proxies-operator/docs/reviews/2026-09-23-proxmox-provider-review/REPORT.md

# 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<Function>_<scenario>` 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.