egress-proxies-operator/docs/plans/2026-09-10-2222-kvm-review-fixes.md

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) returns ErrPermanent, and reconcileDelete (proxy_controller.go:281-291) drops the finalizer only on ErrNotFound. deletionFailure (:327-336) 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.

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), 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 ifs 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) for a missing pool. Get is unchanged.
• Tests: extend the deletion table in internal/controller/reconcile_test.go (:357-393) 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 flips from ErrPermanent to success; add a named test beside TestDelete_missingPoolIsNotAnError (:715).
• 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) allowing letters, digits, dot, dash and underscore. Apply it to hosts[].name after the existing empty check (:129-131) and to each images map key, which becomes the cached volume name base-<key>.<format> (image.go:49-55) and half of the image-cache lock key (image.go:96). 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).
• Tests: two entries in the TestLoadConfig_kvmInvalid table (config_kvm_test.go:76), 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, conn.go:182-185) 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) 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.