Add plan: fix the five code-review findings on the KVM branch

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-10 22:26:47 +00:00
parent 0daf56bbad
commit ce365e2602
1 changed files with 195 additions and 0 deletions
@@ -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) <noreply@anthropic.com>`.
 ### 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-<key>.<format>`
  ([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/<name>/domains/<vm>`, 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.