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

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

---

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.

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.

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.

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.

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:

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

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:

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

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:

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:

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

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