kacerr/egress-proxies-operator
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Compare commits

base: kacerr/egress-proxies-operator:2510842396bdfa0b0d3471b8149def1461e48f4d
Branches Tags
kacerr/egress-proxies-operator:feat/proxy-operator kacerr/egress-proxies-operator:main kacerr/egress-proxies-operator:feat/proxmox-provider kacerr/egress-proxies-operator:feat/kvm2-provider kacerr/egress-proxies-operator:feat/provider-blueprint kacerr/egress-proxies-operator:feat/provider-health kacerr/egress-proxies-operator:feat/kvm-provider kacerr/egress-proxies-operator:feat/otel-tracing kacerr/egress-proxies-operator:feat/demo-scripts kacerr/egress-proxies-operator:feat/gitea-build-workflow
kacerr/egress-proxies-operator:0.01
...
compare: kacerr/egress-proxies-operator:feat/kvm2-provider
Branches Tags
kacerr/egress-proxies-operator:main kacerr/egress-proxies-operator:feat/proxmox-provider kacerr/egress-proxies-operator:feat/kvm2-provider kacerr/egress-proxies-operator:feat/provider-blueprint kacerr/egress-proxies-operator:feat/provider-health kacerr/egress-proxies-operator:feat/kvm-provider kacerr/egress-proxies-operator:feat/proxy-operator kacerr/egress-proxies-operator:feat/otel-tracing kacerr/egress-proxies-operator:feat/demo-scripts kacerr/egress-proxies-operator:feat/gitea-build-workflow
kacerr/egress-proxies-operator:0.01
11 Commits
2510842396 ... feat/kvm2-provider
Author SHA1 Message Date
kacerrandClaude Opus 5 54525bf67a kvm2: gofmt volume.go 
Making baseVolumeName multi-line in the H4 commit collapsed the
alignment gofmt gives a run of single-line funcs, and the commit went out
with the old spacing. Caught by make test, whose fmt target rewrites the
file and leaves the tree dirty.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 21:07:10 +00:00
kacerrandClaude Opus 5 41fbd5e7e6 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>
 2026-09-17 20:50:45 +00:00
kacerrandClaude Opus 5 4114d43e87 Route klog and net/http's own errors into the structured logger 
Some lines ignored the zap encoder entirely, so `kubectl logs | jq` could
never work:

  I0917 19:50:02.580097  1 leaderelection.go:258] "Attempting to acquire
  leader lease..." lock="egress-proxies-operator-system/b47711d1..."

That is klog's own stderr writer, not ours. client-go's leader elector
calls klog.FromContext(ctx), and controller-runtime hands the elector a
context with no logr in it -- mgr.Start receives ctrl.SetupSignalHandler(),
which carries no logger. FromContext falls back to Background(), and with
no logger registered that is klog's text sink. Nothing here imported or
configured klog; it was an indirect dependency that around 60 linked
packages use, including client-go/tools/{leaderelection,cache,record} and
client-go/transport. controller-runtime does not route it either -- its
only klog.SetLogger call is in its own test suite.

Plain klog.SetLogger, deliberately not SetLoggerWithOptions with
ContextualLogger(true). The contextual form is what klog's docs recommend
because it skips a klogr round trip, but it also disables klog's verbosity
gate and hands the decision to zap, whose level under Development is Debug
-- so client-go's V(1) chatter, suppressed today by klog's own -v=0, would
have started appearing. Plain SetLogger keeps klog's check in front and
still redirects what it does emit. Same fix to the format, no new volume,
which matters because volume is what prompted this.

The discovery API's http.Server set no ErrorLog, so net/http's own faults
(a failed Accept, a panic escaping a handler) went to the stdlib logger on
stderr. It now drains into logr through logr.ToSlogHandler, the pattern
internal/provider/gcp/wirelog.go already uses for the same purpose.

The deployment gets --zap-encoder=json and --zap-time-encoding=rfc3339.
The flag beats the in-code Development default -- addDefaults fills
NewEncoder only when it is nil -- and the time encoding is not optional,
since the JSON encoder defaults to epoch seconds. Verbosity stays at debug
on purpose; --zap-log-level=info drops the per-RPC lines.

zap.Options{Development: true} stays as the in-code default: flipping it
would change the format for every deployment of this operator, and the
flags get the same result here without that reach.

k8s.io/klog/v2 moves from indirect to direct in go.mod. It was already in
the module graph, so nothing new is downloaded.

No tests: three changes, all wiring, in a file with no test coverage, and
pinning klog.SetLogger means mutating process-global state that leaks into
every other test in the package. Verified in-cluster instead --
`kubectl logs | jq -e -s 'length > 0'` now passes, which fails if any
single line is not JSON.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 20:24:37 +00:00
kacerrandClaude Opus 5 82d01efb96 Plan routing klog into the structured logger (Step 6) 
Found while watching the Step 5 validation, not a review finding: the
leader election lines ignore the zap encoder because they are not ours.
client-go calls klog.FromContext, controller-runtime hands the elector a
context with no logr in it, and klog falls back to its own stderr writer.
Nothing in the repo configures klog; it is an indirect dependency that
~60 linked packages use.

Plain klog.SetLogger rather than the contextual form the klog docs
recommend: the contextual option disables klog verbosity checking and
hands the decision to zap, which at Development level would let
client-go V(1) chatter through that klog suppresses today. Same fix to
the format, no new volume.

Also the one other in-process stderr bypass (the discovery http.Server
has no ErrorLog) and json/rfc3339 encoder flags on the deployment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 20:05:02 +00:00
kacerrandClaude Opus 5 966807bf57 Plan the in-cluster kvm2 validation (Step 5) 
First contact between the kvm2 provider and a real hypervisor: it has
never run against libvirt, so H1-H4 landed on code no host has executed.

The run happens in the existing kind cluster on purpose. That cluster
already owns two live kvm-home VMs on beelink, and orphan GC deletes any
instance whose UID is missing from its own cluster live set after
gc-min-age. A second operator carrying the kvm-home entry would therefore
destroy both within ten minutes; using the existing cluster makes its
live set the thing that protects them.

The plan also records what the run cannot prove: H2 error paths need a
body that fails part way through, and a healthy download never finishes
short, so the unverified libvirtd short-stream behaviour stays open.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 19:23:44 +00:00
kacerrandClaude Opus 5 c10f98a352 kvm2: name the cached base image after the provider entry (H4) 
baseVolumeName was base-<key>.<format>, byte-identical to what the kvm
provider builds in the same pool -- for URL images too, since kvm takes
hex(sum[:8]) and kvm2 takes hex(sum[:])[:16], the same eight bytes -- with
a completely separate lock table. The shipped config has a live kvm-home
entry and a commented-out kvm2-home entry on the same host, the same key
debian-13 and the same default pool, and the comment invites the operator
to enable it. Both integration tests use key test-image. On beelink both
live overlays back onto kvm's base-debian-13.qcow2.

Since cachedImage accepts any 64-hex marker when no sha256 is configured
and never re-hashes the base, that meant two entries pointing one key at
different URLs silently shared a file; a concurrent cold fetch let one
entry delete the other's half-uploaded base and boot a guest from the
remains; and a sha256 edit deleted the base in place under live overlays,
which libvirt permits without complaint.

The name now carries the provider entry: base-<entry>-<key>.<format>.
imageSource gained an Entry field and resolveImage takes the provider
name, so every production call site follows from imageSource.volumeName().
That closes the cross-entry sharing outright, kvm<->kvm2 and kvm2<->kvm2
alike, and turns the per-Provider imageLocks from a latent bug into a
correct one: with names unique per entry, two entries cannot contend for a
file, so no process-wide lock table is needed.

The entry name now reaches a file name, so New checks it against the
existing kvmIdentifier rule, exported as provider.IsKVMIdentifier rather
than duplicated. Provider names are otherwise validated for all types at
once and only for being non-empty and unique, so a name with a slash would
have produced a volume nothing could address.

One companion fix, because the naming alone does not stop it:
cachedImage's digest-mismatch branch deleted the marker and the base and
refetched. It now returns ErrPermanent naming both digests and telling the
operator what to do. That branch was the remaining path that unlinks a
good file under running domains, and the contract already said "add a new
key instead" -- the code was contradicting it. The no-marker and
non-hex-marker branches keep deleting, since there the base is debris.

Not fixed, and said so in the docs rather than left to look closed:
editing images.<key>.url is still a silent no-op, since nothing records
the URL; and cross-process fencing still needs the deferred atomic publish
(upload to a temp name, verify, publish), which lands where H2 put the
discard.

Migration is a non-event. The base-debian-13.qcow2 on beelink is kvm's,
kvm keeps using it untouched, and kvm2 downloads its own copy once per
host. Nothing is replaced under a running guest; the pool needs room for
both, which the prerequisites now say.

The eviction recipe in the guide named base-debian-13.qcow2, which after
this change is kvm's file -- following it would have deleted the wrong
provider's base. It now greps for the prefix and deletes the marker first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 17:57:02 +00:00
kacerrandClaude Opus 5 9682737c69 kvm2: an unreadable address is not a missing one (H3) 
address() tried the guest agent then the host's ARP table and skipped any
error that was not a context error, returning ("", nil) after both. An
active domain with no IP maps to StateProvisioning; the controller clears
status.IP on that, and the health engine prunes the proxy's counters and
calls ForgetProxy in one tick once EffectiveHost() goes empty.

The damaging trigger was not the guest. The libvirt connection is pooled
per host, so when another reconcile's cancel or budget overrun killed it,
call returned "connection is closed" -- carrying no context sentinel, so
the check did not recognise it -- and Get returned a *successful*
instance in Provisioning with an empty IP. An unrelated proxy's timeout
dropped a healthy proxy out of rotation.

The discriminator was already in the spec without the conclusion being
drawn: during a normal boot on beelink exactly one source fails. The
agent errors because it is not up yet, while ARP answers without error
and with nothing, since the host never sees the guest's traffic. So one
source failing stays "no address yet"; every source failing, or a
connection already dead, is ErrTransient.

The explicit provider.ErrTransient wrap is load-bearing, not decoration.
Without it classify picks the first libvirt code out of the joined error,
and ErrInvalidArg, ErrAccessDenied and ErrNoSupport are Permanent -- an
address read failing would latch a running proxy as Failed, a worse
version of the harm being fixed.

Half of the finding is deliberately still open, and it is the half that
fires more often: a guest agent restarting inside a running VM still
demotes it, because that is indistinguishable from a VM still booting.
The discriminator is the previous IP, which the provider does not hold by
design and the controller already has. Follow-up finding to file against
proxy_controller.go:208-211 -- on StateProvisioning, keep an existing IP
and Provisioned condition for a few polls instead of clearing on the
first. The kvm2-local alternative (use the mark's createdAt as a
provisioning window) was rejected: createdAt is creation, not boot, and
it invents a deadline that belongs to the controller.

TestGet_addressErrorIsProvisioning failed against the new rule, correctly
-- it fails both sources with ErrAgentUnresponsive, and code 86 is
agent-specific, so the ARP source cannot produce it. Re-pointed as
TestGet_agentFailsAndARPEmptyIsProvisioning, the beelink boot it was
reaching for, which nothing covered before.
TestGet_everyAddressSourceFailingIsTransient covers the other side, both
sources erroring and the pooled link dying under the read; against the
unfixed code both return a Provisioning instance instead of an error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 17:48:57 +00:00
kacerrandClaude Opus 5 62561c21fe kvm2: stop handing go-libvirt a reader that can error (H2) 
uploadImage streamed the HTTP body straight into StorageVolUpload. When
that body failed part way through, go-libvirt deadlocked: requestStream
starts its stream sender on two unbuffered channels and, on the error
path, returns without ever reading outErr, so SendStream emits a
StatusError packet and parks on `outErr <-` forever while the main
goroutine parks on `abort <- true` forever. StorageVolUpload never
returned. The 20-minute transfer budget rescued the caller only, and the
socket kill did nothing, because both ends are blocked on channel sends
rather than on I/O -- which is what the comment at backend.go:152 got
wrong. Each failed upload leaked kvm2's call goroutine, go-libvirt's
sender goroutine and a 4 MiB buffer, permanently, against a 128Mi pod
limit and a reconciler that retries on backoff.

The defect is upstream and stays there: no fork, no replace. Instead
eofReader, at the go-libvirt seam so it covers the seed and marker
uploads too, converts any source error to io.EOF -- the one outcome that
ends a stream the normal way -- and keeps the real error for the caller.
countingReader sits underneath untouched, so the existing length and
digest checks report exactly what they did before.

The download also gets its own deadline a minute inside the transfer
budget. fetchImage already bounded the whole fetch at 20 minutes, so the
body's death and the budget's socket kill were racing and the socket
usually won; with the body a minute ahead, a stalled mirror ends through
its reader and unwinds cleanly instead of leaking.

discardVolume moves from uploadImage into fetchImage's upload-error
branch, so the dedicated connection is closed and libvirtd has let go of
the stream's fd before the file is unlinked. Moving it rather than adding
a second Close keeps image_test's closeCount() == 1 true. That discard is
still unfenced against a second provider entry sharing the volume name;
H4 closes that by making the name per-entry.

What remains: a failure originating in libvirtd mid-upload still leaks,
since SendStream has already returned by then. Confirmed unfixable from
here. Known gaps quotes the two-character upstream fix
(make(chan bool, 1) / make(chan error, 1)) and the replace escape hatch.

Unverified at ~85%: that libvirtd answers a short-but-clean stream finish
with OK rather than an error. virFDStreamWrite enforces length only on
writes past it and nothing compares offset to length on the close path,
which is also why virsh vol-upload of a small file into a large volume
works. If it errors we are back to a deadlock by a different door, which
is exactly today's behaviour, so this cannot be worse. The execution file
carries a probe recipe to settle it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 17:46:15 +00:00
kacerrandClaude Opus 5 d826114fd3 Plan the kvm2 H2, H3 and H4 fixes 
Extends the campaign plan with the three remaining high findings, each its
own step and commit, in the order H2 -> H3 -> H4 (H2 and H4 both touch
image.go, and H2 restructures the failure path H4 would otherwise be
written against).

H2 is a workaround, not a repair: the deadlock is upstream in go-libvirt,
whose requestStream never drains an unbuffered outErr on its error path.
Per the decision taken while planning, no fork and no replace directive;
we stop handing it a reader that can error, move the transfer deadline
onto the HTTP source so a stalled mirror unwinds cleanly, and document the
residual with the two-character upstream patch.

H4 names the base image per provider entry, which is what was chosen over
a content fingerprint. That fixes cross-entry sharing but not the silent
no-op on a url edit, so the plan says so rather than implying the finding
is closed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 17:42:34 +00:00
kacerrandClaude Opus 5 0239fb7e69 kvm2: do not let a stalled write delay the connection teardown (H1) 
sshSocket.Close was errors.Join(s.Conn.Close(), s.client.Close()). Go
evaluates those left to right, so the blocking close ran before the
unblocking one. s.Conn is the forwarded libvirt socket, and closing an SSH
channel writes a close message under ch.writeMu — which go-libvirt already
holds while streaming a base image through that same channel. Against a
host that vanished without RST or FIN, that write blocks until the kernel
stops retransmitting (tcp_retries2, ~15 min), and s.client.Close(), a bare
net.Conn.Close() that takes no lock and unblocks the stalled writer through
netpoll, was never reached. Close the client only; nothing wants the polite
channel-close message, and reordering instead would leave an error to
filter in a memoised closeErr.

link.markDead compounded it by running kill() before close(l.closed), both
inside a sync.Once. Reordering alone was not enough: sync.Once.Do blocks
concurrent callers until the first body returns, so every other markDead —
including the one in call is own ctx-cancel branch — queued behind the
stalled kill, and cancelling the reconcile did not help. The once field is
gone; the mutex the struct already had guards the close-exactly-once
invariant, reason is set before the signal, and kill runs outside the lock.

Blast radius was the whole operator, not one host: MaxConcurrentReconciles
is 3 and ensureImage holds a per-image lock whose acquire is ctx-aware but
reconcile contexts carry no deadline, so three Proxies on one host with an
uncached image parked every worker.

The same edits close a second route to the same pin: go-libvirt is
Disconnect closes the net.Conn it was handed (libvirt.go:331 ->
socket.go:164), and that conn is our sshSocket, so libvirtBackend.Close is
Disconnect goroutine entered closeOnce and hung; the 5 s closeBudget
released Close, which called markDead -> kill -> sshSocket.Close and
blocked in the other goroutine is sync.Once.

Verified against the pinned x/crypto v0.54.0 rather than assumed: the
flow-control park (remoteWin.reserve, channel.go:268) is outside writeMu
and only writePacket holds it across the TCP write, so a writer parked in
SSH flow control never blocks the close and a writer inside the TCP write
always does. To reproduce by hand, shrink SO_SNDBUF well under the 2 MiB
channel window (channelWindowSize = 64 * channelMaxPacket) so the socket
buffer fills before the window does.

The kvm provider does not have this bug: it closes client only and never
touches the forwarded channel.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 15:30:41 +00:00
kacerrandClaude Opus 5 85ea319cc2 Plan the kvm2 code-review fixes 
The review of feat/kvm2-provider confirmed 52 findings (4 high, 20 medium,
28 low); review/REPORT.md has them. This is the campaign plan: one step and
one commit per finding, highest severity first, with only the step in hand
detailed so the plan is not a fiction about work not yet designed. Same
shape as the kvm campaign in 2026-09-10-2222-kvm-review-fixes.md.

Step 1 (H1) is detailed: sshSocket.Close runs the blocking close before the
unblocking one, so a transfer stuck in a TCP write pins the teardown for as
long as the kernel retransmits, and markDead compounds it by killing inside
a sync.Once before signalling dead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-09-17 15:27:04 +00:00
35 changed files with 2694 additions and 141 deletions
Expand all files Collapse all files
.claude/agents/deep-reviewer.md
+11
View File
@@ -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.
.claude/agents/sweep-reviewer.md
+23
View File
@@ -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.
CHANGELOG.md
+40
View File
@@ -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-<entry>-<key>.<format>`), 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
cmd/main.go
+10 -1
View File
@@ -38,6 +38,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
"k8s.io/klog/v2"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -162,7 +163,15 @@ func main() {
os.Exit(0)
}
ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))
logger := zap.New(zap.UseFlagOptions(&opts))
ctrl.SetLogger(logger)
// client-go logs through klog, and controller-runtime hands the leader
// elector a context with no logr in it, so klog.FromContext falls back
// to klog's own stderr writer and those lines ignore the encoder
// entirely. Plain SetLogger rather than the contextual form: it keeps
// klog's verbosity check in front, so this changes the format of what
// klog already emits without letting client-go's V(1) chatter through.
klog.SetLogger(logger)
setupLog.Info("Starting egress-proxies-operator",
"commit", version.Resolve(), "goVersion", goruntime.Version())
config/manager/manager.yaml
+7
View File
@@ -64,6 +64,13 @@ spec:
- --leader-elect
- --health-probe-bind-address=:8081
- --providers-config=/etc/proxy-operator/providers.yaml
# Structured logs: every line is one JSON object, so `kubectl logs
# | jq` works. The time encoding is not optional — the JSON encoder
# defaults to epoch seconds. Verbosity stays at the binary's
# default (debug, one line per provider API call); add
# --zap-log-level=info for outcomes only.
- --zap-encoder=json
- --zap-time-encoding=rfc3339
# Add --trace-health-probes to emit one span per health probe
# (high volume: ~1/s per proxy).
# Provider health checks (kubectl get providerhealth) run every
config/manager/providers_config.yaml
+31 -29
View File
@@ -53,33 +53,35 @@ data:
# pool: default
# kvm2 takes the same block shape as kvm: an entry ports by changing
# type to kvm2 and the block name to kvm2. kvm2 does not adopt domains
# made by kvm, so delete those Proxies first. 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
# made by kvm, so delete those Proxies first. It caches its base images
# under base-<entry>-<key>.<format>, 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
config/samples/providers-config.yaml
+3 -1
View File
@@ -57,7 +57,9 @@ providers:
# pool: default
# kvm2 takes the same block shape as kvm: an entry ports by changing type
# to kvm2 and the block name to kvm2. kvm2 does not adopt domains made by
# kvm, so delete those Proxies first. See docs/kvm2-provider.md.
# kvm, so delete those Proxies first. It caches its base images under
# base-<entry>-<key>.<format>, 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:
docs/kvm2-provider-spec.md
+21 -9
View File
@@ -74,11 +74,18 @@ field and that catalogue edits affect only new VMs.
`sha256`, `format` qcow2 or raw, `firmware` uefi or bios) or a URL given
directly in `placement.image`. The provider downloads the image into the
host's storage pool on first use and caches it there as a base volume named
`base-<key>.<format>` (for a URL: `base-<first 16 hex of sha256(url)>.<format>`),
with a sidecar marker volume `base-<key>.<format>.sha256` holding the digest of
the verified upload, written last. A base volume without a marker is
`base-<entry>-<key>.<format>` (for a URL: `<key>` is the first 16 hex of
sha256(url)), where `<entry>` is the provider entry's name, with a sidecar
marker volume `base-<entry>-<key>.<format>.sha256` holding the digest of
the verified upload, written last. The entry is in the name because the `kvm`
provider builds `base-<key>.<format>` in the same pool from the same catalogue
shape, and two entries pointing one key at different URLs would otherwise share
a file; the cost is one copy per entry per host, and the entry's name is checked
against `kvmIdentifier` at construction. A base volume without a marker is
incomplete and is deleted and redone. When `sha256` is configured and differs
from the marker's content, the image is re-downloaded. The download streams
from the marker's content, the image is **not** replaced: that is `ErrPermanent`
naming both digests, because overlays back onto the base file and libvirt
unlinks it under running domains without complaint. The download streams
from the URL through the operator into `StorageVolUpload` with a running
sha256; nothing is buffered. `Content-Length` is required because the volume's
capacity must be declared before the upload; a response without it is
@@ -89,8 +96,9 @@ Proxy cannot cut it off. After a successful upload `StoragePoolRefresh` makes
the file usable as a backing image.
Catalogue edit warning for the guide: overlays back onto the base file, so
replacing or evicting a base volume while Proxies use it corrupts those VMs;
add a new key instead.
evicting a base volume while Proxies use it corrupts those VMs. A `sha256`
change is refused for that reason; a `url` change is a silent no-op, since the
cache is keyed by entry and key. Add a new key for either.
**Evidence:** kvm-provider.md "The image cache" and "Known gaps"; execution log
of 2026-09-04 (raw qcow2 upload plus pool refresh accepted by libvirt 11.6;
@@ -146,8 +154,12 @@ cluster can route to. The address comes from `DomainInterfaceAddresses`, guest
agent source first, then the ARP table (on beelink the agent answered about
20 s after start where ARP never did, because the host itself does not talk to
the guest). The first IPv4 that is not loopback, link-local or unspecified is
reported; errors from either source mean "no address yet" and `Get` returns
`StateProvisioning`. The guest needs the agent: it is installed by the
reported. One source failing means "no address yet" and `Get` returns
`StateProvisioning`, which is the ordinary boot on beelink: the agent is not up
while ARP answers with nothing. A connection already gone, or every source
failing, is `ErrTransient` instead — a running VM whose address cannot be read
is not a VM without an address, and reporting it as one would make the
reconciler clear its IP and the health engine drop its counters. The guest needs the agent: it is installed by the
vendor-data script (entry 5) unless `installGuestAgent: false`, which needs an
image that ships it or a network where ARP sees guest traffic. `Port` is not
used by the provider; nothing on the host filters it. Reachability is a
@@ -377,7 +389,7 @@ read `kvm2.hosts` and not `kvm.hosts`.
| `diskSizeGb` | | 10 | not negative | overlay capacity, quota precheck |
| `vendorData` | | | | seed `vendor-data` |
| `installGuestAgent` | | true | | seed `vendor-data` |
| `images.<key>` | | | key matches `kvmIdentifier` | volume name `base-<key>.<format>` |
| `images.<key>` | | | key matches `kvmIdentifier` | volume name `base-<entry>-<key>.<format>` |
| `images.<key>.url` | yes per entry | | http(s) URL | download |
| `images.<key>.sha256` | | | 64 hex | marker comparison |
| `images.<key>.format` | | qcow2 | qcow2 or raw | base volume XML, overlay backing format |
docs/kvm2-provider.md
+92 -17
View File
@@ -37,7 +37,8 @@ Per host, once:
API. Nothing on the host filters the port; the provider does not use
`spec.port` at all.
4. **A storage pool** (default `default`, usually `/var/lib/libvirt/images`)
with room for one base image per distro (a few hundred MB) plus the
with room for one base image per distro and provider entry (a few
hundred MB each) plus the
per-VM overlays (`diskSizeGb` each, thin-provisioned). The pool must be
running: a stopped pool fails the health check with `PoolNotActive` and
fails `Create` permanently.
@@ -202,13 +203,22 @@ host's pool:
| Object | Name | Lifetime |
|---|---|---|
| base image | `base-<key>.<format>` (for a URL image, `<key>` is the first 16 hex characters of the URL's sha256) | cached; survives Proxy deletion |
| image marker | `base-<key>.<format>.sha256`, a 64-byte raw volume holding the digest | with the base image |
| base image | `base-<entry>-<key>.<format>`, where `<entry>` is the provider entry's name (for a URL image, `<key>` is the first 16 hex characters of the URL's sha256) | cached; survives Proxy deletion |
| image marker | `base-<entry>-<key>.<format>.sha256`, a 64-byte raw volume holding the digest | with the base image |
| overlay disk | `proxy-abc….qcow2`, qcow2 backed by the base file, `diskSizeGb` capacity | with the Proxy |
| seed ISO | `proxy-abc…-seed.iso`, raw | with the Proxy |
| domain | `proxy-abc…` | with the Proxy |
The names are built in `internal/provider/kvm2/volume.go`.
The names are built in `internal/provider/kvm2/volume.go`. The base image
carries the **provider entry's name** because the file is shared by every
Proxy on that host but must not be shared by two entries: the `kvm`
provider builds `base-<key>.<format>` in the same pool from the same
catalogue shape, and two kvm2 entries pointing one key at different URLs
would otherwise agree on the name and disagree on the contents. The cost
is one copy of each image per entry per host. For that reason the entry's
name must itself be a usable file name — letters, digits, dots, dashes and
underscores — which `New` checks at startup, since provider names are
otherwise only required to be non-empty and unique.
The domain (`buildDomainXML` in `xml.go`) is a `q35` machine with a
host-passthrough CPU, ACPI and APIC, memory in MiB and vcpus from the
@@ -285,6 +295,18 @@ operations share. A cancelled or overlong call kills the connection it
ran on (see *Timeouts and connections*), and a poll for some other Proxy
being cancelled must not cut off a 20-minute upload.
A body that fails part way through is reported to the caller **out of
band**: the reader handed to `StorageVolUpload` turns any source error
into a clean end of stream and keeps the real error for afterwards, so
the short upload is caught by the length and digest checks rather than
by the stream. This is not tidiness. go-libvirt answers a reader error
by parking its stream sender forever on a channel it no longer reads, so
handing it one wedges the call and leaks the sender with its 4 MiB
buffer; see *Known gaps*. For the same reason the download carries its
own deadline, a minute inside the transfer budget, so a stalled mirror
ends through its body rather than through the killed socket the budget
would otherwise leave behind.
The download needs a `Content-Length`: the volume's capacity is declared
before the upload starts. The request sends `Accept-Encoding: identity`
so a transparently gzipped body cannot hide the length. A response
@@ -306,16 +328,27 @@ the rest find the marker.
To evict manually:
```sh
ssh novakj@beelink virsh -c qemu:///system vol-delete --pool default base-debian-13.qcow2.sha256
ssh novakj@beelink virsh -c qemu:///system vol-delete --pool default base-debian-13.qcow2
ssh novakj@beelink virsh -c qemu:///system vol-list --pool default | grep base-kvm2-home-
ssh novakj@beelink virsh -c qemu:///system vol-delete --pool default base-kvm2-home-debian-13.qcow2.sha256
ssh novakj@beelink virsh -c qemu:///system vol-delete --pool default base-kvm2-home-debian-13.qcow2
```
**Warning: do not change an image entry's `url` or `sha256`, and do not
evict its base volume, while Proxies using that image exist on the
host.** The overlays back onto the base file; deleting or replacing it
corrupts every VM that uses it. Delete those Proxies first, or add the
new image under a new key and move Proxies over by changing their
`placement.image` (which replaces the VM).
Marker first: a base without its marker is refetched, a marker without its
base is cleaned up, and neither leaves a Proxy pointing at nothing.
**Warning: do not evict a base volume while Proxies using that image exist
on the host.** The overlays back onto the base file, and libvirt will
unlink it under running domains without complaint; the guests keep running
on the open file descriptor and fail at their next start. Delete those
Proxies first.
Changing `sha256` on an entry whose base is already cached is **refused**,
permanently and by name, rather than resolved by replacing the file — that
replacement was the same corruption, just automated. Add the image under a
new key and move Proxies over by changing their `placement.image` (which
replaces the VM), or delete the Proxies and evict the base by hand.
Changing `url` alone does nothing at all: the cache is keyed by entry and
key, so the new URL is never fetched. Add a new key for that too.
## Quota: the pool-space precheck
@@ -369,6 +402,13 @@ A `kvm` domain left behind on a host is invisible to kvm2 forever; clean
it up on the host with `virsh undefine --nvram <name>` and
`virsh vol-delete` for its `<name>.qcow2` and `<name>-seed.iso`.
The base image cache is **not** shared between the two types, and that is
deliberate: kvm names its copy `base-<key>.<format>` while kvm2 names its
own `base-<entry>-<key>.<format>`, so kvm2 downloads the image once more
per host rather than adopting a file another provider maintains. Nothing
needs deleting; the `kvm` base stays where it is and keeps serving the
overlays that back onto it. Budget the extra few hundred MB in the pool.
## Logging
The provider logs through controller-runtime's logger, named `kvm2`, with
@@ -450,9 +490,15 @@ applied by `call`):
go-libvirt's calls take no context and block, so the only way to abandon
one is to close the connection under it. That is what a cancelled or
overlong call does: the adapter kills the connection (socket and SSH
client), returns `ErrTransient` wrapping the context or deadline error,
and marks the connection dead so the next call redials. The consequence:
overlong call does: the adapter marks the connection dead so the next call
redials, kills it by closing the SSH client's TCP connection, which takes
the forwarded socket with it, and returns `ErrTransient` wrapping the
context or deadline error. It marks dead before it kills, and closes the
TCP connection rather than the forwarded channel, because both of those are
lock-free: closing the channel writes a close message under the channel's
write lock, so it would queue behind a transfer stuck in a TCP write to a
host that has gone away — for as long as the kernel retransmits, which is
the one case the kill exists to survive. The consequence:
**other RPCs in flight on that host's pooled connection fail once with
`ErrTransient`** and their callers retry on a fresh connection. In the
reconciler that is one requeue with backoff; in GC it is one skipped
@@ -528,13 +574,37 @@ waits 5 minutes on `ErrQuotaExceeded`, recreates on `ErrNotFound` from
| `ErrPermanent` | placement values not in the catalogue; a malformed providerID; a host no longer in the config (from `Get`; `Delete` treats it as done); libvirt `ErrXMLError`, `ErrNoSupport`, `ErrInvalidArg`, `ErrAuthFailed`, `ErrAccessDenied` and `ErrNoStoragePool`; a pool that is not running; SSH authentication refused (`unable to authenticate`); a host key missing from known_hosts or changed (`knownhosts.KeyError`); a key or known_hosts file that exists but does not parse; a domain of the same name owned by another UID or carrying no kvm2 mark; a domain of the same name that is paused or pmsuspended; corrupt metadata on a domain; an image URL answering 4xx, a response without `Content-Length` or with length 0, a sha256 that does not match the configured one |
| `ErrQuotaExceeded` | the pool-space precheck |
| `ErrNotFound` | libvirt `ErrNoDomain` or `ErrNoStorageVol` from `Get`, `Delete` or `ListByTag`; inside `Create` the same codes are `ErrTransient`, because the contract forbids NotFound from Create |
| `ErrTransient` | everything else: a missing key or known_hosts file (the Secret has not landed yet), TCP and network errors, context cancellation, an abandoned or budget-exceeded call, a connection killed by another caller, libvirt `ErrOperationInvalid` and every unrecognised libvirt code, an image URL answering 429 or 5xx or ending the body early, the wait for running expiring or the domain going shut off or crashed right after its start |
| `ErrTransient` | everything else: a missing key or known_hosts file (the Secret has not landed yet), TCP and network errors, context cancellation, an abandoned or budget-exceeded call, a connection killed by another caller, libvirt `ErrOperationInvalid` and every unrecognised libvirt code, an image URL answering 429 or 5xx or ending the body early, every address source failing on an active domain, the wait for running expiring or the domain going shut off or crashed right after its start |
Error strings name paths, hosts, volumes and libvirt's own text. They
never contain key bytes, the known_hosts content or cloud-init.
## Known gaps
- **An upload that libvirtd itself fails leaks a goroutine, upstream.**
go-libvirt's `requestStream` hands its stream sender an unbuffered
channel and then, on the error path, returns without ever reading it:
```go
abort := make(chan bool) // rpc.go
outErr := make(chan error)
go func() { outErr <- l.socket.SendStream(...) }()
resp, err = l.processIncomingStream(c, in)
if err != nil {
abort <- true
return resp, err // outErr is never drained
}
```
Whenever the daemon errors mid-upload — a pool that fills, a volume
removed underneath — the sender parks on `outErr` for good, taking its
4 MiB buffer with it. Closing the socket does not help: both ends are
blocked on channel sends, not on I/O. kvm2 avoids the two cases it
controls (a failing body, and its own transfer deadline; see *The image
cache*), so what is left needs a server-side failure. The fix is two
characters upstream, `make(chan bool, 1)` and `make(chan error, 1)`; if
it ever bites hard enough, a `replace` carrying that patch is the
escape hatch.
- **Create blocks**, including the first image download on each host,
holding one of the three reconcile workers for its duration (LAN
speed, a few hundred MB, once per image per host). Other Proxies keep
@@ -585,7 +655,12 @@ never contain key bytes, the known_hosts content or cloud-init.
vendor script's output in `/var/log/cloud-init-output.log` (a VM without
outbound internet cannot install it). A user-data that disables
vendor-data (`vendor_data: {enabled: false}`) also removes the agent, by
design. `virsh domifaddr <name> --source agent` and `--source arp` show
design. A host where the ARP source *errors* rather than answering with
nothing is the one case this reads differently: with no source left to
answer, a booting VM retries with `ErrTransient` instead of resting in
`Provisioning`. It still converges; the giveaway is two "no addresses
from source" lines per poll at `-v 2`.
`virsh domifaddr <name> --source agent` and `--source arp` show
what the operator sees, in that order.
- **Debian 13 cloud images hang in GRUB under BIOS on a headless VM**
([Debian bug #1111240](https://bugs.debian.org/1111240)), which is why
docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md
+531
View File
@@ -0,0 +1,531 @@
# Execution: fix the kvm2 code-review findings
Plan: [docs/plans/2026-09-17-1515-kvm2-review-fixes.md](../plans/2026-09-17-1515-kvm2-review-fixes.md)
- [x] Step 0 — Save plan into the repo
- [x] Step 1 — H1: a stalled write must not delay the teardown
- [x] Step 2 — H2: a source error must not reach go-libvirt's stream sender
- [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
- [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
checklist line here when it is planned, so this list stays a record of decided work rather
than a wish.
## Step 0 — plan committed
Copied out of `~/.claude/plans/` and committed alone before touching code, as the repo's
plan rules require.
## Step 1 — H1: close the SSH client, and signal dead before killing
Two edits, both small, in the order the bug unfolds.
`sshSocket.Close` was `errors.Join(s.Conn.Close(), s.client.Close())`. Go evaluates those
arguments left to right, so the blocking close ran before the unblocking one. `s.Conn` is
the forwarded libvirt socket, and closing an SSH channel writes a close message under
`ch.writeMu`, which go-libvirt already holds while streaming a base image through the same
channel. Against a host that vanished without RST or FIN, that write blocks until the
kernel stops retransmitting — `tcp_retries2`, about 15 minutes. `s.client.Close()`, a bare
`net.Conn.Close()` on the TCP socket that takes no lock and unblocks the stalled writer
through netpoll, was never reached. It now closes the client only.
**Dropping the channel close rather than reordering it** was the judgment call. Reordering
works, but afterwards the channel close returns an error — `net.ErrClosed`, a
`*net.OpError`, the transport's sticky write error, or `io.EOF` once `mux.loop` has run
`ch.close()` — and `closeErr` is memoised, so a filter that missed a case would poison
every later `Close` and break `ssh_test.go:66,69`, which assert both closes return nil.
Nothing wants the polite channel-close message: `dialSSHSocket` builds the client solely
for this one channel, the `sshSocket` owns it exclusively, and `dialSSHSocket`'s own error
paths already tear down by closing the client.
`link.markDead` then compounded it by running `kill()` before `close(l.closed)`, and doing
both inside a `sync.Once`. Reordering alone would not have been enough: `sync.Once.Do`
blocks concurrent callers until the first body returns, so every other `markDead` — including
the one in `call`'s own ctx-cancel branch — queued behind the stalled kill. **Cancelling the
reconcile did not help.** The `once` field is gone; the mutex the struct already had now
guards the "close it exactly once" invariant, `reason` is set before the signal so a
released caller always reads a populated `deadReason()`, and `kill` runs outside the lock.
**A second route to the same pin, found while planning and fixed by the same edits.**
go-libvirt's `Disconnect` closes the `net.Conn` it was handed (`libvirt.go:331` →
`socket.go:164`) — and that conn *is* our `*sshSocket`. So `libvirtBackend.Close`'s
`Disconnect` goroutine entered `sshSocket.closeOnce` and hung there; the 5 s `closeBudget`
released `Close`, which called `markDead("closed")` → `kill()` → `sshSocket.Close()` →
blocked in the other goroutine's `sync.Once`. The budget did not protect it.
The precondition was verified in the pinned x/crypto v0.54.0 rather than assumed: the
flow-control park (`remoteWin.reserve`, `channel.go:268`) happens *outside* `writeMu`, and
only `writePacket` holds it across the TCP write. So a writer parked in SSH flow control
never blocks the close; a writer inside the TCP write always does. Since the channel window
is 2 MiB (`channelWindowSize = 64 * channelMaxPacket`), reproducing it by hand needs a
kernel send buffer that fills first — shrink `SO_SNDBUF` well under 2 MiB.
The older kvm provider does not have this bug: it closes `client` only
(`internal/provider/kvm/conn.go:429,456`) and never touches the forwarded channel. kvm2's
rewrite introduced it.
### Tests: planned four, shipped one
The first draft had three hermetic tests plus a `//go:build integration && linux`
stalled-write test with a TCP proxy and shrunken socket buffers. The user pushed back —
once the bug is fixed the connection closes properly, so what is left to test? — and they
were right about most of it. `TestDial_forwardsUnixSocket` already covers "Close tears the
connection down", and after the fix `Close` is a single call with no ordering to get wrong.
The stalled-write test was the worst of the four: ~150 lines of Linux-only,
buffer-autotuning-dependent scaffolding whose own false-green guard made it *skip* whenever
the kernel did not cooperate, to demonstrate a bug that reading x/crypto settles and that
the fix deletes the code path for.
What survived is `TestMarkDead_slowKillDoesNotBlockCallers`, because the `markDead` change
*adds* a guarantee that did not exist before and nothing else touches it —
`TestCall_killedByAnotherCallerFailsTransient` passes a nil kill, so it cannot see the
property. It is also the plausible regression: restoring the `sync.Once` looks like a
correctness improvement. The call's own `fn` signals when it is in flight, so the parked
caller is genuinely parked rather than refused by the dead check on the way in.
The step that actually validates a regression test is running it against the unfixed code.
`markDead` was reverted in place, the test run, and the fixed version restored from a copy:
```bash
cp internal/provider/kvm2/backend.go "$SCRATCH/backend.go.fixed"
# ... revert markDead to the sync.Once form and re-add the once field ...
go test -race -count=1 -run 'TestMarkDead_slowKillDoesNotBlockCallers' ./internal/provider/kvm2/
# --- FAIL: TestMarkDead_slowKillDoesNotBlockCallers (1.00s)
# backend_test.go:186: link not signalled dead while its kill is still running
cp "$SCRATCH/backend.go.fixed" internal/provider/kvm2/backend.go
```
It fails in a second with the right diagnostic rather than hanging the suite, because the
blocked kill is released by `t.Cleanup`.
```bash
go build ./... && go vet ./... && go test -race -count=1 ./internal/provider/kvm2/
```
### Worth noting
**An accepted risk, deliberately not covered by a test.** Nothing now stops someone
re-adding `s.Conn.Close()` to `sshSocket.Close` as tidiness; it would look harmless and
every test would still pass, because in a healthy test the channel close returns instantly.
The guard is the doc comment on `sshSocket`, which says outright that closing the channel
would queue behind a stalled transfer. That is in idiom for this package — `hosts.go:25-34`
and `create.go:151-153` carry the same kind of hard-won decision — but it is a comment, not
a test, and the failure it prevents is a 15-minute operator-wide stall.
**Left for later, on purpose.** `ReconciliationTimeout` in `SetupWithManager` does not exist
in this repo at all; adding a per-reconcile deadline would change the contract for every
provider, since `hosts.go` and `image.go` both cite "no caller deadline", so it needs its own
change and its own spec entry. `sshSocket.keepalive` still has no test (sweep finding S1.2).
**Docs.** `docs/kvm2-provider.md` said the adapter "kills the connection (socket and SSH
client)", which is no longer true and was the description that made the bug look correct.
It now states the order (mark dead, then kill) and why both steps are lock-free.
`docs/kvm2-provider-spec.md` entries 18 and 19 were checked and are not falsified.
**No CHANGELOG entry yet** — the repo's rule is to add one once the user confirms the change
works, so it lands with a later "validate and record" step, as in the kvm campaign.
## Step 2 — H2: a source error must not reach go-libvirt's stream sender
This one is a workaround, not a repair. The defect is upstream, and the entry says so in
three places (code comment, `docs/kvm2-provider.md` *Known gaps*, commit message) so nobody
later reads the wrapper as ours to delete.
`requestStream` in the pinned go-libvirt starts its stream sender on two **unbuffered**
channels and then, on the error path, returns without ever reading `outErr`. A non-EOF error
from the reader makes `SendStream` emit a StatusError packet and return, whereupon it parks on
`outErr <-` forever; libvirtd answers the StatusError with an error, so the main goroutine
parks on `abort <- true` forever. `StorageVolUpload` never returns. The 20-minute budget
rescues only the caller, and the socket kill does nothing at all here, because both ends are
blocked on channel sends rather than on I/O — which is what the comment at `backend.go:152`
got wrong. Every failed upload leaked two goroutines and 4 MiB against a 128Mi pod limit,
with the reconciler retrying on backoff.
Three changes.
`eofReader` sits at the go-libvirt seam in `backend.go`, not in `image.go`, so it also covers
the seed and marker uploads, and so it stays out of the file Step 4 rewrites. It converts any
source error to `io.EOF` — the one outcome that ends a stream the normal way — and keeps the
real error for `StorageVolUpload` to return afterwards. `countingReader` is untouched
underneath it and still sees the true error, so `uploadImage`'s existing length and digest
checks produce exactly the messages they did before.
The download now carries its own deadline, one minute inside the transfer budget.
`fetchImage` already bounded the whole fetch at 20 minutes, so the body's death and the
budget's socket kill were racing, and the socket usually won. With the body a minute ahead, a
stalled mirror ends through the reader and unwinds cleanly. This is the difference between a
slow mirror costing a Transient error and costing a permanent leak, and slow mirrors are the
realistic repeat offender.
`discardVolume` moved from `uploadImage` up into `fetchImage`'s upload-error branch, so the
dedicated connection is closed — and libvirtd has let go of the stream's fd — before the file
is unlinked. Moving it rather than adding a second `Close` is what keeps
`image_test.go`'s `closeCount() == 1` assertion true. `uploadImage` lost its `be` parameter
and its private logger in the process.
**What is left, deliberately.** A failure that originates in libvirtd mid-upload (ENOSPC, or
the volume deleted underneath) still leaks, because by then `SendStream` has already returned
and nothing we pass, cancel or close can reach it. Confirmed unfixable from this side. The
upstream fix is two characters and is quoted in *Known gaps* along with the `replace` escape
hatch, should it ever bite hard enough to be worth the maintenance.
**Unverified, ~85%.** That libvirtd answers a short-but-clean stream finish with OK rather
than an error. `virFDStreamWrite` enforces `length` only on writes past it and nothing
compares `offset` to `length` on the close path, which is also why `virsh vol-upload` of a
small file into a large volume works. If it answers with an error we land back in a deadlock
by a different door — but that is precisely today's behaviour, so the change cannot make
things worse. To settle it, on any box with a libvirtd (not beelink, which is read-only
here), against the pinned go-libvirt:
```sh
virsh -c qemu:///session pool-define-as probe dir --target ~/vmprobe
virsh -c qemu:///session pool-start probe
virsh -c qemu:///session vol-create-as probe short.raw 10M --format raw
# then a ~40-line main.go: StorageVolUpload(vol, r, 0, 10<<20, 0) where r
# yields 1 MiB then io.EOF; print the error, then runtime.NumGoroutine()
# and a goroutine profile after a two-second settle.
```
Expected: `err == nil` and the goroutine count back at baseline. The same program with a
reader that returns a non-EOF error should wedge against the *unpatched* seam, which also
confirms the diagnosis end to end.
The negative check that matters was run on the unit test instead — `eofReader.Read` was
changed back to returning the real error, and both new tests failed with
`io.Copy through eofReader = connection reset by peer, want nil`, then passed again once
restored.
```bash
go build ./... && go vet ./... && go test -race -count=1 ./internal/provider/kvm2/
```
### Worth noting
The `fetchImage` discard is still unfenced against a second provider entry that shares the
volume name — Step 4 (H4) is what closes that, by making the name per-entry. The deferred
atomic-publish work then lands exactly where this step put the discard.
**Deliberately not tested:** the abort protocol end to end. `libvirttest` reads a fixed
28-byte header per packet and implements no stream procedures, so pinning this hermetically
would mean a 150-250 line libvirt RPC server stub whose only assertion is about *upstream's*
goroutines. Same call as H1's: the test would cost more than it protects. The two tests that
did land pin the one property the seam actually needs.
## Step 3 — H3: an unreadable address must not look like a missing one
`address()` tried the guest agent then the host's ARP table and skipped any error that was
not a context error, returning `("", nil)` after both. An active domain with no IP maps to
`StateProvisioning`, the controller clears `status.IP` on that, and the health engine prunes
the proxy's counters and calls `ForgetProxy` in one tick once `EffectiveHost()` is empty.
The damaging trigger was not the guest. The libvirt connection is pooled per host, so when
another reconcile's cancel or budget overrun killed it, `call` returned
`"connection is closed"` — carrying no context sentinel, so the check did not recognise it —
and `Get` returned a *successful* instance in Provisioning with an empty IP. An unrelated
proxy's timeout dropped a healthy proxy out of rotation.
The fix rests on a discriminator the spec had already recorded without drawing the
conclusion: during a normal boot on beelink exactly one source fails. The agent errors
because it is not up yet, while ARP answers *without* error and with nothing, since the host
never sees the guest's traffic. So one source failing is "no address yet"; every source
failing, or a connection already dead, is `ErrTransient`.
The explicit `provider.ErrTransient` wrap on the joined error is load-bearing rather than
decoration. Without it `classify` picks the first libvirt code out of the join tree, and
`ErrInvalidArg`, `ErrAccessDenied` and `ErrNoSupport` are Permanent — an address read failing
would have latched a *running* proxy as Failed, a worse version of the harm being fixed.
`isDead(be)` is checked first in the error branch. It is not strictly needed, since a dead
link fails both sources and the count rule catches it anyway, but it fails on the first
source instead of the second, attributes the error better, and is the same predicate M15 will
want for `ListByTag`.
### Tests
`TestGet_addressErrorIsProvisioning` failed against the new rule, which is correct: it fails
*both* sources with `ErrAgentUnresponsive`, and code 86 is agent-specific — the ARP source
cannot produce it. Re-pointed as `TestGet_agentFailsAndARPEmptyIsProvisioning`, the beelink
boot it was really reaching for: agent errors, ARP returns no addresses and no error, result
is Provisioning. Nothing covered that before, and it is the case the new rule must not break.
`TestGet_everyAddressSourceFailingIsTransient` covers the other side, two subtests: both
sources erroring, and the pooled connection dying under the read. Against the unfixed code
both return a Provisioning instance instead of an error, which is exactly the bug:
```bash
# with isDead() and the every-source-failed arm removed from address():
go test -count=1 -run TestGet_everyAddressSourceFailingIsTransient ./internal/provider/kvm2/
# get_test.go:215: Get = &{... IP: State:Provisioning ...}, want no instance
```
`TestGet_arpFallbackWhenAgentFails` passes untouched — one source of two.
### Worth noting
**Half of H3 is deliberately still open, and it is the half that fires more often.** A guest
agent restarting inside a running VM still demotes it to Provisioning and clears its IP: the
agent errors, ARP answers empty, and that is indistinguishable from a VM still booting. The
discriminator is the *previous* IP, which the provider does not hold by design
(`TestGet_readsUIDAndCreatedAtFromBackend` pins "read back, not remembered") and the
controller already has. Follow-up finding, to be filed against
`internal/controller/proxy_controller.go:208-211`: on `StateProvisioning`, keep an existing
IP and `Provisioned` condition for a few polls rather than clearing on the first, or give the
health engine a grace period before pruning.
The one kvm2-local alternative was considered and rejected: the mark carries `createdAt`, so
`get` could treat "active, no address from any source, domain older than a provisioning
window" as an error. It uses state the provider genuinely has, but `createdAt` is creation
rather than boot, it fires on every genuinely stuck VM as a retry-forever error, and it
invents a provisioning deadline that belongs to the controller. A design change wearing a bug
fix's clothes.
**L6 not folded in.** A domain that vanishes between `DomainGetState` and the address read
now returns Transient rather than a fake Provisioning — better, and self-correcting on the
next poll. Returning NotFound one poll earlier is a two-line addition at the same spot, left
for when L6 comes up so this commit stays one idea.
## Step 4 — H4: the base image cache must not be shared across provider entries
`baseVolumeName` was `base-<key>.<format>` — byte-identical to what
`internal/provider/kvm/image.go` builds in the same pool, for URL images too (kvm takes
`hex(sum[:8])`, kvm2 takes `hex(sum[:])[:16]`, the same eight bytes), with a completely
separate lock table. The shipped config has a live `kvm-home` entry and a commented-out
`kvm2-home` entry on the same host, the same key `debian-13` and the same default pool, and
the comment invites the operator to enable it. Both integration tests use key `test-image`.
On beelink both live overlays back onto kvm's `base-debian-13.qcow2`.
Since `cachedImage` accepts any 64-hex marker when no `sha256` is configured and never
re-hashes the base, that meant: two entries pointing one key at different URLs silently share
a file; a concurrent cold fetch lets one entry delete the other's half-uploaded base and boot
a guest from the remains; and a `sha256` edit deletes the base in place under live overlays,
which libvirt permits without complaint.
The name now carries the provider entry: `base-<entry>-<key>.<format>`. `imageSource` gained
an `Entry` field and `resolveImage` takes the provider name, so the four production call
sites all follow from `imageSource.volumeName()`. That closes the cross-entry sharing
outright, kvm↔kvm2 and kvm2↔kvm2 alike, and it turns the existing per-`Provider` `imageLocks`
from a latent bug into a correct one — with names unique per entry, no two entries can
contend for a file, so a process-wide lock table is not needed.
**The entry name reaches a file name, so it now has to be a usable one.** Provider names are
validated for all types at once and only for being non-empty and unique
(`internal/provider/config.go`), so `New` checks its own against the existing `kvmIdentifier`
rule, exported as `provider.IsKVMIdentifier` rather than duplicated. A name with a slash
fails at startup instead of producing a volume nothing can address.
**One companion fix, because the naming alone does not stop it.** `cachedImage`'s
digest-mismatch branch deleted the marker and the base and refetched. It now returns
`ErrPermanent` naming both digests and telling the operator what to do. That branch was the
remaining path that unlinks a good file under running domains, and the contract already said
"add a new key instead" — the code was contradicting it. The no-marker and non-hex-marker
branches keep deleting, since there the base is genuine debris.
### What this does NOT fix
Stated plainly rather than left to look closed:
- **Editing `images.<key>.url` is still a silent no-op.** The cache is keyed by entry and
key, and nothing records the URL, so the new one is never fetched. Pre-existing, already
documented as "add a new key"; the eviction section now says it outright.
- **Cross-process fencing.** Two managers, or a manager plus a hand-run `virsh`, can still
meet on one name. E4 refuted the rollout and leader-handover triggers for the shipped
manifests, so this is narrow, and its fix is the deferred atomic publish — upload to a temp
name, verify, publish — which lands where H2 put the discard.
### Migration
Nothing to do. The `base-debian-13.qcow2` on beelink is kvm's, kvm keeps using it untouched,
and kvm2 downloads its own copy once per host when `kvm2-home` is enabled. No window in which
anything is replaced under a running guest. The pool needs room for both, which the
prerequisites now say.
### Tests
Mostly literals becoming helpers, plus a new `fx.baseName()` in `provider_test.go` for the
three fixtures that asserted `"base-debian-13.qcow2"` by hand. The log and span assertions in
`TestEnsureImage_downloadsAndWritesMarkerLast` keep asserting a full name rather than loosening
to a prefix, so they still pin that the key stays readable in operator-facing output.
One test changed behaviour rather than spelling: `TestEnsureImage_sha256ChangeRedownloads`
became `TestEnsureImage_sha256ChangeRefusesInsteadOfReplacing`, asserting Permanent, zero
server hits, zero deletes, and both volumes untouched. Two added:
`TestNew_rejectsUnsafeEntryName`, and a `TestVolumeNames` row for a second entry with the same
key.
Both negative checks were run:
```bash
# baseVolumeName reverted to base-<key>.<format>:
go test -count=1 -run TestVolumeNames ./internal/provider/kvm2/
# volume_test.go:135: got "base-debian-13.qcow2", want "base-kvm2-lab-debian-13.qcow2"
# the digest branch reverted to delete-and-refetch:
go test -count=1 -run TestEnsureImage_sha256ChangeRefuses ./internal/provider/kvm2/
# image_test.go:412: ensureImage() error = <nil>, want ErrPermanent telling the operator what to do
```
`go vet -tags=integration` was run too, since `kvm2_integration_test.go:484` builds the base
name and only compiles under that tag.
### Worth noting
**The integration test has still never run on beelink.** Step 11 is unchecked in
`docs/plans-executions/2026-09-16-1333-kvm2-provider.md`, every `base-*` on the host predates
kvm2, and both live domains carry the kvm v1 metadata namespace. After this change it is also
the first end-to-end exercise of the new name, and it will do a real cold download under
`base-kvm2-integration-test-image.qcow2` beside kvm's untouched `base-test-image.qcow2`.
**Docs touched beyond the obvious:** the eviction recipe named `base-debian-13.qcow2`, which
after this change is *kvm's* file — following it would have deleted the wrong provider's base.
It now greps for the prefix and deletes the marker first.
## Step 6 — Route klog into the structured logger
Not a review finding. Found while watching the Step 5 validation: some lines ignored the zap
encoder no matter what, so `kubectl logs | jq` could never work.
```
I0917 19:50:02.580097 1 leaderelection.go:258] "Attempting to acquire leader lease..." lock="..."
```
That is klog's own stderr writer, not ours. client-go's leader elector calls
`klog.FromContext(ctx)` (`leaderelection.go:257`), and controller-runtime hands the elector a
context with no logr in it — `mgr.Start` receives `ctrl.SetupSignalHandler()`, which carries no
logger (`pkg/manager/internal.go:461-473`). `FromContext` falls back to `Background()`, and with
no logger registered that is klog's text sink. Nothing in this repo imported or configured klog;
it was an indirect dependency that around 60 linked packages use, including
`client-go/tools/{leaderelection,cache,record}` and `client-go/transport`. controller-runtime
does not route it either — its only `klog.SetLogger` call is in its own test suite.
**`klog.SetLogger(logger)`, deliberately not `SetLoggerWithOptions(logger, ContextualLogger(true))`.**
The contextual form is what klog's docs recommend, because it skips a klogr round trip. It also
disables klog's verbosity gate ("Such a logger cannot rely on verbosity checking in klog",
`contextual.go:83-86`), handing the decision to zap — whose level under `Development: true` is
Debug, which enables V(1). client-go's V(1) chatter, suppressed today by klog's own `-v=0`,
would have started appearing. Plain `SetLogger` keeps klog's check in front and still redirects
what it does emit ("all log lines will be suppressed from the regular output, and redirected to
the logr implementation", `contextual.go:47-48`). Same fix to the format, no new volume — which
matters, because the complaint that started this was volume.
`k8s.io/klog/v2` moves from indirect to direct in `go.mod`. It was already in the module graph;
nothing new is downloaded.
**The other bypass.** The discovery API's `http.Server` set no `ErrorLog`, so `net/http`'s own
faults — a failed `Accept`, a panic escaping a handler — went to the stdlib logger on stderr.
It now drains into logr through `logr.ToSlogHandler`, the pattern
`internal/provider/gcp/wirelog.go:31-36` already uses. This server is plain HTTP with no TLS, so
`ErrorLog` carries real faults rather than handshake noise, and Error level is right.
**Deployment.** `--zap-encoder=json` and `--zap-time-encoding=rfc3339` on the manager.
Verified the flag beats the in-code default rather than assuming it: `addDefaults` fills
`NewEncoder` only `if o.NewEncoder == nil` (`zap.go:176-191`) and `BindFlags` sets it from the
flag. The time encoding is not optional — the JSON encoder defaults to epoch seconds.
Verbosity was left at debug on purpose; `--zap-log-level=info` drops the per-RPC lines.
`opts := zap.Options{Development: true}` stays. Flipping it would change the format for every
deployment of this operator, and the flags get the same result here without that reach.
### Tests
None. Three changes, all wiring: two struct fields and a manifest key. `cmd/main.go` has no test
coverage, and pinning `klog.SetLogger` means mutating process-global state that leaks into every
other test in the package. Verified by observation instead, which for this change is stronger
than a unit test would be.
### Verification
```bash
kubectl -n egress-proxies-operator-system logs deploy/... | grep -i leader
# {"level":"info","ts":"2026-09-17T20:10:00Z","msg":"Attempting to acquire leader lease...",
# "lock":"egress-proxies-operator-system/b47711d1.example.com"}
kubectl -n egress-proxies-operator-system logs deploy/... | jq -e -s 'length > 0'
# YES - every line is valid JSON
```
The `jq -s` check is the one that counts: it fails if *any* line is not JSON, which is exactly
the property being fixed. All four existing proxies stayed Ready across the rollout and all
three ProviderHealth entries stayed Ready.
### Worth noting
**The JSON encoder renders durations as float seconds**, so `"duration":"566.12µs"` became
`"duration":0.000336664`. Machine-friendlier, worse to read. Either format in the query —
`(.duration*1000000|round)` for µs — or pass an encoder config option in code, which
controller-runtime exposes (`zap.Options.EncoderConfigOptions`, `zap.go:140-144`):
```go
opts := zap.Options{
Development: true,
EncoderConfigOptions: []zap.EncoderConfigOption{
func(c *zapcore.EncoderConfig) { c.EncodeDuration = zapcore.StringDurationEncoder },
},
}
```
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.
docs/plans/2026-09-17-1515-kvm2-review-fixes.md
+640
View File
@@ -0,0 +1,640 @@
# Plan: fix the kvm2 code-review findings
**Created:** 2026-09-17 15:15
## Context
The review of `feat/kvm2-provider` (`review/REPORT.md`) confirmed 52 findings: 4 high,
20 medium, 28 low. We fix them one at a time, highest severity first, each as its own step and
its own commit. This plan is the campaign; the execution file's checklist is the authoritative
progress record. Steps are detailed as we reach them, so the plan is never a fiction about work
not yet designed.
Precedent for this shape: `docs/plans/2026-09-10-2222-kvm-review-fixes.md`.
## Steps
- [x] **Step 0** — Save this plan into the repo, commit alone, before any code.
- [x] **Step 1** — H1: `sshSocket.Close` ran the blocking close before the unblocking one, and
`markDead` killed inside a `sync.Once` before signalling dead. Shipped in `0239fb7`; the
reasoning is in the execution file.
- [ ] **Step 2** — H2: an error during `StorageVolUpload` wedges go-libvirt's stream sender.
- [ ] **Step 3** — H3: `address()` swallows every error and demotes healthy proxies.
- [ ] **Step 4** — H4: the base image cache is shared across provider entries.
- [ ] **Step 5** — Validate H1-H4 in-cluster against the real hypervisor (detailed below).
- [ ] **Step 6** — Route klog into the structured logger (found during Step 5; detailed below).
- [ ] **Step 7+** — the mediums, from `review/REPORT.md`. Detailed when reached.
---
## Step 2 — H2: a source error must not reach go-libvirt's stream sender
### The bug
`uploadImage` (`image.go:255-296`) hands a reader over `resp.Body` to `StorageVolUpload`. I
verified the mechanism in the pinned `go-libvirt@v0.0.0-20260814190004`:
```go
// rpc.go requestStream
abort := make(chan bool) // UNBUFFERED
outErr := make(chan error) // UNBUFFERED
go func() { outErr <- l.socket.SendStream(serial, proc, program, out, abort) }()
resp, err = l.processIncomingStream(c, in)
if err != nil {
abort <- true
return resp, err // outErr is NEVER drained on this path
}
err = <-outErr
```
`SendStream` (`socket/socket.go`) holds a 4 MiB buffer, polls `abort` once per loop, and on a
**non-EOF reader error** sends a `StatusError` packet and returns. It then blocks forever on
`outErr <-`, because the main goroutine is still inside `processIncomingStream`. libvirtd
answers the `StatusError` with an error, so `processIncomingStream` returns an error and the
main goroutine blocks forever on `abort <- true`. **Deadlock: `StorageVolUpload` never returns.**
kvm2's `call` rescues only the caller, after `budgets.transfer` = 20 minutes, and the socket
kill does **not** unblock either goroutine — both are parked on channel sends, not on socket
I/O, which is what the comment at `backend.go:152-156` gets wrong. So every failed upload leaks
kvm2's call goroutine, go-libvirt's sender goroutine and 4 MiB, permanently. The pod limit is
128Mi and the reconciler retries on backoff.
Three shapes, only the first of which is ours to fix:
- **(A) reader error** — a truncated or failing HTTP body. The common case.
- **(B) server-side error mid-upload** — ENOSPC, or the volume deleted underneath. After
`SendStream` has returned there is nothing any reader, ctx or socket close can do: confirmed
unfixable from our side.
- **(C) our own abandonment** — ctx cancel or the 20 m budget kills the socket, which makes
`processIncomingStream` error. Same tail as (B).
### The fix (workaround; the defect stays upstream)
No fork, no `replace`. Two changes that between them close (A) and the realistic half of (C).
**2a. Never hand go-libvirt a reader that errors** — `internal/provider/kvm2/backend.go`, at the
seam rather than in `image.go`, so it covers the seed and marker uploads too and stays out of
the file Step 4 rewrites:
```go
type eofReader struct {
r io.Reader
mu sync.Mutex
err error
}
func (e *eofReader) Read(b []byte) (int, error) {
n, err := e.r.Read(b)
if err == nil || err == io.EOF {
return n, err
}
e.mu.Lock()
if e.err == nil {
e.err = err
}
e.mu.Unlock()
return n, io.EOF
}
func (e *eofReader) sourceErr() error { e.mu.Lock(); defer e.mu.Unlock(); return e.err }
```
`libvirtBackend.StorageVolUpload` wraps `r` in one, and after a nil-error return converts a
recorded `sourceErr()` into the returned error. `io.EOF` is the one outcome that ends the stream
the normal way: libvirtd gets `StatusOK`, replies OK, `processIncomingStream` returns nil, the
main goroutine reaches `err = <-outErr` and both goroutines unwind.
`countingReader` is untouched — it sits under the wrapper, still sees the real error, so
`uploadImage`'s `readErr != nil` branch and `TestEnsureImage_shortReadRemovesVolumeAndIsTransient`
keep working verbatim.
**Unverified, ~85%:** that libvirtd answers a short-but-clean stream finish with OK rather than
an error. `virFDStreamWrite` enforces `length` only on writes *past* it and nothing compares
`offset` to `length` on the close path, which is also why `virsh vol-upload` of a small file
into a large volume works. If it answers with an error instead, we land back in a deadlock by a
different door — but that is exactly today's behaviour, so the change is non-worsening either
way. Probe recipe goes in the commit message; it needs a real libvirtd and a write, so it is
for the user to run, not an agent.
**2b. Make the transfer deadline land on the HTTP source, not on the libvirt socket.**
`p.do` already builds the request with `http.NewRequestWithContext` (`image.go:360`), so the
body is ctx-bound, and `http.DefaultClient` has no timeout of its own by deliberate choice
(`kvm2.go:100`). Give the image GET in `fetchImage` a deadline of `defaultBudgets.transfer`
minus a grace of one minute (precedent for reaching for `defaultBudgets` directly:
`discardVolume`, `image.go:331`).
A stalled mirror then ends by the *body* failing, which 2a turns into `io.EOF` and a clean
unwind, roughly a minute before the libvirt budget would have killed the socket and leaked.
That converts the realistic repeated (C) — a slow or stalling mirror on a 415 MB image — from a
permanent leak into an ordinary Transient error. The socket kill stays as the backstop for
libvirtd itself hanging, which is the residual we cannot close.
**2c. Close the dedicated connection before unlinking the file.** Move the `discardVolume` call
out of `uploadImage` (`image.go:291-294`) up into `fetchImage`'s upload-error branch, where the
deferred `fresh.Close()` has already run, so libvirtd has let go of the stream's fd before the
volume is deleted. `uploadImage` then drops its `be` parameter and its private logger — a net
deletion. Doing it this way rather than adding an explicit `Close` keeps
`image_test.go:573`'s `closeCount() == 1` assertion true.
### Tests, docs, scope
One test: `TestEOFReader_sourceErrorNeverReachesTheStream` in `backend_test.go` — a table with
"error after data", "error with no data", "plain io.EOF passes through", "first error wins".
~35 lines.
**Deliberately not tested:** the abort protocol end to end. `libvirttest` reads a fixed 28-byte
header per packet and implements no stream procedures, so pinning this hermetically would mean
writing a libvirt RPC server stub — 150-250 lines whose only assertion is about *upstream's*
goroutines. That is the bloat we cut for H1.
Docs: a paragraph in `docs/kvm2-provider.md`'s *The image cache* section saying a truncated
response is reported out of band and why, and a *Known gaps* bullet naming shapes (B) and (C)
as an upstream defect with the two-character patch quoted, so a future reader knows it is not
ours and what the escape hatch is:
```go
abort := make(chan bool, 1)
outErr := make(chan error, 1)
```
File that as an upstream issue/PR against `digitalocean/go-libvirt` and link it from the bullet.
---
## Step 3 — H3: an unreadable address must not look like a missing one
### The bug
`address()` (`get.go:104-119`) tries the guest agent then the host's ARP table. Any error that
is not `context.Canceled`/`DeadlineExceeded` is logged at V(2) and skipped; after both it
returns `("", nil)`. `mapState(active, "")` is `StateProvisioning` (`state.go:20-21`), the
controller clears `p.Status.IP` on Provisioning (`proxy_controller.go:208-211`), and the health
engine prunes the proxy's accumulated state and calls `Metrics.ForgetProxy` in one tick once
`EffectiveHost()` is empty.
The trigger that matters: another reconcile's cancel or budget overrun kills the **shared**
pooled connection, and `call` then returns `transientf("... connection is closed: ...")` with no
ctx sentinel, which `get.go:108` does not recognise. `Get` returns a *successful* Instance in
Provisioning with `IP: ""`, and a healthy proxy is pulled out of rotation by an unrelated
reconcile's timeout.
### The fix
A booting VM and a dead link are distinguishable: on beelink the agent source errors while the
ARP source returns success-with-no-addresses (`docs/kvm2-provider-spec.md:146` — the host never
sees the guest's traffic). So *one* source failing is normal; *every* source failing, or a
connection already gone, is not.
```go
func address(ctx context.Context, log logr.Logger, be backend, dom libvirt.Domain) (string, error) {
errs := make([]error, 0, len(addressSources))
for _, s := range addressSources {
ifaces, err := be.DomainInterfaceAddresses(ctx, dom, uint32(s.src), 0)
if err != nil {
if isDead(be) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return "", fmt.Errorf("reading addresses of domain %s from the %s source: %w", dom.Name, s.name, err)
}
log.V(2).Info("no addresses from source", "source", s.name, "error", err.Error())
errs = append(errs, fmt.Errorf("%s source: %w", s.name, err))
continue
}
if ip := firstIPv4(ifaces); ip != "" {
return ip, nil
}
}
if len(errs) == len(addressSources) {
return "", fmt.Errorf("%w: reading addresses of domain %s: every source failed: %w",
provider.ErrTransient, dom.Name, errors.Join(errs...))
}
return "", nil
}
```
The explicit `provider.ErrTransient` wrap is load-bearing, not decoration: without it `classify`
picks the first libvirt code out of the join tree, and `ErrInvalidArg`/`ErrAccessDenied`/
`ErrNoSupport` map to **Permanent** — an address read failing would latch a *running* proxy as
Failed, a worse version of the harm being fixed. `isDead(be)` (`backend.go:50-61`, already
works through the `observed` decorator and the test fake) is not strictly needed — a dead link
fails both sources anyway — but it fails faster, attributes better, and is the same predicate
M15 will need for `ListByTag`.
Also fix the now-wrong comment at `get.go:100-103` ("only ctx ending is an error").
**What this does not fix, on purpose.** A guest agent restarting inside a running VM still
demotes it: the agent errors, ARP returns empty, and the provider cannot tell that from a VM
that is still booting. The discriminator is the *previous* IP, which the provider deliberately
does not hold (`TestGet_readsUIDAndCreatedAtFromBackend` pins "read back, not remembered") and
the controller already has. File as a follow-up finding against
`proxy_controller.go:208-211`: on Provisioning, keep a Proxy's existing IP and `Provisioned`
condition for N polls before clearing, or give the health engine a grace period before pruning.
The one kvm2-local alternative — treating "active, no address, domain older than a provisioning
window" as an error using the mark's `createdAt` — is a design change wearing a bug fix's
clothes, and it invents a provisioning deadline that belongs to the controller. Rejected.
### Tests
- `TestGet_arpFallbackWhenAgentFails` (`get_test.go:135`) passes unchanged — one source of two.
- `TestGet_addressErrorIsProvisioning` (`get_test.go:159`) must be **re-pointed**. It fails both
sources with `ErrAgentUnresponsive`, which is not realistic: code 86 is agent-specific and the
ARP source never produces it. Rewrite as `TestGet_agentFailsAndARPEmptyIsProvisioning` — agent
errors, ARP returns no addresses and no error — which is the beelink boot and the behaviour
the new rule must not break. Nothing covers it today.
- One new test, `TestGet_everyAddressSourceFailingIsTransient`, two subtests: both sources error;
and the pooled link is dead before the address read (the fake implements `liveness`). Assert
Transient, not NotFound, not Permanent.
Docs: `docs/kvm2-provider-spec.md:146-151` says "errors from either source mean 'no address yet'",
now false. The `ErrTransient` row of the error-class table in `docs/kvm2-provider.md`.
---
## Step 4 — H4: the base image cache must not be shared across provider entries
### The bug
`baseVolumeName` (`volume.go:17`) is `base-<key>.<format>`, with a `.sha256` marker beside it.
`internal/provider/kvm/image.go:44-56` builds **byte-identical** names in the same pool — for
URL images too (kvm takes `hex(sum[:8])`, kvm2 takes `hex(sum[:])[:16]`, the same 8 bytes) —
with a completely separate lock table. The shipped config has a live `kvm-home` entry and a
commented-out `kvm2-home` entry on the same host, the same key `debian-13` and the same default
pool, and the comment tells the operator to enable it. Both integration tests use key
`test-image`. On beelink both live overlays back onto `base-debian-13.qcow2`, which kvm created.
Consequences, given `cachedImage` accepts *any* 64-hex marker when no `sha256` is configured and
never re-hashes the base:
- Two entries, one key, different URLs: whichever fetched first owns the file and the other
silently boots the wrong image.
- A concurrent cold fetch: B finds A's base-without-marker, deletes it, recreates and uploads
its own; A finishes into the unlinked inode, writes the marker, and `StorageVolGetPath`
resolves A's handle to B's partial file — a guest boots from a half-written base.
- A `sha256` edit deletes the base in place under live overlays (`cachedImage` branch E), and
libvirt does not refuse to delete a volume that running domains use as a backing store.
### The fix — the provider entry's name in the volume name
`base-<entry>-<key>.<format>`, e.g. `base-kvm2-home-debian-13.qcow2`, with the marker following
automatically. Every production call site already goes through `imageSource.volumeName()`
(`image.go:49`), so this is a small change: give `resolveImage` the provider name and add an
`Entry` field to `imageSource`.
`Provider.name` is already carried (`kvm2.go:104`). But provider names are validated only as
non-empty and unique (`config.go:96-105`) — no charset rule — and a name with a `/` would
produce an unusable volume name, so **`New` must reject an entry name that is not a safe volume
name component**, using the existing `kvmIdentifier` shape. Fail fast at startup, in kvm2 only:
tightening the rule for every provider type is a broader change than this fix.
**What this buys, and what it does not.** It fixes the cross-entry sharing outright — kvm↔kvm2
and kvm2↔kvm2 — which is the wrong-image bug and the concurrent-fetch corruption, and it makes
the existing per-`Provider` `imageLocks` correct rather than a latent bug, since no two entries
can now contend for one name. It does **not** make the name mean the content, so two things
stay open and must be said plainly rather than quietly skipped:
- Editing `images.<key>.url` without changing the key is still a silent no-op. Already
documented behaviour ("add a new key instead"); left as is.
- Cross-*process* fencing still depends on the atomic-publish work (upload to a temp name,
verify, then publish). E4 refuted the rollout and leader-handover triggers for the shipped
manifests, so this is a narrow residual. Its own later commit.
**One companion fix, in this commit**, because the naming alone does not stop it and it is the
remaining path that unlinks a good file under a running domain: `cachedImage` branch E
(`image.go:129-130`, digest differs from the configured `sha256`) currently deletes the marker
and the base. Make it return Permanent instead — "this key is cached with digest X but Y is
configured; add the image under a new key, or delete the Proxies using it and evict the base."
That enforces the rule the contract already states instead of contradicting it. Keep the delete
for the no-marker and non-hex-marker branches, where the base is genuine debris.
### Migration
Nothing to do. The existing `base-debian-13.qcow2` on beelink is kvm's, kvm keeps using it
untouched, and kvm2 downloads its own copy once per host (~415 MB) when `kvm2-home` is enabled.
No window in which anything is replaced under a running guest. The pool needs room for both.
### Tests and docs
Tests are mostly literals becoming helpers: `volume_test.go:122-125`, `image_test.go:184,190,196`
(`wantVolume`), `create_test.go:64-70`, `delete_test.go:67`, `provider_test.go:409`; a small
`fx.baseName()` helper covers the last three. `image_test.go:281,298` should assert the log
field and the `kvm2.volume` span attribute **by prefix**, which keeps pinning that the key stays
readable in logs. `kvm2_integration_test.go:481-484` switches from `baseVolumeName(...)` to
`resolveImage(...).volumeName()`; its assertion that the base survives `Delete` is unchanged.
Two new tests:
- `TestBaseVolumeName_isPerProviderEntry` — two entries, same key, different names; the produced
name never equals the kvm provider's `base-<key>.<format>`.
- `TestNew_rejectsUnsafeEntryName` — an entry name with a `/` fails at construction.
**Not tested:** that libvirt permits deleting an in-use backing file (established from libvirt
source by the review; re-proving it needs a write on a real host), and the cross-process race
(no hermetic way, and its fix is deferred).
Docs: the *What lands on the host* table (`docs/kvm2-provider.md:198-212`), the eviction recipe
(`:305-318`, which currently names `base-debian-13.qcow2` and would delete kvm's file), the
migration section (`:346-370` — kvm2 never reuses a kvm base, one extra download, nothing to
delete), the prerequisites note that the pool needs one base per distro **per entry**, the
naming contract in `docs/kvm2-provider-spec.md` entries 4 and 10, the shared validator's message
at `internal/provider/config_kvm.go:139-141` ("it becomes the cached volume name
`base-<key>.<format>`", now wrong for kvm2), and the kvm2 block's comment in both
`providers-config.yaml` files. Leave `docs/kvm-provider.md` alone; kvm is unchanged.
---
## Step 5 — Validate in-cluster against beelink
### Why, and what this run can and cannot prove
The kvm2 provider has **never run against real libvirt**: Step 11 is unchecked in
`docs/plans-executions/2026-09-16-1333-kvm2-provider.md`, every `base-*` volume on beelink
predates it, and both live `proxy-*` domains carry the kvm v1 metadata namespace. Four fixes
have landed on code no hypervisor has executed. This run is first contact, and it is also the
gate the campaign's `CHANGELOG` entry waits on.
It proves the happy paths: the dial and health probe, a real 415 MB image download through the
reader H2 now wraps, H4's new volume name appearing beside kvm's untouched one, a second Proxy
hitting the cache, H3's address path reaching Running with an IP, and Squid answering a real
CONNECT probe.
It does **not** prove H2's error paths. The unverified ~85% claim — that libvirtd answers a
short stream finish with OK — only arises when a body *fails*, and a healthy download never
finishes short. The 19-minute HTTP deadline will not fire either. A green run means "the
wrapper did not break normal uploads", not "the deadlock is settled". That still needs the
scratch-pool probe recorded in the Step 2 execution entry.
### The cluster this runs in, and the one hazard
`kind-proxy-operator-demo` is **not a throwaway**. It runs the operator that currently owns
four Ready proxies, two of them real VMs on beelink:
```
proxy-beelink-1 Managed kvm-home Ready 192.168.0.166 True
proxy-beelink-2 Managed kvm-home Ready 192.168.0.94 True
proxy-kubernetes-demo-1/2 Managed kubernetes Ready 10.244.0.7/.8 True
```
Validating kvm2 means rolling the operator that owns them. That is low risk — the controller
and the whole `kvm` provider are untouched by H1-H4 — and rollback is one `kubectl set image`.
**Do not run a second operator against beelink with `kvm-home` in its config.** Orphan GC
(`internal/gc/gc.go:86-131`) lists every Proxy in *its own* cluster, and deletes any instance
whose UID is not in that set and is older than `--gc-min-age` (10 m, swept every 10 m). A
second cluster's live set would not contain `proxy-beelink-1/2`'s UIDs, so its GC would destroy
them within ten minutes. Using the existing cluster removes the hazard entirely: its live set
is the one that protects them.
### Steps
**0. Baseline** — record what is there before touching anything, so "did we break it" is
answerable: `kubectl get px -A`, `kubectl get ph`, and a read-only `virsh vol-list default`
plus `virsh list --all` on beelink.
**1. Build and load, under a new tag.**
```bash
make docker-build IMG=egress-proxies-operator:kvm2-dev
kind load docker-image egress-proxies-operator:kvm2-dev --name proxy-operator-demo
```
The tag **must** differ from the running `kvm-dev`. `make deploy` sets the image through
kustomize; an identical tag leaves the Deployment spec unchanged, so no rollout happens, so the
new process never starts, so the new providers config is never read. The ConfigMap carries no
generator hash, so nothing else forces a restart either.
The working tree is dirty (`review/`, `.claude/`), so `GIT_COMMIT` bakes in as `c10f98a-dirty`
and `--version` will say so. Cosmetic; commit or stash `review/` first to avoid it.
**2. Enable kvm2 in the deployed config.** Uncomment the `kvm2-home` block in
`config/manager/providers_config.yaml` (it is the shipped default, commented out). Keep its
`address: beelink.tail8c69c.ts.net:22` — the Tailscale FQDN, which is what
`~/.ssh/kvm_known_hosts` pins. `config/samples/providers-config.yaml` uses the short
`beelink:22`, which **fails host-key verification**; I hit exactly that when validating SSH
earlier in this session.
**3. Deploy.**
```bash
make deploy IMG=egress-proxies-operator:kvm2-dev
kubectl -n egress-proxies-operator-system rollout status deploy/egress-proxies-operator-controller-manager
```
`make deploy` rewrites `config/manager/kustomization.yaml`'s image block as a side effect; it
already carries `kvm-dev` from a previous run.
**4. STOP GATE — provider health, before anything is created.**
```bash
kubectl get ph
kubectl get ph kvm2-home -o yaml
```
`kvm2-home` must be `Ready 1/1` with reason `Reachable` and details carrying `hostname`,
`libvirtVersion`, `poolState: running` and `poolAvailableGiB`. This proves SSH, the host key,
the libvirt handshake and the pool **without creating a thing**, and it exercises the
dedicated-connection dial and close that H1 touched. A failure here is free to diagnose:
`CredentialsUnavailable`, `Unauthorized`, `Forbidden`, `PoolNotActive` each name their own
cause. Report and wait for a go before step 5.
**5. Two Proxies.** Two manifests in the scratchpad (not committed), modelled on
`config/samples/proxy_kvm2.yaml` — `proxy-kvm2-1` and `proxy-kvm2-2` in `default`, matching the
existing `proxy-beelink-*` convention, `provider: kvm2-home`, `zone: beelink`,
`machineType: small`, `image: debian-13`, port 3128 and the sample's Squid cloud-init. Apply
the first, let it finish, then the second — sequencing them is what makes the cache hit
observable.
```bash
kubectl get px -w
kubectl -n egress-proxies-operator-system logs -f deploy/egress-proxies-operator-controller-manager
```
**6. What to check, and which finding it exercises.**
| Check | Finding |
|---|---|
| `fetching base image into host pool` then `base image cached` with size, duration, digest | H2's wrapped reader on a real 415 MB upload |
| `virsh vol-list default` shows `base-kvm2-home-debian-13.qcow2` **and** `base-debian-13.qcow2` still there | H4: per-entry naming, kvm's cache untouched |
| Second Proxy logs no `fetching`, and the server is not hit again | H4 + the image lock: cache hit under the new name |
| `px` goes Provisioning → Running with an IP, without flapping back to Provisioning | H3: the address path, and no spurious demotion |
| `Healthy=True` | Squid answers a real CONNECT probe end to end |
| `virsh list --all` shows two new `proxy-*` domains | Create |
| After deleting both: domains and their `.qcow2`/`-seed.iso` gone, base image still present | Delete, and the cache surviving |
Optionally `curl -x http://<ip>:3128 https://example.com -I` from this box, if it routes to
192.168.0.0/24 — the existing kvm proxies live on that LAN.
**7. Cleanup.** `kubectl delete px proxy-kvm2-1 proxy-kvm2-2` and let the finalizer do the
work, then verify on beelink read-only. **Any leftover domain or volume: I print the exact
`virsh undefine --nvram` / `vol-delete` lines and you run them** — no write reaches the shared
host from me. Then decide whether the `kvm2-home` ConfigMap entry stays (keep it if you want
kvm2 running for real; `git checkout config/manager/providers_config.yaml` otherwise).
**Rollback at any point:**
```bash
kubectl -n egress-proxies-operator-system set image \
deploy/egress-proxies-operator-controller-manager manager=egress-proxies-operator:kvm-dev
```
### Who runs what
I drive steps 0-6 and stop at gate 4 for a go. Every beelink write is yours. Nothing here
touches `proxy-beelink-1/2`, `base-debian-13.qcow2`, your unrelated VMs, or the five
pre-existing orphan volumes the review found.
---
## Step 6 — Route klog into the structured logger
**Not a review finding.** Found while watching the Step 5 validation: some lines ignore the
zap encoder entirely, so no amount of `--zap-encoder=json` makes the stream parseable.
```
I0917 19:50:02.580097 1 leaderelection.go:258] "Attempting to acquire leader lease..." lock="..."
```
That is klog's own text sink. client-go's leader elector does `logger := klog.FromContext(ctx)`
(`leaderelection.go:257`), and controller-runtime hands the elector a context with no logr in it
(`pkg/manager/internal.go:461-473` — `mgr.Start` gets `ctrl.SetupSignalHandler()`, which carries
no logger). `klog.FromContext` falls back to `Background()`, and with no logger registered that
is klog's stderr writer. Nothing in this repo imports or configures klog; it is an indirect
dependency (`k8s.io/klog/v2 v2.140.0`) that ~60 linked packages use, including
`client-go/tools/{leaderelection,cache,record}`, `client-go/transport` and `client-go/rest`.
controller-runtime does not route it either — its only `klog.SetLogger` call is in its own test
suite.
### 6a. `cmd/main.go` — one line and an import
```go
logger := zap.New(zap.UseFlagOptions(&opts))
ctrl.SetLogger(logger)
klog.SetLogger(logger)
```
**Plain `SetLogger`, deliberately not `SetLoggerWithOptions(logger, klog.ContextualLogger(true))`.**
The contextual form is what klog's own docs recommend, because it skips a klogr round trip — but
it also disables klog's verbosity gate ("Such a logger cannot rely on verbosity checking in
klog", `contextual.go:83-86`), handing the decision to zap's level. With `Development: true`
that level is Debug, which enables V(1), so client-go's V(1) chatter that klog currently
suppresses at its default `-v=0` would start appearing. Plain `SetLogger` keeps klog's own check
in front ("klog will do its own verbosity checks before calling logger.V().Info",
`contextual.go:43-44`) and still redirects everything it does emit ("all log lines will be
suppressed from the regular output, and redirected to the logr implementation", `:47-48`).
Same fix, no new volume. Performance is irrelevant at this scale.
`go.mod` promotes `k8s.io/klog/v2` from indirect to direct. It is already in the module graph,
so nothing new is downloaded.
### 6b. `internal/discovery/server.go` — the other stderr bypass
The discovery API's `http.Server` (`:113-119`) sets no `ErrorLog`, so `net/http`'s own errors —
`Accept` failures, handler panics that escape — go to `log.Default()` and out to stderr in
stdlib format. `s.log` is already in scope two lines above:
```go
srv := &http.Server{
Handler: s.handler(),
ErrorLog: slog.NewLogLogger(logr.ToSlogHandler(s.log), slog.LevelError),
ReadHeaderTimeout: readHeadTimeout,
...
}
```
`logr.ToSlogHandler` is the pattern the repo already uses for the same purpose in
`internal/provider/gcp/wirelog.go:31-36`. This server is plain HTTP with no TLS, so `ErrorLog`
carries genuine faults rather than handshake noise from scanners — Error level is right.
### 6c. `config/manager/manager.yaml` — make the deployed stream parseable
```yaml
args:
- --leader-elect
- --health-probe-bind-address=:8081
- --providers-config=/etc/proxy-operator/providers.yaml
- --zap-encoder=json
- --zap-time-encoding=rfc3339
```
Verified that the flag wins over the in-code default: `addDefaults` only fills `NewEncoder`
`if o.NewEncoder == nil` (`zap.go:176-191`), and `BindFlags` sets it from `--zap-encoder`.
`--zap-time-encoding=rfc3339` matters because the JSON encoder defaults to `epoch`, which would
give `1758138422.31` instead of a timestamp anyone can read. Verbosity is left at debug, so the
per-RPC lines stay — that was the choice.
### Deliberately not doing
`opts := zap.Options{Development: true}` (`cmd/main.go:154-155`) stays. Flipping it to a
production default would change the format for every existing deployment, not just this one, and
the flags in 6c get the same result here without that blast radius.
### What stays unformatted regardless, and why that is fine
Flag-parse errors and `--version` (`cmd/main.go:160-163`) print before any logger exists, and
`utilruntime.Must` panics in `init()` run before `main`. controller-runtime's own metrics and
health servers also set no `ErrorLog` (`pkg/internal/httpserver/server.go:11-19`) and expose no
option to set one. All three are outside reach; none can fire during steady-state operation.
### Tests
None. All three changes are wiring: two struct fields and one manifest key. `cmd/main.go` has no
test coverage at all, and pinning `klog.SetLogger` hermetically means mutating global state that
would leak into every other test in the package. Verified by observation instead — see below.
Same call as H1's: a test that costs more than it protects does not earn its place.
### Verification
Rebuild, redeploy, and watch the rollout produce the line that started this:
```bash
kubectl -n egress-proxies-operator-system logs deploy/egress-proxies-operator-controller-manager \
| grep -i 'leader' | head -3
```
It must be JSON with an rfc3339 `ts`, not `I0917 ...`. Then confirm the whole stream parses:
```bash
kubectl -n egress-proxies-operator-system logs deploy/egress-proxies-operator-controller-manager \
| jq -e -s 'length > 0' >/dev/null && echo "every line is JSON"
```
That second command is the real check — it fails if *any* line is not JSON, which is exactly the
property being fixed.
---
## Ordering
**H2 → H3 → H4**, three commits.
H3 is orthogonal: it touches only `get.go` and `get_test.go`.
H2 and H4 both touch `image.go` but at different heights, and the order matters. H2's `image.go`
edit is a deletion — `discardVolume` moves out of `uploadImage` and up into `fetchImage` — while
H4 edits `imageSource.volumeName()`, `resolveImage`, and `cachedImage`'s digest branch ten lines
below. Landing H2 first means H4 rebases cleanly; the reverse means H4 is written against a
failure path H2 is about to restructure. Keeping H2's guard in `backend.go` rather than in
`countingReader` is part of the same separation.
Semantically, H2 makes `fetchImage` delete the base after the transfer connection is closed, and
that delete is only unambiguously safe once names are per-entry — so H2's commit message should
say the discard is still unfenced against a second entry sharing the name and that H4 closes it,
and H4's should point back. The deferred atomic-publish work then lands exactly where H2 put the
discard, on a volume this call created under a name no other entry can produce.
## Verification
```bash
go build ./... && go vet ./...
go test -race -count=1 ./internal/provider/kvm2/
make test
```
Per step, the check that actually matters is that the new test fails against the unfixed code —
stash the source edit, keep the test, watch it fail, restore. For H2 that means asserting
`eofReader` converts before the fix exists; for H3, that the dead-link subtest returns a
successful Provisioning instance today; for H4, that two entries produce one name today.
After H4, run the kvm2 integration test on beelink. **It has never run** — Step 11 is unchecked
in `docs/plans-executions/2026-09-16-1333-kvm2-provider.md`, every `base-*` on the host predates
kvm2, and both live domains carry the kvm v1 metadata namespace. It is the first end-to-end
exercise of the new name and does a real cold download beside kvm's untouched base.
Each step: its own commit on `feat/kvm2-provider` with the execution-file entry and its checklist
line ticked. `CHANGELOG.md` lands with a "validate and record" step once the user confirms the
work, as in the kvm campaign. Trailer:
`Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>`.
go.mod
+1 -1
View File
@@ -23,6 +23,7 @@ require (
k8s.io/api v0.36.0
k8s.io/apimachinery v0.36.0
k8s.io/client-go v0.36.0
k8s.io/klog/v2 v2.140.0
sigs.k8s.io/controller-runtime v0.24.1
sigs.k8s.io/yaml v1.6.0
)
@@ -104,7 +105,6 @@ require (
k8s.io/apiextensions-apiserver v0.36.0 // indirect
k8s.io/apiserver v0.36.0 // indirect
k8s.io/component-base v0.36.0 // indirect
k8s.io/klog/v2 v2.140.0 // indirect
k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a // indirect
k8s.io/streaming v0.36.0 // indirect
k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2 // indirect
internal/discovery/server.go
+6 -1
View File
@@ -9,6 +9,7 @@ import (
"context"
"crypto/subtle"
"encoding/json"
"log/slog"
"net"
"net/http"
"strings"
@@ -111,7 +112,11 @@ func (s *Server) Start(ctx context.Context) error {
s.mu.Unlock()
srv := &http.Server{
Handler: s.handler(),
Handler: s.handler(),
// Without this, net/http's own faults (a failed Accept, a panic that
// escapes a handler) go to the stdlib logger on stderr, outside the
// structured stream.
ErrorLog: slog.NewLogLogger(logr.ToSlogHandler(s.log), slog.LevelError),
ReadHeaderTimeout: readHeadTimeout,
ReadTimeout: 10 * time.Second,
WriteTimeout: 10 * time.Second,
internal/provider/config_kvm.go
+10 -4
View File
@@ -108,11 +108,17 @@ var sha256Hex = regexp.MustCompile(`^[0-9a-fA-F]{64}$`)
// kvmIdentifier constrains the config names that are pasted into
// identifiers elsewhere: a host name goes into the providerID
// hosts/<name>/domains/<vm>, which is split on "/" to parse, and an image
// key becomes the file name of the cached base volume. A slash in either
// produces an ID or a path that never resolves again, so the VM leaks with
// nothing able to address it.
// key becomes part of the file name of the cached base volume. A slash in
// either produces an ID or a path that never resolves again, so the VM
// leaks with nothing able to address it.
var kvmIdentifier = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]*$`)
// IsKVMIdentifier reports whether s is safe to paste into a providerID or a
// volume name. The provider entry's own name reaches a volume name too, and
// that name is validated for every provider type at once (only non-empty
// and unique), so the kvm2 provider checks its own at construction.
func IsKVMIdentifier(s string) bool { return kvmIdentifier.MatchString(s) }
func validateKVM(prefix string, c *KVMConfig) error {
return validateKVMBlock(prefix, "kvm", c)
}
@@ -137,7 +143,7 @@ func validateKVMBlock(prefix, block string, c *KVMConfig) error {
p := fmt.Sprintf("%s: %s.images[%q]", prefix, block, name)
if !kvmIdentifier.MatchString(name) {
return fmt.Errorf("%s: key must be letters, digits, dots, dashes or underscores"+
" — it becomes the cached volume name base-<key>.<format>", p)
" — it becomes part of the cached base volume's name", p)
}
if !IsHTTPURL(img.URL) {
return fmt.Errorf("%s.url must be an http(s) URL (got %q)", p, img.URL)
internal/provider/kvm2/backend.go
+65 -13
View File
@@ -2,6 +2,7 @@ package kvm2
import (
"context"
"errors"
"fmt"
"io"
"sync"
@@ -90,6 +91,12 @@ const smallUploadBytes = 16 << 20
// closeBudget bounds the courtesy ConnectClose RPC in Close.
const closeBudget = 5 * time.Second
// transferGrace keeps the image download's own deadline inside the transfer
// budget. A stalled mirror then ends by its body failing, which the upload
// seam turns into a clean end of stream, instead of by the budget killing
// the socket, which go-libvirt's stream sender does not survive.
const transferGrace = time.Minute
func (b budgets) upload(length uint64) time.Duration {
if length <= smallUploadBytes {
return b.seedUpload
@@ -106,7 +113,6 @@ type link struct {
label string
kill func()
once sync.Once
closed chan struct{}
mu sync.Mutex
reason string
@@ -133,18 +139,23 @@ func (l *link) deadReason() string {
return l.reason
}
// markDead records why the connection is unusable, closes the transport and
// signals dead. Later calls are no-ops.
// markDead records why the connection is unusable, signals dead and then
// closes the transport. Later calls are no-ops. Signalling before killing,
// and killing outside the lock, is what keeps a kill that takes its time
// from holding up the callers waiting on dead or the next markDead: a
// transport close can block for as long as the kernel retransmits.
func (l *link) markDead(reason string) {
l.once.Do(func() {
l.mu.Lock()
l.reason = reason
l.mu.Lock()
if l.isDead() {
l.mu.Unlock()
if l.kill != nil {
l.kill()
}
close(l.closed)
})
return
}
l.reason = reason
close(l.closed)
l.mu.Unlock()
if l.kill != nil {
l.kill()
}
}
// call executes a blocking go-libvirt call with a budget and returns its
@@ -321,9 +332,50 @@ func (b *libvirtBackend) StorageVolCreateXML(ctx context.Context, pool libvirt.S
}
func (b *libvirtBackend) StorageVolUpload(ctx context.Context, vol libvirt.StorageVol, r io.Reader, offset, length uint64, flags libvirt.StorageVolUploadFlags) error {
return b.link.run(ctx, "StorageVolUpload", b.budgets.upload(length), func() error {
return b.lv.StorageVolUpload(vol, r, offset, length, flags)
src := &eofReader{r: r}
err := b.link.run(ctx, "StorageVolUpload", b.budgets.upload(length), func() error {
return b.lv.StorageVolUpload(vol, src, offset, length, flags)
})
if err == nil {
if srcErr := src.sourceErr(); srcErr != nil {
return fmt.Errorf("reading the upload source for %s: %w", vol.Name, srcErr)
}
}
return err
}
// eofReader hides a source error from go-libvirt's stream sender and keeps
// it for the caller. SendStream answers a non-EOF error with a StatusError
// packet and then blocks forever handing its result to an unbuffered
// channel that requestStream no longer reads (go-libvirt rpc.go), which
// wedges the call and leaks the goroutine along with its 4 MiB buffer;
// closing the socket does not help, because both ends are parked on
// channel sends rather than on I/O. io.EOF is the one outcome that ends
// the stream the normal way. The field is guarded because an abandoned
// call returns while go-libvirt is still reading.
type eofReader struct {
r io.Reader
mu sync.Mutex
err error
}
func (e *eofReader) Read(b []byte) (int, error) {
n, err := e.r.Read(b)
if err == nil || errors.Is(err, io.EOF) {
return n, err
}
e.mu.Lock()
if e.err == nil {
e.err = err
}
e.mu.Unlock()
return n, io.EOF
}
func (e *eofReader) sourceErr() error {
e.mu.Lock()
defer e.mu.Unlock()
return e.err
}
func (b *libvirtBackend) StorageVolDownload(ctx context.Context, vol libvirt.StorageVol, w io.Writer, offset, length uint64, flags libvirt.StorageVolDownloadFlags) error {
internal/provider/kvm2/backend_test.go
+117
View File
@@ -3,6 +3,7 @@ package kvm2
import (
"context"
"errors"
"io"
"strings"
"sync/atomic"
"testing"
@@ -148,6 +149,122 @@ func TestCall_killedByAnotherCallerFailsTransient(t *testing.T) {
}
}
// TestMarkDead_slowKillDoesNotBlockCallers pins the guarantee the pooled
// connection rests on: a kill that takes its time — closing a transport can
// block for as long as the kernel retransmits to a host that went away —
// holds up neither the callers waiting on dead nor the next markDead. The
// call's own fn signals when it is in flight, so the parked caller is
// genuinely parked rather than refused by the dead check on the way in.
func TestMarkDead_slowKillDoesNotBlockCallers(t *testing.T) {
t.Parallel()
release := make(chan struct{})
t.Cleanup(func() { close(release) })
killing := make(chan struct{})
var killed atomic.Int32
l := newLink("beelink", func() {
killed.Add(1)
close(killing)
<-release
})
inflight := make(chan struct{})
waiting := make(chan error, 1)
go func() {
waiting <- l.run(context.Background(), "StorageVolUpload", time.Minute, func() error {
close(inflight)
<-release
return nil
})
}()
<-inflight
go l.markDead("keepalive failed")
<-killing
select {
case <-l.dead():
case <-time.After(time.Second):
t.Fatal("link not signalled dead while its kill is still running")
}
select {
case err := <-waiting:
if !errors.Is(err, provider.ErrTransient) || !strings.Contains(err.Error(), "keepalive failed") {
t.Errorf("parked call error = %v, want ErrTransient with the death reason", err)
}
case <-time.After(time.Second):
t.Fatal("parked call not released while the kill is still running")
}
second := make(chan struct{})
go func() {
l.markDead("closed")
close(second)
}()
select {
case <-second:
case <-time.After(time.Second):
t.Fatal("second markDead blocked behind the running kill")
}
if killed.Load() != 1 {
t.Errorf("kill hook called %d times, want 1", killed.Load())
}
if !strings.Contains(l.deadReason(), "keepalive failed") {
t.Errorf("deadReason() = %q, want the first reason", l.deadReason())
}
}
// TestEOFReader_sourceErrorNeverReachesTheStream pins the one property the
// upload seam needs: go-libvirt's stream sender must never see an error
// other than io.EOF, because it answers one by parking forever on a channel
// nothing reads. The real error has to survive for the caller.
func TestEOFReader_sourceErrorNeverReachesTheStream(t *testing.T) {
t.Parallel()
boom := errors.New("connection reset by peer")
tests := []struct {
name string
reader io.Reader
wantN int
wantErr error
}{
{name: "error after data", reader: io.MultiReader(strings.NewReader("abc"), errReader{boom}), wantN: 3, wantErr: boom},
{name: "error with no data", reader: errReader{boom}, wantErr: boom},
{name: "clean eof passes through", reader: strings.NewReader("abc"), wantN: 3},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
e := &eofReader{r: tc.reader}
n, err := io.Copy(io.Discard, e)
if err != nil {
t.Fatalf("io.Copy through eofReader = %v, want nil: the stream sender must only ever see io.EOF", err)
}
if n != int64(tc.wantN) {
t.Errorf("copied %d bytes, want %d", n, tc.wantN)
}
if !errors.Is(e.sourceErr(), tc.wantErr) {
t.Errorf("sourceErr() = %v, want %v", e.sourceErr(), tc.wantErr)
}
})
}
}
func TestEOFReader_keepsTheFirstError(t *testing.T) {
t.Parallel()
first, second := errors.New("first"), errors.New("second")
e := &eofReader{r: io.MultiReader(errReader{first}, errReader{second})}
if _, err := io.Copy(io.Discard, e); err != nil {
t.Fatalf("io.Copy through eofReader = %v, want nil", err)
}
if !errors.Is(e.sourceErr(), first) {
t.Errorf("sourceErr() = %v, want the first error", e.sourceErr())
}
}
type errReader struct{ err error }
func (r errReader) Read([]byte) (int, error) { return 0, r.err }
func TestBudgets_upload(t *testing.T) {
t.Parallel()
tests := []struct {
internal/provider/kvm2/create.go
+1 -1
View File
@@ -38,7 +38,7 @@ func (p *Provider) Create(ctx context.Context, req provider.CreateRequest) (stri
if !ok {
return "", p.wrapErr("create", "", permanentf("placement.machineType %q is not in kvm2.machineTypes", req.Placement.MachineType))
}
src, err := resolveImage(p.cfg, req.Placement.Image)
src, err := resolveImage(p.name, p.cfg, req.Placement.Image)
if err != nil {
return "", p.wrapErr("create", "", err)
}
internal/provider/kvm2/create_test.go
+3 -3
View File
@@ -61,13 +61,13 @@ func TestCreate_resumesAfterEachPartialStep(t *testing.T) {
{
name: "base without marker is refetched",
setup: func(fx *fixture) {
fx.putVol("base-debian-13.qcow2", []byte("truncated"))
fx.putVol(fx.baseName(), []byte("truncated"))
},
check: func(t *testing.T, fx *fixture) {
if got := fx.fake.vol("base-debian-13.qcow2"); got == nil || !bytes.Equal(got.content, fx.image) {
if got := fx.fake.vol(fx.baseName()); got == nil || !bytes.Equal(got.content, fx.image) {
t.Errorf("base volume not refetched")
}
if fx.fake.vol("base-debian-13.qcow2.sha256") == nil {
if fx.fake.vol(fx.baseName()+".sha256") == nil {
t.Errorf("marker not written")
}
},
internal/provider/kvm2/delete_test.go
+2 -1
View File
@@ -64,7 +64,8 @@ func TestDelete_removesOverlayAndSeed(t *testing.T) {
t.Errorf("volume %s still present after Delete", name)
}
}
for _, name := range []string{"base-debian-13.qcow2", "base-debian-13.qcow2.sha256"} {
base := "base-" + testProvider + "-debian-13.qcow2"
for _, name := range []string{base, base + ".sha256"} {
if fx.fake.vol(name) == nil {
t.Errorf("shared base volume %s removed by Delete", name)
}
internal/provider/kvm2/get.go
+19 -5
View File
@@ -98,22 +98,36 @@ var addressSources = []struct {
}
// address returns the first usable IPv4 from the first source that yields
// one, or "" for "no address yet". A source that fails is logged and
// skipped; only ctx ending is an error, so a cancelled Get does not read
// as Provisioning.
// one, or "" for "no address yet". One source failing is normal and is
// skipped: a booting VM has no agent yet, while the host's ARP table
// simply reports nothing, since it never sees traffic addressed
// elsewhere. A connection already gone, and every source failing, are
// errors instead — otherwise a link some other reconcile killed reads as
// a running VM that lost its address, and the controller clears its IP
// and the health engine drops its accumulated state.
func address(ctx context.Context, log logr.Logger, be backend, dom libvirt.Domain) (string, error) {
errs := make([]error, 0, len(addressSources))
for _, s := range addressSources {
ifaces, err := be.DomainInterfaceAddresses(ctx, dom, uint32(s.src), 0)
if err != nil {
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return "", fmt.Errorf("reading addresses of domain %s: %w", dom.Name, err)
if isDead(be) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return "", fmt.Errorf("reading addresses of domain %s from the %s source: %w", dom.Name, s.name, err)
}
log.V(2).Info("no addresses from source", "source", s.name, "error", err.Error())
errs = append(errs, fmt.Errorf("%s source: %w", s.name, err))
continue
}
if ip := firstIPv4(ifaces); ip != "" {
return ip, nil
}
}
// Transient explicitly: classify would otherwise pick a libvirt code
// out of the joined error, and ErrInvalidArg, ErrAccessDenied and
// ErrNoSupport are Permanent, which would latch a running proxy as
// Failed over an address read.
if len(errs) == len(addressSources) {
return "", fmt.Errorf("%w: reading addresses of domain %s: every source failed: %w",
provider.ErrTransient, dom.Name, errors.Join(errs...))
}
return "", nil
}
internal/provider/kvm2/get_test.go
+55 -3
View File
@@ -156,17 +156,69 @@ func TestGet_arpFallbackWhenAgentFails(t *testing.T) {
}
}
func TestGet_addressErrorIsProvisioning(t *testing.T) {
// TestGet_agentFailsAndARPEmptyIsProvisioning is a VM part way through
// booting on a plain bridge: the agent is not up yet, and the host's ARP
// table answers without error because it has nothing to report. One
// source failing must stay "no address yet", not an error.
func TestGet_agentFailsAndARPEmptyIsProvisioning(t *testing.T) {
t.Parallel()
fx := newFixture(t, 1)
fx := newFixture(t, 2)
id := fx.create(testName)
fx.fake.failWith("DomainInterfaceAddresses", fakeErr(libvirt.ErrAgentUnresponsive, "Guest agent is not responding"))
var calls int
fx.fake.mu.Lock()
fx.fake.hooks["DomainInterfaceAddresses"] = func() error {
calls++
if calls%2 == 1 {
return fakeErr(libvirt.ErrAgentUnresponsive, "Guest agent is not responding")
}
return nil
}
fx.fake.mu.Unlock()
inst := fx.get(id)
if inst.State != provider.StateProvisioning || inst.IP != "" {
t.Errorf("Get = %+v, want Provisioning with no IP", inst)
}
}
// TestGet_everyAddressSourceFailingIsTransient covers the other side of
// that line. A running VM whose address cannot be read is not a VM
// without an address: reporting Provisioning would make the controller
// clear its IP and the health engine drop its counters.
func TestGet_everyAddressSourceFailingIsTransient(t *testing.T) {
t.Parallel()
tests := []struct {
name string
setup func(fx *fixture)
}{
{
name: "both sources error",
setup: func(fx *fixture) {
fx.fake.failWith("DomainInterfaceAddresses", fakeErr(libvirt.ErrOperationInvalid, "domain is not running"))
},
},
{
name: "the pooled connection died under the read",
setup: func(fx *fixture) {
fx.fake.failWith("DomainInterfaceAddresses", transientf("libvirt call DomainInterfaceAddresses on beelink: connection is closed: keepalive failed"))
fx.fake.markDead()
},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
fx := newFixture(t, 1)
id := fx.create(testName)
tc.setup(fx)
inst, err := fx.p.Get(fx.ctx, id)
if inst != nil {
t.Errorf("Get = %+v, want no instance", inst)
}
assertProviderError(t, err, "get", provider.ErrTransient)
})
}
}
func TestGet_readsUIDAndCreatedAtFromBackend(t *testing.T) {
t.Parallel()
fx := newFixture(t, 1)
internal/provider/kvm2/image.go
+27 -13
View File
@@ -42,22 +42,25 @@ var hexDigest = regexp.MustCompile(`^[0-9a-f]{64}$`)
// catalogue entry, or a URL given directly with the catalogue defaults, plus
// the key its base volume is named after.
type imageSource struct {
Key string
Entry string
Key string
provider.KVMImage
}
func (s imageSource) volumeName() string { return baseVolumeName(s.Key, s.Format) }
func (s imageSource) volumeName() string { return baseVolumeName(s.Entry, s.Key, s.Format) }
func (s imageSource) markerName() string { return markerVolumeName(s.volumeName()) }
// resolveImage maps placement.image to a source. cfg is already defaulted,
// so a catalogue entry carries its format and firmware; a URL gets qcow2
// and uefi.
func resolveImage(cfg provider.KVMConfig, image string) (imageSource, error) {
// and uefi. entry is the provider entry's name, which the base volume is
// named after so two entries never share one file.
func resolveImage(entry string, cfg provider.KVMConfig, image string) (imageSource, error) {
if img, ok := cfg.Images[image]; ok {
return imageSource{Key: image, KVMImage: img}, nil
return imageSource{Entry: entry, Key: image, KVMImage: img}, nil
}
if provider.IsHTTPURL(image) {
return imageSource{
Entry: entry,
Key: urlImageKey(image),
KVMImage: provider.KVMImage{URL: image, Format: provider.KVMFormatQcow2, Firmware: provider.KVMFirmwareUEFI},
}, nil
@@ -127,7 +130,11 @@ func cachedImage(ctx context.Context, log logr.Logger, pool libvirt.StoragePool,
case !hexDigest.MatchString(digest):
log.V(1).Info("base image marker holds no digest; refetching")
case src.SHA256 != "" && !strings.EqualFold(digest, src.SHA256):
log.V(1).Info("base image digest differs from the configured sha256; refetching", "cached", digest, "configured", strings.ToLower(src.SHA256))
// Refusing rather than replacing: every overlay on this host names
// the base as its backing file, and libvirt will unlink it under
// running domains without a word.
return "", permanentf("base image %s is cached with sha256 %s but %s is configured; add the image under a new key and move Proxies to it, or delete the Proxies using it and evict the base by hand",
name, digest, strings.ToLower(src.SHA256))
default:
base, err := be.StorageVolLookupByName(ctx, pool, name)
switch {
@@ -214,7 +221,12 @@ func (p *Provider) fetchImage(ctx context.Context, host provider.KVMHost, pool l
log := p.logger(ctx).WithValues("host", host.Name, "volume", name)
start := time.Now()
resp, err := p.do(ctx, http.MethodGet, src.URL)
// The body runs out of time before the surrounding budget does, so a
// mirror that stalls mid-image ends the upload through its reader
// rather than through the killed socket the budget would leave.
httpCtx, cancelHTTP := context.WithTimeout(ctx, defaultBudgets.transfer-transferGrace)
defer cancelHTTP()
resp, err := p.do(httpCtx, http.MethodGet, src.URL)
if err == nil {
err = checkResponse(src.URL, resp)
}
@@ -227,8 +239,11 @@ func (p *Provider) fetchImage(ctx context.Context, host provider.KVMHost, pool l
span.SetAttributes(attribute.Int64("kvm2.bytes", size))
log.V(1).Info("fetching base image into host pool", "url", src.URL, "bytes", size)
vol, digest, err := p.uploadImage(ctx, host, pool, be, src, resp.Body, size)
vol, digest, err := p.uploadImage(ctx, host, pool, src, resp.Body, size)
if err != nil {
// uploadImage has returned, so its dedicated connection is closed
// and libvirtd has let go of the stream before the file goes.
p.discardVolume(ctx, log, be, pool, name)
span.SetStatus(codes.Error, err.Error())
return libvirt.StorageVol{}, err
}
@@ -249,10 +264,10 @@ func (p *Provider) fetchImage(ctx context.Context, host provider.KVMHost, pool l
// uploadImage creates the base volume and streams body into it over a
// dedicated connection with a running sha256, then checks length and
// digest. On any failure the volume is discarded and the error says why:
// a body that ended early is Transient, a digest that differs from the
// configured one is Permanent.
func (p *Provider) uploadImage(ctx context.Context, host provider.KVMHost, pool libvirt.StoragePool, be backend, src imageSource, body io.Reader, size int64) (libvirt.StorageVol, string, error) {
// digest. The error says why: a body that ended early is Transient, a
// digest that differs from the configured one is Permanent. Discarding
// the volume is the caller's, once this connection is closed.
func (p *Provider) uploadImage(ctx context.Context, host provider.KVMHost, pool libvirt.StoragePool, src imageSource, body io.Reader, size int64) (libvirt.StorageVol, string, error) {
name := src.volumeName()
xmlDoc, err := buildVolumeXML(volumeSpec{Name: name, CapacityBytes: size, Format: src.Format})
if err != nil {
@@ -289,7 +304,6 @@ func (p *Provider) uploadImage(ctx context.Context, host provider.KVMHost, pool
err = permanentf("image %s: sha256 %s does not match the configured %s", src.URL, digest, strings.ToLower(src.SHA256))
}
if err != nil {
p.discardVolume(ctx, p.logger(ctx).WithValues("host", host.Name, "volume", name), be, pool, name)
return libvirt.StorageVol{}, "", err
}
return vol, digest, nil
internal/provider/kvm2/image_test.go
+28 -26
View File
@@ -100,7 +100,7 @@ func (fx *imageFixture) sameConnection() {
func (fx *imageFixture) src() imageSource {
fx.t.Helper()
src, err := resolveImage(fx.p.cfg, imageKey)
src, err := resolveImage(fx.p.name, fx.p.cfg, imageKey)
if err != nil {
fx.t.Fatalf("resolveImage(%q) error = %v", imageKey, err)
}
@@ -180,20 +180,20 @@ func TestResolveImage_table(t *testing.T) {
{
name: "catalogue key with defaults applied",
image: "debian-13",
want: imageSource{Key: "debian-13", KVMImage: provider.KVMImage{URL: "https://cloud.debian.org/debian-13.qcow2", SHA256: "ABCD", Format: "qcow2", Firmware: "uefi"}},
wantVolume: "base-debian-13.qcow2",
want: imageSource{Entry: testProvider, Key: "debian-13", KVMImage: provider.KVMImage{URL: "https://cloud.debian.org/debian-13.qcow2", SHA256: "ABCD", Format: "qcow2", Firmware: "uefi"}},
wantVolume: "base-" + testProvider + "-debian-13.qcow2",
},
{
name: "catalogue key with explicit format and firmware",
image: "alpine",
want: imageSource{Key: "alpine", KVMImage: provider.KVMImage{URL: "https://alpine.example/alpine.img", Format: "raw", Firmware: "bios"}},
wantVolume: "base-alpine.raw",
want: imageSource{Entry: testProvider, Key: "alpine", KVMImage: provider.KVMImage{URL: "https://alpine.example/alpine.img", Format: "raw", Firmware: "bios"}},
wantVolume: "base-" + testProvider + "-alpine.raw",
},
{
name: "http URL used directly with defaults",
image: rawURL,
want: imageSource{Key: urlImageKey(rawURL), KVMImage: provider.KVMImage{URL: rawURL, Format: "qcow2", Firmware: "uefi"}},
wantVolume: "base-" + urlImageKey(rawURL) + ".qcow2",
want: imageSource{Entry: testProvider, Key: urlImageKey(rawURL), KVMImage: provider.KVMImage{URL: rawURL, Format: "qcow2", Firmware: "uefi"}},
wantVolume: "base-" + testProvider + "-" + urlImageKey(rawURL) + ".qcow2",
},
{
name: "unknown key is permanent and names the value",
@@ -214,7 +214,7 @@ func TestResolveImage_table(t *testing.T) {
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
got, err := resolveImage(cfg, tc.image)
got, err := resolveImage(testProvider, cfg, tc.image)
if tc.wantErr != "" {
if err == nil || !errors.Is(err, provider.ErrPermanent) || !strings.Contains(err.Error(), tc.wantErr) {
t.Fatalf("resolveImage(%q) = %+v, %v; want ErrPermanent containing %q", tc.image, got, err, tc.wantErr)
@@ -278,7 +278,7 @@ func TestEnsureImage_downloadsAndWritesMarkerLast(t *testing.T) {
t.Errorf("got %d lines with %s, want 1:\n%s", len(lines), want, strings.Join(fx.logs.all(), "\n"))
continue
}
for _, kv := range []string{`"provider"="kvm2-home"`, `"host"="beelink"`, `"volume"="base-debian-13.qcow2"`, `"bytes"=` + strconv.Itoa(len(body)), `"traceID"=`} {
for _, kv := range []string{`"provider"="kvm2-home"`, `"host"="beelink"`, `"volume"="base-` + testProvider + `-debian-13.qcow2"`, `"bytes"=` + strconv.Itoa(len(body)), `"traceID"=`} {
if !strings.Contains(lines[0], kv) {
t.Errorf("line lacks %s: %s", kv, lines[0])
}
@@ -295,7 +295,7 @@ func TestEnsureImage_downloadsAndWritesMarkerLast(t *testing.T) {
attrs := spans[0].Attributes()
for _, want := range []attribute.KeyValue{
attribute.String("kvm2.host", "beelink"),
attribute.String("kvm2.volume", "base-debian-13.qcow2"),
attribute.String("kvm2.volume", "base-"+testProvider+"-debian-13.qcow2"),
attribute.String("kvm2.url_host", strings.TrimPrefix(fx.srv.URL, "http://")),
attribute.Int64("kvm2.bytes", int64(len(body))),
attribute.Bool("kvm2.sha256_configured", false),
@@ -391,7 +391,13 @@ func TestEnsureImage_missingMarkerRedownloads(t *testing.T) {
}
}
func TestEnsureImage_sha256ChangeRedownloads(t *testing.T) {
// TestEnsureImage_sha256ChangeRefusesInsteadOfReplacing pins the rule the
// contract already stated and the code contradicted. Every overlay on the
// host names the base as its backing file, and libvirt unlinks it under
// running domains without complaint, so a digest that no longer matches is
// for the operator to resolve, not for the next Create to resolve by
// deleting.
func TestEnsureImage_sha256ChangeRefusesInsteadOfReplacing(t *testing.T) {
t.Parallel()
body := randomBody(t, 8192)
fx := newImageFixture(t, serveBytes(body))
@@ -401,25 +407,21 @@ func TestEnsureImage_sha256ChangeRedownloads(t *testing.T) {
fx.putVol(src.volumeName(), []byte("the old image"))
fx.putVol(src.markerName(), []byte(digestA))
if _, err := fx.ensure(fx.ctx, src); err != nil {
t.Fatalf("ensureImage() error = %v", err)
_, err := fx.ensure(fx.ctx, src)
if !errors.Is(err, provider.ErrPermanent) || !strings.Contains(err.Error(), "add the image under a new key") {
t.Fatalf("ensureImage() error = %v, want ErrPermanent telling the operator what to do", err)
}
if fx.hits.Load() != 1 {
t.Errorf("server hits = %d, want 1", fx.hits.Load())
if fx.hits.Load() != 0 {
t.Errorf("server hits = %d, want 0: nothing should be refetched", fx.hits.Load())
}
if n := fx.pooled.callCount("StorageVolDelete"); n != 2 {
t.Errorf("StorageVolDelete called %d times, want 2 (marker and base)", n)
if n := fx.pooled.callCount("StorageVolDelete"); n != 0 {
t.Errorf("StorageVolDelete called %d times, want 0: the base backs live overlays", n)
}
base := fx.pooled.vol(src.volumeName())
if base == nil || !bytes.Equal(base.content, body) {
t.Errorf("base volume was not replaced by the download")
if base := fx.pooled.vol(src.volumeName()); base == nil || !bytes.Equal(base.content, []byte("the old image")) {
t.Errorf("base volume = %+v, want the cached one untouched", base)
}
marker := fx.pooled.vol(src.markerName())
if marker == nil || string(marker.content) != digestOf(body) {
t.Errorf("marker = %+v, want the new digest %s", marker, digestOf(body))
}
if lines := fx.logs.matching("differs from the configured sha256"); len(lines) != 1 {
t.Errorf("got %d refetch lines, want 1", len(lines))
if marker := fx.pooled.vol(src.markerName()); marker == nil || string(marker.content) != digestA {
t.Errorf("marker = %+v, want the cached digest untouched", marker)
}
}
internal/provider/kvm2/kvm2.go
+3
View File
@@ -74,6 +74,9 @@ var _ provider.Provider = (*Provider)(nil)
// files are only checked for existence, and a missing one is a warning, not
// an error, because the Secret mount is optional and may land later.
func New(ctx context.Context, pc provider.ProviderConfig) (provider.Provider, error) {
if !provider.IsKVMIdentifier(pc.Name) {
return nil, permanentf("provider name %q: kvm2 names its cached base images after the entry, so the name must be letters, digits, dots, dashes or underscores", pc.Name)
}
cfg := provider.KVMConfig{}
if pc.KVM2 != nil {
cfg = *pc.KVM2
internal/provider/kvm2/kvm2_integration_test.go
+1 -1
View File
@@ -481,7 +481,7 @@ func TestKVM2_lifecycle(t *testing.T) {
// 10. The pool keeps the shared base image and its marker, and holds
// no per-VM volume.
start = time.Now()
base := baseVolumeName(integrationImage, provider.KVMFormatQcow2)
base := baseVolumeName(integrationProvider, integrationImage, provider.KVMFormatQcow2)
marker := markerVolumeName(base)
overlay, seed := overlayVolumeName(name), seedVolumeName(name)
present := poolVolumes(ctx, t, p, host, base, marker, overlay, seed)
internal/provider/kvm2/kvm2_test.go
+12
View File
@@ -110,6 +110,18 @@ func TestNew_nilBlockDoesNotPanic(t *testing.T) {
}
}
// TestNew_rejectsUnsafeEntryName guards the one config value that reaches a
// volume name without passing the kvm validator: the provider entry's own
// name, which is checked only for being non-empty and unique.
func TestNew_rejectsUnsafeEntryName(t *testing.T) {
t.Parallel()
ctx, _ := captureContext()
_, err := New(ctx, provider.ProviderConfig{Name: "kvm2/home", Type: "kvm2"})
if !errors.Is(err, provider.ErrPermanent) || !strings.Contains(err.Error(), "kvm2/home") {
t.Fatalf("New() error = %v, want ErrPermanent naming the entry", err)
}
}
func TestHost_table(t *testing.T) {
t.Parallel()
tests := []struct {
internal/provider/kvm2/provider_test.go
+13 -2
View File
@@ -129,10 +129,21 @@ func (fx *fixture) putVol(name string, content []byte) *fakeVol {
return v
}
// baseName is the cached base image's volume name for this fixture's
// provider entry and image key.
func (fx *fixture) baseName() string {
fx.t.Helper()
src, err := resolveImage(fx.p.name, fx.p.cfg, imageKey)
if err != nil {
fx.t.Fatal(err)
}
return src.volumeName()
}
// cacheImage places a complete base image and its marker, as a previous
// Create on the host would have.
func (fx *fixture) cacheImage() {
src, err := resolveImage(fx.p.cfg, imageKey)
src, err := resolveImage(fx.p.name, fx.p.cfg, imageKey)
if err != nil {
fx.t.Fatal(err)
}
@@ -406,7 +417,7 @@ func TestLogging_v0SilentV1OutcomeV2Detail(t *testing.T) {
if len(resolved) != 1 {
t.Fatalf("V(2) lines matching resolved = %d, want 1", len(resolved))
}
for _, key := range []string{`"cloudInitBytes"=`, `"image"="base-debian-13.qcow2"`, `"firmware"="uefi"`, `"vcpus"=1`, `"memoryMiB"=1024`, `"machineType"="small"`, `"bridge"="br0"`, `"pool"="default"`} {
for _, key := range []string{`"cloudInitBytes"=`, `"image"="base-` + testProvider + `-debian-13.qcow2"`, `"firmware"="uefi"`, `"vcpus"=1`, `"memoryMiB"=1024`, `"machineType"="small"`, `"bridge"="br0"`, `"pool"="default"`} {
if !strings.Contains(resolved[0], key) {
t.Errorf("resolved line lacks %s: %s", key, resolved[0])
}
internal/provider/kvm2/ssh.go
+7 -5
View File
@@ -96,8 +96,12 @@ func loadCredentials(cfg provider.KVMConfig) (credentials, error) {
}
// sshSocket is the libvirt socket forwarded through one SSH connection. The
// embedded Conn is the channel; Close also closes the SSH client, which is
// what unblocks anything reading the channel.
// embedded Conn is the channel; Close closes the SSH client, whose TCP
// connection carries it, and that alone unblocks the channel's readers and
// writers. Closing the channel as well would be worse than useless: its
// Close writes a close message under the channel's write lock, so it would
// queue behind a transfer stuck in a TCP write, which is the case Close
// exists to survive.
type sshSocket struct {
net.Conn
client *ssh.Client
@@ -107,9 +111,7 @@ type sshSocket struct {
}
func (s *sshSocket) Close() error {
s.closeOnce.Do(func() {
s.closeErr = errors.Join(s.Conn.Close(), s.client.Close())
})
s.closeOnce.Do(func() { s.closeErr = s.client.Close() })
return s.closeErr
}
internal/provider/kvm2/volume.go
+9 -2
View File
@@ -14,8 +14,15 @@ import (
func overlayVolumeName(name string) string { return name + ".qcow2" }
func seedVolumeName(name string) string { return name + "-seed.iso" }
func baseVolumeName(key, format string) string { return "base-" + key + "." + format }
func markerVolumeName(baseName string) string { return baseName + ".sha256" }
// baseVolumeName carries the provider entry because the base is shared by
// every Proxy on a host but must not be shared by two entries: the kvm
// provider builds base-<key>.<format> in the same pool from the same
// catalogue shape, and two kvm2 entries pointing one key at different URLs
// would otherwise agree on the name and disagree on the contents.
func baseVolumeName(entry, key, format string) string {
return "base-" + entry + "-" + key + "." + format
}
func markerVolumeName(baseName string) string { return baseName + ".sha256" }
// urlImageKey stands in for the catalogue key of an image given as a URL:
// the first 16 hex characters of sha256(url).
internal/provider/kvm2/volume_test.go
+7 -3
View File
@@ -119,10 +119,14 @@ func TestVolumeNames(t *testing.T) {
}{
{name: "overlay", got: overlayVolumeName("proxy-abc"), want: "proxy-abc.qcow2"},
{name: "seed", got: seedVolumeName("proxy-abc"), want: "proxy-abc-seed.iso"},
{name: "base qcow2", got: baseVolumeName("debian-13", provider.KVMFormatQcow2), want: "base-debian-13.qcow2"},
{name: "base raw", got: baseVolumeName("alpine", provider.KVMFormatRaw), want: "base-alpine.raw"},
{name: "base qcow2", got: baseVolumeName("kvm2-home", "debian-13", provider.KVMFormatQcow2), want: "base-kvm2-home-debian-13.qcow2"},
{name: "base raw", got: baseVolumeName("kvm2-home", "alpine", provider.KVMFormatRaw), want: "base-kvm2-home-alpine.raw"},
{name: "marker", got: markerVolumeName("base-debian-13.qcow2"), want: "base-debian-13.qcow2.sha256"},
{name: "marker of url base", got: markerVolumeName(baseVolumeName(urlImageKey("https://x/y.img"), "qcow2")), want: "base-" + urlImageKey("https://x/y.img") + ".qcow2.sha256"},
{name: "marker of url base", got: markerVolumeName(baseVolumeName("kvm2-home", urlImageKey("https://x/y.img"), "qcow2")), want: "base-kvm2-home-" + urlImageKey("https://x/y.img") + ".qcow2.sha256"},
// Two entries pointing one key at different images must not agree
// on a file name, and neither may collide with the kvm provider's
// base-<key>.<format> in the same pool.
{name: "another entry, same key", got: baseVolumeName("kvm2-lab", "debian-13", provider.KVMFormatQcow2), want: "base-kvm2-lab-debian-13.qcow2"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
review/REPORT.md
+183
View File
@@ -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-<key>.<format>` 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.<key>.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 `<base>.part-<random>`, 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-<x>.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 `<uid>` 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.
review/deep.md
+265
View File
@@ -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"
```
review/escalations.md
+102
View File
@@ -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 `<uid>` or `<createdAt>` 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 `<name>.qcow2`, `<name>-seed.iso`, `base-<key>.<format>` 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.<key>.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 <base> … <capacity>` 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.
review/scope.md
+133
View File
@@ -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 `<metadata>`: 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.
review/sweep.md
+216
View File
@@ -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<chunk>.<n>`; "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-<x>.qcow2` (10 GiB capacity) and `proxy-<x>-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/<host>/domains/<name>` 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.<key>.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-<key>.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-<key>.<format>` 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`, `<os firmware="efi">` (libvirt ≥ 5.2.0) and `<firmware><feature name="secure-boot">` (≥ 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 `<createdAt>` 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 `<vcpu>0</vcpu>`. 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 `<capacity>` smaller than `<backingStore>`.
**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 `<permissions>`, 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-<key>.<format>`, 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.<key>.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 `<instance>` 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.