proxmox: Get, ListByTag and task polling
Get follows spec entry 9's check order (lock and unknown status before the name, suspended is Stopped after the name and mark checks), reads status/current so paused is Stopped, and maps the three agent messages of entry 7. ListByTag reads one config per candidate and fails whole only on the resources call or an ended ctx. waitTask polls the UPID's node every budgets.poll, logs failed tasks at V(1) and counts "WARNINGS: <n>" as success; waitUnlocked serves Create's ownership check. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -9,7 +9,7 @@ Pairs with [docs/plans/2026-09-18-1502-proxmox-provider.md](../plans/2026-09-18-
|
||||
- [x] Phase 2 — Config block, registration, package skeleton
|
||||
- [x] Phase 3 — Extract `nocloud` and `keyedlock` from kvm2
|
||||
- [x] Phase 4 — Seam, HTTP client, decorator, fake
|
||||
- [ ] Phase 5 — Get, ListByTag, task helpers
|
||||
- [x] Phase 5 — Get, ListByTag, task helpers
|
||||
- [ ] Phase 6 — Create
|
||||
- [ ] Phase 7 — Delete, Checker, CRD comment
|
||||
- [ ] Phase 8 — Contract and observability sweep, integration test, testing.md
|
||||
@@ -554,3 +554,106 @@ Worth noting:
|
||||
`fake: VM <vmid> is running - destroy failed`, so Phase 7's
|
||||
stop-before-destroy order is enforced by the fake even though the real
|
||||
text is unknown.
|
||||
|
||||
## Phase 5 — Get, ListByTag, task helpers
|
||||
|
||||
`task.go` holds `parseUPID`, a shared `pollUntil` loop, `waitTask` and
|
||||
`waitUnlocked`. `get.go` holds `Get`, the guest-agent read and `resolve`
|
||||
(one resources call, QEMU entries only, which Delete will reuse). `list.go`
|
||||
holds `ListByTag` and its candidate rule. The Get and ListByTag stubs are
|
||||
gone, and `TestStubs_answerTransient` now covers only Create and Delete.
|
||||
|
||||
The spec overrides the plan in the places the Phase 4 notes listed. Get
|
||||
checks in entry 9's order, so a lock or `status: unknown` in the resources
|
||||
entry is Transient before the name is ever read, and a running Get makes
|
||||
four calls (`TestGet_runningMakesFourCalls`). The three agent messages
|
||||
follow entry 7. The poll interval is `budgets.poll`, and a failed task logs
|
||||
a V(1) `proxmox task failed` line with `upid` and `exitstatus`.
|
||||
|
||||
Judgment calls, most of them spec gaps:
|
||||
|
||||
- **`WARNINGS: <n>` counts as a successful task.** Proxmox reports a task
|
||||
that finished but logged warnings with that exitstatus and treats it as
|
||||
finished, not failed. Under the spec's old "anything but `OK` is
|
||||
Transient" rule, a start that logged a warning would make Create retry
|
||||
forever. Recon never produced one, so this row relies on how Proxmox is
|
||||
known to behave, not on a capture. The spec's entry 10 text and entry 13
|
||||
table now say so.
|
||||
- **A `suspended` lock still goes through the name and mark checks.** The
|
||||
entry 9 table puts "lock suspended is Stopped" before the config rows.
|
||||
Taken literally, a hibernated VM that reused our VMID would be reported
|
||||
as ours and Stopped, and Delete would then skip it as foreign in a loop.
|
||||
Get therefore reads the config, which it needs anyway for UID and
|
||||
CreatedAt, applies the name and mark checks, and only then answers
|
||||
Stopped. It skips `status/current` and the agent. Entry 9 now says this.
|
||||
- **`TestGet_twoVMsWithOurNameIsPermanent` became
|
||||
`TestGet_otherVMWithOurNameIsIgnored`.** Spec entries 2 and 9 address Get
|
||||
by VMID, and only Create refuses a duplicate name. Get answering
|
||||
Permanent would latch a working Proxy as Failed because of a VM someone
|
||||
else created.
|
||||
- **`TestGet_unmanagedDescriptionHasEmptyUID` tests a mark with no `uid`
|
||||
line.** The plan's name read literally ("unmanaged") would contradict
|
||||
"no mark is NotFound".
|
||||
- **Agent errors outside the entry 7 table are forced to Transient**, even
|
||||
a 403 or a `does not exist`. Otherwise `classify` would make them
|
||||
Permanent or NotFound, and the spec says "anything else: Transient".
|
||||
- **`waitUnlocked` returns the unlocked config**, which saves Create's
|
||||
ownership check one read. When the VM disappears while it waits (a
|
||||
failed clone), it returns the does-not-exist error and lets Create
|
||||
decide.
|
||||
|
||||
Pulled forward and left behind:
|
||||
|
||||
- **`provider_test.go` now exists** with `newFixture`, `testBudgets`,
|
||||
`addOurVM` and `assertProviderError`, because every Get and ListByTag
|
||||
test needs them. The plan created this file in Phase 8. Phase 8 adds the
|
||||
contract sweep to it.
|
||||
- **`waitRunning` is left to Phase 6.** The plan's package layout puts it
|
||||
in `task.go`, but Phase 5 lists no test for it and only Create uses it.
|
||||
It should be built on `pollUntil`.
|
||||
- **The fake gained two things.** `fakeVM.kind` makes an entry listed as
|
||||
`lxc` invisible to the qemu endpoints. `hookOn` is a general per-method
|
||||
hook, and `failWith` is now built on it. Tests use `hookOn` (through
|
||||
`mutateOn` in `get_test.go`) to change a VM between two of Get's calls.
|
||||
|
||||
A mutation check confirmed the new tests can fail. Each of these failed a
|
||||
test: classifying agent errors instead of forcing Transient; dropping the
|
||||
config lock check; dropping the `suspended` special case; ignoring an ended
|
||||
ctx in ListByTag; treating `WARNINGS` as failure; and not mapping
|
||||
`No QEMU guest agent configured` to Stopped. Removing the resources-entry
|
||||
lock and `unknown` checks together failed only
|
||||
`TestGet_unknownStatusIsTransient`. The fake reports a lock in the config
|
||||
as well, so the config check catches every lock row, including a mid-clone
|
||||
VM. The resources check stays because the spec orders it first and
|
||||
because it spares a config read. Every mutation was reverted, and the
|
||||
files were compared identical to backups.
|
||||
|
||||
```bash
|
||||
go test -race -count=2 ./internal/provider/proxmox/ # ok
|
||||
grep -n 'not implemented' internal/provider/proxmox/*.go | grep -v _test # Create and Delete only
|
||||
go build ./... && go vet ./... && go vet -tags=integration ./...
|
||||
go test -race -count=1 ./internal/provider/... ./cmd/... # all ok
|
||||
go test -short -count=1 ./... # all ok
|
||||
go test -count=1 -v ./internal/provider/proxmox/ | grep -c '^=== RUN' # 289 before, 393 after
|
||||
```
|
||||
|
||||
Lint ran with staticcheck disabled, because staticcheck panics on
|
||||
Go 1.27:
|
||||
|
||||
```bash
|
||||
bin/golangci-lint run --disable staticcheck ./internal/provider/proxmox/...
|
||||
```
|
||||
|
||||
Worth noting:
|
||||
|
||||
- The lint findings in the new files are goconst on test strings and
|
||||
logcheck on `get`, `agentAddress` and `waitTask` taking both a ctx and a
|
||||
logger. That shape copies kvm2, which has five of the same findings. The
|
||||
errcheck hits on `assertGetFails` were fixed by returning the error text.
|
||||
The two unparam hits and the mapsloop hits predate this phase.
|
||||
- `waitUnlocked` waits on any lock, `suspended` included. Phase 6's
|
||||
ownership check should decide whether a hibernated VM at our VMID is
|
||||
worth waiting out the clone budget for, or should be refused at once.
|
||||
- Nothing in this phase talked to the host. Every Proxmox message a test
|
||||
relies on comes from `testdata/` or from fake errors that
|
||||
`TestFakeAPI_errorsMatchCaptures` ties to the captures.
|
||||
|
||||
Reference in New Issue
Block a user