diff --git a/.claude/agents/deep-reviewer.md b/.claude/agents/deep-reviewer.md new file mode 100644 index 0000000..31e7b39 --- /dev/null +++ b/.claude/agents/deep-reviewer.md @@ -0,0 +1,11 @@ +--- +name: deep-reviewer +description: Deep-dive verification of a single escalated hypothesis from the sweep. +model: fable +effort: high +tools: Read, Grep, Glob, Bash +--- +You get ONE hypothesis with entry points. Follow it across the whole repo: callers, +callees, tests, configs. Verdict: CONFIRMED / REFUTED / PLAUSIBLE, with the exact +path/interleaving that breaks, or the reason it can't. Propose a minimal fix. +Don't re-review the package generally. Stay on the hypothesis. \ No newline at end of file diff --git a/.claude/agents/sweep-reviewer.md b/.claude/agents/sweep-reviewer.md new file mode 100644 index 0000000..4d4cf23 --- /dev/null +++ b/.claude/agents/sweep-reviewer.md @@ -0,0 +1,23 @@ +--- +name: sweep-reviewer +description: First-pass broad code review of one package/module. Use for fan-out sweeps. +model: opus +effort: high +tools: Read, Grep, Glob, Bash +--- +You review ONE package/module given in the prompt. Read-only, never edit. + +Output two sections: + +## Findings +Concrete defects you verified. Per item: file:line, severity, one-line defect, +failure scenario (inputs/state -> wrong behavior). No style nits unless they hide a bug. + +## Escalate +Things you could NOT confirm or refute within this package, where being wrong is costly. +Escalate when: shared mutable state across goroutines, lock ordering, context/cancel +propagation, retry+timeout+idempotency interactions, reconcile races, authn/authz +boundaries, invariants that depend on code outside this package, state machines. +Per item: entry-point files, the specific hypothesis ("X may be read without mu +on path A->B->C"), what you already checked, what's still unknown. +Do NOT escalate things you can resolve yourself. Go resolve them. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f1c060..7d6ba6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,45 @@ # Changelog +## 2026-09-17 20:36 UTC — kvm2: the four high findings from the code review, and its first run on a real hypervisor + +A scoped review of the kvm2 provider (`review/REPORT.md`: 52 findings, 4 high, 20 medium, +28 low) produced four fixes, one commit each, then the provider was deployed in-cluster and +validated against beelink — the first time any of this code had run against real libvirt. + +- **H1 — a stalled write no longer delays a connection teardown.** `sshSocket.Close` ran the + blocking close before the unblocking one, so a transfer stuck in a TCP write pinned the + teardown for as long as the kernel retransmitted (~15 min). `link.markDead` compounded it by + killing inside a `sync.Once` before signalling dead, which made even cancelling the reconcile + useless. With `MaxConcurrentReconciles: 3`, one silent host could stall every Proxy on every + provider. `internal/provider/kvm2/{ssh,backend}.go`. +- **H2 — a source error no longer reaches go-libvirt's stream sender.** A failing HTTP body + deadlocked `StorageVolUpload` outright and leaked two goroutines plus 4 MiB per occurrence, + against a 128Mi pod limit. The defect is upstream (`requestStream` never drains an unbuffered + `outErr`); the workaround wraps the reader so it can only ever end the stream cleanly, and + moves the transfer deadline onto the HTTP source so a stalled mirror unwinds instead of + leaking. A daemon-side failure mid-upload still leaks — documented in *Known gaps* with the + two-character upstream patch. `internal/provider/kvm2/{backend,image}.go`. +- **H3 — an unreadable address is no longer reported as a missing one.** Any error from both + address sources read as "no address yet", so a pooled connection killed by an unrelated + reconcile demoted a healthy proxy to Provisioning, cleared its IP and made the health engine + drop its counters. One source failing stays "no address yet"; every source failing, or a dead + connection, is now Transient. `internal/provider/kvm2/get.go`. +- **H4 — the base image cache is no longer shared between provider entries.** kvm2 built + byte-identical volume names to the `kvm` provider in the same pool, so two entries could share + one file, a concurrent fetch could boot a guest from a half-written base, and a `sha256` edit + deleted the base under live overlays. Names now carry the entry + (`base--.`), and a digest mismatch is refused rather than resolved by + deleting. `internal/provider/kvm2/{volume,image}.go`, `internal/provider/config_kvm.go`. +- **klog and `net/http` errors now join the structured log stream.** client-go's leader election + wrote `I0917 ...` lines straight to stderr because nothing routed klog into logr, and the + discovery server's `http.Server` had no `ErrorLog`. The deployment also gained + `--zap-encoder=json` and `--zap-time-encoding=rfc3339`, so `kubectl logs | jq` works. + `cmd/main.go`, `internal/discovery/server.go`, `config/manager/manager.yaml`. +- **The `kvm2-home` provider entry is enabled** in `config/manager/providers_config.yaml`. + Validated end to end: health check Ready, a 433 MB image cached in 8.3 s under the new name + beside kvm's untouched one, a second Proxy reusing the cache, Squid answering a real CONNECT + probe, and Delete leaving the base image while removing the domain and its volumes. + ## 2026-09-16 11:46 UTC — Provider blueprint split into contract, spec template and appendix - **`docs/provider-contract.md`** is the generic provider specification: MUST/SHOULD/MAY diff --git a/config/manager/providers_config.yaml b/config/manager/providers_config.yaml index 0602819..b3ae1c5 100644 --- a/config/manager/providers_config.yaml +++ b/config/manager/providers_config.yaml @@ -56,32 +56,32 @@ data: # made by kvm, so delete those Proxies first. It caches its base images # under base--., so it downloads its own copy rather # than sharing kvm's; budget the pool space. See docs/kvm2-provider.md. - # - name: kvm2-home - # type: kvm2 - # kvm2: - # sshKeyFile: /etc/proxy-operator/kvm/id_ed25519 - # knownHostsFile: /etc/proxy-operator/kvm/known_hosts - # # insecureIgnoreHostKey: true - # # diskSizeGb: 10 - # # installGuestAgent: false - # # vendorData: | - # # #cloud-config - # # ... - # images: - # debian-13: - # url: https://cloud.debian.org/images/cloud/trixie/latest/debian-13-generic-amd64.qcow2 - # # sha256: "<64 hex>" - # # firmware: uefi - # ubuntu-24.04: - # url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img - # machineTypes: - # small: {vcpus: 1, memoryMiB: 1024} - # medium: {vcpus: 2, memoryMiB: 2048} - # hosts: - # - name: beelink - # address: beelink.tail8c69c.ts.net:22 - # user: novakj - # bridge: br0 - # # socket: /var/run/libvirt/libvirt-sock - # # pool: default + - name: kvm2-home + type: kvm2 + kvm2: + sshKeyFile: /etc/proxy-operator/kvm/id_ed25519 + knownHostsFile: /etc/proxy-operator/kvm/known_hosts + # insecureIgnoreHostKey: true + # diskSizeGb: 10 + # installGuestAgent: false + # vendorData: | + # #cloud-config + # ... + images: + debian-13: + url: https://cloud.debian.org/images/cloud/trixie/latest/debian-13-generic-amd64.qcow2 + # sha256: "<64 hex>" + # firmware: uefi + ubuntu-24.04: + url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img + machineTypes: + small: {vcpus: 1, memoryMiB: 1024} + medium: {vcpus: 2, memoryMiB: 2048} + hosts: + - name: beelink + address: beelink.tail8c69c.ts.net:22 + user: novakj + bridge: br0 + # socket: /var/run/libvirt/libvirt-sock + # pool: default diff --git a/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md b/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md index 08ef135..7920e8d 100644 --- a/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md +++ b/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md @@ -7,7 +7,7 @@ Plan: [docs/plans/2026-09-17-1515-kvm2-review-fixes.md](../plans/2026-09-17-1515 - [x] Step 3 — H3: an unreadable address must not look like a missing one - [x] Step 4 — H4: the base image cache must not be shared across provider entries -- [ ] Step 5 — Validate H1-H4 in-cluster against beelink (in progress) +- [x] Step 5 — Validate H1-H4 in-cluster against beelink - [x] Step 6 — Route klog into the structured logger Remaining after those: the 48 mediums and lows in `review/REPORT.md`. Each gets its own @@ -467,3 +467,65 @@ opts := zap.Options{ ``` Left out of this step as scope the plan did not cover. + +## Step 5 — Validation against beelink + +First contact between the kvm2 provider and real libvirt. Everything before this ran against +an in-memory fake of the twenty libvirt calls the provider uses. + +Run in the existing `kind-proxy-operator-demo` cluster rather than a fresh one, deliberately. +That cluster already owned two live `kvm-home` VMs on beelink, and orphan GC deletes any +instance whose UID is missing from *its own* cluster's live set after `gc-min-age`. A second +operator carrying the `kvm-home` entry would have destroyed both within ten minutes. Using the +existing cluster made its live set the thing that protected them — the isolation argument +pointed the opposite way from instinct. + +Baseline recorded first (4 Ready proxies, 3 ProviderHealth entries, beelink's domains, volumes +and pool space), so "did we break something" stayed answerable. + +The image tag had to change from the running `kvm-dev`. `make deploy` sets the image through +kustomize, so an identical tag leaves the Deployment spec untouched, no rollout happens, the +new process never starts, and the providers config — which is read once per process — is never +re-read. The ConfigMap carries no generator hash either, so nothing else would have forced it. + +Results, in the order they were checked: + +| Check | Outcome | +|---|---| +| `kvm2-home` ProviderHealth | `Ready 1/1`, `Reachable`, 189 ms — hostname, libvirt 11.6.0, pool running, 247.8 GiB free | +| First Create, cold cache | `base image cached`, 433,651,712 bytes in 8.3 s, digest verified | +| Volume naming (H4) | `base-kvm2-home-debian-13.qcow2` created **beside** kvm's untouched `base-debian-13.qcow2` | +| Second Proxy | no `fetching` line at all — cache hit under the new name | +| State reporting (H3) | `Running` held steady across every poll, no flapping to Provisioning | +| Squid | `Healthy=True` on a real CONNECT probe | +| Delete | domain and both per-VM volumes gone; base image survived | +| Existing infrastructure | all four pre-existing proxies stayed Ready across two rollouts | + +The health check alone is worth calling out as a gate: it dials SSH, verifies the host key, +completes the libvirt handshake, runs four RPCs and tears down a dedicated connection — the +exact path H1 changed — without creating anything. Putting the stop gate there meant the +riskiest unknown was settled before a single byte was written to the hypervisor. + +### What this run did NOT prove + +**H2's error paths.** The unverified claim — that libvirtd answers a short stream finish with +OK rather than an error — only arises when a body *fails* part way through, and a healthy +433 MB download never finishes short. The 19-minute HTTP deadline never fired either. What was +proved is narrower: the wrapper does not break normal uploads. Settling the rest still needs +the scratch-pool probe in the Step 2 entry, and it would be easy to mistake this green run for +more than it is. + +**The `kvm2` integration test still has not run.** `make test-integration-kvm2` remains +unexecuted; this was a cluster-level validation, not that suite. + +### Worth noting + +`config/manager/providers_config.yaml` now ships with the `kvm2-home` entry **enabled**, since +the operator is running it for real. `config/manager/kustomization.yaml` was left at its +committed tag: `make deploy` rewrites it to whatever local image was last deployed, which is +churn rather than work. + +Two host-side leftovers were *not* touched, both pre-existing and unrelated to kvm2: five +orphan `proxy-*` volumes from earlier kvm runs, and the `base-test-image.qcow2` pair from the +kvm integration test. Every write to beelink in this step came from the operator itself; no +`virsh` write was issued by hand. diff --git a/review/REPORT.md b/review/REPORT.md new file mode 100644 index 0000000..dec9137 --- /dev/null +++ b/review/REPORT.md @@ -0,0 +1,183 @@ +# kvm2 provider review: final report + +Branch `feat/kvm2-provider` at `2510842`. Scope in `scope.md`; phase 1 detail in `sweep.md`; +phase 2 detail, measurements and unrun probes in `deep.md`. + +**What this contains.** Confirmed findings only: the sweep findings (each reported by its +sweeper as verified in code) plus the escalations that came back CONFIRMED. Where a deep dive +refined a sweep finding, the refined version is given. **52 findings: 4 high, 20 medium, 28 low.** + +**How far to trust it.** Nothing here was fixed or re-verified by a second reader except where +two or three reviewers reported the same thing independently (marked ×2/×3). Hypervisor access +was read-only, so every claim about what libvirt does on a *write* is from libvirt source or +memory, with the reviewer's stated confidence; the scripts that would settle them are in +`deep.md`. The kvm2 integration test has **not yet run on beelink** (Step 11 is unchecked in +the plan execution file; every `base-*` volume there predates kvm2), so no kvm2 behaviour in +this report has been observed against real libvirt. + +Themes: **T1** unknown errors default to Transient and retry forever with no condition on the +Proxy · **T2** the ownership mark is enforced on Create/ListByTag but not Get/Delete · +**T3** base image cache identity, lifetime and fencing · **T4** any caller can kill the shared +pooled link, and several callers misread the resulting error. + +--- + +## High + +### H1 — `markDead` blocks on a silent host and pins every reconcile worker (S1.1 + E1) +`internal/provider/kvm2/backend.go:138-148`, `ssh.go:109-114` +`markDead` runs `kill()` inside `once.Do` and only then closes `l.closed`. `kill` is `sshSocket.Close`, which closes the SSH channel first; that needs `ch.writeMu` and `t.mu`, both held by a writer stuck in a TCP write. Reproduced against x/crypto v0.54.0: with a 256 K send buffer the close was still blocked after 8 s, and a plain `client.Close()` released it instantly. Precondition: a bulk write in flight and a send buffer under the 2 MiB SSH window. +**Scenario.** Three Proxies placed on one host with an uncached image. Worker 1 uploads (`image.go:270`, 20 min budget); workers 2 and 3 park on the image lock (`kvm2.go:224-233`) with no deadline; `MaxConcurrentReconciles` is 3. Host is power-cut. The keepalive's `markDead` (`hosts.go:158`) blocks in the channel close; worker 1's `call` cannot see `done`, `l.closed`, or even a ctx cancel (it queues in `once.Do`). For ~15 min no Proxy on any provider reconciles. No `ReconciliationTimeout` is set, and one alone would not release worker 1. +**Fix.** Close the TCP connection first in `sshSocket.Close` (`s.client.Close()` before, or instead of, `s.Conn.Close()`); in `markDead`, `close(l.closed)` before `kill()` or run `kill` in a goroutine. Regression test with a frozen proxy and small `SO_SNDBUF`. Optionally set `ReconciliationTimeout`. + +### H2 — An error during `StorageVolUpload` wedges go-libvirt's stream sender forever (S4.1) +`internal/provider/kvm2/image.go:273` (upstream `rpc.go:290-308`, `socket/socket.go:347-376`) +`abort` and `outErr` are unbuffered. On a reader error `SendStream` blocks forever on `outErr <-`; on a server-side error `requestStream` blocks forever on `abort <- true`. A socket kill does not unblock them (E6 found why: `listen` blocks in `callback`'s send under `cmux`), contrary to the comment at `backend.go:152-156`. +**Scenario.** A mirror truncates the body, or libvirtd hits ENOSPC mid-upload. The caller is rescued only after the 20 min transfer budget; each failed upload leaks 1-2 goroutines plus a 4 MiB buffer, accumulating under reconcile retries; `discardVolume` (`image.go:292`) deletes the base before the deferred `fresh.Close()` (`:265`), so the file is unlinked while the server stream holds the fd. `TestEnsureImage_shortReadRemovesVolumeAndIsTransient` cannot catch it: the fake returns the reader error immediately. +**Fix.** Have `countingReader` convert a read error to `io.EOF` and report it out of band; the existing `readErr` / `read != size` checks (`image.go:281-290`) still catch truncation. Close the dedicated connection before `discardVolume`. Consider an upstream patch for the unbuffered channels. + +### H3 — `address()` swallows every error, demoting healthy Running proxies (S3.1) +`internal/provider/kvm2/get.go:104-119` +Every non-`context` error from both address sources becomes `("", nil)`, so `mapState(running, "")` = Provisioning. +**Scenario.** (a) On beelink the ARP source never answers (`kvm2-provider-spec.md:146`), so a `qemu-guest-agent` restart in the guest (`ErrAgentUnresponsive`) empties the address. (b) Another reconcile's cancel kills the shared pooled link between `DomainGetState` and the address read; the "connection is closed" error carries no ctx sentinel, so `Get` *succeeds* with `IP=""`. Then `proxy_controller.go:208-211` clears `status.IP` and sets `Provisioned=False`, and `internal/health/engine.go:200-207` prunes the proxy's health state. One RPC hiccup pulls a working proxy out of rotation. `TestGet_addressErrorIsProvisioning` pins this. +**Fix.** Return Transient from `Get` when the address read fails with a connection-class error (`isDead(be)` or the closed-link error) and return NotFound on `ErrNoDomain`; keep "no address yet" only for agent-unavailable/ARP-empty on a domain that has never reported an address. Consider having the controller keep the last known IP while an instance is Running. + +### H4 — The base image cache is shared and unfenced across provider entries (S7.1 ×2, E3-3a/3b, E4) +`internal/provider/kvm2/volume.go:17-25`, `image.go:113-146`, `:235-245`, `kvm2.go:111`; `internal/provider/kvm/image.go:44-57`; `config/manager/providers_config.yaml`, `config/samples/providers-config.yaml:41,73` +The base is `base-.` with a `.sha256` marker, byte-identical in the kvm provider, in the same default pool, under **separate, per-entry locks**. The shipped config puts `kvm-home` and `kvm2-home` on `beelink`, key `debian-13`, pool `default`, and the docs say both types may share a host. On the host both live (kvm-made) overlays back onto that very file. Neither the name nor the marker records the URL. +**Scenarios.** +1. *Wrong image, silently.* The two entries' URLs drift and neither sets `sha256`: whoever fetched first owns the file. Likewise, editing `images..url` alone is a silent no-op. +2. *Digest mismatch (3a).* One entry pins `sha256: D2` while the cached base is D1: `cachedImage` (`image.go:129` → `:145`) deletes the base under the other provider's live overlays. libvirt refuses a delete only on `in_use`/`building` (`storage_driver.c:1844-1856`, read at v11.6.0). With two different pins, every Create on either side deletes and refetches. +3. *Concurrent cold fetch (3b, by source, unexecuted).* B finds A's base-without-marker and deletes it (`image.go:117-118`), recreates it and uploads. libvirt holds `in_use` only during stream setup (`storage_driver.c:2421-2427`). A's upload completes into the unlinked inode, A writes the marker, and `StorageVolGetPath` resolves to B's partial file: **A boots a guest from a half-written base.** If B is kvm2, its `writeMarker` then fails `ErrStorageVolExist` and `image.go:241-243` discards the complete base under A's running domain. If the process dies during B's upload, a truncated base stays under a valid marker forever, since nothing re-hashes the base. +The rollout/leader-handover variant of this was **refuted** for the shipped manifests (lease held until the old pod dies; SIGKILL at 10 s). +**Fix.** kvm2-specific prefix including the entry name, or one process-wide image lock table; reject two kvm/kvm2 entries with the same address+socket+pool at config load. Upload to `.part-`, verify, then publish the final name; never delete a name this call did not create, in particular drop `discardVolume(base)` on a marker `ErrStorageVolExist`. Record size and URL next to the digest and check the base's allocation before trusting a marker. + +--- + +## Medium + +### M1 — Paused domains are destroyed (S2.2 ×2) +`create.go:230-231`, `get.go:56`, `state.go:16-27`. `get.go:56` discards the `DomainGetState` reason; paused/pmsuspended map to Stopped; `reconcileManaged` (`proxy_controller.go:217-225`) then deletes and recreates. The "virsh resume it" guard in `ensureRunning` only fires before an ID is persisted. **Scenario:** the pool fills and QEMU pauses the guest with `PAUSED_IOERROR` (or `virsh suspend`/save/snapshot/migrate); at the next drift poll the VM and its disks are gone and the recreate refills the pool. **Fix:** keep the reason; map paused to a non-destructive state (Provisioning or a new Degraded) except for reasons that are terminal. + +### M2 — `Get` and `Delete` do not enforce the kvm2 mark (S2.3 ×2, E3-2) [T2] +`get.go:52-73`, `delete.go:55-98` vs `create.go:80-82`. IDs are byte-identical to the kvm provider's (`kvm/id.go:11-13`). **Scenario:** an entry is re-typed `kvm` → `kvm2` under the same name without first deleting its Proxies (the documented migration, out of order): `status.ProviderID` survives, `Get` returns the v1 domain with `UID: ""`, and anything but Running leads to kvm2 destroying a domain the docs say it can never touch. A tampered `status.providerID` such as `hosts/beelink/domains/docker-29` destroys an unrelated VM. Nothing prevents a re-type (no webhook, `config.go:93-135`). The reverse direction (kvm provider against kvm2 domains) has no guard at all (`kvm/kvm.go:177-181`). **Fix:** `removeDomain` and `get` require `meta.managed()`; pass the expected UID to Delete or compare `inst.UID` with `p.UID` in the controller. + +### M3 — Volumes created before `DomainDefineXML` are unreachable when Create returns no ID (S2.1) +`create.go:107-114`. **Scenario:** overlay and seed succeed, define fails Permanent (or the manager dies). No `ProviderID` is set; on Proxy deletion `reconcileDelete` takes the empty-ID shortcut (`proxy_controller.go:270-274`); `ListByTag` lists domains only. `proxy-.qcow2` and `-seed.iso` leak permanently. Five such leftovers already sit in beelink's `default` pool (Appendix C). **Fix:** on a failed define, best-effort remove the two volumes before returning; or have `Delete`/GC derive volume names from the Proxy UID. + +### M4 — The pool-space precheck reads a cached, reserve-inflated number (E8) +`create.go:92-100`, `image.go:450-458`, `check.go:106-112`. Measured on beelink: libvirt's `available` is cached per pool (three pools on one filesystem report two values, 1.68 GB apart; frozen over 80 s while `f_bfree` moved) and is `f_bfree`, **24.7 GB above** what unprivileged QEMU can use on the kvm2 filesystem. The only refresh is after an image upload. **Scenario:** with `f_bavail` between 0 and ~10.7 GB the precheck passes for a 10 GiB disk, `ErrQuotaExceeded` never fires, and Create, the upload and the guests hit ENOSPC (feeding H2 and M1). `poolAvailableGiB` in ProviderHealth over-reports the same way. **Fix:** `StoragePoolRefresh` right before `checkPoolSpace` (not in Check); add headroom to `neededBytes` from the discarded `capacity` or a `poolReserveGiB` setting; make the fake's pool info cached-until-refresh. + +### M5 — An overlay smaller than the base's virtual size is accepted; the Proxy hangs in Provisioning (S5.6 + E10) +`volume.go:72`, `create.go:107,158`, `internal/provider/config_kvm.go:133`. Measured locally: `qemu-img create` with a capacity below the backing file exits 0; reads past the capacity fail; a GPT base under a short overlay shows only the protective MBR. Bases on the host are 3 GiB virtual. **Scenario:** `diskSizeGb: 2` validates; Create succeeds; the guest has no root device and never reports an address; `Get` says Provisioning forever (no provisioning deadline exists), nothing names `diskSizeGb`, and raising it later does not repair that Proxy. libvirt's pass-through is ~90% from source; the 200 KiB probe is in `deep.md`. **Fix:** add `StorageVolGetInfo` to the backend seam and fail Permanent when the base's capacity exceeds the overlay's; same check on a reused overlay. + +### M6 — A peer-close right after ConnectOpen wedges a pooled link forever (E6a) +`backend.go:177-178`, `:229-236`, `hosts.go:75`, `errors.go:55` (upstream race `libvirt.go:297/305/821`, reported by `-race`). **Scenario:** libvirtd or the forwarded socket goes away within ~1 ms of a successful open (daemon restart). `Connect()` returns nil with a `Disconnected()` channel nothing will close; every later RPC fails `EINVAL`, classified Transient; no result marks the link dead; the SSH keepalive keeps succeeding. The host answers "invalid argument" until the operator restarts. Wedged in 546/1000 cycles over real SSH in the reviewer's harness. The hypothesised trigger (kvm2's own kill) is harmless. **Fix:** in `call`, `markDead` on `syscall.EINVAL` / `libvirt.ErrInterrupted`; or wrap the `net.Conn` so a failed `Read` marks the link dead. + +### M7 — Permanent host/config rejections retry forever with nothing in status (S5.2 ×2, S3.5, E9b) [T1] +`errors.go:58-71`, `xml.go:26-27,194-206`, `proxy_controller.go:344-345`. `x86_64`/`q35`/`kvm`/host-passthrough/efi are unconditional with no capability gate; code 67 (`ErrConfigUnsupported`) and the bare `libvirt.ErrUnsupported` sentinel are unmapped; `providerFailure`'s Transient arm sets no condition. **Scenario:** an aarch64 or KVM-less host is added; Check says Ready; every Create retries an impossible define forever, rerunning `writeSeed` each time; a missing bridge (38, at start) or missing OVMF (9) behaves the same and Check looks at neither. "Unknown means retry" is a recorded spec decision (`kvm2-provider-spec.md:317`), but it never considered 67. **Fix:** 67 → Permanent on create; `ErrUnsupported` → Permanent; leave 9/38/1/84. Surface a non-latching condition for Transient create failures, or check domain capabilities and the bridge in the health probe. + +### M8 — Seed ISO I/O failures latch as Permanent with no requeue (S5.1) [T1, inverted] +`create.go:199-201`, `seed.go:66-74`. `permanentf("building seed ISO …: %v")` covers a missing `/tmp`, ENOSPC, and even a `RemoveAll` failure on an otherwise successful build. **Scenario:** node disk pressure for a minute leaves the Proxy unprovisioned until a spec edit or the ~10 h resync. **Fix:** Permanent only for the empty-name guard; wrap the rest Transient with `%w`. + +### M9 — `HEAD` 2xx without a usable Content-Length becomes a latching Permanent (S4.2) [T1] +`image.go:434-439`, `:391-396`. The GET fallback fires only on non-2xx. **Scenario:** a CDN (or Go's own `net/http`) answers HEAD with `Content-Length: 0`; the Proxy never provisions although GET works. **Fix:** fall back to GET whenever HEAD yields no positive length. + +### M10 — A completed upload is discarded when the pooled link died meanwhile (S4.3) [T4] +`image.go:235-245`. Refresh and `writeMarker` run on the pooled `be` after a transfer on a dedicated link. **Scenario:** any other reconcile's cancel during a 20 min download kills the pooled link; the verified base is deleted and refetched. **Fix:** run the refresh and marker on the still-open dedicated connection. + +### M11 — Precheck uses HEAD's length, the volume uses GET's, and there is no size cap (S4.4) +`image.go:419-443`, `:226-230`. **Scenario:** HEAD says 1 MB, GET says 4 TiB: a sparse 4 TiB volume is created and the stream fills the host pool. **Fix:** re-check space against the GET length; add `maxImageBytes`. + +### M12 — `placement.image` is an unvalidated user-supplied URL fetched by the operator (S4.5) +`image.go:59-65`, `internal/provider/config_kvm.go:101-104`. No CRD validation, no host allowlist, `http.DefaultClient` follows redirects, and a URL image can never carry a digest. **Scenario:** anyone who can create a Proxy makes the operator issue requests to cluster-internal addresses and boots arbitrary bytes on the egress bridge. **Fix:** a config switch to allow URL images at all (default off) or a host allowlist; `CheckRedirect` policy; block link-local/private ranges. + +### M13 — A `sha256` change replaces the base in place under live overlays (S4.6 ×3) +`image.go:118,129,145`. No in-use check, `V(1)` log; a mistyped digest deletes the base before anything is verified. The kvm2 sample omits the warning the kvm sample carries. Whether guests actually corrupt is Appendix A (E7). **Fix:** on a digest mismatch return Permanent "add the image under a new key"; or put the digest in the volume name. + +### M14 — IP detection takes the first IPv4 from any guest interface (S3.2) +`state.go:35-49`. `Hwaddr` is available but unused. **Scenario:** `docker0`/`tailscale0` comes up before the DHCP lease; the Proxy latches `Provisioned=True` with 172.17.0.1 and is never corrected. **Fix:** match the interface by the domain NIC's MAC. + +### M15 — A link that dies mid-list yields a silently partial `ListByTag` (S3.3) [T4] +`list.go:38-45`. The guard tests `ctx.Err()`, but a killed link has no ctx sentinel. GC only under-deletes, so nothing wrong is removed, but the sweep covers a fraction of the fleet and says so nowhere. **Fix:** fail the list on any connection-class error (`isDead(be)`). + +### M16 — `KVMHost.Address` is not validated as `host:port` (S7.2) [T1] +`internal/provider/config_kvm.go:180-182`. **Scenario:** `address: beelink` loads; every dial fails "missing port in address", classified Transient, and Check reports `Unreachable`. **Fix:** `net.SplitHostPort` at load, or default `:22`. + +### M17 — Plain `http://` images with no digest are accepted silently (S7.3) +`internal/provider/config_kvm.go:101-104,142-147`. The root disk of every VM can come from an unauthenticated channel and the marker then pins it. **Fix:** require `sha256` for `http://` catalogue entries, or warn loudly at load. + +### M18 — The fake ignores the metadata namespace (S7.4) [T2] +`fake_test.go:362-376`, `:259-282`. No unit test can build a kvm-v1-marked domain and assert `ListByTag` skips it; if that filter regressed, kvm2 would hand kvm's VMs to orphan GC with the suite green. E3/E9 observed that real libvirt 11.6 does scope by namespace. **Fix:** key the fake's metadata on the element's namespace; add the test. + +### M19 — The integration target's timeout can skip VM cleanup on the real host (S7.5) +`Makefile:75`. `-timeout 40m` is fixed while two tests each take up to `KVM2_TEST_TIMEOUT` (20 m) plus 5 m cleanup. **Scenario:** a cold download or a raised env timeout trips go test's own timeout before `t.Cleanup` (`kvm2_integration_test.go:373-382`); a domain and two volumes stay on beelink under a random name. **Fix:** derive `-timeout` from the env var with margin; print the name up front; add a `review`-style sweep-by-prefix cleanup target. + +### M20 — `sshSocket.keepalive` has no test (S1.2) +`ssh.go:119-147`. It is the only detector of a silently dead pooled link between calls, and `testSSHServer` already services global requests. **Fix:** an end-to-end test that stops the server's request handling and asserts `markDead`. + +--- + +## Low + +| ID | Where | Defect → scenario | Fix sketch | +|---|---|---|---| +| L1 (S1.3) | `ssh.go:120` | `NewTicker` panics on interval ≤ 0 in a goroutine → a future `libvirtDialer{}` literal kills the operator on first dial | default the interval when ≤ 0 | +| L2 (S1.4) [T1] | `errors.go:45-51` | `*knownhosts.RevokedError` unrecognised → a revoked host key retries forever | classify it Permanent | +| L3 (S1.5) | `ssh.go:167-172` | `HostKeyAlgorithms` empty → a host pinned by RSA only but serving ed25519 reports a Permanent "mismatch" | on `KeyError` with `Want`, retry with those key types; or document `ssh-keyscan` without `-t` | +| L4 (S2.4) | `create.go:87` | Adoption path starts a defined domain without checking its disks → missing overlay/seed retries forever (E5's EIO residual lands here) | on a storage error in the adoption branch, undefine and fall through to a full create | +| L5 (S2.5) [T1] | `delete.go:15` | `undefineFlags` lacks `CheckpointsMetadata` → an admin/backup checkpoint wedges the finalizer | add the flag | +| L6 (S3.4) | `get.go:104-119` | `ErrNoDomain` swallowed → a vanished domain reads as Provisioning for one poll; comment about ctx-only errors is wrong (a budget expiry never falls back to ARP) | return NotFound; fix the comment | +| L7 (S4.7) | `image.go:62` | URL images assumed qcow2/uefi → a raw URL image fails to boot with a misleading error | sniff the qcow2 magic from the first bytes | +| L8 (S4.8) [T3] | `volume.go:22-25` | 64-bit URL key, namespace shared with catalogue keys → colliding URLs or a 16-hex catalogue key share a base | longer key and a distinct `url-` prefix | +| L9 (S5.3, narrowed by E2) | `metadata.go:44-50` | future-dated mark → a true orphan is never collected. (The zero-time half is benign: `gc.go:118` checks the UID first.) | clamp future timestamps to now | +| L10 (S5.4) | `seed.go:48-53` | `rc-update` fallback unguarded under `set -e` → agent installed but not started on non-systemd, non-OpenRC images | `elif command -v rc-update`, as the kvm provider does | +| L11 (S5.5) | `xml.go:178-193` | `VCPUs`/`MemoryMiB` not validated in the builder (latent) | reject ≤ 0 | +| L12 (S5.7) | `internal/provider/config_kvm.go:186-188` | `bridge`/`pool` only non-empty → opaque libvirt error at Create | apply `kvmIdentifier`-style validation | +| L13 (S5.8) | `seed.go:107-108` | multipart or headerless configured `vendorData` silently dropped with `installGuestAgent` | reject at load what cloud-init will not recognise | +| L14 (S6.1) | `check.go:90`, `backend.go:162,351` | `Close()` can send `ConnectClose` for up to 5 s after the round deadline | `markDead` when `call` refuses on a done ctx, or skip the RPC in Close then | +| L15 (S6.2) | `check.go:57-58` | Latency includes the teardown RPC | stop the clock before Close | +| L16 (S6.3) | `check.go:66` | round-wide `ctx.Err()` masks a per-host reason landing on the deadline | classify the error first; Timeout only if it carries the ctx error | +| L17 (S6.4) | `check.go:135-148`, `internal/providerhealth/monitor.go:113` | a 10 s round timeout collapses Unreachable into Timeout | validate round timeout > dial budget, or shrink the dial budget to fit | +| L18 (S6.5) | `check.go:101` | unbounded remote `hostname` in status `details` → status patch "too large" | truncate; add `maxLength` to the CRD | +| L19 (S6.6) | `check.go:41-45` | unbounded per-host fan-out, key re-read every dial | cap concurrency; cache credentials | +| L20 (S7.6) [T1] | `internal/provider/config_kvm.go:133`, `image.go:466` | no upper bound on `diskSizeGb`; `neededBytes` wraps at 2^34 → precheck passes; huge `memoryMiB` retries forever | bound both at load | +| L21 (S7.7) | `fake_test.go:190-208` | fake accepts calls on a done ctx → `discardVolume`'s `WithoutCancel` is untested | make `enter()` refuse like `call()` | +| L22 (S7.8) | `fake_test.go:579-584` | fake `Close()` leaves the backend live → use-after-close passes in tests | mark dead on Close | +| L23 (S7.9) | `Makefile:60-75`, `.gitea/workflows/build.yaml:27-32` | nothing compiles `-tags=integration` | add `go vet -tags=integration ./...` to the `check` job | +| L24 (S7.10) | `image.go:228,283-289,385-395` | full image URL, including any userinfo, in logs and wrapped errors | use `urlHost`/redacted URL everywhere; reject userinfo at load | +| L25 (S7.11) | `kvm2_integration_test.go:89,307` | `KVM2_TEST_KEEP=0` enables keep; 100 ms wall-clock assertion on `New()` | parse the bool; drop or loosen the timing check | +| L26 (S7.12) | `config/manager/manager.yaml:163-178`, `seed.go:56-74` | cloud-init payloads staged on node-local disk; comment names kvm only | `medium: Memory`; fix the comment | +| L27 (E6 side) | go-libvirt `socket/socket.go:233-234` | packet length < 28 panics the listen goroutine and the operator; a huge length allocates up to 4 GiB → `host.Socket` pointed at a non-libvirt socket (reproduced) | upstream bounds check; until then validate the socket in Check before pooling | +| L28 (E6 side) | go-libvirt `rpc.go:76-84` | duplicate reply deadlocks `callback` under `cmux`, leaking 3 goroutines that survive a socket kill (measured +420/200 cycles); same mechanism keeps H2's leak alive | upstream: send outside the lock | + +--- + +## Appendix A — PLAUSIBLE, with what would settle it + +- **E2 — a padded or case-changed `` in the GC mark makes GC delete a live Proxy's VM.** Reproduced on a verbatim copy of the parser and the `gc.go` switch; there is no second guard before `Delete`. No trigger libvirt produces on its own was found (it re-indents between children only), so it needs a hand edit or a reformatting tool. The kvm provider has the same field, and the two live VMs are kvm v1. *Settle:* the `virsh metadata --set` round trip in `deep.md` on a scratch libvirt. *The fix is three `TrimSpace` lines and worth making regardless.* +- **E7 — in-place base replacement corrupts existing overlays.** The delete is confirmed (M13). Corruption needs a new QEMU process on an old overlay; the controller never does that, but `libvirt-guests` is enabled on beelink and restarts guests after a host reboot. *Settle:* `grep -Ev '^\s*(#|$)' /etc/default/libvirt-guests` on beelink, and the `review-e7-*` inode probe in `deep.md`. +- **Unexecuted links inside confirmed findings.** H4-3b: libvirt allows `vol-delete` during an upload (read in 11.6.0 source; the `probe-race`/`e4probe` scripts). M5: libvirt passes a short capacity through (~90%; `review-e10-short`). H1: the real path's send buffer is under ~2.2 MB (`ss -tmi` during an upload). E9a: duplicate volume → code 90 (~95%; `review-e9-dup`). + +## Appendix B — REFUTED + +- **E5** overlay left corrupt by an abandoned `StorageVolCreateXML`: libvirtd finishes or unlinks the build regardless of the client (~85%, source memory); creation is 2-11 ms on the host; the residual path fails loudly. +- **E3-1** GC reconciling one provider's list against another's Proxies: the live set is global and the list is namespace-filtered (observed on the host). +- **E3-2** re-type destroying *another* Proxy's instance: the ID always names the Proxy's own UID-derived domain (the remaining hole is M2). +- **E4 rollout/leader handover** deleting a base mid-upload: the lease is held until the old pod dies; SIGKILL at 10 s; the residual window is theoretical and self-healing. +- **E6b** goroutine/fd accumulation from per-round check dials: +0/+0 over 1000 cycles on three transports. +- **E9a** idempotency codes differing from the fake: 42, 49, 50, 80 observed on libvirt 11.6; 90 and 55 sourced at high confidence. +- **E2 (half)** an unparsable `createdAt` deleting a live VM: `gc.go:118` checks the UID first. +- **E7 (half)** marker-missing trigger without an admin edit: marker errors are returned, not treated as unreadable. +- **No XML injection**: three sweepers independently found every value goes through `encoding/xml`. + +## Appendix C — Out of scope but noticed + +- **Orphan volumes on beelink, pool `default`:** `proxy-7hjzuf3d3w2mjnye.qcow2` + `-seed.iso`, `proxy-qxge3dbm2mnpc3ih.qcow2` + `-seed.iso`, `proxy-sbs36ruo73sxxtqc.qcow2`. 196 KiB each, root-owned, no domain. Reported independently by four deep reviewers; they are M3 in the wild (likely from kvm v1 integration runs). Nothing was deleted. +- **The kvm2 integration test has not run on beelink** (plan execution Step 11 unchecked). +- **The kvm provider shares most of these defects:** untrimmed mark UID (E2), no mark check on Get/Delete and adoption of any same-named domain (`kvm/kvm.go:177-181`), short-overlay hole (`kvm/volume.go:24-32`), same base names. +- **No provisioning deadline in the controller:** any VM that boots but never reports an address stays Provisioning forever with no signal (what makes M5 silent). +- **`providerFailure` sets no condition for Transient errors** (`proxy_controller.go:344-345`), which is what makes all of T1 invisible. +- **Manager wiring:** no `ReconciliationTimeout`, no `LeaderElectionReleaseOnCancel`, default RollingUpdate with one replica, 128Mi memory limit (an OOM mid-upload is one way to H4's truncated base). +- **Host key naming:** `~/.ssh/kvm_known_hosts` pins beelink under its Tailscale FQDN only, while the sample config says `address: beelink:22`; an operator using that file would fail host key verification (seen from this workstation; not verified from inside the operator). +- **Dropped escalations not investigated:** D1 delete-during-open-stream disk-space effect, D2 cloud-init `text/plain` vendor-data dispatch, D3 ProviderHealth status churn, D4 nested `kvm-ssh` volumeMount under the read-only config mount, D5 whether error strings with image URLs reach Proxy status. diff --git a/review/deep.md b/review/deep.md new file mode 100644 index 0000000..bebdfc0 --- /dev/null +++ b/review/deep.md @@ -0,0 +1,265 @@ +# kvm2 provider review: phase 2 deep dives + +Ten `deep-reviewer` runs, one per escalation in `escalations.md`. Hypervisor access was +**read-only** (beelink, libvirt 11.6.0, QEMU 10.1.0); every probe that needs a write was left +unrun and is reproduced under "Probe to settle". Scratch programs live under the session +scratchpad, not in the repo. The orchestrator did not re-read code. + +| ID | Verdict | One line | +|---|---|---| +| E1 | **CONFIRMED** (narrowed) | Blocking kill reproduced against x/crypto v0.54.0; pins all 3 reconcile workers ~15 min per incident, trigger is a bulk upload, not the name lock | +| E2 | **PLAUSIBLE** | Padded `` text does make GC delete a live VM (reproduced on a copy of the parser); no trigger libvirt produces on its own. `createdAt` half REFUTED | +| E3 | **mixed** | GC cross-provider: REFUTED. Re-type beyond S2.3: REFUTED. Shared `base-.` cache between kvm and kvm2: CONFIRMED | +| E4 | **CONFIRMED** (narrowed) | Rollout/leader-handover trigger REFUTED; the unfenced base is reachable in one process via the kvm provider or a second kvm2 entry on the same host+pool | +| E5 | **REFUTED** (~85%) | libvirtd finishes or unlinks a volume build regardless of client disconnect; overlay create is 2-11 ms on the host | +| E6 | (a) **CONFIRMED**, (b) **REFUTED** | Upstream race on `l.disconnected` is real; a peer-close right after ConnectOpen wedges a pooled link forever. No goroutine/fd growth over 1000 cycles | +| E7 | **PLAUSIBLE** | Unguarded in-place delete is real; corruption needs an admin `sha256` edit plus a host-side restart of an old overlay (`libvirt-guests` is enabled on beelink) | +| E8 | **CONFIRMED** | `available` is cached per pool in libvirtd (measured), and it is `f_bfree`, 24.7 GB above what QEMU can use on the kvm2 filesystem | +| E9 | (a) **REFUTED**, (b) **CONFIRMED** | Idempotency codes match libvirt 11.6 (observed 42/49/50/80). Codes 67 and the bare `ErrUnsupported` retry forever, per a recorded spec decision | +| E10 | **CONFIRMED** (libvirt half ~90%) | qemu-img accepts a short overlay (measured locally); guest sees a truncated disk, GPT rejected; Proxy stuck Provisioning forever | + +Two facts the deep dives surfaced that correct earlier documents: +- **The kvm2 integration test has not run on beelink.** Step 11 is unchecked in `docs/plans-executions/2026-09-16-1333-kvm2-provider.md`; every `base-*` volume on the host is dated 2026-09-10 (kvm v1's test, same `test-image` key); both live `proxy-*` domains carry the kvm v1 metadata namespace. The "Checked" line of E2 in `escalations.md` (round trip asserted on libvirt 11.6) was a sweeper claim and is not yet true. +- **Sweep finding S5.3 is half wrong.** A zero `createdAt` does not endanger a live VM: `gc.go:118` tests `live[inst.UID]` before the age case, so it only affects true orphans. The future-dated-mark leak stands. + +--- + +## E1 — wedged `markDead` starves the reconcile workers — CONFIRMED (narrowed) + +**Premise reproduced** (`scratchpad/e1/main.go`, in-process SSH server behind a freezable TCP proxy, x/crypto v0.54.0). `sshSocket.Close` (`ssh.go:109-114`) calls `s.Conn.Close()` first: `channel.Close` (`channel.go:594`) → `sendMessage` → `writePacket` takes `ch.writeMu`, then `handshakeTransport.writePacket` (`handshake.go:579`) takes `t.mu`. A data writer stuck in a TCP write holds both. `ssh.Client.Close` is a bare TCP close (`connection.go:111`) and takes no locks. + +| Scenario | Client send buffer | Writer blocked in | Channel `Close` | +|---|---|---|---| +| Bulk write, loopback default | 2.6 MB | `window.reserve` (no mutex) | 0 s | +| Bulk write, LAN-sized buffer | 256 K | `net.(*conn).Write` under `writeCipherPacket` | still blocked after 8 s | +| Small RPC-sized write only | 2.6 MB | not blocked | 0 s | + +Precondition: the writer must be stuck in the TCP write, not SSH flow control (2 MiB window). That needs a send buffer below ~2 MiB, which the reviewer estimates (not measured) is the case on a LAN/Tailscale path during a bulk upload. In the blocked case a separate `client.Close()` released the channel close at once. + +**Breaking path.** +1. Three Proxies placed on one kvm2 host with an uncached image. Worker 1 takes the name lock (`create.go:53`) and image lock (`image.go:87`), then `uploadImage` → `dialFresh` (`image.go:261`) → `StorageVolUpload` (`image.go:270`, 20 min budget). Workers 2 and 3 park in `imageLocks.acquire` (`kvm2.go:224-233`); the reconcile ctx has no deadline. `MaxConcurrentReconciles` is 3 (`proxy_controller.go:442`): all workers occupied. +2. Host is power-cut mid-upload. The upload goroutine blocks in the TCP write holding both mutexes. +3. The dedicated link's keepalive (`hosts.go:156-160`) cannot send (needs `t.mu`); after 15 s `hosts.go:158` calls `markDead`, whose `kill()` blocks in `s.Conn.Close()`. `close(l.closed)` (`backend.go:146`) is never reached. +4. Worker 1 sits in `call`'s select (`backend.go:176-187`): `done` cannot fire, `l.closed` is not closed, and a ctx cancel reaches `markDead` at `:180`, which blocks in `once.Do` behind the keepalive goroutine. Even cancellation is uninterruptible. +5. After ~15.5 min (`tcp_retries2=15`) or the 20 min budget everything unwinds Transient. Meanwhile no Proxy on any provider reconciles. + +**Unknowns settled.** No `ReconciliationTimeout` is set (`cmd/main.go:288-295`; controller-runtime v0.24.1 applies one only when > 0), and one alone would not release worker 1. The name-lock part of the hypothesis is practically unreachable (controller-runtime never runs one key concurrently; Delete's RPCs are too small to fill the buffer). GC (`gc.go:104-130`, one goroutine, no deadline) normally moves on; it stalls only if the blocked kill is on the *pooled* link, whose only bulk write is the ~50 KiB seed upload (`create.go:210`): marginal. Of the five `markDead` sites, `hosts.go:153` runs under `e.sem` but only handshake bytes have been sent, so it does not block. + +**Minimal fix.** (1) In `sshSocket.Close`, close the TCP connection first: `errors.Join(s.client.Close(), s.Conn.Close())`, filtering the expected "use of closed network connection", or drop `Conn.Close()`. (2) In `markDead` (`backend.go:138-148`), `close(l.closed)` before `l.kill()`, or run `kill` in a goroutine. (3) Regression test in `ssh_test.go`: frozen proxy, small client `SO_SNDBUF`, assert `Close` returns within 1 s while a writer is blocked. (4) Optional: set `ReconciliationTimeout` so image-lock waiters requeue. + +**Probe to settle** (not run, needs a bulk upload in flight): `ss -tmi dst :22` on the operator host during an image upload; `tb` below ~2.2 MB means the blocking case applies. + +## E2 — reformatted GC mark makes GC delete a live VM — PLAUSIBLE + +Throwaway program (`scratchpad/e2/main.go`) with a verbatim copy of `instanceMetadata`, the same `time.Parse(time.RFC3339, …)` and the `gc.go:112-122` switch; clock 16 days after the mark, MinAge 10 m. + +| Input | Parsed UID | zero time | GC | +|---|---|---|---| +| single line (as written) | clean | no | KEEP | +| libvirt's re-indent between children | clean | no | KEEP | +| ` X ` or newline-padded | padded | no | **DELETE** | +| uppercase UID | differs | no | **DELETE** | +| ` ` (whitespace only) | `" "` | no | **DELETE** | +| `` | `""` | no | KEEP (guard `gc.go:113`) | +| bad/missing `createdAt`, UID clean | clean | yes | KEEP | +| padded `` | clean | no | KEEP (not listed) | +| future-dated, UID clean | clean | no | KEEP | + +`encoding/xml` does not trim chardata. **Path:** padded or case-changed `` with `true` intact → `parseInstanceMetadata` (`metadata.go:66-78`) returns it untrimmed → `ListByTag` (`list.go:46-49`) emits it → `Sweeper.sweep` (`gc.go:113-121`): not empty, not live, older than MinAge → `prov.Delete` (`gc.go:125`) → `delete.go:21-53` destroys the domain and its disks. There is no second guard: no name-to-Proxy lookup, `Delete` does not re-read the mark, and provider scoping does not help because the owning provider lists it. + +**Why only PLAUSIBLE.** On the real host libvirt puts newline + two spaces *between* children, strips the xmlns and leaves text bytes intact (`od -c` of the live `…/kvm` marks). Per libxml2, text-bearing elements are never re-indented, so a version change alone would not pad text. The reachable trigger is a hand edit or a tool that pads text nodes via `virsh edit`/`define`: rare, but the result is an unrecoverable delete. The old `internal/provider/kvm/metadata.go` has the same untrimmed field, and the two live VMs are kvm v1 domains. + +**Refuted parts.** A bad `createdAt` never deletes a live VM (UID check comes first, consistent with `docs/provider-contract.md:238-241`). A future-dated mark is a leak only, and marks are stamped from the operator's clock (`create.go:127`), so host skew cannot cause it. + +**Minimal fix.** At the end of `parseInstanceMetadata`: `TrimSpace` on `Managed`, `UID` (plus `ToLower`), `CreatedAt`. A whitespace-only UID then equals `""`, which `gc.go:113` keeps. Add table rows to `metadata_test.go`. Apply the same to `internal/provider/kvm/metadata.go`. Optional second guard in `gc.go`: skip and log when `provider.NameFromUID(inst.UID)` is not the name part of `inst.ID` (check the other providers' naming first). + +**Probe to settle** (not run; use a scratch libvirt, not beelink): +``` +virsh -c qemu:///system metadata https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/kvm2 --key kvm2 --set 'true 3f1c2a9e-0000-4000-8000-000000000001 2026-09-01T10:00:00Z' --config +virsh -c qemu:///system metadata https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/kvm2 | od -c +``` + +## E3 — kvm and kvm2 on the same hypervisor — three paths, three verdicts + +**1. GC reconciles provider A's list against provider B's Proxies — REFUTED.** `gc.go:91-101` builds one global `live` set from every Proxy; sharing it only makes GC more conservative. kvm2 `ListByTag` (`list.go:38-48`) is namespace-filtered, and that was checked on the host: both live domains return the mark under `…/kvm` and nothing under `…/kvm2`. What remains is leaks: after a re-type the old type's orphans are never collected, and the scoping still rests on libvirt behaviour no unit test covers (S7.4). + +**2. Same UID under both types; reverse direction — REFUTED beyond S2.3.** `NameFromUID` depends only on the UID and `spec.provider` is CEL-immutable (`proxy_types.go:221`), so the same name under both types only comes from re-typing an entry. Nothing prevents that: `config.go:93-135` checks only name uniqueness and block/type agreement, there is no webhook (`cmd/main.go:262`), `status.providerID` records no type, and the controller never compares `inst.UID` with `p.UID`. What gets deleted is still that Proxy's own instance. Exceptions: a tampered `status.providerID` (kvm2 `Delete` checks no mark, so `hosts/beelink/domains/docker-29` would destroy an unrelated VM); the reverse direction has no guard at all (kvm `Create` adopts any same-named domain, `kvm/kvm.go:177-181`; its `Get`/`Delete` check no mark); and after a re-type kvm2 `ensureOverlay` boots a stale overlay the kvm provider left behind (`create.go:171-177`), one Proxy's own disk only. + +**3. Shared `base-.` cache — CONFIRMED** (kvm2 code + libvirt 11.6.0 source, `storage_driver.c`). The marker protocols are compatible (both write `.sha256` with 64 hex; both key URL images as `sha256(url)[:16]`), so kvm2 adopts a kvm-cached base. On the host every base and marker dates from 2026-09-10 (kvm-made), both live overlays back onto `/srv/vms/images/base-debian-13.qcow2`, and the sample `kvm2-home` entry's `debian-13` key would share exactly that volume. Both integration tests use key `test-image`. +- **3a Digest mismatch.** `kvm-home` cached `debian-13` from a `latest` URL (digest D1). The operator gives `kvm2-home` `sha256: D2`. First kvm2 Create: `cachedImage` `image.go:129` → `:145` deletes marker and base. libvirt's `storageVolDelete` refuses only on `in_use`/`building` (`storage_driver.c:1844-1856`), not on backing-store use. kvm2 uploads a different image to the same path under the kvm provider's live overlays. With both entries pinned to different digests, every Create on either side deletes and refetches. +- **3b Concurrent cold fetch** (by source, not executed). Image locks are per `Provider` instance (`kvm2.go:111`; `kvm/image.go:62-74`), so this applies kvm-vs-kvm2 and kvm2-vs-kvm2 alike. libvirt holds `in_use` only while the stream is set up (`storage_driver.c:2421-2427`), so the volume is unguarded for the whole transfer. Entry B finds base-without-marker and deletes it (`kvm2/image.go:117-118`; `kvm/image.go:103-108`), recreates it and starts its own upload. A's upload finishes cleanly into the unlinked inode, the digest matches, A writes the marker, and `StorageVolGetPath` resolves A's handle to B's partial file: A boots a guest from a half-written base. If B is kvm2, `writeMarker` then fails `ErrStorageVolExist` and `image.go:241-243` discards the now-complete base under A's running domain. + +**Minimal fix.** Give kvm2 its own prefix (`volume.go:17` → `kvm2-base-…`, one extra ~415 MB download per host). Reject two kvm/kvm2 entries with the same address+socket+pool at load, or treat `ErrStorageVolExist` on the marker as success when its digest equals ours. Put the digest in the base name when `sha256` is set. Make `removeDomain` require `meta.managed()` and the expected UID; give `kvm/kvm.go:177` the same guard. Add a fake keyed on the metadata URI (S7.4). + +**Probe to settle 3b** (not run): +```sh +truncate -s 4G /tmp/probe-race.bin +virsh -c qemu:///system vol-create-as default probe-race.raw 4G --format raw +virsh -c qemu:///system vol-upload --pool default probe-race.raw /tmp/probe-race.bin & +sleep 1; virsh -c qemu:///system vol-delete --pool default probe-race.raw; echo "delete exit=$?" +wait; echo "upload exit=$?"; virsh -c qemu:///system vol-list default | grep probe-race; rm /tmp/probe-race.bin +``` + +## E4 — no cross-process fencing on the base volume — CONFIRMED (narrowed) + +**Rollout / leader handover: REFUTED for the shipped manifests.** `manager.yaml:64` passes `--leader-elect` (the `false` default at `cmd/main.go:107` applies only to a bare binary or `make run`). No strategy is set, so RollingUpdate with surge 1: old and new pods coexist, but the reconciler is a leader-election runnable, `LeaderElectionReleaseOnCancel` is not set (`cmd/main.go:288-295`), and the old pod holds the lease until it dies. Lease 15 s / renew 10 s / retry 2 s; the kubelet SIGKILLs at 10 s (`manager.yaml:180`). On cancel kvm2 stops the upload at once (`backend.go:179-181` → `markDead`); `discardVolume` runs uncancelled for up to 30 s (`image.go:331`) but is cut by SIGKILL. The residual two-process window needs A's delete RPC to stall ~7 s while B dials, HEADs and creates within ~5 s: theoretical, and self-healing (`image.go:100`, `:135`). Other two-process cases: `make run` without `--leader-elect` against the cluster's hypervisor; `make test-integration-kvm` and `-kvm2` at the same time (same `test-image` key). + +**In one process: CONFIRMED.** The kvm provider is an always-present second writer: same names (`kvm/image.go:44-57` vs `kvm2/volume.go:17-25`), same default pool, same marker format, separate locks (`kvm/image.go:77-87` vs `Provider.imageLocks`; `newProvider` builds fresh `keyedLocks` per entry, `kvm2.go:111`, so two kvm2 entries do not share one either), concurrent via `MaxConcurrentReconciles: 3`, documented as supported (`docs/kvm2-provider.md:6-8`) and shipped that way (`config/manager/providers_config.yaml`: both entries on `beelink`, key `debian-13`, default pool). The interleaving is E3-3b. Extra consequence: after A's marker is written, if the process dies during B's upload (rollout SIGKILL, OOM at the 128Mi limit) the pool keeps a **truncated base under a valid marker**, and nothing re-hashes the base (`cachedImage` compares marker text only), so every later Create on that host backs onto it until a manual `vol-delete`. +Caveat: that `StorageVolDelete` succeeds mid-stream came from memory here; the E3 reviewer read it in the 11.6.0 source, which corroborates it. Still unexecuted. + +**Minimal fix.** (1) Remove the shared name (kvm2-specific prefix; include the entry name or share one process-wide `keyedLocks`). (2) Make the base appear atomically: upload to `.part-`, verify, then create the final name; treat `ErrStorageVolExist` as "someone else won" and re-check; never delete a name this call did not create, in particular not `discardVolume(base)` on a marker `ErrStorageVolExist` (`image.go:241-243`). (3) Store the size next to the digest and check the base's allocation before trusting the marker. Set `strategy: Recreate` and `LeaderElectionReleaseOnCancel` deliberately. + +**Probe to settle** (not run): +``` +virsh -c qemu:///system vol-create-as default e4probe.raw 2G --format raw +mkfifo /tmp/e4.fifo; ( head -c 2G /dev/zero | pv -L 20m > /tmp/e4.fifo & ) +virsh -c qemu:///system vol-upload --pool default e4probe.raw /tmp/e4.fifo & +sleep 5 +virsh -c qemu:///system vol-delete --pool default e4probe.raw # succeeds => confirmed path; "still in use" => narrowed +virsh -c qemu:///system vol-create-as default e4probe.raw 2G --format raw +virsh -c qemu:///system pool-refresh default +wait; virsh -c qemu:///system vol-delete --pool default e4probe.raw; rm /tmp/e4.fifo +``` + +## E5 — overlay atomicity under an abandoned RPC — REFUTED (~85%, libvirtd side from source memory) + +`storageVolCreateXML` rejects a duplicate name, adds the volume with `building = true`, bumps asyncjobs, drops the pool lock, runs `qemu-img create` via `virCommandRun`; on build failure the file is unlinked and the volume removed from the list. It runs on an RPC worker thread holding a client reference, so a disconnect does not cancel it; storage objects have no close callbacks. After a client drop a name is either still building or complete. Pool refresh refuses while asyncjobs > 0, and kvm2's XML has no `` and passes `flags = 0`, so the preallocating path is never taken. The spec's "creation is atomic" (`kvm2-provider-spec.md:227-229`) holds. + +**Cost on the host.** All six pools are `type='dir'` on local storage. Three leftover overlays show birth-to-mtime gaps of 2-11 ms and are each exactly 196768 bytes, complete. A create over 30 s needs a stalled filesystem. + +**Residual path** (loud, never boots a corrupt overlay): Create #1 stalls > 30 s, Create #2 gets `ErrStorageVolExist` while `building` is set; if it reaches `DomainCreate`, QEMU cannot open the file (write lock, or "not in qcow2 format") → Transient → next retry starts cleanly. If `qemu-img` ends in EIO, libvirtd removes the volume and the defined domain points at a missing file; every retry fails until the Proxy is deleted (this is sweep finding S2.4's mechanism). + +**Fix options.** None needed for the hypothesis. Minimal: correct the comment at `create.go:151-153` and the spec line to state the real reasons. Hardening: `deleteVolumeIfPresent` before `StorageVolCreateXML` in `ensureOverlay`, as the seed already does (no domain exists at that point under the name lock; cost 2-11 ms; a building volume returns `ErrOperationInvalid`, already Transient). + +**Probe to settle** (not run, ~420 MiB in `default`): +```sh +C=qemu:///system; V=review-e5-clone.qcow2 +virsh -c $C vol-clone --pool default base-debian-13.qcow2 $V & P=$! +sleep 0.3; kill -9 $P +for i in $(seq 1 40); do virsh -c $C vol-info --pool default $V 2>&1 | tr '\n' ' '; echo; stat -c '%s' /srv/vms/images/$V 2>&1; sleep 0.5; done +virsh -c $C vol-dumpxml --pool default $V +until virsh -c $C vol-delete --pool default $V 2>&1 | grep -qv 'still being allocated'; do sleep 1; done +virsh -c $C vol-list default | grep review-e5- || echo clean +``` + +## E6 — go-libvirt when the socket is killed under it — (a) CONFIRMED, (b) REFUTED + +Scratch module `scratchpad/lvleak/` (same go-libvirt and x/crypto versions; copies kvm2's `link`, `call`, `connect`, `Close`, `sshSocket` in shape). + +**(a) Data race on `Libvirt.disconnected`, reported by `-race`** over a real in-process SSH `direct-streamlocal` channel: read in `waitAndDisconnect` (`libvirt.go:821`, goroutine started at `:297`), write in `ConnectToURI` (`:305`), unsynchronised. Interleaving: ConnectOpen reply is routed → socket dies → `listen` closes `s.disconnected` (`socket.go:208`) → `waitAndDisconnect` reads the old channel and returns (`:828`) → the requester installs a fresh channel at `:305` that nothing will ever close, and `Connect()` returns nil. +- **Hypothesised trigger (kvm2's own `markDead`): harmless.** `call` has already taken the timer/ctx branch; the nil result lands in the buffered `done` and is dropped; `hosts.go:153` discards the backend. Measured 5, 1 and 33 times per 1000 cycles, no effect. +- **Peer-close trigger: real defect.** If libvirtd or the forwarded socket goes away within ~1 ms of a successful open (e.g. a daemon restart), `Connect` wins, the watcher (`backend.go:229-236`) holds a channel that never closes, every later RPC fails at `SendPacket` with `syscall.EINVAL` (`socket.go:321-325`), which `errors.go:55` classifies Transient. No call result marks the link dead, `hostTable.get` (`hosts.go:75`) keeps returning it, and the SSH keepalive keeps succeeding if sshd is alive. The host answers "invalid argument" until the operator restarts. Wedged in 953/1000 (unix), 824/1000 (`net.Pipe`), 546/1000 (SSH), 186/500 (SSH, `-race`) when the peer closes right after the open reply. On the Check path the same loser only costs a 5.001 s `Close()`. + +**(b) No accumulation.** Six scenarios (peer never answers then kill; RPC hangs; ConnectClose hangs into the close budget; peer closes mid-handshake; clean probe; kill racing the end of the handshake) x three transports, 1000 cycles each (500 under `-race`): goroutines +0, fds +0. Peak in flight 26. + +**Side findings (need a misbehaving peer).** (i) `callback` does an unbuffered `c <- res` holding `cmux` (`rpc.go:76-84`): a duplicate reply deadlocks `listen`, leaks three goroutines per hit (+420 over 200 cycles), and a socket kill does not help. On the stream path serials do repeat, which is why the S4.1 leak also survives a kill. (ii) `socket.go:233-234` computes `int(length) - 28` and `make`s it: a length field under 28 panics "makeslice: len out of range" in the listen goroutine and takes the operator down (reproduced); a large bogus length allocates up to 4 GiB. Reachable if `host.Socket` points at something that is not libvirt. + +**Minimal fix.** In `call` (`backend.go:177-178`): on `done`, if the error is `syscall.EINVAL` or `libvirt.ErrInterrupted`, `markDead` before returning; add a `backend_test.go` case where the peer closes right after the open reply. Sturdier: give `libvirt.New` a `net.Conn` wrapper whose `Read` calls `link.markDead` on any non-temporary error, dropping the dependence on `Disconnected()` and the 5 s Close stall. Upstream: assign `l.disconnected` before `go l.waitAndDisconnect()`; send outside the lock in `callback`; reject `length < 28` and oversized lengths. + +## E7 — in-place base replacement under live overlays — PLAUSIBLE + +**Confirmed part.** Admin sets or changes `images..sha256` and restarts the manager (config is not hot-reloaded). Next Create: `image.go:129` sees the mismatch, `:145` deletes marker and base with no in-use check and a `V(1)` log; `fetchImage` creates a new file at the same path. `TestEnsureImage_sha256ChangeRedownloads` (`image_test.go:394`) asserts exactly this. A mistyped `sha256` is worse: the base is deleted first, then `image.go:289` returns Permanent, leaving every old overlay with a missing backing file (loud). libvirt does not refuse (~95% from the reviewer; the E3 reviewer read it in source). + +**Why corruption is only plausible.** Running VMs keep the old inode, and `on_reboot=restart` (`xml.go:217`) keeps the same QEMU process. Corruption needs a *new* QEMU process on an old overlay: +- Controller: safe. Stopped → Delete (`proxy_controller.go:216-225`) removes the overlay (`delete.go:91`). +- **`libvirt-guests` on host reboot.** Enabled on beelink (`multi-user.target.wants/libvirt-guests.service`); `/etc/default/libvirt-guests` could not be read. Stock `ON_BOOT=start` restarts every guest that was running at shutdown regardless of autostart; the reviewer is ~70% sure the distro default is also `ON_SHUTDOWN=suspend`. While the host is down `Get` is Transient, not Stopped, so nothing is deleted; after boot the operator normally sees Running and keeps the VM. +- A manual `virsh start`. +- Create adoption path (`create.go:73-87` calls `ensureRunning` without `ensureImage`): only when `providerID` is empty and the overlay already holds guest writes. Narrow. + +The missing/unreadable-marker trigger is **not** reachable within kvm2 without an edit (marker errors are returned, not treated as unreadable, `image.go:119-125`). The second deleter that does exist is the kvm provider (E3/E4). Host state: both live domains running, persistent, autostart disabled, no managed save; the hazard is documented (`kvm2-provider-spec.md:91-93`, `kvm2-provider.md:313-318`), but the code contradicts the "add a new key" rule instead of enforcing it, and the guide's warning names `url` although a `url` change alone never refetches. + +**Minimal fix.** At `image.go:129-130` do not delete; return `permanentf("base %s is cached with sha256 %s but %s is configured; add the image under a new key, …")`. Keep deletion for the no-marker and non-hex branches. Alternative: digest in the volume name. Either way log at `V(0)`. + +**Probe to settle** (not run): +``` +P=default; V="virsh -c qemu:///system" +$V vol-create-as $P review-e7-base.qcow2 16M --format qcow2 +$V vol-create-as $P review-e7-overlay.qcow2 16M --format qcow2 --backing-vol review-e7-base.qcow2 --backing-vol-format qcow2 +stat -c '%i' /srv/vms/images/review-e7-base.qcow2 +$V vol-delete --pool $P review-e7-base.qcow2 # expected: succeeds +$V vol-create-as $P review-e7-base.qcow2 16M --format qcow2 +stat -c '%i' /srv/vms/images/review-e7-base.qcow2 # expected: different inode +$V vol-delete --pool $P review-e7-overlay.qcow2; $V vol-delete --pool $P review-e7-base.qcow2 +``` +Read-only, for the owner: `grep -Ev '^\s*(#|$)' /etc/default/libvirt-guests` (empty output = defaults apply). + +## E8 — pool-space precheck freshness — CONFIRMED, and staleness is the smaller of two errors + +Measured 2026-09-17 08:28-08:30 UTC, read-only. kvm2's pool is `default` at `/srv/vms/images`. + +| pool | filesystem | libvirt Available | `f_bfree` | `f_bavail` | libvirt − bfree | libvirt − bavail | vols | +|---|---|---|---|---|---|---|---| +| default (kvm2) | ubuntu--vg-vms--lv | 266,038,706,176 | 266,038,378,496 | 241,304,276,992 | +327,680 | **+24,734,429,184** | 24 | +| pool | same | 267,718,541,312 | 266,038,378,496 | 241,304,276,992 | +1,680,162,816 | +26,414,264,320 | 15 | +| vms | same | 267,718,541,312 | 266,038,378,496 | 241,304,276,992 | +1,680,162,816 | +26,414,264,320 | 9 | +| boot / images | ubuntu--lv | 73,877,696,512 | 73,918,029,824 | 68,532,543,488 | −40,333,312 | +5,345,153,024 | 0 / 2 | +| vms-2 | nvme1n1p1 | 820,622,606,336 | 820,602,261,504 | 770,575,294,464 | +20,344,832 | +50,047,311,872 | 2 | + +- **Cached per pool.** Three pools on one filesystem report two different values; a call-time value could not differ. Pool A's allocation is invisible to pool B. Over 80 s `f_bfree` moved on the `images` filesystem while libvirt stayed frozen, 40 MB stale in the pessimistic direction, so the cache errs both ways. `default` is nearly exact only because libvirt touched it ~1.6 h earlier. +- **The root reserve.** libvirt reports `f_bfree`, which includes ext4's 5% reserve. Overlays are owned by `libvirt-qemu:kvm`, so guest writes stop at `f_bavail = 0` while libvirt still reports ≥ 24.7 GB. + +**Failing path.** `create.go:92-100`: pool lookup → `imageSizeIfUncached` → `checkPoolSpace`, no refresh; `image.go:450-458` compares raw `available`; the only refresh is `image.go:235` after an upload, which never runs again once the base is cached. Overlays start at ~196 KB and grow invisibly (live ones at 1.47 and 1.54 GB of 10 GiB), as does `kube-node-33.qcow2` (50.6 GB) in the same directory. Reserve scenario (deterministic here): with `f_bavail` between 0 and ~10.7 GB the precheck passes for a 10 GiB disk, `ErrQuotaExceeded` never fires, and Create, the upload and the guests hit ENOSPC, the condition behind S4.1's wedge and S2.2's paused guests. `check.go:106-112` has the same defect: `poolAvailableGiB` over-reports by ~23 GiB plus drift. + +**Minimal fix.** (1) `StoragePoolRefresh` immediately before `checkPoolSpace`, after `imageSizeIfUncached`; failure is Transient; only on the domain-not-defined branch. Cost: readdir + stat per volume (24 today), already paid at `image.go:235`. (2) Do not refresh in Check (a mutation every round on a shared host); document the semantics. (3) Add headroom to `neededBytes`: per-host `poolReserveGiB` or a fraction of `capacity` (already returned and discarded at `image.go:450`); or document `tune2fs -m 0`. (4) Make the fake's `StoragePoolGetInfo` return a cached value that only `StoragePoolRefresh` updates; add `TestCreate_refreshesPoolBeforeSpaceCheck`. + +**Probe to settle** (for the owner): `virsh pool-info --bytes vms`, `virsh pool-refresh vms`, `pool-info` again; expect Available to drop ~1.68 GB and still sit ~24.7 GB above `df -B1 --output=avail /srv/vms`. + +## E9 — libvirt error-code assumptions — (a) REFUTED, (b) CONFIRMED + +**(a) Idempotency codes match libvirt 11.6.** Codes identified from libvirt's fixed per-code message prefix with `LIBVIRT_DEBUG=1`. + +| Operation | kvm2 expects | Real | Evidence | +|---|---|---|---| +| `DomainLookupByName`, missing (`create.go:88`, `delete.go:57`, `get.go:34`) | 42 | 42 | observed | +| `StorageVolLookupByName`, missing (`image.go:117,134,176,424`) | 50 | 50 | observed | +| `StoragePoolLookupByName`, missing (`delete.go:84`) | 49 | 49 | observed | +| `DomainGetMetadata`, no kvm2 element (`get.go:81`) | 80 | 80 | observed on `docker-29`, `postgres`, and a kvm-v1-marked proxy domain; namespace scoping holds | +| `StorageVolCreateXML`, duplicate (`create.go:171`) | 90 | 90 | sourced, ~95% | +| `DomainDestroy` inactive (`delete.go:67`) | 55 | 55 | sourced, high | +| `DomainCreate` active (`create.go:236`) | 55 | 55 | sourced, high; `waitRunning` re-checks anyway | +| Destroy/Undefine vanished domain | 42 | 42 | sourced, high | +| `StorageVolDelete` vanished (`image.go:182`) | 50 | 50 | sourced, high | + +`TestKVM2_lifecycle` would exercise 42 and 50 but not 90 (the second Create returns from the adoption branch before `ensureOverlay`), not 55 from Destroy, and a wrong 80 would not fail it (`list.go:39-45` logs and continues). Two fake-vs-real differences, neither breaking convergence: an overlay file unknown to libvirt returns 55 not 90; an inactive pool returns 55 from the marker lookup in `imageSizeIfUncached` (`create.go:96`) before `checkPoolSpace` (`create.go:100`), so the Permanent arm at `image.go:454-456` is unreachable through Create (`image_test.go:821-828` tests it by direct call). + +**(b) Classification.** `classifyLibvirt` (`errors.go:58-71`) sends every unlisted code to Transient; `providerFailure` (`proxy_controller.go:344-345`) returns the error and sets **no condition**, so the Proxy shows nothing and retries under workqueue backoff, rerunning `writeSeed` each time when the failure is at define. + +| Rejection | Where | Code | Evidence | +|---|---|---|---| +| Host has no usable KVM | define | 67 | sourced, high | +| No OVMF firmware | define | 9 | sourced, ~85% | +| Bridge missing | `DomainCreate` | 38 | sourced, high; `check.go` never looks at the bridge | +| host-passthrough unsupported | `DomainCreate` | 67 | sourced | +| Non-x86 host | define | 67 or 1 | observed 67 on the `domcapabilities --arch aarch64` path | +| Unknown machine type | define | 8 (already Permanent) | observed | + +beelink rejects none of these. The bare `libvirt.ErrUnsupported` sentinel (`rpc.go:392-394`) bypasses `libvirtCode`; small impact. This follows a recorded decision: spec entry 13 (`kvm2-provider-spec.md:317`) "unknown means retry", and contract §5 makes Permanent sticky until the generation changes, so a host-side fix would not unlatch. The spec never looked at 67, and it already latches `ErrNoStoragePool`, which is also host-fixed. + +**Minimal fix.** `ErrConfigUnsupported` → Permanent when `op == "create"`; `errors.Is(err, libvirt.ErrUnsupported)` → Permanent before the `libvirtCode` check; leave 9, 38, 1, 84 alone. Give the Transient arm of `providerFailure` a non-latching `Provisioned=False` condition with the message, or have the health checker read `ConnectGetDomainCapabilities` and check the bridge. In the integration test: destroy+undefine over `dialFresh` then Create again (exercises 90), destroy before step 9's Delete (exercises 55), assert "skipping domain with unreadable metadata" never logs. + +**Probe to settle** (not run): +``` +virsh -c qemu:///system vol-create-as default review-e9-dup.qcow2 1M --format qcow2 && \ +LIBVIRT_DEBUG=1 virsh -c qemu:///system vol-create-as default review-e9-dup.qcow2 1M --format qcow2 2>&1 | grep ' error : ' ; \ +virsh -c qemu:///system vol-delete --pool default review-e9-dup.qcow2 +``` + +## E10 — overlay capacity below the base's virtual size — CONFIRMED (libvirt pass-through ~90%, unexecuted) + +**Host numbers.** `base-debian-13.qcow2` and `base-test-image.qcow2`: libvirt capacity 3221225472 (3 GiB virtual), allocation 435683328. Live overlays: 10 GiB over that base. So the 10 GiB default has ~7 GiB margin; only `diskSizeGb: 1` or `2` breaks Debian 13 (Ubuntu ~3.5 GiB and Fedora ~5 GiB from memory). libvirt reports the true virtual size because of the refresh at `image.go:235`. + +**Local experiment** (qemu-img 10.2.1; host has 10.1.0): `qemu-img create -f qcow2 -b <64M base> -F qcow2 short.qcow2 16384K` exits 0, virtual size 16 MiB, `qemu-img check` clean; reads inside 16 MiB match the base, a read at 16M fails `Input/output error`. A 64M GPT base under a 16M overlay: `sfdisk` sees only the protective MBR, `sgdisk -v` reports the disk smaller than the header says and the backup header in error. The Linux kernel rejects such a GPT, so there is no root device; the guest would load GRUB and the kernel then drop to initramfs (inferred, no VM booted). libvirt's `virStorageBackendCreateQemuImgCmdFromVol` appends the capacity with no size comparison (from source memory). + +**Failing path.** `diskSizeGb: 2` passes validation (`config_kvm.go:133` rejects only negatives; `kvm2.go:127-128` changes only 0) → `checkPoolSpace` with the small number (`create.go:100`) → base cached (`:103`) → `ensureOverlay` sends `diskSizeGB << 30` (`create.go:107,158`; `volume.go:72` checks only positive) → domain defined and started, `waitRunning` sees Running, Create succeeds (`:148,260`) → `Get`: active, no address → `StateProvisioning` (`state.go:20-21`) → requeue at `ProvisioningPoll` forever (`proxy_controller.go:208-211`; there is no provisioning deadline). Correction to the hypothesis: the Proxy is stuck Provisioning with "waiting for the instance to run", not reported healthy. Nothing points at `diskSizeGb`, and raising it later does not repair that Proxy (`create.go:75-87`, `:171-177`). The kvm provider has the same hole (`kvm/volume.go:24-32`). + +**Minimal fix.** Add `StorageVolGetInfo` to `backend` (libvirtBackend, `observed`, fake already stores `fakeVol.capacity` at `fake_test.go:494`). Between `ensureImage` and `ensureOverlay`, if the base's capacity exceeds `DiskSizeGB << 30` return `permanentf("kvm2.diskSizeGb %d is smaller than image %s's virtual size …")`. One or two RPCs on the first Create only. Run the same check on a reused overlay and recreate it when short. `TestCreate_diskSmallerThanImage`. One sentence in the `diskSizeGb` doc rows. A config-time check is not possible. + +**Probe to settle** (not run; one ~200 KiB volume, self-cleaning): +```bash +V=review-e10-short.qcow2; P=default +virsh -c qemu:///system vol-create-as $P $V 1G --format qcow2 \ + --backing-vol /srv/vms/images/base-test-image.qcow2 --backing-vol-format qcow2; echo "create exit=$?" +virsh -c qemu:///system vol-dumpxml --pool $P $V | grep -E "capacity|backingStore|" +virsh -c qemu:///system vol-delete --pool $P $V; echo "delete exit=$?" +virsh -c qemu:///system vol-list $P | grep review-e10- || echo "clean" +``` diff --git a/review/escalations.md b/review/escalations.md new file mode 100644 index 0000000..682ce40 --- /dev/null +++ b/review/escalations.md @@ -0,0 +1,102 @@ +# kvm2 provider review: escalations for phase 2 + +18 raw Escalate items from the seven sweepers, merged to 15 root hypotheses, top 10 kept. +Ranked by blast radius x uncertainty. `deep-reviewer` is static (Read, Grep, Glob, Bash; it +can read the Go module cache and run local tests, but not touch the hypervisor), so each item +says whether it can be **settled statically** or is likely to end PLAUSIBLE without a probe on +beelink. Finding IDs (`S…`) refer to `sweep.md`. + +| # | Hypothesis in one line | Blast | Uncertainty | Static? | +|---|---|---|---|---| +| E1 | One wedged `markDead` starves every reconcile worker, not just one host | operator-wide stall | medium | yes | +| E2 | A reformatted GC mark makes GC delete a live Proxy's VM | live VM destroyed | medium | mostly | +| E3 | kvm and kvm2 on one hypervisor: cross-provider destruction beyond the Get/Delete hole | other provider's VMs/disks | medium | yes | +| E4 | No cross-process fencing on the base volume: leader handover deletes a base mid-upload | corrupt base under new overlays | medium | yes | +| E5 | An abandoned `StorageVolCreateXML` leaves a corrupt overlay that the next Create adopts | silent guest disk corruption | high | partly | +| E6 | go-libvirt leaks or races when the socket is killed mid-connect; per-round check dials accumulate | slow leak in long-running leader | medium | yes | +| E7 | In-place base replacement corrupts existing overlays on their next boot | guest disk corruption, many VMs | low-medium | partly | +| E8 | Pool-space precheck reads a stale `available`; quota backoff never engages | host pool fills | medium | partly | +| E9 | libvirt error-code assumptions: idempotency codes and define-time rejection codes | retry loops that never converge | medium | partly | +| E10 | Overlay capacity below the base's virtual size is accepted and truncates the disk | silent guest corruption | high | no | + +--- + +## E1 — A wedged `markDead` blocks name locks and starves the reconcile workers +Sources: chunk 1 Escalate 2; builds on finding S1.1. +**Entry points:** `internal/provider/kvm2/kvm2.go:186-243` (`lockName`, `keyedLocks`), `create.go`, `delete.go`, `backend.go:138-188`, `hosts.go:66-95`, `internal/gc/gc.go`, `internal/controller/proxy_controller.go` (+ `SetupWithManager`, `MaxConcurrentReconciles`), `cmd/main.go` manager wiring. +**Hypothesis:** a goroutine blocked inside `markDead` (S1.1) holds the per-name lock taken by Create/Delete, and reconcile and GC contexts carry no deadline, so `keyedLocks.acquire`'s ctx escape never fires. Delete/GC for that name block indefinitely instead of requeueing, and with a small `MaxConcurrentReconciles` a single silent host can pin every worker, stalling all Proxies on all providers. +**Checked:** lock acquisition is ctx-aware and refcounted correctly; only `internal/providerhealth/monitor.go:180` sets a deadline; the health checker and image upload use `dialFresh` and are insulated; `markDead` at `hosts.go:153` runs under `e.sem`. +**Unknown:** whether the controller-runtime work queue tolerates a permanently blocked Reconcile for one key (worker starvation across all Proxies); the actual `MaxConcurrentReconciles` and any reconcile timeout in the manager wiring; whether GC's single goroutine is blocked the same way; how many distinct call sites can enter `markDead` while holding `e.sem` or a name lock. + +## E2 — GC mark parse fragility: a reformatted mark makes GC delete a live VM +Sources: chunk 3 Escalate 1; finding S5.3. +**Entry points:** `internal/provider/kvm2/metadata.go:44-78` (`createdAt`, `parseInstanceMetadata`), `list.go:49`, `get.go:79-88`, `internal/gc/gc.go:113-121`. +**Hypothesis:** if libvirt hands `` or `` back with surrounding whitespace or re-indented content (another libvirt version, or an admin's `virsh edit`/`virsh define` through a reformatting tool), `UID` stops matching `live[inst.UID]` (exact string compare, `gc.go:118`) and/or `createdAt()` returns the zero time, which makes the instance immediately past `MinAge`. GC then deletes a live Proxy's VM. No trim or tolerant parse exists as a backstop. +**Checked:** `newInstanceMetadata` writes a single-line element; `kvm2_integration_test.go:422-449` asserts the UID round-trips and `CreatedAt` is non-zero (sweeper's claim that this ran against real libvirt 11.6 was wrong: phase 2 found the kvm2 integration test has not yet run on beelink, see `deep.md`); libxml2 does not re-indent an element whose children have text content. +**Unknown:** behaviour on other libvirt versions and after `virsh edit`; whether `encoding/xml` unmarshal plus the current struct tags already tolerate whitespace; whether a UID mismatch alone (timestamp intact) is enough for deletion, and what GC does with `UID == ""`; whether GC has any second guard (name-to-Proxy lookup) before `Delete`. + +## E3 — kvm and kvm2 on the same hypervisor +Sources: chunk 3 Escalate 2, chunk 7 Escalate 4; builds on findings S2.3, S7.1, S7.4. +**Entry points:** `internal/provider/kvm2/list.go:20-56`, `get.go:52-88`, `delete.go:55-98`, `volume.go:14-18`, `internal/provider/kvm/kvm.go:379`, `internal/provider/kvm/id.go:11-13`, `internal/provider/kvm/volume.go:12`, `internal/provider/kvm/image.go:46`, `internal/gc/gc.go:86-131`, `internal/provider/config.go:108-130`, `internal/controller/proxy_controller.go:197,237,287`, `docs/kvm2-provider.md:346-370`. +**Hypothesis:** the only thing keeping kvm2's `ListByTag` from handing kvm-made domains to GC is libvirt's namespace-scoped `DomainGetMetadata`; both providers also share `.qcow2`, `-seed.iso`, `base-.` names and the providerID format. Beyond the confirmed Get/Delete hole (S2.3), GC's per-provider scoping may let provider A's list be reconciled against provider B's Proxies, and a Proxy moved between a kvm and a kvm2 entry (same UID, same domain name) may have one side delete volumes the other side's domain uses. +**Checked:** domain names are UID-derived so one Proxy cannot exist under both at once; the two metadata URIs differ; `ListByTag` is mark-filtered; `spec.provider` is immutable but an entry's `name` is independent of its `type`; the fake cannot prove the namespace rule (S7.4). +**Unknown:** whether GC builds its live-UID set per provider or globally, and what it deletes when the same UID is visible through two entries; whether anything (config reload guard, admission check on ProviderID, deploy tooling) prevents a re-typed entry from inheriting live `status.ProviderID`s; the reverse direction (kvm v1 `Get`/`Delete`/`ListByTag` against kvm2-made domains). + +## E4 — No cross-process fencing on the base volume +Source: chunk 4 Escalate A. +**Entry points:** `internal/provider/kvm2/image.go:85-146` (`ensureImage`, `lockImage`, `cachedImage`), `image.go:200-300` (`fetchImage`, `uploadImage`), `kvm2.go:199-243`, `cmd/main.go:107` (leader election flags), `config/manager/manager.yaml:24`. +**Hypothesis:** process A is inside `fetchImage` (base created, marker not yet written); process B's `cachedImage` sees base-without-marker and deletes it (`image.go:118`), then its `StorageVolCreateXML` either fails `ErrStorageVolExist` or succeeds against a half-uploaded file. A's stream writes into an unlinked inode and A writes a marker for a base that no longer exists. An overlay created in between backs onto a deleted or wrong-content path. +**Checked:** `keyedLocks` is process-local; `replicas: 1`; `--leader-elect` defaults to false; `Delete` never touches base volumes; nothing else in the repo names base or marker volumes. +**Unknown:** whether a leader-election handover or a rolling update (old pod still inside a 20 minute `fetchImage` while the new pod reconciles the same Proxy) reaches this with the shipped manifests; whether the manager cancels in-flight reconciles on lost lease and how long `terminationGracePeriodSeconds: 10` leaves the old process uploading; whether B's marker written over A's bytes can ever validate. + +## E5 — Overlay atomicity under an abandoned RPC +Source: chunk 2 Escalate 1. +**Entry points:** `internal/provider/kvm2/create.go:151-181` (`ensureOverlay`), `backend.go:157-188` (`call`, budget, `markDead`), `backend.go:319-321` (`StorageVolCreateXML` under `budgets.rpc` = 30 s). +**Hypothesis:** if `StorageVolCreateXML` for the overlay exceeds the 30 s budget or the caller's ctx ends, `call` kills the SSH transport mid-RPC. If libvirtd still registers the volume, possibly partial, the next Create hits `ErrStorageVolExist`, logs "overlay volume already exists" and defines a domain over a corrupt overlay. The base image has a marker and the seed is rebuilt unconditionally for exactly this reason; the overlay has neither. +**Checked:** the reuse branch distinguishes only `ErrStorageVolExist`; no size, format or backing-path verification anywhere; `Delete` is the only thing that removes an overlay. +**Unknown:** libvirtd's storage-driver behaviour when the client drops mid `virStorageVolCreateXML` (unlink vs leave registered); whether overlay creation can exceed 30 s in normal operation (NFS, LVM, preallocating filesystems) rather than only on cancellation; whether a cheap verification (`StorageVolGetInfo` capacity, or delete-and-recreate when no domain references it) is available through the existing `backend` seam. + +## E6 — go-libvirt internals when the socket is killed under it +Sources: chunk 1 Escalate 1, chunk 6 Escalate 1. +**Entry points:** `internal/provider/kvm2/backend.go:225-238` (`libvirtBackend.connect`, the `Disconnected()` watcher), `backend.go:351-367` (`Close`), `hosts.go:91-162` (`dialFresh`, `connect`), `check.go:85-119`; upstream `go-libvirt@v0.0.0-20260814190004-1a83157e1858`: `libvirt.go:286-305` (`ConnectToURI`), `:821/:834` (`waitAndDisconnect`), `socket/`. +**Hypothesis:** (a) when `call(…, "Connect", …)` expires and `markDead` closes the socket just as `ConnectToURI` reaches `l.disconnected = make(chan struct{})`, there is a data race on that field against `waitAndDisconnect`, and the loser leaves a `Libvirt` whose `Disconnected()` never fires; (b) `Check` opens a fresh connection per host per round (~1440/day/host), so any goroutine or fd go-libvirt leaves behind on the killed-socket path (abandoned Connect, or `Close` hitting its 5 s budget mid `ConnectClose`) accumulates linearly in a long-running leader. +**Checked:** kvm2's own goroutines are clean (watcher selects on `link.closed`, keepalive exits on `stop`, `sshSocket.Close` is once-guarded); `Disconnected()` is read only after a successful Connect and the backend is discarded on failure; `go test -race -count=2` of the unit tests is clean. +**Unknown:** whether `waitAndDisconnect`/`listenAndRespond` always terminate when the conn is closed mid-handshake; whether the socket layer leaves an fd; whether a looped dial-and-kill test (N=1000, sampling `runtime.NumGoroutine()`) against the in-process SSH test server shows growth. Note finding S4.1 already confirms one permanent go-libvirt goroutine leak on the stream path; this asks whether the connect/close path has another. + +## E7 — In-place base-image replacement under overlays that pin its path +Source: chunk 2 Escalate 2; the code behaviour is finding S4.6 (three sweepers). +**Entry points:** `internal/provider/kvm2/create.go:103,154-165`, `image.go:113-146` (`cachedImage`), `image.go:235` (`StoragePoolRefresh`), `docs/provider-contract.md` ("Catalogue changes do not replace instances"). +**Hypothesis:** when `images..sha256` changes (or the marker is missing or unreadable), the base is deleted and different content is downloaded to the same deterministic path while every earlier overlay records that path as its qcow2 backing file. Any such overlay that is later restarted (adoption path, host reboot, `ensureRunning` on a shutoff domain) reads mismatched backing data: guest filesystem corruption across every VM on that key. +**Checked:** `Delete` never removes the base; the path is stable per key+format; `BackingPath` is resolved once at define time and never re-verified; the contract explicitly allows catalogue edits not to replace instances. +**Unknown:** which restart paths actually exist for an existing overlay in this operator (does anything restart a shutoff domain rather than delete and recreate it, given Stopped → Delete in the controller?); whether the missing/unreadable-marker trigger can fire without any admin edit (e.g. after E4 or a failed `writeMarker`), which would make this reachable in normal operation; whether libvirt refuses `StorageVolDelete` on a volume in use as a backing store. + +## E8 — Pool-space precheck freshness +Source: chunk 4 Escalate B. +**Entry points:** `internal/provider/kvm2/image.go:419-467` (`imageSizeIfUncached`, `checkPoolSpace`, `neededBytes`), `create.go:96-106`, `check.go:85-119` (also reads pool info). +**Hypothesis:** for a `dir` pool, `StoragePoolGetInfo.available` is what libvirtd computed at the last pool refresh, adjusted only for creates and deletes it performed; guest writes growing sparse overlays do not update it. `create.go` never issues `StoragePoolRefresh` before the check (the only refresh is after an upload, `image.go:235`), so on a host its VMs have filled, the precheck reports tens of GB free indefinitely, `ErrQuotaExceeded`/`QuotaRetry` never engages, and Create fails later with an ENOSPC-flavoured transient, which is also the case that wedges the upload stream (S4.1) and pauses guests (S2.2). +**Checked:** no refresh anywhere on the pre-create path; `available` is used raw; unit tests set the fake's value directly; the spec describes the precheck but says nothing about freshness. +**Unknown:** libvirt 11.x dir-pool semantics for `available` (statvfs at call time vs cached); whether `Check`'s `poolAvailableGiB` has the same staleness; the cost of a per-Create refresh on the shared pooled connection. + +## E9 — libvirt error-code assumptions +Sources: chunk 5 Escalate 2, chunk 7 Escalate 2; findings S3.5, S5.2. +**Entry points:** `internal/provider/kvm2/errors.go:33-97` (`classify`, `classifyLibvirt`, `libvirtCode`, `hasCode`), `create.go:139-142,166-177`, `get.go:79-88`, `delete.go:55-98`, `image.go:174-186`, `fake_test.go:491-493`, `kvm2_integration_test.go`. +**Hypothesis:** two halves of one assumption. (a) Idempotency depends on exact codes: `ensureOverlay` needs `ErrStorageVolExist` for a duplicate volume on a dir pool, `readMetadata` needs `ErrNoDomainMetadata`, the delete paths need `ErrNoDomain`/`ErrNoStorageVol`. The fake returns exactly the codes the code expects, so unit tests cannot refute a mismatch; if real libvirt returns `ErrOperationFailed` or `ErrInternalError` for any of them, a retried Create after a partial failure never converges. (b) Every define-time rejection of the generated XML (arch, machine, firmware feature, host-passthrough) surfaces as `ErrConfigUnsupported` (67) or `ErrOperationUnsupported` (84), which default to Transient: an unbounded retry with no `ReasonPermanentError`. +**Checked:** only `ErrNoStoragePool`, `ErrXMLError`, `ErrNoSupport`, `ErrInvalidArg`, `ErrAuthFailed`, `ErrAccessDenied` map to Permanent; 67 and 84 exist in go-libvirt's const set and are unlisted; bare `libvirt.ErrUnsupported` bypasses `libvirtCode`; transient errors requeue forever. +**Unknown:** which idempotency codes the integration test actually exercises against libvirt 11.6 (a second Create on an existing overlay? Delete twice? Get on a foreign domain?), which settles (a) for those paths; for (b), which code libvirt really returns for each rejection, and whether keeping unknown codes Transient is a deliberate spec decision. Mapping too much to Permanent latches Proxies that would recover. + +## E10 — Overlay capacity smaller than the backing image's virtual size +Source: chunk 5 Escalate 1; finding S5.6. +**Entry points:** `internal/provider/kvm2/volume.go:68-89` (`buildVolumeXML`), `create.go:107,154-181`, `image.go:449-467`, `internal/provider/config_kvm.go:133` (`diskSizeGb` validation). +**Hypothesis:** with `kvm2.diskSizeGb` below the base image's virtual size, libvirt runs `qemu-img create -b … ` and succeeds, producing a truncated virtual disk whose partition table overruns the device. The VM fails to boot or boots with a corrupt root filesystem, and kvm2 reports a healthy running domain that never answers. +**Checked:** nothing in the package compares overlay capacity to the base's virtual size; the validator rejects only `diskSizeGb < 0`; the 10 GiB default exceeds current Debian/Ubuntu/Fedora cloud images. +**Unknown:** whether libvirt's storage backend or qemu-img errors on a short capacity (then this is only a poor error message). Needs a real qemu-img; the deep reviewer can at most establish whether kvm2 has any way to learn the base's virtual size through the existing seam (`StorageVolGetInfo` on the base) and what a guard would cost. **Expect PLAUSIBLE.** + +--- + +## Dropped (not in the top 10) + +- **D1** Deleting a volume while a libvirt upload stream is open on it (chunk 4 C): a consequence of confirmed finding S4.1, only changes whether its disk-space effect is real; needs a truncated-source probe on beelink. +- **D2** cloud-init dispatch of a `text/plain` part inside vendor-data (chunk 5 E3): invisible-failure risk but only a boot test on a real image settles it; nothing static to add beyond S5.8. +- **D3** ProviderHealth status churn every round waking watchers (chunk 6 E2): low blast radius, arguably intended per the code comment. +- **D4** Nested volumeMount, `kvm-ssh` Secret inside the read-only `providers-config` mount (chunk 7 E1): pre-existing and shared with kvm; refuted if kvm has ever run in-cluster with the Secret present, otherwise one `kubectl apply` settles it. +- **D5** Whether provider error strings (with the full image URL, S7.10) reach Proxy status conditions (chunk 7 E3): low blast radius; cheap and fully static if you want it added. diff --git a/review/scope.md b/review/scope.md new file mode 100644 index 0000000..2d64a18 --- /dev/null +++ b/review/scope.md @@ -0,0 +1,133 @@ +# kvm2 provider review: scope + +Branch `feat/kvm2-provider` at `2510842`. Seed: `grep -ril 'kvm2\|libvirt'` excluding vendor, +generated, testdata, docs. The minikube layout hints in the brief do not apply; this repo is +the egress-proxies operator and kvm2 is `internal/provider/kvm2/`, a pure-Go provider +(`digitalocean/go-libvirt` RPC over an SSH-tunnelled socket, no cgo, no driver plugin binary). + +Checklist items with no counterpart here, so reviewers can answer them quickly rather than hunt: +- libvirt **network** lifecycle / DHCP / static IP: kvm2 creates no networks; it attaches to a + configured bridge and reads the address via `DomainInterfaceAddresses`. The live questions are + IP detection, timeouts and stale addresses. +- GPU/PCI/NUMA passthrough: none. +- driver plugin RPC boundary: none. Closest analogue is the libvirt RPC link over SSH + (`link`, `call`, budgets, dead-link detection, libvirt version differences). +- cgo/build tags: no cgo. Only tag is `//go:build integration`. +- Handle leaks: go-libvirt handles are plain values (no `Free`); the leakable resources are + SSH connections, the libvirt link, goroutines (keepalive, `await`, `call`), HTTP bodies, + temp dirs and half-written storage volumes. + +## CORE (kvm2-owned) + +`internal/provider/kvm2/` — 18 source files (~3.2k lines), 22 test files (~7.3k lines). + +| File | Lines | What | +|---|---|---| +| kvm2.go | 243 | `Provider`, `New`, options, `withDefaults`, `host`, `wrapErr`, `lockName`, `keyedLocks` | +| ssh.go | 216 | `dialStage`/`dialError`, `loadCredentials`, `sshSocket` + keepalive, `dialSSHSocket`, `await` | +| hosts.go | 162 | `dialer`, `hostTable` (per-host cached backend, `get`/`dialFresh`), `libvirtDialer` | +| backend.go | 369 | `backend` interface, `budgets`, `link` (dead marking), `call`/`run`, `libvirtBackend` | +| observe.go | 263 | `observed` backend decorator: span + log per libvirt call | +| create.go | 280 | `Create`, `create`, `ensureOverlay`, `writeSeed`, `ensureRunning`, `waitRunning` | +| delete.go | 98 | `Delete`, `removeDomain`, `removeVolumes` | +| get.go | 119 | `Get`, `get`, `readMetadata`, `address` | +| list.go | 56 | `ListByTag` | +| state.go | 49 | `mapState`, `isActive`, `firstIPv4` | +| id.go | 28 | `formatProviderID`, `parseProviderID` | +| metadata.go | 78 | GC mark in domain ``: marshal/parse, `managed`, `createdAt` | +| errors.go | 97 | `permanentf`/`transientf`, `classify`, `classifyLibvirt`, `libvirtCode`, `hasCode` | +| image.go | 467 | `resolveImage`, `lockImage`, `ensureImage`, `cachedImage`, marker read/write, `fetchImage`, `uploadImage`, `do`, `checkResponse`, pool space checks | +| volume.go | 95 | volume naming, `urlImageKey`, `buildVolumeXML` | +| xml.go | 256 | `domainParams`, `buildDomainXML` and the XML structs | +| seed.go | 124 | `buildSeedISO` (temp dir, iso9660), `buildVendorData` | +| check.go | 189 | `Check`, `checkHost`, `probeHost`, `checkReason`, formatting helpers | + +Tests (each reviewed with its source file): `*_test.go` next to every file above, plus +`provider_test.go` (contract tests), `fake_test.go` (fake backend), `sshserver_test.go` +(in-process SSH server), `kvm2_integration_test.go` (`//go:build integration`, `TestKVM2*`). + +Other kvm2-owned bits: +- `cmd/main.go:62,206` — import and `"kvm2": kvm2.New` registration +- `Makefile:73-75` — `test-integration-kvm2` +- `config/samples/proxy_kvm2.yaml`, `config/samples/kustomization.yaml:8` +- `config/samples/providers-config.yaml:58-`, `config/manager/providers_config.yaml:54-` (commented kvm2 entries) + +Reference only, not under review: `docs/kvm2-provider-spec.md`, `docs/kvm2-provider.md`, +`docs/provider-contract.md`. Reviewers may read them to judge code against stated intent. + +## SHARED (on the kvm2 call path; listed symbols only) + +Called by kvm2: +- `internal/provider/config_kvm.go` — `KVMConfig`, `KVMImage`, `KVMMachineType`, `KVMHost`, + `KVMFormat*`/`KVMFirmware*` consts, `IsHTTPURL`, `validateKVM`, `validateKVMBlock` + (the validator is what stands between user YAML and kvm2's XML/URL/volume-name building) +- `internal/provider/config.go` — `ProviderConfig.KVM2`, `setBlocks`, the `kvm2` arms of `Config.validate` +- `internal/provider/provider.go` — `Provider` interface, `CreateRequest`, `Placement`, `Instance`, + `InstanceState` + `State*` consts, `LabelManagedYes` +- `internal/provider/errors.go` — `Error`, `Wrap`, `Class`, `ErrTransient`/`ErrPermanent`/`ErrNotFound`/`ErrQuotaExceeded` +- `internal/provider/check.go` — `Checker`, `TargetStatus`, `Reason*` consts +- `internal/provider/name.go` — `NameFromUID` (produces the domain name kvm2 puts in XML and volume names) +- `internal/tracing` — `StartSpan`, `Tracer`, `Option` + +Calling into kvm2: +- `internal/controller/proxy_controller.go` — `reconcileManaged` (Create/Get/Delete calls at + 156/197/218), `replaceInstance`, `reconcileDelete`, `providerFailure`, `deletionFailure`, + `resolveCloudInit`, `placementFrom` +- `internal/gc/gc.go` — `Sweeper.sweep` (`ListByTag` at 105, `Delete` at 125) +- `internal/providerhealth/monitor.go` — `Monitor.checkOnce`, `Monitor.run` (`Check` at 214; the timeout it passes) +- `internal/provider/registry/registry.go` — `Constructor`, `Build` +- `internal/provider/metrics.go`, `internal/provider/tracing.go` — `WithMetrics`/`WithTracing` + decorators (`instrumented`, `traced`): error passthrough, and that they hide `Checker` +- `cmd/main.go:196-225` — registry build, checker extraction from unwrapped providers, wrapping +- `config/manager/manager.yaml:150-178` — `kvm-ssh` Secret mount, `/tmp` emptyDir (64Mi) that `buildSeedISO` uses + +## EXCLUDED but considered + +- `internal/provider/kvm/` — the older kvm provider; kvm2 is a clean-room rewrite and imports nothing from it. +- `internal/provider/config_kvm_test.go`, `config_test.go` — reviewed only for kvm2 cases, inside chunk 7; rest is kvm. +- `internal/provider/gcp/`, `internal/provider/kubernetes/` — other providers, not on a kvm2 path. +- `internal/health`, `internal/discovery`, `internal/lease` — consume `Proxy` status, never call the provider. +- `internal/metrics`, `internal/tracing` internals — only the three entry points above are on the path. +- `internal/providerhealth/envtest_test.go` — mentions kvm2 only as a type string in a fixture. +- `api/v1alpha1/providerhealth_types.go:71`, `config/crd/bases/...providerhealths.yaml` — comment-only / generated. +- `go.mod`, `go.sum` — dependency pins (`go-libvirt` 20260814 pseudo-version, `iso9660` v0.4.0, `x/crypto` v0.54.0); noted, not reviewed. Third-party source may be read to understand a call, not reported on. +- `config/samples/proxy_kvm.yaml`, `Makefile:69-71,185-196` (`test-integration-kvm`, `kvm-ssh-secret`) — kvm-only or shared tooling with no kvm2 logic. `kvm-ssh-secret` is mentioned in chunk 7 for context only. +- `README.md`, `CHANGELOG.md`, `AGENTS.md`, `.claude/settings.json`, `docs/` — prose / tooling. +- `.gitea/workflows/build.yaml`, `Dockerfile` — no kvm2-specific content; integration test is not run in CI (worth stating in chunk 7, not a file to review). + +## Proposed phase 1 chunking (7 chunks) + +1. **Connection layer (SSH + libvirt link)** + Files: `ssh.go`, `hosts.go`, `backend.go`, + `ssh_test.go`, `sshserver_test.go`, `hosts_test.go`, `backend_test.go`. + SHARED in play: `KVMConfig` (SSHKeyFile, KnownHostsFile, InsecureIgnoreHostKey), `KVMHost`. + Focus: connection/goroutine leaks on error paths, dead-link detection and redial races in `hostTable`, budgets vs caller context, keepalive, host key handling. + +2. **Domain lifecycle (Create / Delete)** + Files: `kvm2.go`, `create.go`, `delete.go`, `id.go`, + `kvm2_test.go`, `create_test.go`, `delete_test.go`, `id_test.go`, `provider_test.go`. + SHARED in play: `CreateRequest`, `Placement`, `NameFromUID`; `reconcileManaged`, `replaceInstance`, `reconcileDelete` (how Create/Delete are retried). + Focus: partial-create cleanup, idempotency, foreign-domain refusal, half-deleted domains, `keyedLocks` correctness, `waitRunning` timeout/cancellation. + +3. **Observation, state and error classification (Get / ListByTag)** + Files: `get.go`, `list.go`, `state.go`, `errors.go`, + `get_test.go`, `list_test.go`, `state_test.go`, `errors_test.go`. + SHARED in play: `Instance`, `InstanceState`, `Error`/`Wrap`/`Class` and the `Err*` classes; `Sweeper.sweep`, `providerFailure`, `deletionFailure`. + Focus: libvirt state mapping (paused/crashed/pmsuspended/shutoff), IP detection and stale addresses, NotFound vs transient, one dead host failing the whole list and what GC does with a partial list. + +4. **Base image cache and volumes** + Files: `image.go`, `volume.go`, + `image_test.go`, `volume_test.go`. + SHARED in play: `KVMImage`, `KVMFormat*`, `IsHTTPURL`, `validateKVMBlock` (image key charset). + Focus: download/upload streaming, sha256 verify + marker protocol, cross-replica and cross-Create races on the same base volume, cleanup of partial volumes, URL handling (SSRF, redirects, size caps), pool space check, volume-name injection via URL/key. + +5. **XML and cloud-init seed templating** + Files: `xml.go`, `seed.go`, `metadata.go`, (`buildVolumeXML` in `volume.go`), + `xml_test.go`, `seed_test.go`, `metadata_test.go`. + SHARED in play: `KVMMachineType`, `KVMHost` (bridge, pool), `KVMFirmware*`, `CreateRequest.CloudInit`, `NameFromUID`; `resolveCloudInit`; `manager.yaml` `/tmp` emptyDir. + Focus: escaping of config- and user-supplied values, validity across libvirt versions, metadata parse robustness (GC trusts it), temp-dir cleanup and size limit, vendor-data merging. + +6. **Health check and observability** + Files: `check.go`, `observe.go`, + `check_test.go`, `observe_test.go`. + SHARED in play: `Checker`, `TargetStatus`, `Reason*`; `Monitor.checkOnce`/`run`; `tracing.StartSpan`/`Tracer`; `WithMetrics`/`WithTracing` decorators; `cmd/main.go:213-225`. + Focus: dedicated check connections closed on every path, check timeout vs dial budget, reason mapping, secrets or cloud-init leaking into logs/spans, span end on every path. + +7. **Config, registration, deployment and test gating** + Files: `internal/provider/config.go` + `config_kvm.go` (kvm2 arms), their tests' kvm2 cases, `internal/provider/registry/registry.go`, `cmd/main.go:196-225`, `config/manager/manager.yaml:150-178`, `config/manager/providers_config.yaml`, `config/samples/providers-config.yaml`, `config/samples/proxy_kvm2.yaml`, `Makefile:73-75`, `kvm2_integration_test.go`, `fake_test.go`. + SHARED in play: `validateKVMBlock`, `setBlocks`, `Config.validate`, `registry.Build`, `Constructor`. + Focus: validation gaps that let bad values reach chunks 4/5, `New` behaviour with a nil block or unreadable key, Secret mount perms, integration test gating/cleanup and whether the fake backend diverges from real libvirt in ways that hide bugs. diff --git a/review/sweep.md b/review/sweep.md new file mode 100644 index 0000000..f7728ad --- /dev/null +++ b/review/sweep.md @@ -0,0 +1,216 @@ +# kvm2 provider review: phase 1 sweep + +Seven `sweep-reviewer` runs, one per chunk in `scope.md`, against `feat/kvm2-provider` at `2510842`. +Findings below are as the sweepers reported them (they state each was verified in code); the +orchestrator merged duplicates and ranked, and did not re-read code. 54 raw findings, 49 after +merging. IDs are `S.`; "also: chunk N" marks a finding reported independently by +another sweeper. + +Severity counts after dedupe: **high 4, medium 17, low 28**. + +Recurring themes across chunks: +- **T1 Unknown or unmapped errors default to Transient, so permanent misconfigurations retry forever** + (S1.4, S2.5, S3.7, S5.2, S7.2, S7.6b) while one genuinely transient case is made Permanent and latches (S5.1, S4.2). +- **T2 Ownership mark enforced on Create and ListByTag but not on Get/Delete** (S2.3), and the fake cannot test the namespace rule (S7.4). +- **T3 Base image cache identity and lifetime**: keyed by key+format only, deleted in place under live overlays, no cross-process fencing (S4.6, S7.1, S4.8, escalations E4/E7). +- **T4 The shared pooled link**: any caller's cancel or budget expiry kills it for everyone, and several callers misread the resulting "connection is closed" error (S1.1, S3.1, S3.6, S4.3). + +## Top of the list (high) + +| ID | Where | Defect | +|---|---|---| +| S1.1 | `backend.go:138-148`, `ssh.go:109-114` | `markDead` closes the transport before signalling death; the close can block ~15 min on a silent host, wedging callers and the host's dial semaphore | +| S4.1 | `image.go:273` | A reader or server-side error during `StorageVolUpload` wedges go-libvirt's stream sender forever: goroutine + 4 MiB leak per failed upload, 20 min stall | +| S3.1 | `get.go:104-119` | `address()` swallows every non-ctx error, so a dead link or a guest-agent restart demotes a healthy Running instance to Provisioning and wipes `status.IP` | +| S7.1 | `volume.go:17`, `image.go:113-146` | Base image cache is keyed by key+format, never URL: two provider entries on one host+pool (the shipped sample does this) silently share one image; a URL edit is a silent no-op | + +--- + +## Chunk 1 — Connection layer (`ssh.go`, `hosts.go`, `backend.go`) + +**S1.1 — high — `internal/provider/kvm2/backend.go:138-148` (kill at 143-145, `close(l.closed)` at 146), `ssh.go:109-114`** +`link.markDead` runs `l.kill()` synchronously inside `once.Do` and only then closes `l.closed`. `kill` is `sshSocket.Close`, which calls `s.Conn.Close()` (x/crypto `chanConn` → `channel.Close` → a raw TCP write under `writeMu`/`t.mu`) before `s.client.Close()`. That write blocks while the send buffer is full. +Scenario: hypervisor is power-cut or partitioned (no RST/FIN, normal over Tailscale) with data queued. A pooled call hits its 30 s/90 s budget, `call` (`backend.go:183`) invokes `markDead`, which blocks until the kernel gives up (`tcp_retries2`, ~15 min). During that window: the abandoning caller never returns (reconcile ctx has no deadline); `l.closed` is still open so `isDead` reports the backend live and `hostTable.get` keeps handing it out, each new call wedging behind the first in `once.Do`; `sshSocket.closeOnce` memoises the in-progress close so `libvirtBackend.Close`'s 5 s budget cannot bound it; when the blocked `markDead` is the one at `hosts.go:153` it runs while `hostTable.get` holds `e.sem`, blocking every caller for that host. +Fix direction: close `s.client` (plain `net.Conn.Close`) first and/or close `l.closed` before calling `kill`. + +**S1.2 — medium — `ssh.go:119-147`** +`sshSocket.keepalive`, the only detector of a silently dead pooled connection between calls, has no test. `testSSHServer` already services global requests, so it is testable end to end. A regression is invisible to CI and shows up only as a stale "live" connection that burns a full 30 s budget. Violates the repo rule that all new Go code has tests. + +**S1.3 — low — `ssh.go:120`** +`time.NewTicker(interval)` panics for `interval <= 0`, in a goroutine, which exits the operator. Only `newLibvirtDialer` sets the field today; a future struct literal without it compiles and panics on first successful dial. No guard at either end. + +**S1.4 — low — `errors.go:45-51` (dial path from `ssh.go:87`)** [T1] +`classify` recognises only `*knownhosts.KeyError`. A key matched by an `@revoked` line yields `*knownhosts.RevokedError`, which falls through to Transient: a deliberately revoked hypervisor key is retried forever instead of failing Permanent. + +**S1.5 — low — `ssh.go:167-172`** +`ClientConfig.HostKeyAlgorithms` is left empty. If `known_hosts` pins only the RSA key of a host that also serves ed25519, the server offers ed25519, the callback returns a `KeyError` with non-empty `Want`, and `classify` maps it to Permanent "host key mismatch" although the pinned entry is genuine. + +Verified clean: all dial error paths close the TCP conn and SSH client; `call`/`await` orphan goroutines write into buffered channels; the never-closed pooled table is not a leak (Provider built once, no hot reload). + +## Chunk 2 — Domain lifecycle (`kvm2.go`, `create.go`, `delete.go`, `id.go`) + +**S2.1 — medium — `create.go:107-114`** +Per-VM volumes created before `DomainDefineXML` are unreachable by both `Delete` and orphan GC when `Create` never returns an ID. +Scenario: `ensureOverlay` and `writeSeed` succeed, `DomainDefineXML` (139) fails Permanent (or the manager dies between). `Create` returns `("", err)`; `reconcileManaged` never sets `status.ProviderID` and latches. On Proxy deletion, `reconcileDelete` takes the `ProviderID == ""` shortcut (`proxy_controller.go:270-274`) without calling `Delete`. `ListByTag` enumerates domains only, so `proxy-.qcow2` (10 GiB capacity) and `proxy--seed.iso` stay in the pool permanently. + +**S2.2 — medium — `create.go:230-231`, `state.go:16-27`, `get.go:56`** (also: chunk 3) +Paused domains are destroyed. `ensureRunning` refuses a paused/pmsuspended domain with a Permanent "virsh resume it" error, but that guard only fires before an ID is persisted. In steady state the reconciler calls `Get`; `get.go:56` discards the `DomainGetState` reason and `mapState` maps paused/pmsuspended to `StateStopped`, which sends `reconcileManaged` (`proxy_controller.go:217-225`) to `prov.Delete`: destroy, undefine, delete disks, recreate. +Scenario: QEMU pauses the guest on an I/O error because the pool is full (`VIR_DOMAIN_PAUSED_IOERROR`), or an admin runs `virsh suspend`/`save`/snapshot/live-migrate. At the next drift poll the VM is killed; in the ENOSPC case the recreate refills the pool or fails the space precheck. + +**S2.3 — medium — `delete.go:56-63`, `get.go:52-73`** (also: chunk 3) [T2] +`Get` and `Delete` do not enforce the kvm2 mark that `Create` (`create.go:80-82`) enforces. `readMetadata` returns the zero value for an unmarked domain and `get` builds an Instance anyway (`UID: ""`); `removeDomain`/`removeVolumes` destroy whatever the name resolves to. `internal/provider/kvm/id.go:11-13` emits byte-identical `hosts//domains/` IDs and both providers use `NameFromUID`. +Scenario (the documented migration path, `docs/kvm2-provider.md:346-370`, done out of order): an admin re-types an entry from `kvm` to `kvm2` keeping its name without first deleting every Proxy. `status.ProviderID` survives; kvm v1's mark is in another namespace, so `Get` returns the v1 domain as this Proxy's instance; if it is anything but Running the controller calls `Delete` and kvm2 destroys a kvm-v1 domain the docs promise it can never touch. Same exposure for a hand-edited `status.ProviderID`. `TestGet_unmanagedDomainHasEmptyUID` pins the current behaviour. `ListByTag` is correctly mark-filtered, so GC is not affected. + +**S2.4 — low — `create.go:87`** +The adoption path starts an already-defined domain without checking its disks exist and cannot heal them. If the overlay or seed is gone and no ID was persisted, every retry skips the volume steps, `DomainCreate` fails with a storage error classified Transient, and the reconciler retries forever; the drift path that would heal it is unreachable without an ID. + +**S2.5 — low — `delete.go:15`** [T1] +`undefineFlags` omits `DomainUndefineCheckpointsMetadata`. An admin- or backup-tool-created checkpoint makes `DomainUndefineFlags` fail Transient; `deletionFailure` retries forever and the Proxy cannot be deleted without manual `virsh checkpoint-delete`. + +Verified clean: `keyedLocks` refcounting, cancel-while-waiting and unlock on every path; `waitRunning` cap and cancellation; providerID round trip. + +## Chunk 3 — Observation, state, errors (`get.go`, `list.go`, `state.go`, `errors.go`) + +**S3.1 — high — `get.go:104-119`** [T4] +`address()` treats every non-`context` error from both address sources as "skip" and returns `("", nil)`. +Triggers: (a) routine — on beelink the ARP source never answers (`docs/kvm2-provider-spec.md:146`), so a `qemu-guest-agent` restart in the guest returns `ErrAgentUnresponsive` → `""` → `mapState(running, "")` = Provisioning; (b) racy — any concurrent call that exceeds its budget kills the shared pooled link (`backend.go:180-184`), after which `call` returns `transientf("... connection is closed")` with no ctx sentinel, which `get.go:108` does not recognise; if the link dies between `DomainGetState` and the address read, `Get` succeeds with `State=Provisioning, IP=""`. +Downstream: `proxy_controller.go:208-211` clears `status.IP` and sets `Provisioned=False`; `internal/health/engine.go:200-207` prunes the proxy's health state and the Healthy counters restart from zero. One RPC hiccup pulls a working proxy out of rotation. `TestGet_addressErrorIsProvisioning` pins the behaviour. + +**S3.2 — medium — `state.go:35-49`** +IP detection has no interface selection. The domain has exactly one bridge NIC and libvirt returns `Hwaddr` per interface, but `firstIPv4` filters only loopback/link-local/unspecified. +Scenario: the image or cloud-init brings up `docker0` (172.17.0.1), `tailscale0` or `wg0` before the DHCP lease lands and the agent reports it first. `Get` returns Running with a non-routable IP, the controller latches `Provisioned=True`, and the value is never corrected because any IPv4 keeps the state Running. + +**S3.3 — medium — `list.go:38-45`** [T4] +A pooled link that dies mid-list makes `ListByTag` a silently successful partial list. The fail-whole guard is `ctx.Err() != nil`, but a killed link yields "connection is closed" with no ctx sentinel; the remaining domains each log "skipping domain with unreadable metadata" and the call returns nil error with "managed=5" as if authoritative. GC only under-deletes, so no wrong deletion, but nothing marks the result partial. A Permanent corrupt-mark parse error is likewise skipped forever with only a V(1) line. + +**S3.4 — low — `get.go:104-119`** +`address()` also swallows `ErrNoDomain`: a domain that vanishes between `DomainGetState` and the address read is reported as live Provisioning for one poll. The comment "only ctx ending is an error" is also inaccurate: a 30 s budget expiry on the agent source wraps `DeadlineExceeded`, fails `Get`, and never falls back to ARP. + +**S3.5 — low — `errors.go:58-71`** [T1] (also: chunks 5, 7) +Version and capability errors escape classification. go-libvirt turns an "unknown procedure" reply into the bare sentinel `libvirt.ErrUnsupported` (not a `libvirt.Error`), so `libvirtCode` misses it and it becomes Transient, while the same condition as `VIR_ERR_NO_SUPPORT` is Permanent. `ErrConfigUnsupported` (67) and `ErrOperationUnsupported` (84) are also unmapped. `Check` records `libvirtVersion` but gates nothing on a minimum. See S5.2 and escalation E9. + +## Chunk 4 — Base image cache and volumes (`image.go`, `volume.go`) + +**S4.1 — high — `image.go:273`** +`uploadImage` hands a `TeeReader` over `resp.Body` straight to `StorageVolUpload`. In go-libvirt (`rpc.go:290-308`, `socket/socket.go:347-376`) `abort` and `outErr` are unbuffered: on a reader error `SendStream` sends a `StatusError` packet then blocks forever on `outErr <-`, and `requestStream` blocks forever on `abort <- true`. `StorageVolUpload` never returns. +Scenario: mirror truncates the body (or libvirtd errors mid-upload, e.g. ENOSPC). kvm2's `call` rescues the caller only after `budgets.transfer` = 20 min, and `markDead`/socket close does not unblock either goroutine (contrary to the comment at `backend.go:152-156`). Consequences: 1-2 goroutines + 4 MiB buffer leaked per failed upload, accumulating under reconcile retries against a flaky mirror; a truncated download reported after 20 min instead of at once; `discardVolume` (`image.go:292`) deletes the base on the pooled connection before the deferred `fresh.Close()` (`image.go:265`), so the file is unlinked while the server-side stream holds the fd. `TestEnsureImage_shortReadRemovesVolumeAndIsTransient` gives false confidence: the fake returns the reader error immediately and never exercises the abort protocol. +Fix direction: have `countingReader` convert errors to `io.EOF` and report them out of band; the existing `readErr`/`read != size` checks still catch truncation. + +**S4.2 — medium — `image.go:434-439`, `391-396`** [T1] +A `HEAD` that answers 2xx with `Content-Length: 0` or none (Go's own `net/http` does this; several CDNs too) becomes `ErrPermanent`, which latches `ReasonPermanentError` for the generation. The GET fallback fires only on non-2xx HEAD, although GET on the same URL would work. The tests never hit it because `serveBytes` sets Content-Length on HEAD. + +**S4.3 — medium — `image.go:235-245`** [T4] +A completed upload is discarded when the pooled connection died during it. The base is uploaded on a dedicated connection, but `StoragePoolRefresh` and `writeMarker` then run on the pooled `be`, which any other reconcile's cancel can kill. On failure `discardVolume` deletes the fully downloaded, digest-verified base and the next Create re-downloads. The window is as wide as the download. The dedicated connection is still open at that point and could carry the refresh and marker. + +**S4.4 — medium — `image.go:419-443`, `226-230`, `create.go:96-106`** +The pool-space precheck uses the **HEAD** Content-Length; `fetchImage` declares the volume capacity from the **GET** Content-Length and never re-checks. There is no `maxImageBytes` cap. +Scenario: a server answers HEAD with 1 MB and GET with 4 TiB; the precheck passes, a sparse 4 TiB qcow2 is created and the stream fills the host pool until ENOSPC, degrading every VM on the hypervisor. Benign variant: a mirror rotates a build between HEAD and GET. + +**S4.5 — medium — `image.go:59-65`** +`placement.image` is an unvalidated user-supplied URL fetched by the operator. `PlacementSpec.Image` has no CRD validation; `IsHTTPURL` checks only scheme and non-empty host; `http.DefaultClient` follows redirects with no policy. Anyone who can create a Proxy in a watched namespace makes the operator issue HEAD/GET from its network position (cluster services, `169.254.169.254`) and makes the hypervisor boot the returned bytes on the egress bridge. URL images can never carry a `sha256`. + +**S4.6 — medium — `image.go:118`, `image.go:145`** (also: chunks 2, 7) [T3] +Any Create can evict a base volume that live overlays back onto, with no in-use check and only a V(1) log. `cachedImage` runs on every Create and deletes the base when the marker is missing, unreadable, non-hex, or when `src.SHA256` differs, then `fetchImage` recreates the same path with different content. +Scenario: an admin adds or updates `images..sha256` while Proxies exist and the mirror has republished. Running qemu processes keep the old inode; every later boot of an existing overlay reads a different base. The spec records this as a doc warning ("add a new key instead"); the kvm sample carries the warning (`config/samples/providers-config.yaml:40`) but the kvm2 block (`:72`) omits it. See escalation E7. + +**S4.7 — low — `image.go:62`** +A URL image is always assumed qcow2/uefi. A raw image given as `placement.image` is cached as `base-.qcow2` with `BackingFormat: "qcow2"`; qemu rejects the chain or the VM fails to boot, and the diagnostic points at the domain. + +**S4.8 — low — `volume.go:22-25`** [T3] +`urlImageKey` truncates sha256 to 64 bits and shares a namespace with catalogue keys. Two colliding URLs share one base (a ~2^32 birthday search for someone who can create Proxies), and URL images have no digest to catch it. A catalogue key that is 16 lowercase hex chars occupies the same `base-.` name as a URL image. + +Verified clean: HTTP bodies and the dedicated upload connection are closed on every path; `cappedWriter` and `countingReader` are correct as written. + +## Chunk 5 — XML and cloud-init seed (`xml.go`, `seed.go`, `metadata.go`, `buildVolumeXML`) + +**No injection defect found.** Every config- and user-influenced value reaching domain, volume or metadata XML goes through `encoding/xml` struct marshalling; there is no string concatenation into markup; `Name` is `NameFromUID`'s base32 hash. Chunks 2 and 7 reached the same conclusion independently. + +**S5.1 — medium — `create.go:199-201`, `seed.go:66-74`** [T1, inverted] +Every I/O failure inside `buildSeedISO` is turned into `ErrPermanent` by `permanentf("building seed ISO for %s: %v", ...)`: `os.MkdirTemp` when `/tmp` is missing, ENOSPC during `AddFile`/`WriteTo`, and via the deferred `errors.Join` even a `Cleanup()`/`RemoveAll` failure on an otherwise successful build. `providerFailure` then returns `ctrl.Result{}` with nil error: no requeue at all until a spec edit or the ~10 h resync. The `%v` also discards the wrapped error. The empty-name guard is correctly permanent. + +**S5.2 — medium — `xml.go:26-27`, `194-206`** (also: chunk 7) [T1] +`x86_64`, `q35`, `type="kvm"`, `host-passthrough`, `` (libvirt ≥ 5.2.0) and `` (≥ 7.2.0) are unconditional with no capability or version gate, and no config field or validator mentions architecture. +Scenario: an aarch64 hypervisor or a libvirt 7.0 host is added to `kvm2.hosts`; `Check` reports Ready; every Create there fails at `DomainDefineXML` with code 67 or 84, which `classifyLibvirt` leaves Transient, so the reconciler retries an impossible define indefinitely with no actionable condition. + +**S5.3 — low — `metadata.go:44-50`** (see escalation E2) +An unparsable `` returns `time.Time{}`; `gc.go:120` computes `now.Sub(CreatedAt) < MinAge`, so a mangled mark is immediately past the grace period, exactly the case MinAge protects. Mirror case: a mark written under a forward-skewed clock yields a negative age that is always `< MinAge`, so the domain is never collected. Only the first direction is mentioned in the doc comment. + +**S5.4 — low — `seed.go:48-53`** +The guest-agent script's non-systemd fallback runs `rc-update` unguarded (the kvm provider guards it with `elif command -v rc-update`). With `set -e`, an image with neither exits 127 after the package install; the agent is installed but never started and discovery silently falls back to ARP only. + +**S5.5 — low — `xml.go:178-193`** +`buildDomainXML` validates name, paths and disk format but not `VCPUs`/`MemoryMiB`; it will emit `0`. Unreachable today because `validateKVMBlock` checks them; latent for any future caller. + +**S5.6 — low — `volume.go:68-89`** (see escalation E10) +No check that an overlay's capacity is at least the backing image's virtual size. `kvm2.diskSizeGb: 2` against a 3.5 GiB Ubuntu image produces `` smaller than ``. + +**S5.7 — low — `internal/provider/config_kvm.go:186-188`** +`bridge` and `pool` are checked non-empty only, unlike host names and image keys (`kvmIdentifier`). A value with whitespace or markup is faithfully escaped into the XML and surfaces as an opaque libvirt failure at Create time rather than a config-load error. + +**S5.8 — low — `seed.go:107-108`** +A configured `vendorData` that is itself MIME multipart (or starts with any unrecognised line) is nested as `text/plain`, matches no cloud-init prefix, and is silently dropped when `installGuestAgent` is on. No guard in `buildVendorData`. + +Verified clean: the iso9660 staging dir is cleaned on every return path; volume XML sets no ``, so the seed ISO gets libvirt's 0600 default. + +## Chunk 6 — Health check and observability (`check.go`, `observe.go`) + +In good shape: the dedicated check connection is released on every path, every seam method is wrapped and forwards correctly (enforced by reflection in `observe_test.go:77`), spans always end, `liveness` survives the decorator, and no cloud-init or key material reaches a log or span at any verbosity. `go vet` and `go test -race` on the chunk were clean. + +**S6.1 — low — `check.go:90`, `backend.go:162`, `:351`** +`call()` returns at its pre-flight `ctx.Err()` check without `markDead`, so if the round deadline falls between two probe RPCs the link is still live and the deferred `Close()` sends `ConnectClose` on a ctx-less 5 s budget. `Check` can work past the deadline the Monitor gave it, against `provider.Checker`'s "must return promptly once ctx ends". Bounded at 5 s, narrow window. + +**S6.2 — low — `check.go:57-58`** +Reported `Latency` includes the teardown RPC because `defer b.Close()` runs inside `probeHost` before `time.Since(start)`. About one RTT slower than the spec's cost model. + +**S6.3 — low — `check.go:66`** +The round-wide `ctx.Err()` test masks an actionable per-host reason. Host A blackholes and eats the round; host B fails at the deadline with "no route to host" and is reported `Timeout`. Same for `PoolNotActive`/`CredentialsUnavailable` landing on the deadline. + +**S6.4 — low — `check.go:135-148` vs `internal/providerhealth/monitor.go:113`** +The Unreachable/Timeout distinction only holds while the round timeout exceeds the 10 s dial budget. `ValidateSchedule` accepts `--provider-check-interval=10s` or `--provider-check-timeout=10s`, under which a blackholed host reports `Timeout` instead of `Unreachable`. + +**S6.5 — low — `check.go:101` → `api/v1alpha1/providerhealth_types.go:65`** +The remote-controlled `hostname` is copied unbounded into ProviderHealth `details`; `Message` is truncated to 1024 but `details` has no `maxLength`/`maxProperties`. A multi-MB hostname makes every status patch fail "too large". Requires a broken or hostile hypervisor. + +**S6.6 — low — `check.go:41-45`** +Unbounded fan-out: one goroutine, key re-read and full SSH handshake per host per round, no cap. Harmless at 2-5 hosts. + +## Chunk 7 — Config, registration, deployment, test gating + +**S7.1 — high — `internal/provider/kvm2/volume.go:17`, `image.go:113-146`** (also: chunk 4) [T3] +The base image cache is keyed by image key + format only. `baseVolumeName` → `base-.`, and `internal/provider/kvm/image.go:46` produces the identical name. The shipped sample has `kvm-home` (`config/samples/providers-config.yaml:41`) and `kvm2-home` (`:73`) both with key `debian-13`, host `beelink`, pool `default`, and `:58-60` tells the admin to run both while porting. +Scenario: the two entries' URLs drift and neither sets `sha256`; `cachedImage` accepts any 64-hex marker when `src.SHA256 == ""`, so whichever provider downloaded first owns the file and the other boots the wrong image. Same for two `kvm2` entries naming one physical host. Related: changing `images..url` without changing the key or `sha256` is a silent no-op, since neither the volume name nor the marker records the URL. + +**S7.2 — medium — `internal/provider/config_kvm.go:180-182`** [T1] +`KVMHost.Address` is validated only as non-empty although it must be `host:port`; nothing applies a default port. `address: beelink` passes `LoadConfig`; `dialSSHSocket` (`ssh.go:163`) gets "missing port in address", `classify` falls through to Transient, every operation retries forever, and `checkReason` reports `Unreachable`, pointing at the hypervisor rather than the config. + +**S7.3 — medium — `internal/provider/config_kvm.go:101-104`, `142-147`** +`IsHTTPURL` accepts plain `http://` and `sha256` stays optional, so a VM root disk can come from an unauthenticated channel; `writeMarker` then pins whatever digest arrived. Nothing warns. + +**S7.4 — medium — `internal/provider/kvm2/fake_test.go:362-376`, `:259-282`** [T2] +The fake's `DomainGetMetadata` ignores the namespace of the stored element, returning it whenever `uri[0] == metadataURI`; real libvirt selects strictly by namespace. So no unit test can build a domain carrying the kvm provider's `` and assert `list.go:38-48` skips it. If that filter regressed, kvm2's `ListByTag` would hand kvm's VMs to orphan GC and the suite would stay green. + +**S7.5 — medium — `Makefile:75`** +`-timeout 40m` is not derived from `KVM2_TEST_TIMEOUT`. Two integration tests each take up to 20 m plus a 5 m cleanup budget; with `KVM2_TEST_TIMEOUT=40m` or a cold image download, go test's own timeout panics before `t.Cleanup` (`kvm2_integration_test.go:373-382`) runs, leaving a domain, overlay and seed on the real hypervisor under a random name printed only mid-run. + +**S7.6 — low — `internal/provider/config_kvm.go:133`, `internal/provider/kvm2/image.go:466`** [T1] +No upper bound on `diskSizeGb`, and `neededBytes` wraps: `diskSizeGb: 17179869184` (2^34) shifts to 0, so `checkPoolSpace` passes and the failure surfaces later as "capacity must be positive". `machineTypes` vcpus/memoryMiB have no upper bound either (`config_kvm.go:158-162`); `memoryMiB: 1024000` yields an internal libvirt error that is Transient and retried forever. + +**S7.7 — low — `internal/provider/kvm2/fake_test.go:190-208`** +The fake's `enter()` accepts calls on an expired ctx and never marks the connection dead, unlike `call()`. So `discardVolume`'s `context.WithoutCancel` (`image.go:331`) is not exercised: deleting it leaves every test green while production skips cleanup of a half-uploaded base on a cancelled Create. + +**S7.8 — low — `internal/provider/kvm2/fake_test.go:579-584`** +The fake's `Close()` only bumps a counter; it stays usable and `isDead()` stays false. A use-after-close of a dedicated connection passes in tests and fails in production. + +**S7.9 — low — `Makefile:60-75`, `.gitea/workflows/build.yaml:27-32`** +Nothing compiles the integration file: `go vet`/`go build`/`go test -short`/`make test` all run without `-tags=integration`. It compiles today (`go vet -tags=integration ./internal/provider/kvm2/` exits 0), but nothing keeps it that way. + +**S7.10 — low — `internal/provider/kvm2/image.go:228`, `283-289`, `385-395`** +The full image URL reaches log lines and error messages while the span deliberately carries only the host (`urlHost`). `IsHTTPURL` accepts userinfo, so `http://user:pass@mirror/...` lands in the error that `provider.Wrap` propagates. Whether it reaches Proxy status is unverified (dropped escalation D5). + +**S7.11 — low — `internal/provider/kvm2/kvm2_integration_test.go:89`, `:307`** +`KVM2_TEST_KEEP=0` or `false` enables keep, since the check is `!= ""`. And `if elapsed > 100*time.Millisecond` on `New()` is a wall-clock `t.Errorf` on a possibly loaded machine, a flaky failure at the head of a 20 minute test. + +**S7.12 — low/informational — `config/manager/manager.yaml:175-178`, `internal/provider/kvm2/seed.go:56-74`** +The seed ISO staging dir is the `/tmp` emptyDir with no `medium: Memory`, so Proxy cloud-init payloads (which can carry proxy credentials) are written unencrypted to node-local disk for the duration of each Create. The mount comment (`manager.yaml:163-165`) attributes `/tmp` to the kvm provider only. + +Verified clean: Secret mount permissions (x/crypto does no permission check); both `Close` defers on dedicated connections.