proxmox: Delete with seed cleanup, health Checker, CRD type comment
Delete stops and destroys a VM of ours under the per-name lock, then deletes its seed on the VM's node, or on every node when no VM of ours was left. A hibernated VM of ours is Permanent, as in Create. The Checker verifies each node concurrently (version, node status, ISO storage, templates) and adds CertificateInvalid, CredentialsUnavailable, StorageNotFound and TemplateNotFound to the shared reasons. The ProviderHealth type comment gains proxmox. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5.5
@@ -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.
|
||||
|
||||
@@ -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/<isoStorage>:iso/<name>-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`.
|
||||
|
||||
Reference in New Issue
Block a user