diff --git a/docs/plans/2026-09-10-2222-kvm-review-fixes.md b/docs/plans/2026-09-10-2222-kvm-review-fixes.md new file mode 100644 index 0000000..fbc67de --- /dev/null +++ b/docs/plans/2026-09-10-2222-kvm-review-fixes.md @@ -0,0 +1,195 @@ +# Plan: Fix the five code-review findings on `feat/kvm-provider` +**Created:** 2026-09-10 22:22 UTC + +## Context + +PR #5 (`feat/kvm-provider` → `main`, 19 commits) is open and awaiting merge. A +`/code-review high` pass over the branch returned five confirmed findings: two that can +pin a Proxy's finalizer forever, one startup crash-loop, and two in the guest-agent merge +committed earlier today. All five are fixed on the same branch so the PR updates in place, +then re-validated against the real hypervisor before merging. + +Four decisions, made by the user during planning: + +1. **Wedged deletes** — attempt `Delete` even when `Get` fails permanently, make an + unconfigured KVM host a no-op delete, and add an annotation that abandons cleanup for + whatever still cannot finish. +2. **Missing `kvm-ssh` Secret** — read the SSH material lazily, so only KVM Proxies fail. +3. **Guest agent** — stop rewriting the user's cloud-config. Ship the bootstrap as a + vendor-data shell script instead. +4. Validation is unit tests, the integration test against beelink, and the kind e2e. The + uncommitted dev-only files stay exactly as they are and out of the PR. + +## Why the guest-agent fix is a rewrite, not a patch + +Both cloud-init findings come from the same root cause: this operator edits the user's +document. Reading cloud-init 26.1's sources settles that it never has to. + +Vendor-data is consumed by its **own** handler chain with separate script and config paths +(`stages.py:_default_vendordata_handlers`). A vendor-data **shell script** is therefore +written to the vendor script directory and run by `cc_scripts_vendor`, never passing +through the config merger that drops colliding keys. On the final-stage module list it +runs after `package_update_upgrade_install` and before the user's own `runcmd`, so the +agent comes up early regardless of what the user's config does. + +That also closes the door the previous approach left open. The merge cannot be rescued +from inside the documents: `merge_how` in the user-data governs only the first source, and +`merge_how` in vendor-data is popped and discarded by `dict_extract_mergers` before the +document is ever written. There is no supported way to make vendor-data cloud-config keys +survive a user-data that defines the same key. + +Consequences: user-data reaches the seed byte for byte in any format, so Jinja templates, +MIME archives, gzip payloads and shell scripts stop being special cases (finding 4), and +nothing re-serialises YAML, so unquoted scalars keep their type (finding 5). The cost is +that the script picks the package manager itself where cloud-init's `packages` key was +distro-agnostic. + +## Why the delete fix is shaped this way + +KVM is the only provider that resolves a providerID segment against operator config. GCP +and Kubernetes let the backend answer "not found", so retiring a location yields +`ErrNotFound` and the finalizer drops. `Provider.host()` +([kvm.go:118-139](internal/provider/kvm/kvm.go#L118-L139)) returns `ErrPermanent`, and +`reconcileDelete` ([proxy_controller.go:281-291](internal/controller/proxy_controller.go#L281-L291)) +drops the finalizer only on `ErrNotFound`. `deletionFailure` +([:327-336](internal/controller/proxy_controller.go#L327-L336)) says in its own comment +that it retries "until an operator intervenes", and no intervention mechanism exists. The +annotation is that mechanism, and it also rescues the unconfigured-provider wedge at +[:276-279](internal/controller/proxy_controller.go#L276-L279). + +`Get` keeps returning `ErrPermanent` for an unknown host deliberately. Turning it into +`ErrNotFound` would make a typo in the host list look like a missing VM on the live path, +and the reconciler would recreate every affected Proxy elsewhere while the originals keep +running where garbage collection cannot see them. + +## Steps + +One commit per step on `feat/kvm-provider`, each ending with +`Co-Authored-By: Claude Opus 5 (1M context) `. + +### Step 0 — Save this plan into the repo +Copy to `docs/plans/2026-09-10-2222-kvm-review-fixes.md` with the same header timestamp and +commit it alone, before any code change. Start +`docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md` with the status checklist when +Step 1 lands. + +### Step 1 — Unwedge deletion (finding 1) +- `api/v1alpha1/proxy_types.go`: add `AnnotationSkipCleanup = "crawl.example.com/skip-cleanup"` + beside `AnnotationSpecHash` ([:59-63](api/v1alpha1/proxy_types.go#L59-L63)), documented as + abandoning the backend instance. +- `internal/controller/proxy_controller.go`, `reconcileDelete`: honour the annotation after + the empty-providerID branch and **before** the provider lookup, so it also covers a + provider that is no longer configured. Log the abandoned providerID at Info. + `ProxyReconciler` has no `EventRecorder`; do not add one for this. +- Same function: replace the two `if`s after `Get` with a switch. On + `provider.Class(err) == provider.ErrPermanent`, call `Delete` anyway, since it is + idempotent, and remove the finalizer when it succeeds rather than requeueing into a `Get` + that can never converge. On failure, `deletionFailure(errors.Join(getErr, delErr))`. + Guard on `err != nil`, since `Class(nil)` is nil. +- `internal/provider/kvm/kvm.go`, `Delete`: an unconfigured host segment means there is + nothing this operator can clean, so log it and return success, mirroring `removeVolumes` + ([kvm.go:506-514](internal/provider/kvm/kvm.go#L506-L514)) for a missing pool. `Get` is + unchanged. +- Tests: extend the deletion table in `internal/controller/reconcile_test.go` + ([:357-393](internal/controller/reconcile_test.go#L357-L393)) with permanent-Get plus + succeeding Delete, permanent-Get plus failing Delete, the annotation with the provider + never called, and the annotation on an unconfigured provider. In `kvm_test.go`, the + `"host not configured"` case at [:737](internal/provider/kvm/kvm_test.go#L737) flips from + `ErrPermanent` to success; add a named test beside `TestDelete_missingPoolIsNotAnError` + ([:715](internal/provider/kvm/kvm_test.go#L715)). +- Docs: the annotation in `README.md` (~line 163) and `docs/architecture.md` (~line 487); + a *Known gaps* entry in `docs/kvm-provider.md` saying a domain on a host removed from + config can no longer be cleaned up by the operator, the Proxy still finishes deleting, + and the VM must be removed by hand because garbage collection only sweeps configured + hosts. + +### Step 2 — Validate names that become identifiers (finding 2) +- `internal/provider/config_kvm.go`: add a name regexp beside `sha256Hex` + ([:6](internal/provider/config_kvm.go#L6)) allowing letters, digits, dot, dash and + underscore. Apply it to `hosts[].name` after the existing empty check + ([:129-131](internal/provider/config_kvm.go#L129-L131)) and to each `images` map key, + which becomes the cached volume name `base-.` + ([image.go:49-55](internal/provider/kvm/image.go#L49-L55)) and half of the image-cache + lock key ([image.go:96](internal/provider/kvm/image.go#L96)). `machineTypes` keys are only + compared, never embedded, so leave them. +- Keep the file's first-error-wins style and `fmt.Sprintf` field paths. The host message + should name the consequence: the value becomes part of `hosts//domains/`, which + `parseProviderID` splits on `/` ([id.go:11-21](internal/provider/kvm/id.go#L11-L21)). +- Tests: two entries in the `TestLoadConfig_kvmInvalid` table + ([config_kvm_test.go:76](internal/provider/config_kvm_test.go#L76)), existing + name/yaml/wantErrSub shape. + +### Step 3 — Read the SSH material lazily (finding 3) +- `internal/provider/kvm/conn.go`: keep `newSSHAuth`, and add a lazy dialer that builds it + on first dial and caches it once it succeeds, so a `kvm-ssh` Secret created after the + manager starts heals on the next reconcile with no restart. +- `internal/provider/kvm/kvm.go`, `New`: stop calling `newSSHAuth`; pass the lazy dialer. + Use the context parameter it currently ignores to log a warning when either file is + unreadable at startup, keeping the diagnostic without the exit. +- Error classes: a missing or unreadable file stays unclassified, so `Class` defaults it to + transient and the Proxy retries until the Secret appears; an unparseable key or malformed + known_hosts wraps `ErrPermanent`. Distinguish with `errors.Is(err, fs.ErrNotExist)`. +- Fix the two doc comments promising startup failure + ([kvm.go:79-81](internal/provider/kvm/kvm.go#L79-L81), + [conn.go:182-185](internal/provider/kvm/conn.go#L182-L185)) and the matching claim in + `docs/kvm-provider.md` (~lines 107-109). `DISCOVERY_TOKEN` already sets the house + convention for an optional mount: degrade with a loud warning. +- Tests in `conn_test.go`, which already writes key files: `New` succeeds with no key + present, the first dial fails transiently, a dial after the file appears succeeds, and a + corrupt key is permanent. + +### Step 4 — Guest agent via vendor-data script (findings 4 and 5) +- `internal/provider/kvm/seed.go`: delete `withGuestAgent`, `isCloudConfig`, `appendUnique` + and the `sigs.k8s.io/yaml` import. User-data goes onto the seed exactly as the Proxy + supplied it. +- Add the bootstrap as a `#!/bin/sh` vendor-data script: skip when the agent is already + present, otherwise install `qemu-guest-agent` through whichever of `apt-get`, `dnf`, + `yum`, `zypper` or `apk` exists, then `systemctl enable --now`. Run `apt-get update` + before installing, since nothing else guarantees a fresh index when the user's config has + no `packages` key. Keep it idempotent and let it log to stdout, which lands in + `cloud-init-output.log`. +- When the provider config also sets `vendorData`, wrap both into a MIME multipart + vendor-data: the user's document as one part, the script as another. Cloud-init detects + multipart by finding `mime-version:` in the first 4096 bytes, so the header must lead. +- `internal/provider/config_kvm.go`: add `installGuestAgent`, defaulting to on. Off means + the seed carries only the user's own vendor-data and IP discovery falls back to the host + ARP table. Nothing fails permanently any more, so the error path in `writeSeed` + ([kvm.go:252-255](internal/provider/kvm/kvm.go#L252-L255)) narrows to MIME assembly. +- Tests in `seed_test.go`: user-data byte-identical for cloud-config, Jinja, MIME, gzip and + script inputs; the script present in vendor-data; the opt-out; the multipart case + including the `mime-version:` position; keep the existing ISO tests. +- Docs: rewrite the *What lands on the host* passage added earlier today, the `vendorData` + row and a new `installGuestAgent` row in the config table, and the ARP note in + *Known gaps*. Cite the cloud-init behaviour that forced the change. + +### Step 5 — Validate, record, push +- `gofmt -l internal/`, `go vet ./...`, `go test -short ./...`. +- `make test-integration-kvm` against beelink, environment as recorded in the KVM execution + log. +- Rebuild the image, `kind load`, restart the manager, delete and recreate both Proxies, + curl a test URL through each. +- `CHANGELOG.md` entry at the top with a real `date` timestamp, noting it supersedes the + merge approach described in today's earlier entry. Add a Run 8 section to + `docs/plans-executions/2026-09-04-1404-kvm-provider.md` plus this plan's own execution + file. Push with the token helper; PR #5 updates in place. + +## Verification +- Unit: the controller cases prove a permanent `Get` no longer pins the finalizer and that + the annotation short-circuits before the provider is touched. `TestDelete_*` proves an + unconfigured host is a clean delete while `TestGet_*` stays permanent, which is what keeps + the live path from recreating VMs. +- Seed: assert the uploaded user-data equals the Proxy's `cloudInit` byte for byte, which is + the property the old merge could not offer. +- Integration on beelink: Create, IP via the guest agent, ListByTag, Delete. In the guest, + `cloud-init status --long` clean and `systemctl is-active qemu-guest-agent` active. +- In-cluster: both Proxies reach Ready with an IP and return 204 through squid. Their + cloud-config defines its own `packages`, which is the exact case that failed in Run 7. +- Negative check in kind: rename the `kvm-ssh` Secret away and restart. The pod must stay + up, the Kubernetes Proxies must stay Ready, and only the KVM Proxies degrade. Restore the + Secret and confirm they recover without a restart. + +## Out of scope +- The two items the review noted but did not report: deleting a cached base image while + running VMs back onto it, and an ad-hoc image URL always being treated as qcow2. Both are + documented known gaps and neither is a regression from this branch. +- The uncommitted dev-only files, by the user's decision.