egress-proxies-operator/docs/plans/2026-09-17-1515-kvm2-review-fixes.md

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

• Step 0 — Save this plan into the repo, commit alone, before any code.
• 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+ — 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:

// 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:

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:

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.

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.

---

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

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>.