# Plan: fix the kvm2 code-review findings **Created:** 2026-09-17 15:15 ## Context The review of `feat/kvm2-provider` (`review/REPORT.md`) confirmed 52 findings: 4 high, 20 medium, 28 low. We fix them one at a time, highest severity first, each as its own step and its own commit. This plan is the campaign; the execution file's checklist is the authoritative progress record. Steps are detailed as we reach them, so the plan is never a fiction about work not yet designed. Precedent for this shape: `docs/plans/2026-09-10-2222-kvm-review-fixes.md`. ## Steps - [x] **Step 0** — Save this plan into the repo, commit alone, before any code. - [x] **Step 1** — H1: `sshSocket.Close` ran the blocking close before the unblocking one, and `markDead` killed inside a `sync.Once` before signalling dead. Shipped in `0239fb7`; the reasoning is in the execution file. - [ ] **Step 2** — H2: an error during `StorageVolUpload` wedges go-libvirt's stream sender. - [ ] **Step 3** — H3: `address()` swallows every error and demotes healthy proxies. - [ ] **Step 4** — H4: the base image cache is shared across provider entries. - [ ] **Step 5** — Validate H1-H4 in-cluster against the real hypervisor (detailed below). - [ ] **Step 6** — Route klog into the structured logger (found during Step 5; detailed below). - [ ] **Step 7+** — the mediums, from `review/REPORT.md`. Detailed when reached. --- ## Step 2 — H2: a source error must not reach go-libvirt's stream sender ### The bug `uploadImage` (`image.go:255-296`) hands a reader over `resp.Body` to `StorageVolUpload`. I verified the mechanism in the pinned `go-libvirt@v0.0.0-20260814190004`: ```go // rpc.go requestStream abort := make(chan bool) // UNBUFFERED outErr := make(chan error) // UNBUFFERED go func() { outErr <- l.socket.SendStream(serial, proc, program, out, abort) }() resp, err = l.processIncomingStream(c, in) if err != nil { abort <- true return resp, err // outErr is NEVER drained on this path } err = <-outErr ``` `SendStream` (`socket/socket.go`) holds a 4 MiB buffer, polls `abort` once per loop, and on a **non-EOF reader error** sends a `StatusError` packet and returns. It then blocks forever on `outErr <-`, because the main goroutine is still inside `processIncomingStream`. libvirtd answers the `StatusError` with an error, so `processIncomingStream` returns an error and the main goroutine blocks forever on `abort <- true`. **Deadlock: `StorageVolUpload` never returns.** kvm2's `call` rescues only the caller, after `budgets.transfer` = 20 minutes, and the socket kill does **not** unblock either goroutine — both are parked on channel sends, not on socket I/O, which is what the comment at `backend.go:152-156` gets wrong. So every failed upload leaks kvm2's call goroutine, go-libvirt's sender goroutine and 4 MiB, permanently. The pod limit is 128Mi and the reconciler retries on backoff. Three shapes, only the first of which is ours to fix: - **(A) reader error** — a truncated or failing HTTP body. The common case. - **(B) server-side error mid-upload** — ENOSPC, or the volume deleted underneath. After `SendStream` has returned there is nothing any reader, ctx or socket close can do: confirmed unfixable from our side. - **(C) our own abandonment** — ctx cancel or the 20 m budget kills the socket, which makes `processIncomingStream` error. Same tail as (B). ### The fix (workaround; the defect stays upstream) No fork, no `replace`. Two changes that between them close (A) and the realistic half of (C). **2a. Never hand go-libvirt a reader that errors** — `internal/provider/kvm2/backend.go`, at the seam rather than in `image.go`, so it covers the seed and marker uploads too and stays out of the file Step 4 rewrites: ```go type eofReader struct { r io.Reader mu sync.Mutex err error } func (e *eofReader) Read(b []byte) (int, error) { n, err := e.r.Read(b) if err == nil || err == io.EOF { return n, err } e.mu.Lock() if e.err == nil { e.err = err } e.mu.Unlock() return n, io.EOF } func (e *eofReader) sourceErr() error { e.mu.Lock(); defer e.mu.Unlock(); return e.err } ``` `libvirtBackend.StorageVolUpload` wraps `r` in one, and after a nil-error return converts a recorded `sourceErr()` into the returned error. `io.EOF` is the one outcome that ends the stream the normal way: libvirtd gets `StatusOK`, replies OK, `processIncomingStream` returns nil, the main goroutine reaches `err = <-outErr` and both goroutines unwind. `countingReader` is untouched — it sits under the wrapper, still sees the real error, so `uploadImage`'s `readErr != nil` branch and `TestEnsureImage_shortReadRemovesVolumeAndIsTransient` keep working verbatim. **Unverified, ~85%:** that libvirtd answers a short-but-clean stream finish with OK rather than an error. `virFDStreamWrite` enforces `length` only on writes *past* it and nothing compares `offset` to `length` on the close path, which is also why `virsh vol-upload` of a small file into a large volume works. If it answers with an error instead, we land back in a deadlock by a different door — but that is exactly today's behaviour, so the change is non-worsening either way. Probe recipe goes in the commit message; it needs a real libvirtd and a write, so it is for the user to run, not an agent. **2b. Make the transfer deadline land on the HTTP source, not on the libvirt socket.** `p.do` already builds the request with `http.NewRequestWithContext` (`image.go:360`), so the body is ctx-bound, and `http.DefaultClient` has no timeout of its own by deliberate choice (`kvm2.go:100`). Give the image GET in `fetchImage` a deadline of `defaultBudgets.transfer` minus a grace of one minute (precedent for reaching for `defaultBudgets` directly: `discardVolume`, `image.go:331`). A stalled mirror then ends by the *body* failing, which 2a turns into `io.EOF` and a clean unwind, roughly a minute before the libvirt budget would have killed the socket and leaked. That converts the realistic repeated (C) — a slow or stalling mirror on a 415 MB image — from a permanent leak into an ordinary Transient error. The socket kill stays as the backstop for libvirtd itself hanging, which is the residual we cannot close. **2c. Close the dedicated connection before unlinking the file.** Move the `discardVolume` call out of `uploadImage` (`image.go:291-294`) up into `fetchImage`'s upload-error branch, where the deferred `fresh.Close()` has already run, so libvirtd has let go of the stream's fd before the volume is deleted. `uploadImage` then drops its `be` parameter and its private logger — a net deletion. Doing it this way rather than adding an explicit `Close` keeps `image_test.go:573`'s `closeCount() == 1` assertion true. ### Tests, docs, scope One test: `TestEOFReader_sourceErrorNeverReachesTheStream` in `backend_test.go` — a table with "error after data", "error with no data", "plain io.EOF passes through", "first error wins". ~35 lines. **Deliberately not tested:** the abort protocol end to end. `libvirttest` reads a fixed 28-byte header per packet and implements no stream procedures, so pinning this hermetically would mean writing a libvirt RPC server stub — 150-250 lines whose only assertion is about *upstream's* goroutines. That is the bloat we cut for H1. Docs: a paragraph in `docs/kvm2-provider.md`'s *The image cache* section saying a truncated response is reported out of band and why, and a *Known gaps* bullet naming shapes (B) and (C) as an upstream defect with the two-character patch quoted, so a future reader knows it is not ours and what the escape hatch is: ```go abort := make(chan bool, 1) outErr := make(chan error, 1) ``` File that as an upstream issue/PR against `digitalocean/go-libvirt` and link it from the bullet. --- ## Step 3 — H3: an unreadable address must not look like a missing one ### The bug `address()` (`get.go:104-119`) tries the guest agent then the host's ARP table. Any error that is not `context.Canceled`/`DeadlineExceeded` is logged at V(2) and skipped; after both it returns `("", nil)`. `mapState(active, "")` is `StateProvisioning` (`state.go:20-21`), the controller clears `p.Status.IP` on Provisioning (`proxy_controller.go:208-211`), and the health engine prunes the proxy's accumulated state and calls `Metrics.ForgetProxy` in one tick once `EffectiveHost()` is empty. The trigger that matters: another reconcile's cancel or budget overrun kills the **shared** pooled connection, and `call` then returns `transientf("... connection is closed: ...")` with no ctx sentinel, which `get.go:108` does not recognise. `Get` returns a *successful* Instance in Provisioning with `IP: ""`, and a healthy proxy is pulled out of rotation by an unrelated reconcile's timeout. ### The fix A booting VM and a dead link are distinguishable: on beelink the agent source errors while the ARP source returns success-with-no-addresses (`docs/kvm2-provider-spec.md:146` — the host never sees the guest's traffic). So *one* source failing is normal; *every* source failing, or a connection already gone, is not. ```go func address(ctx context.Context, log logr.Logger, be backend, dom libvirt.Domain) (string, error) { errs := make([]error, 0, len(addressSources)) for _, s := range addressSources { ifaces, err := be.DomainInterfaceAddresses(ctx, dom, uint32(s.src), 0) if err != nil { if isDead(be) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return "", fmt.Errorf("reading addresses of domain %s from the %s source: %w", dom.Name, s.name, err) } log.V(2).Info("no addresses from source", "source", s.name, "error", err.Error()) errs = append(errs, fmt.Errorf("%s source: %w", s.name, err)) continue } if ip := firstIPv4(ifaces); ip != "" { return ip, nil } } if len(errs) == len(addressSources) { return "", fmt.Errorf("%w: reading addresses of domain %s: every source failed: %w", provider.ErrTransient, dom.Name, errors.Join(errs...)) } return "", nil } ``` The explicit `provider.ErrTransient` wrap is load-bearing, not decoration: without it `classify` picks the first libvirt code out of the join tree, and `ErrInvalidArg`/`ErrAccessDenied`/ `ErrNoSupport` map to **Permanent** — an address read failing would latch a *running* proxy as Failed, a worse version of the harm being fixed. `isDead(be)` (`backend.go:50-61`, already works through the `observed` decorator and the test fake) is not strictly needed — a dead link fails both sources anyway — but it fails faster, attributes better, and is the same predicate M15 will need for `ListByTag`. Also fix the now-wrong comment at `get.go:100-103` ("only ctx ending is an error"). **What this does not fix, on purpose.** A guest agent restarting inside a running VM still demotes it: the agent errors, ARP returns empty, and the provider cannot tell that from a VM that is still booting. The discriminator is the *previous* IP, which the provider deliberately does not hold (`TestGet_readsUIDAndCreatedAtFromBackend` pins "read back, not remembered") and the controller already has. File as a follow-up finding against `proxy_controller.go:208-211`: on Provisioning, keep a Proxy's existing IP and `Provisioned` condition for N polls before clearing, or give the health engine a grace period before pruning. The one kvm2-local alternative — treating "active, no address, domain older than a provisioning window" as an error using the mark's `createdAt` — is a design change wearing a bug fix's clothes, and it invents a provisioning deadline that belongs to the controller. Rejected. ### Tests - `TestGet_arpFallbackWhenAgentFails` (`get_test.go:135`) passes unchanged — one source of two. - `TestGet_addressErrorIsProvisioning` (`get_test.go:159`) must be **re-pointed**. It fails both sources with `ErrAgentUnresponsive`, which is not realistic: code 86 is agent-specific and the ARP source never produces it. Rewrite as `TestGet_agentFailsAndARPEmptyIsProvisioning` — agent errors, ARP returns no addresses and no error — which is the beelink boot and the behaviour the new rule must not break. Nothing covers it today. - One new test, `TestGet_everyAddressSourceFailingIsTransient`, two subtests: both sources error; and the pooled link is dead before the address read (the fake implements `liveness`). Assert Transient, not NotFound, not Permanent. Docs: `docs/kvm2-provider-spec.md:146-151` says "errors from either source mean 'no address yet'", now false. The `ErrTransient` row of the error-class table in `docs/kvm2-provider.md`. --- ## Step 4 — H4: the base image cache must not be shared across provider entries ### The bug `baseVolumeName` (`volume.go:17`) is `base-.`, 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--.`, 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..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-.`. - `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-.`", now wrong for kvm2), and the kvm2 block's comment in both `providers-config.yaml` files. Leave `docs/kvm-provider.md` alone; kvm is unchanged. --- ## Step 5 — Validate in-cluster against beelink ### Why, and what this run can and cannot prove The kvm2 provider has **never run against real libvirt**: Step 11 is unchecked in `docs/plans-executions/2026-09-16-1333-kvm2-provider.md`, every `base-*` volume on beelink predates it, and both live `proxy-*` domains carry the kvm v1 metadata namespace. Four fixes have landed on code no hypervisor has executed. This run is first contact, and it is also the gate the campaign's `CHANGELOG` entry waits on. It proves the happy paths: the dial and health probe, a real 415 MB image download through the reader H2 now wraps, H4's new volume name appearing beside kvm's untouched one, a second Proxy hitting the cache, H3's address path reaching Running with an IP, and Squid answering a real CONNECT probe. It does **not** prove H2's error paths. The unverified ~85% claim — that libvirtd answers a short stream finish with OK — only arises when a body *fails*, and a healthy download never finishes short. The 19-minute HTTP deadline will not fire either. A green run means "the wrapper did not break normal uploads", not "the deadlock is settled". That still needs the scratch-pool probe recorded in the Step 2 execution entry. ### The cluster this runs in, and the one hazard `kind-proxy-operator-demo` is **not a throwaway**. It runs the operator that currently owns four Ready proxies, two of them real VMs on beelink: ``` proxy-beelink-1 Managed kvm-home Ready 192.168.0.166 True proxy-beelink-2 Managed kvm-home Ready 192.168.0.94 True proxy-kubernetes-demo-1/2 Managed kubernetes Ready 10.244.0.7/.8 True ``` Validating kvm2 means rolling the operator that owns them. That is low risk — the controller and the whole `kvm` provider are untouched by H1-H4 — and rollback is one `kubectl set image`. **Do not run a second operator against beelink with `kvm-home` in its config.** Orphan GC (`internal/gc/gc.go:86-131`) lists every Proxy in *its own* cluster, and deletes any instance whose UID is not in that set and is older than `--gc-min-age` (10 m, swept every 10 m). A second cluster's live set would not contain `proxy-beelink-1/2`'s UIDs, so its GC would destroy them within ten minutes. Using the existing cluster removes the hazard entirely: its live set is the one that protects them. ### Steps **0. Baseline** — record what is there before touching anything, so "did we break it" is answerable: `kubectl get px -A`, `kubectl get ph`, and a read-only `virsh vol-list default` plus `virsh list --all` on beelink. **1. Build and load, under a new tag.** ```bash make docker-build IMG=egress-proxies-operator:kvm2-dev kind load docker-image egress-proxies-operator:kvm2-dev --name proxy-operator-demo ``` The tag **must** differ from the running `kvm-dev`. `make deploy` sets the image through kustomize; an identical tag leaves the Deployment spec unchanged, so no rollout happens, so the new process never starts, so the new providers config is never read. The ConfigMap carries no generator hash, so nothing else forces a restart either. The working tree is dirty (`review/`, `.claude/`), so `GIT_COMMIT` bakes in as `c10f98a-dirty` and `--version` will say so. Cosmetic; commit or stash `review/` first to avoid it. **2. Enable kvm2 in the deployed config.** Uncomment the `kvm2-home` block in `config/manager/providers_config.yaml` (it is the shipped default, commented out). Keep its `address: beelink.tail8c69c.ts.net:22` — the Tailscale FQDN, which is what `~/.ssh/kvm_known_hosts` pins. `config/samples/providers-config.yaml` uses the short `beelink:22`, which **fails host-key verification**; I hit exactly that when validating SSH earlier in this session. **3. Deploy.** ```bash make deploy IMG=egress-proxies-operator:kvm2-dev kubectl -n egress-proxies-operator-system rollout status deploy/egress-proxies-operator-controller-manager ``` `make deploy` rewrites `config/manager/kustomization.yaml`'s image block as a side effect; it already carries `kvm-dev` from a previous run. **4. STOP GATE — provider health, before anything is created.** ```bash kubectl get ph kubectl get ph kvm2-home -o yaml ``` `kvm2-home` must be `Ready 1/1` with reason `Reachable` and details carrying `hostname`, `libvirtVersion`, `poolState: running` and `poolAvailableGiB`. This proves SSH, the host key, the libvirt handshake and the pool **without creating a thing**, and it exercises the dedicated-connection dial and close that H1 touched. A failure here is free to diagnose: `CredentialsUnavailable`, `Unauthorized`, `Forbidden`, `PoolNotActive` each name their own cause. Report and wait for a go before step 5. **5. Two Proxies.** Two manifests in the scratchpad (not committed), modelled on `config/samples/proxy_kvm2.yaml` — `proxy-kvm2-1` and `proxy-kvm2-2` in `default`, matching the existing `proxy-beelink-*` convention, `provider: kvm2-home`, `zone: beelink`, `machineType: small`, `image: debian-13`, port 3128 and the sample's Squid cloud-init. Apply the first, let it finish, then the second — sequencing them is what makes the cache hit observable. ```bash kubectl get px -w kubectl -n egress-proxies-operator-system logs -f deploy/egress-proxies-operator-controller-manager ``` **6. What to check, and which finding it exercises.** | Check | Finding | |---|---| | `fetching base image into host pool` then `base image cached` with size, duration, digest | H2's wrapped reader on a real 415 MB upload | | `virsh vol-list default` shows `base-kvm2-home-debian-13.qcow2` **and** `base-debian-13.qcow2` still there | H4: per-entry naming, kvm's cache untouched | | Second Proxy logs no `fetching`, and the server is not hit again | H4 + the image lock: cache hit under the new name | | `px` goes Provisioning → Running with an IP, without flapping back to Provisioning | H3: the address path, and no spurious demotion | | `Healthy=True` | Squid answers a real CONNECT probe end to end | | `virsh list --all` shows two new `proxy-*` domains | Create | | After deleting both: domains and their `.qcow2`/`-seed.iso` gone, base image still present | Delete, and the cache surviving | Optionally `curl -x http://:3128 https://example.com -I` from this box, if it routes to 192.168.0.0/24 — the existing kvm proxies live on that LAN. **7. Cleanup.** `kubectl delete px proxy-kvm2-1 proxy-kvm2-2` and let the finalizer do the work, then verify on beelink read-only. **Any leftover domain or volume: I print the exact `virsh undefine --nvram` / `vol-delete` lines and you run them** — no write reaches the shared host from me. Then decide whether the `kvm2-home` ConfigMap entry stays (keep it if you want kvm2 running for real; `git checkout config/manager/providers_config.yaml` otherwise). **Rollback at any point:** ```bash kubectl -n egress-proxies-operator-system set image \ deploy/egress-proxies-operator-controller-manager manager=egress-proxies-operator:kvm-dev ``` ### Who runs what I drive steps 0-6 and stop at gate 4 for a go. Every beelink write is yours. Nothing here touches `proxy-beelink-1/2`, `base-debian-13.qcow2`, your unrelated VMs, or the five pre-existing orphan volumes the review found. --- ## Step 6 — Route klog into the structured logger **Not a review finding.** Found while watching the Step 5 validation: some lines ignore the zap encoder entirely, so no amount of `--zap-encoder=json` makes the stream parseable. ``` I0917 19:50:02.580097 1 leaderelection.go:258] "Attempting to acquire leader lease..." lock="..." ``` That is klog's own text sink. client-go's leader elector does `logger := klog.FromContext(ctx)` (`leaderelection.go:257`), and controller-runtime hands the elector a context with no logr in it (`pkg/manager/internal.go:461-473` — `mgr.Start` gets `ctrl.SetupSignalHandler()`, which carries no logger). `klog.FromContext` falls back to `Background()`, and with no logger registered that is klog's stderr writer. Nothing in this repo imports or configures klog; it is an indirect dependency (`k8s.io/klog/v2 v2.140.0`) that ~60 linked packages use, including `client-go/tools/{leaderelection,cache,record}`, `client-go/transport` and `client-go/rest`. controller-runtime does not route it either — its only `klog.SetLogger` call is in its own test suite. ### 6a. `cmd/main.go` — one line and an import ```go logger := zap.New(zap.UseFlagOptions(&opts)) ctrl.SetLogger(logger) klog.SetLogger(logger) ``` **Plain `SetLogger`, deliberately not `SetLoggerWithOptions(logger, klog.ContextualLogger(true))`.** The contextual form is what klog's own docs recommend, because it skips a klogr round trip — but it also disables klog's verbosity gate ("Such a logger cannot rely on verbosity checking in klog", `contextual.go:83-86`), handing the decision to zap's level. With `Development: true` that level is Debug, which enables V(1), so client-go's V(1) chatter that klog currently suppresses at its default `-v=0` would start appearing. Plain `SetLogger` keeps klog's own check in front ("klog will do its own verbosity checks before calling logger.V().Info", `contextual.go:43-44`) and still redirects everything it does emit ("all log lines will be suppressed from the regular output, and redirected to the logr implementation", `:47-48`). Same fix, no new volume. Performance is irrelevant at this scale. `go.mod` promotes `k8s.io/klog/v2` from indirect to direct. It is already in the module graph, so nothing new is downloaded. ### 6b. `internal/discovery/server.go` — the other stderr bypass The discovery API's `http.Server` (`:113-119`) sets no `ErrorLog`, so `net/http`'s own errors — `Accept` failures, handler panics that escape — go to `log.Default()` and out to stderr in stdlib format. `s.log` is already in scope two lines above: ```go srv := &http.Server{ Handler: s.handler(), ErrorLog: slog.NewLogLogger(logr.ToSlogHandler(s.log), slog.LevelError), ReadHeaderTimeout: readHeadTimeout, ... } ``` `logr.ToSlogHandler` is the pattern the repo already uses for the same purpose in `internal/provider/gcp/wirelog.go:31-36`. This server is plain HTTP with no TLS, so `ErrorLog` carries genuine faults rather than handshake noise from scanners — Error level is right. ### 6c. `config/manager/manager.yaml` — make the deployed stream parseable ```yaml args: - --leader-elect - --health-probe-bind-address=:8081 - --providers-config=/etc/proxy-operator/providers.yaml - --zap-encoder=json - --zap-time-encoding=rfc3339 ``` Verified that the flag wins over the in-code default: `addDefaults` only fills `NewEncoder` `if o.NewEncoder == nil` (`zap.go:176-191`), and `BindFlags` sets it from `--zap-encoder`. `--zap-time-encoding=rfc3339` matters because the JSON encoder defaults to `epoch`, which would give `1758138422.31` instead of a timestamp anyone can read. Verbosity is left at debug, so the per-RPC lines stay — that was the choice. ### Deliberately not doing `opts := zap.Options{Development: true}` (`cmd/main.go:154-155`) stays. Flipping it to a production default would change the format for every existing deployment, not just this one, and the flags in 6c get the same result here without that blast radius. ### What stays unformatted regardless, and why that is fine Flag-parse errors and `--version` (`cmd/main.go:160-163`) print before any logger exists, and `utilruntime.Must` panics in `init()` run before `main`. controller-runtime's own metrics and health servers also set no `ErrorLog` (`pkg/internal/httpserver/server.go:11-19`) and expose no option to set one. All three are outside reach; none can fire during steady-state operation. ### Tests None. All three changes are wiring: two struct fields and one manifest key. `cmd/main.go` has no test coverage at all, and pinning `klog.SetLogger` hermetically means mutating global state that would leak into every other test in the package. Verified by observation instead — see below. Same call as H1's: a test that costs more than it protects does not earn its place. ### Verification Rebuild, redeploy, and watch the rollout produce the line that started this: ```bash kubectl -n egress-proxies-operator-system logs deploy/egress-proxies-operator-controller-manager \ | grep -i 'leader' | head -3 ``` It must be JSON with an rfc3339 `ts`, not `I0917 ...`. Then confirm the whole stream parses: ```bash kubectl -n egress-proxies-operator-system logs deploy/egress-proxies-operator-controller-manager \ | jq -e -s 'length > 0' >/dev/null && echo "every line is JSON" ``` That second command is the real check — it fails if *any* line is not JSON, which is exactly the property being fixed. --- ## Ordering **H2 → H3 → H4**, three commits. H3 is orthogonal: it touches only `get.go` and `get_test.go`. H2 and H4 both touch `image.go` but at different heights, and the order matters. H2's `image.go` edit is a deletion — `discardVolume` moves out of `uploadImage` and up into `fetchImage` — while H4 edits `imageSource.volumeName()`, `resolveImage`, and `cachedImage`'s digest branch ten lines below. Landing H2 first means H4 rebases cleanly; the reverse means H4 is written against a failure path H2 is about to restructure. Keeping H2's guard in `backend.go` rather than in `countingReader` is part of the same separation. Semantically, H2 makes `fetchImage` delete the base after the transfer connection is closed, and that delete is only unambiguously safe once names are per-entry — so H2's commit message should say the discard is still unfenced against a second entry sharing the name and that H4 closes it, and H4's should point back. The deferred atomic-publish work then lands exactly where H2 put the discard, on a volume this call created under a name no other entry can produce. ## Verification ```bash go build ./... && go vet ./... go test -race -count=1 ./internal/provider/kvm2/ make test ``` Per step, the check that actually matters is that the new test fails against the unfixed code — stash the source edit, keep the test, watch it fail, restore. For H2 that means asserting `eofReader` converts before the fix exists; for H3, that the dead-link subtest returns a successful Provisioning instance today; for H4, that two entries produce one name today. After H4, run the kvm2 integration test on beelink. **It has never run** — Step 11 is unchecked in `docs/plans-executions/2026-09-16-1333-kvm2-provider.md`, every `base-*` on the host predates kvm2, and both live domains carry the kvm v1 metadata namespace. It is the first end-to-end exercise of the new name and does a real cold download beside kvm's untouched base. Each step: its own commit on `feat/kvm2-provider` with the execution-file entry and its checklist line ticked. `CHANGELOG.md` lands with a "validate and record" step once the user confirms the work, as in the kvm campaign. Trailer: `Co-Authored-By: Claude Opus 5 (1M context) `.