kvm2: record the review validation, enable the provider entry, changelog
Closes out the review-fix campaign. The kvm2 provider was deployed in-cluster and validated against beelink -- the first time any of this code had run against real libvirt rather than the in-memory fake. config/manager/providers_config.yaml ships the kvm2-home entry enabled, since the operator is now running it for real. review/ carries the code review the four H-commits reference, so H1..H4 resolve to something rather than dangling: scope, the seven-chunk sweep, the escalations, the ten deep dives and the final report. .claude/agents carries the two reviewer definitions that produced them. What the validation did not prove is recorded next to what it did: H2's error paths need a body that fails part way through, and a healthy 433 MB download never finishes short, so the unverified libvirtd short-stream behaviour is still open. The kvm2 integration test has also still never been run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
+265
@@ -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 `<uid>` 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-<key>.<format>` 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 <beelink-ip>: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 |
|
||||
| `<uid> X </uid>` or newline-padded | padded | no | **DELETE** |
|
||||
| uppercase UID | differs | no | **DELETE** |
|
||||
| `<uid> </uid>` (whitespace only) | `" "` | no | **DELETE** |
|
||||
| `<uid></uid>` | `""` | no | KEEP (guard `gc.go:113`) |
|
||||
| bad/missing `createdAt`, UID clean | clean | yes | KEEP |
|
||||
| padded `<managed>` | clean | no | KEEP (not listed) |
|
||||
| future-dated, UID clean | clean | no | KEEP |
|
||||
|
||||
`encoding/xml` does not trim chardata. **Path:** padded or case-changed `<uid>` with `<managed>true</managed>` 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 <scratch-dom> https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/kvm2 --key kvm2 --set '<instance><managed>true</managed><uid> 3f1c2a9e-0000-4000-8000-000000000001 </uid><createdAt>2026-09-01T10:00:00Z</createdAt></instance>' --config
|
||||
virsh -c qemu:///system metadata <scratch-dom> 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-<key>.<format>` cache — CONFIRMED** (kvm2 code + libvirt 11.6.0 source, `storage_driver.c`). The marker protocols are compatible (both write `<base>.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 `<base>.part-<random>`, 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 `<allocation>` 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.<key>.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|<path>"
|
||||
virsh -c qemu:///system vol-delete --pool $P $V; echo "delete exit=$?"
|
||||
virsh -c qemu:///system vol-list $P | grep review-e10- || echo "clean"
|
||||
```
|
||||
Reference in New Issue
Block a user