The clean-room kvm2 provider, plus a scoped code review of it and the four high findings that review produced. Validated in-cluster against a real hypervisor.
The provider
kvm2 runs Managed proxies as VMs on libvirt/KVM hosts reached over SSH, with the libvirt socket forwarded through the connection. Built from docs/provider-contract.md and docs/kvm2-provider-spec.md rather than from the first-generation kvm provider's code. What it adds over kvm: a child span per libvirt RPC, a pool-space precheck that maps to the reconciler's quota wait, SSH credentials re-read on every dial, image downloads and health checks on connections of their own, an adoption guard on the owning UID, and SSH auth failures that stop retrying.
The review, and what it found
review/ carries the whole thing: scope.md (the file set and a seven-chunk split), sweep.md (49 findings after dedupe), escalations.md (ten hypotheses worth a deep dive), deep.md (the verdicts) and REPORT.md (the final ranked report). 52 confirmed findings: 4 high, 20 medium, 28 low. The four high ones are fixed here, one commit each. The mediums and lows are not.
H1 - a stalled write delayed connection teardown.sshSocket.Close ran the blocking close before the unblocking one: closing an SSH channel writes under the transport lock, which a bulk image upload already holds, so on a host that vanished without RST the teardown waited for the kernel to stop retransmitting (~15 min). link.markDead compounded it by killing inside a sync.Once before signalling dead, so even cancelling the reconcile did not help. With MaxConcurrentReconciles: 3, three Proxies on one dead host stalled every Proxy on every provider. Verified against the pinned x/crypto rather than assumed.
H2 - an error mid-upload wedged go-libvirt's stream sender. A failing HTTP body deadlocked StorageVolUpload outright, leaking two goroutines and 4 MiB each time against a 128Mi limit. The defect is upstream - requestStream starts its sender on unbuffered channels and never drains outErr on the error path - so this is a workaround, not a repair: the reader can now only end the stream cleanly, and the transfer deadline moved 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 quoted.
H3 - an unreadable address looked like a missing one. Every error from both address sources read as "no address yet", so a pooled connection killed by an unrelated reconcile's timeout demoted a healthy proxy to Provisioning, cleared its IP, and made the health engine drop its accumulated counters. One source failing stays "no address yet"; every source failing, or a connection already dead, is Transient. Half of this finding is deliberately still open: a guest agent restarting inside a running VM still demotes it, because the discriminator is the previous IP, which the provider does not hold and the controller does. Follow-up recorded against proxy_controller.go.
H4 - the base image cache was shared across provider entries. kvm2 built byte-identical volume names to the kvm provider in the same pool, and the shipped config invited exactly that overlap. Two entries could share one file, a concurrent cold fetch could leave a guest booting from a half-written base, and a sha256 edit deleted the base under live overlays. Names now carry the provider entry, and a digest mismatch is refused rather than resolved by deleting.
Validation
Deployed to a kind cluster and run against a real hypervisor - the first time this code had touched libvirt rather than the in-memory fake.
Check
Result
ProviderHealth
Ready 1/1, Reachable, 189 ms, libvirt 11.6.0
Cold image fetch
433,651,712 bytes in 8.3 s, digest verified
Volume naming (H4)
new per-entry base created beside the kvm provider's untouched one
Second Proxy
cache hit, no re-download
State reporting (H3)
Running steady across every poll, no flapping
Squid
Healthy=True on a real CONNECT probe
Delete
domain and per-VM volumes gone, base image survived
Pre-existing proxies
all four stayed Ready across two rollouts
What the run did not prove, stated plainly because a green run is easy to over-read: H2's error paths need a body that fails part way through, and a healthy download never finishes short, so the unverified claim about libvirtd's response to a short stream finish is still open - a probe recipe is in the execution notes. make test-integration-kvm2 has also still never been run.
Also here
klog and net/http's own errors now join the structured log stream; the deployment logs JSON with rfc3339 timestamps, so kubectl logs | jq works. client-go's leader election was writing I0917 ... straight to stderr because nothing routed klog into logr.
The kvm2-home provider entry ships enabled.
docs/kvm2-provider.md (operator guide), docs/kvm2-provider-spec.md (the design), and the plan/execution pair under docs/plans*/.
make test green, go vet clean including -tags=integration. Every regression test was run against the unfixed code first and watched to fail; those commands are in the execution notes.
The clean-room kvm2 provider, plus a scoped code review of it and the four high findings that review produced. Validated in-cluster against a real hypervisor.
## The provider
`kvm2` runs Managed proxies as VMs on libvirt/KVM hosts reached over SSH, with the libvirt socket forwarded through the connection. Built from `docs/provider-contract.md` and `docs/kvm2-provider-spec.md` rather than from the first-generation `kvm` provider's code. What it adds over `kvm`: a child span per libvirt RPC, a pool-space precheck that maps to the reconciler's quota wait, SSH credentials re-read on every dial, image downloads and health checks on connections of their own, an adoption guard on the owning UID, and SSH auth failures that stop retrying.
## The review, and what it found
`review/` carries the whole thing: `scope.md` (the file set and a seven-chunk split), `sweep.md` (49 findings after dedupe), `escalations.md` (ten hypotheses worth a deep dive), `deep.md` (the verdicts) and `REPORT.md` (the final ranked report). **52 confirmed findings: 4 high, 20 medium, 28 low.** The four high ones are fixed here, one commit each. The mediums and lows are not.
**H1 - a stalled write delayed connection teardown.** `sshSocket.Close` ran the blocking close before the unblocking one: closing an SSH channel writes under the transport lock, which a bulk image upload already holds, so on a host that vanished without RST the teardown waited for the kernel to stop retransmitting (~15 min). `link.markDead` compounded it by killing inside a `sync.Once` before signalling dead, so even cancelling the reconcile did not help. With `MaxConcurrentReconciles: 3`, three Proxies on one dead host stalled every Proxy on every provider. Verified against the pinned x/crypto rather than assumed.
**H2 - an error mid-upload wedged go-libvirt's stream sender.** A failing HTTP body deadlocked `StorageVolUpload` outright, leaking two goroutines and 4 MiB each time against a 128Mi limit. The defect is upstream - `requestStream` starts its sender on unbuffered channels and never drains `outErr` on the error path - so this is a workaround, not a repair: the reader can now only end the stream cleanly, and the transfer deadline moved 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 quoted.
**H3 - an unreadable address looked like a missing one.** Every error from both address sources read as "no address yet", so a pooled connection killed by an *unrelated* reconcile's timeout demoted a healthy proxy to Provisioning, cleared its IP, and made the health engine drop its accumulated counters. One source failing stays "no address yet"; every source failing, or a connection already dead, is Transient. **Half of this finding is deliberately still open**: a guest agent restarting inside a running VM still demotes it, because the discriminator is the previous IP, which the provider does not hold and the controller does. Follow-up recorded against `proxy_controller.go`.
**H4 - the base image cache was shared across provider entries.** kvm2 built byte-identical volume names to the `kvm` provider in the same pool, and the shipped config invited exactly that overlap. Two entries could share one file, a concurrent cold fetch could leave a guest booting from a half-written base, and a `sha256` edit deleted the base under live overlays. Names now carry the provider entry, and a digest mismatch is refused rather than resolved by deleting.
## Validation
Deployed to a kind cluster and run against a real hypervisor - the first time this code had touched libvirt rather than the in-memory fake.
| Check | Result |
|---|---|
| ProviderHealth | `Ready 1/1`, `Reachable`, 189 ms, libvirt 11.6.0 |
| Cold image fetch | 433,651,712 bytes in 8.3 s, digest verified |
| Volume naming (H4) | new per-entry base created *beside* the kvm provider's untouched one |
| Second Proxy | cache hit, no re-download |
| State reporting (H3) | `Running` steady across every poll, no flapping |
| Squid | `Healthy=True` on a real CONNECT probe |
| Delete | domain and per-VM volumes gone, base image survived |
| Pre-existing proxies | all four stayed Ready across two rollouts |
**What the run did not prove**, stated plainly because a green run is easy to over-read: H2's error paths need a body that fails part way through, and a healthy download never finishes short, so the unverified claim about libvirtd's response to a short stream finish is still open - a probe recipe is in the execution notes. `make test-integration-kvm2` has also still never been run.
## Also here
- klog and `net/http`'s own errors now join the structured log stream; the deployment logs JSON with rfc3339 timestamps, so `kubectl logs | jq` works. client-go's leader election was writing `I0917 ...` straight to stderr because nothing routed klog into logr.
- The `kvm2-home` provider entry ships enabled.
- `docs/kvm2-provider.md` (operator guide), `docs/kvm2-provider-spec.md` (the design), and the plan/execution pair under `docs/plans*/`.
`make test` green, `go vet` clean including `-tags=integration`. Every regression test was run against the unfixed code first and watched to fail; those commands are in the execution notes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
The kvm2 block reuses KVMConfig; validateKVM gains a block-name variant so
messages say kvm2. The package skeleton has a non-dialing New, strict
providerID parsing, the state table and a 34-row classify test.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every go-libvirt call runs under its own budget and a cancelled or overlong
call kills the connection; credentials are read on every dial; one span and
one log line per RPC come from a decorator applied where connections are
made.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Shorter start wait with immediate verdicts for dead or paused domains,
ListByTag without state or address RPCs, Timeout for an expired check
round, no adoption of unmarked domains, reference-counted keyed locks,
dead options removed, HEAD fallback on any non-2xx.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
kacerr
merged commit 5928c3fe75 into main2026-09-17 23:12:46 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
The clean-room kvm2 provider, plus a scoped code review of it and the four high findings that review produced. Validated in-cluster against a real hypervisor.
The provider
kvm2runs Managed proxies as VMs on libvirt/KVM hosts reached over SSH, with the libvirt socket forwarded through the connection. Built fromdocs/provider-contract.mdanddocs/kvm2-provider-spec.mdrather than from the first-generationkvmprovider's code. What it adds overkvm: a child span per libvirt RPC, a pool-space precheck that maps to the reconciler's quota wait, SSH credentials re-read on every dial, image downloads and health checks on connections of their own, an adoption guard on the owning UID, and SSH auth failures that stop retrying.The review, and what it found
review/carries the whole thing:scope.md(the file set and a seven-chunk split),sweep.md(49 findings after dedupe),escalations.md(ten hypotheses worth a deep dive),deep.md(the verdicts) andREPORT.md(the final ranked report). 52 confirmed findings: 4 high, 20 medium, 28 low. The four high ones are fixed here, one commit each. The mediums and lows are not.H1 - a stalled write delayed connection teardown.
sshSocket.Closeran the blocking close before the unblocking one: closing an SSH channel writes under the transport lock, which a bulk image upload already holds, so on a host that vanished without RST the teardown waited for the kernel to stop retransmitting (~15 min).link.markDeadcompounded it by killing inside async.Oncebefore signalling dead, so even cancelling the reconcile did not help. WithMaxConcurrentReconciles: 3, three Proxies on one dead host stalled every Proxy on every provider. Verified against the pinned x/crypto rather than assumed.H2 - an error mid-upload wedged go-libvirt's stream sender. A failing HTTP body deadlocked
StorageVolUploadoutright, leaking two goroutines and 4 MiB each time against a 128Mi limit. The defect is upstream -requestStreamstarts its sender on unbuffered channels and never drainsoutErron the error path - so this is a workaround, not a repair: the reader can now only end the stream cleanly, and the transfer deadline moved 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 quoted.H3 - an unreadable address looked like a missing one. Every error from both address sources read as "no address yet", so a pooled connection killed by an unrelated reconcile's timeout demoted a healthy proxy to Provisioning, cleared its IP, and made the health engine drop its accumulated counters. One source failing stays "no address yet"; every source failing, or a connection already dead, is Transient. Half of this finding is deliberately still open: a guest agent restarting inside a running VM still demotes it, because the discriminator is the previous IP, which the provider does not hold and the controller does. Follow-up recorded against
proxy_controller.go.H4 - the base image cache was shared across provider entries. kvm2 built byte-identical volume names to the
kvmprovider in the same pool, and the shipped config invited exactly that overlap. Two entries could share one file, a concurrent cold fetch could leave a guest booting from a half-written base, and asha256edit deleted the base under live overlays. Names now carry the provider entry, and a digest mismatch is refused rather than resolved by deleting.Validation
Deployed to a kind cluster and run against a real hypervisor - the first time this code had touched libvirt rather than the in-memory fake.
Ready 1/1,Reachable, 189 ms, libvirt 11.6.0Runningsteady across every poll, no flappingHealthy=Trueon a real CONNECT probeWhat the run did not prove, stated plainly because a green run is easy to over-read: H2's error paths need a body that fails part way through, and a healthy download never finishes short, so the unverified claim about libvirtd's response to a short stream finish is still open - a probe recipe is in the execution notes.
make test-integration-kvm2has also still never been run.Also here
net/http's own errors now join the structured log stream; the deployment logs JSON with rfc3339 timestamps, sokubectl logs | jqworks. client-go's leader election was writingI0917 ...straight to stderr because nothing routed klog into logr.kvm2-homeprovider entry ships enabled.docs/kvm2-provider.md(operator guide),docs/kvm2-provider-spec.md(the design), and the plan/execution pair underdocs/plans*/.make testgreen,go vetclean including-tags=integration. Every regression test was run against the unfixed code first and watched to fail; those commands are in the execution notes.🤖 Generated with Claude Code
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>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>