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

- [x] **Step 0** — Save this plan into the repo, commit alone, before any code.
- [x] **Step 1** — H1: `sshSocket.Close` ran the blocking close before the unblocking one, and
  `markDead` killed inside a `sync.Once` before signalling dead. Shipped in `0239fb7`; the
  reasoning is in the execution file.
- [ ] **Step 2** — H2: an error during `StorageVolUpload` wedges go-libvirt's stream sender.
- [ ] **Step 3** — H3: `address()` swallows every error and demotes healthy proxies.
- [ ] **Step 4** — H4: the base image cache is shared across provider entries.
- [ ] **Step 5** — Validate H1-H4 in-cluster against the real hypervisor (detailed below).
- [ ] **Step 6** — Route klog into the structured logger (found during Step 5; detailed below).
- [ ] **Step 7+** — the mediums, from `review/REPORT.md`. Detailed when reached.

---

## Step 2 — H2: a source error must not reach go-libvirt's stream sender

### The bug

`uploadImage` (`image.go:255-296`) hands a reader over `resp.Body` to `StorageVolUpload`. I
verified the mechanism in the pinned `go-libvirt@v0.0.0-20260814190004`:

```go
// rpc.go requestStream
abort  := make(chan bool)    // UNBUFFERED
outErr := make(chan error)   // UNBUFFERED
go func() { outErr <- l.socket.SendStream(serial, proc, program, out, abort) }()
resp, err = l.processIncomingStream(c, in)
if err != nil {
    abort <- true
    return resp, err          // outErr is NEVER drained on this path
}
err = <-outErr
```

`SendStream` (`socket/socket.go`) holds a 4 MiB buffer, polls `abort` once per loop, and on a
**non-EOF reader error** sends a `StatusError` packet and returns. It then blocks forever on
`outErr <-`, because the main goroutine is still inside `processIncomingStream`. libvirtd
answers the `StatusError` with an error, so `processIncomingStream` returns an error and the
main goroutine blocks forever on `abort <- true`. **Deadlock: `StorageVolUpload` never returns.**

kvm2's `call` rescues only the caller, after `budgets.transfer` = 20 minutes, and the socket
kill does **not** unblock either goroutine — both are parked on channel sends, not on socket
I/O, which is what the comment at `backend.go:152-156` gets wrong. So every failed upload leaks
kvm2's call goroutine, go-libvirt's sender goroutine and 4 MiB, permanently. The pod limit is
128Mi and the reconciler retries on backoff.

Three shapes, only the first of which is ours to fix:
- **(A) reader error** — a truncated or failing HTTP body. The common case.
- **(B) server-side error mid-upload** — ENOSPC, or the volume deleted underneath. After
  `SendStream` has returned there is nothing any reader, ctx or socket close can do: confirmed
  unfixable from our side.
- **(C) our own abandonment** — ctx cancel or the 20 m budget kills the socket, which makes
  `processIncomingStream` error. Same tail as (B).

### The fix (workaround; the defect stays upstream)

No fork, no `replace`. Two changes that between them close (A) and the realistic half of (C).

**2a. Never hand go-libvirt a reader that errors** — `internal/provider/kvm2/backend.go`, at the
seam rather than in `image.go`, so it covers the seed and marker uploads too and stays out of
the file Step 4 rewrites:

```go
type eofReader struct {
	r   io.Reader
	mu  sync.Mutex
	err error
}

func (e *eofReader) Read(b []byte) (int, error) {
	n, err := e.r.Read(b)
	if err == nil || err == io.EOF {
		return n, err
	}
	e.mu.Lock()
	if e.err == nil {
		e.err = err
	}
	e.mu.Unlock()
	return n, io.EOF
}

func (e *eofReader) sourceErr() error { e.mu.Lock(); defer e.mu.Unlock(); return e.err }
```

`libvirtBackend.StorageVolUpload` wraps `r` in one, and after a nil-error return converts a
recorded `sourceErr()` into the returned error. `io.EOF` is the one outcome that ends the stream
the normal way: libvirtd gets `StatusOK`, replies OK, `processIncomingStream` returns nil, the
main goroutine reaches `err = <-outErr` and both goroutines unwind.

`countingReader` is untouched — it sits under the wrapper, still sees the real error, so
`uploadImage`'s `readErr != nil` branch and `TestEnsureImage_shortReadRemovesVolumeAndIsTransient`
keep working verbatim.

**Unverified, ~85%:** that libvirtd answers a short-but-clean stream finish with OK rather than
an error. `virFDStreamWrite` enforces `length` only on writes *past* it and nothing compares
`offset` to `length` on the close path, which is also why `virsh vol-upload` of a small file
into a large volume works. If it answers with an error instead, we land back in a deadlock by a
different door — but that is exactly today's behaviour, so the change is non-worsening either
way. Probe recipe goes in the commit message; it needs a real libvirtd and a write, so it is
for the user to run, not an agent.

**2b. Make the transfer deadline land on the HTTP source, not on the libvirt socket.**
`p.do` already builds the request with `http.NewRequestWithContext` (`image.go:360`), so the
body is ctx-bound, and `http.DefaultClient` has no timeout of its own by deliberate choice
(`kvm2.go:100`). Give the image GET in `fetchImage` a deadline of `defaultBudgets.transfer`
minus a grace of one minute (precedent for reaching for `defaultBudgets` directly:
`discardVolume`, `image.go:331`).

A stalled mirror then ends by the *body* failing, which 2a turns into `io.EOF` and a clean
unwind, roughly a minute before the libvirt budget would have killed the socket and leaked.
That converts the realistic repeated (C) — a slow or stalling mirror on a 415 MB image — from a
permanent leak into an ordinary Transient error. The socket kill stays as the backstop for
libvirtd itself hanging, which is the residual we cannot close.

**2c. Close the dedicated connection before unlinking the file.** Move the `discardVolume` call
out of `uploadImage` (`image.go:291-294`) up into `fetchImage`'s upload-error branch, where the
deferred `fresh.Close()` has already run, so libvirtd has let go of the stream's fd before the
volume is deleted. `uploadImage` then drops its `be` parameter and its private logger — a net
deletion. Doing it this way rather than adding an explicit `Close` keeps
`image_test.go:573`'s `closeCount() == 1` assertion true.

### Tests, docs, scope

One test: `TestEOFReader_sourceErrorNeverReachesTheStream` in `backend_test.go` — a table with
"error after data", "error with no data", "plain io.EOF passes through", "first error wins".
~35 lines.

**Deliberately not tested:** the abort protocol end to end. `libvirttest` reads a fixed 28-byte
header per packet and implements no stream procedures, so pinning this hermetically would mean
writing a libvirt RPC server stub — 150-250 lines whose only assertion is about *upstream's*
goroutines. That is the bloat we cut for H1.

Docs: a paragraph in `docs/kvm2-provider.md`'s *The image cache* section saying a truncated
response is reported out of band and why, and a *Known gaps* bullet naming shapes (B) and (C)
as an upstream defect with the two-character patch quoted, so a future reader knows it is not
ours and what the escape hatch is:

```go
abort  := make(chan bool, 1)
outErr := make(chan error, 1)
```

File that as an upstream issue/PR against `digitalocean/go-libvirt` and link it from the bullet.

---

## Step 3 — H3: an unreadable address must not look like a missing one

### The bug

`address()` (`get.go:104-119`) tries the guest agent then the host's ARP table. Any error that
is not `context.Canceled`/`DeadlineExceeded` is logged at V(2) and skipped; after both it
returns `("", nil)`. `mapState(active, "")` is `StateProvisioning` (`state.go:20-21`), the
controller clears `p.Status.IP` on Provisioning (`proxy_controller.go:208-211`), and the health
engine prunes the proxy's accumulated state and calls `Metrics.ForgetProxy` in one tick once
`EffectiveHost()` is empty.

The trigger that matters: another reconcile's cancel or budget overrun kills the **shared**
pooled connection, and `call` then returns `transientf("... connection is closed: ...")` with no
ctx sentinel, which `get.go:108` does not recognise. `Get` returns a *successful* Instance in
Provisioning with `IP: ""`, and a healthy proxy is pulled out of rotation by an unrelated
reconcile's timeout.

### The fix

A booting VM and a dead link are distinguishable: on beelink the agent source errors while the
ARP source returns success-with-no-addresses (`docs/kvm2-provider-spec.md:146` — the host never
sees the guest's traffic). So *one* source failing is normal; *every* source failing, or a
connection already gone, is not.

```go
func address(ctx context.Context, log logr.Logger, be backend, dom libvirt.Domain) (string, error) {
	errs := make([]error, 0, len(addressSources))
	for _, s := range addressSources {
		ifaces, err := be.DomainInterfaceAddresses(ctx, dom, uint32(s.src), 0)
		if err != nil {
			if isDead(be) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
				return "", fmt.Errorf("reading addresses of domain %s from the %s source: %w", dom.Name, s.name, err)
			}
			log.V(2).Info("no addresses from source", "source", s.name, "error", err.Error())
			errs = append(errs, fmt.Errorf("%s source: %w", s.name, err))
			continue
		}
		if ip := firstIPv4(ifaces); ip != "" {
			return ip, nil
		}
	}
	if len(errs) == len(addressSources) {
		return "", fmt.Errorf("%w: reading addresses of domain %s: every source failed: %w",
			provider.ErrTransient, dom.Name, errors.Join(errs...))
	}
	return "", nil
}
```

The explicit `provider.ErrTransient` wrap is load-bearing, not decoration: without it `classify`
picks the first libvirt code out of the join tree, and `ErrInvalidArg`/`ErrAccessDenied`/
`ErrNoSupport` map to **Permanent** — an address read failing would latch a *running* proxy as
Failed, a worse version of the harm being fixed. `isDead(be)` (`backend.go:50-61`, already
works through the `observed` decorator and the test fake) is not strictly needed — a dead link
fails both sources anyway — but it fails faster, attributes better, and is the same predicate
M15 will need for `ListByTag`.

Also fix the now-wrong comment at `get.go:100-103` ("only ctx ending is an error").

**What this does not fix, on purpose.** A guest agent restarting inside a running VM still
demotes it: the agent errors, ARP returns empty, and the provider cannot tell that from a VM
that is still booting. The discriminator is the *previous* IP, which the provider deliberately
does not hold (`TestGet_readsUIDAndCreatedAtFromBackend` pins "read back, not remembered") and
the controller already has. File as a follow-up finding against
`proxy_controller.go:208-211`: on Provisioning, keep a Proxy's existing IP and `Provisioned`
condition for N polls before clearing, or give the health engine a grace period before pruning.
The one kvm2-local alternative — treating "active, no address, domain older than a provisioning
window" as an error using the mark's `createdAt` — is a design change wearing a bug fix's
clothes, and it invents a provisioning deadline that belongs to the controller. Rejected.

### Tests

- `TestGet_arpFallbackWhenAgentFails` (`get_test.go:135`) passes unchanged — one source of two.
- `TestGet_addressErrorIsProvisioning` (`get_test.go:159`) must be **re-pointed**. It fails both
  sources with `ErrAgentUnresponsive`, which is not realistic: code 86 is agent-specific and the
  ARP source never produces it. Rewrite as `TestGet_agentFailsAndARPEmptyIsProvisioning` — agent
  errors, ARP returns no addresses and no error — which is the beelink boot and the behaviour
  the new rule must not break. Nothing covers it today.
- One new test, `TestGet_everyAddressSourceFailingIsTransient`, two subtests: both sources error;
  and the pooled link is dead before the address read (the fake implements `liveness`). Assert
  Transient, not NotFound, not Permanent.

Docs: `docs/kvm2-provider-spec.md:146-151` says "errors from either source mean 'no address yet'",
now false. The `ErrTransient` row of the error-class table in `docs/kvm2-provider.md`.

---

## Step 4 — H4: the base image cache must not be shared across provider entries

### The bug

`baseVolumeName` (`volume.go:17`) is `base-<key>.<format>`, with a `.sha256` marker beside it.
`internal/provider/kvm/image.go:44-56` builds **byte-identical** names in the same pool — for
URL images too (kvm takes `hex(sum[:8])`, kvm2 takes `hex(sum[:])[:16]`, the same 8 bytes) —
with a completely separate lock table. The shipped config has a live `kvm-home` entry and a
commented-out `kvm2-home` entry on the same host, the same key `debian-13` and the same default
pool, and the comment tells the operator to enable it. Both integration tests use key
`test-image`. On beelink both live overlays back onto `base-debian-13.qcow2`, which kvm created.

Consequences, given `cachedImage` accepts *any* 64-hex marker when no `sha256` is configured and
never re-hashes the base:

- Two entries, one key, different URLs: whichever fetched first owns the file and the other
  silently boots the wrong image.
- A concurrent cold fetch: B finds A's base-without-marker, deletes it, recreates and uploads
  its own; A finishes into the unlinked inode, writes the marker, and `StorageVolGetPath`
  resolves A's handle to B's partial file — a guest boots from a half-written base.
- A `sha256` edit deletes the base in place under live overlays (`cachedImage` branch E), and
  libvirt does not refuse to delete a volume that running domains use as a backing store.

### The fix — the provider entry's name in the volume name

`base-<entry>-<key>.<format>`, e.g. `base-kvm2-home-debian-13.qcow2`, with the marker following
automatically. Every production call site already goes through `imageSource.volumeName()`
(`image.go:49`), so this is a small change: give `resolveImage` the provider name and add an
`Entry` field to `imageSource`.

`Provider.name` is already carried (`kvm2.go:104`). But provider names are validated only as
non-empty and unique (`config.go:96-105`) — no charset rule — and a name with a `/` would
produce an unusable volume name, so **`New` must reject an entry name that is not a safe volume
name component**, using the existing `kvmIdentifier` shape. Fail fast at startup, in kvm2 only:
tightening the rule for every provider type is a broader change than this fix.

**What this buys, and what it does not.** It fixes the cross-entry sharing outright — kvm↔kvm2
and kvm2↔kvm2 — which is the wrong-image bug and the concurrent-fetch corruption, and it makes
the existing per-`Provider` `imageLocks` correct rather than a latent bug, since no two entries
can now contend for one name. It does **not** make the name mean the content, so two things
stay open and must be said plainly rather than quietly skipped:

- Editing `images.<key>.url` without changing the key is still a silent no-op. Already
  documented behaviour ("add a new key instead"); left as is.
- Cross-*process* fencing still depends on the atomic-publish work (upload to a temp name,
  verify, then publish). E4 refuted the rollout and leader-handover triggers for the shipped
  manifests, so this is a narrow residual. Its own later commit.

**One companion fix, in this commit**, because the naming alone does not stop it and it is the
remaining path that unlinks a good file under a running domain: `cachedImage` branch E
(`image.go:129-130`, digest differs from the configured `sha256`) currently deletes the marker
and the base. Make it return Permanent instead — "this key is cached with digest X but Y is
configured; add the image under a new key, or delete the Proxies using it and evict the base."
That enforces the rule the contract already states instead of contradicting it. Keep the delete
for the no-marker and non-hex-marker branches, where the base is genuine debris.

### Migration

Nothing to do. The existing `base-debian-13.qcow2` on beelink is kvm's, kvm keeps using it
untouched, and kvm2 downloads its own copy once per host (~415 MB) when `kvm2-home` is enabled.
No window in which anything is replaced under a running guest. The pool needs room for both.

### Tests and docs

Tests are mostly literals becoming helpers: `volume_test.go:122-125`, `image_test.go:184,190,196`
(`wantVolume`), `create_test.go:64-70`, `delete_test.go:67`, `provider_test.go:409`; a small
`fx.baseName()` helper covers the last three. `image_test.go:281,298` should assert the log
field and the `kvm2.volume` span attribute **by prefix**, which keeps pinning that the key stays
readable in logs. `kvm2_integration_test.go:481-484` switches from `baseVolumeName(...)` to
`resolveImage(...).volumeName()`; its assertion that the base survives `Delete` is unchanged.

Two new tests:
- `TestBaseVolumeName_isPerProviderEntry` — two entries, same key, different names; the produced
  name never equals the kvm provider's `base-<key>.<format>`.
- `TestNew_rejectsUnsafeEntryName` — an entry name with a `/` fails at construction.

**Not tested:** that libvirt permits deleting an in-use backing file (established from libvirt
source by the review; re-proving it needs a write on a real host), and the cross-process race
(no hermetic way, and its fix is deferred).

Docs: the *What lands on the host* table (`docs/kvm2-provider.md:198-212`), the eviction recipe
(`:305-318`, which currently names `base-debian-13.qcow2` and would delete kvm's file), the
migration section (`:346-370` — kvm2 never reuses a kvm base, one extra download, nothing to
delete), the prerequisites note that the pool needs one base per distro **per entry**, the
naming contract in `docs/kvm2-provider-spec.md` entries 4 and 10, the shared validator's message
at `internal/provider/config_kvm.go:139-141` ("it becomes the cached volume name
`base-<key>.<format>`", now wrong for kvm2), and the kvm2 block's comment in both
`providers-config.yaml` files. Leave `docs/kvm-provider.md` alone; kvm is unchanged.

---

## Step 5 — Validate in-cluster against beelink

### Why, and what this run can and cannot prove

The kvm2 provider has **never run against real libvirt**: Step 11 is unchecked in
`docs/plans-executions/2026-09-16-1333-kvm2-provider.md`, every `base-*` volume on beelink
predates it, and both live `proxy-*` domains carry the kvm v1 metadata namespace. Four fixes
have landed on code no hypervisor has executed. This run is first contact, and it is also the
gate the campaign's `CHANGELOG` entry waits on.

It proves the happy paths: the dial and health probe, a real 415 MB image download through the
reader H2 now wraps, H4's new volume name appearing beside kvm's untouched one, a second Proxy
hitting the cache, H3's address path reaching Running with an IP, and Squid answering a real
CONNECT probe.

It does **not** prove H2's error paths. The unverified ~85% claim — that libvirtd answers a
short stream finish with OK — only arises when a body *fails*, and a healthy download never
finishes short. The 19-minute HTTP deadline will not fire either. A green run means "the
wrapper did not break normal uploads", not "the deadlock is settled". That still needs the
scratch-pool probe recorded in the Step 2 execution entry.

### The cluster this runs in, and the one hazard

`kind-proxy-operator-demo` is **not a throwaway**. It runs the operator that currently owns
four Ready proxies, two of them real VMs on beelink:

```
proxy-beelink-1           Managed  kvm-home     Ready  192.168.0.166  True
proxy-beelink-2           Managed  kvm-home     Ready  192.168.0.94   True
proxy-kubernetes-demo-1/2 Managed  kubernetes   Ready  10.244.0.7/.8  True
```

Validating kvm2 means rolling the operator that owns them. That is low risk — the controller
and the whole `kvm` provider are untouched by H1-H4 — and rollback is one `kubectl set image`.

**Do not run a second operator against beelink with `kvm-home` in its config.** Orphan GC
(`internal/gc/gc.go:86-131`) lists every Proxy in *its own* cluster, and deletes any instance
whose UID is not in that set and is older than `--gc-min-age` (10 m, swept every 10 m). A
second cluster's live set would not contain `proxy-beelink-1/2`'s UIDs, so its GC would destroy
them within ten minutes. Using the existing cluster removes the hazard entirely: its live set
is the one that protects them.

### Steps

**0. Baseline** — record what is there before touching anything, so "did we break it" is
answerable: `kubectl get px -A`, `kubectl get ph`, and a read-only `virsh vol-list default`
plus `virsh list --all` on beelink.

**1. Build and load, under a new tag.**

```bash
make docker-build IMG=egress-proxies-operator:kvm2-dev
kind load docker-image egress-proxies-operator:kvm2-dev --name proxy-operator-demo
```

The tag **must** differ from the running `kvm-dev`. `make deploy` sets the image through
kustomize; an identical tag leaves the Deployment spec unchanged, so no rollout happens, so the
new process never starts, so the new providers config is never read. The ConfigMap carries no
generator hash, so nothing else forces a restart either.

The working tree is dirty (`review/`, `.claude/`), so `GIT_COMMIT` bakes in as `c10f98a-dirty`
and `--version` will say so. Cosmetic; commit or stash `review/` first to avoid it.

**2. Enable kvm2 in the deployed config.** Uncomment the `kvm2-home` block in
`config/manager/providers_config.yaml` (it is the shipped default, commented out). Keep its
`address: beelink.tail8c69c.ts.net:22` — the Tailscale FQDN, which is what
`~/.ssh/kvm_known_hosts` pins. `config/samples/providers-config.yaml` uses the short
`beelink:22`, which **fails host-key verification**; I hit exactly that when validating SSH
earlier in this session.

**3. Deploy.**

```bash
make deploy IMG=egress-proxies-operator:kvm2-dev
kubectl -n egress-proxies-operator-system rollout status deploy/egress-proxies-operator-controller-manager
```

`make deploy` rewrites `config/manager/kustomization.yaml`'s image block as a side effect; it
already carries `kvm-dev` from a previous run.

**4. STOP GATE — provider health, before anything is created.**

```bash
kubectl get ph
kubectl get ph kvm2-home -o yaml
```

`kvm2-home` must be `Ready 1/1` with reason `Reachable` and details carrying `hostname`,
`libvirtVersion`, `poolState: running` and `poolAvailableGiB`. This proves SSH, the host key,
the libvirt handshake and the pool **without creating a thing**, and it exercises the
dedicated-connection dial and close that H1 touched. A failure here is free to diagnose:
`CredentialsUnavailable`, `Unauthorized`, `Forbidden`, `PoolNotActive` each name their own
cause. Report and wait for a go before step 5.

**5. Two Proxies.** Two manifests in the scratchpad (not committed), modelled on
`config/samples/proxy_kvm2.yaml` — `proxy-kvm2-1` and `proxy-kvm2-2` in `default`, matching the
existing `proxy-beelink-*` convention, `provider: kvm2-home`, `zone: beelink`,
`machineType: small`, `image: debian-13`, port 3128 and the sample's Squid cloud-init. Apply
the first, let it finish, then the second — sequencing them is what makes the cache hit
observable.

```bash
kubectl get px -w
kubectl -n egress-proxies-operator-system logs -f deploy/egress-proxies-operator-controller-manager
```

**6. What to check, and which finding it exercises.**

| Check | Finding |
|---|---|
| `fetching base image into host pool` then `base image cached` with size, duration, digest | H2's wrapped reader on a real 415 MB upload |
| `virsh vol-list default` shows `base-kvm2-home-debian-13.qcow2` **and** `base-debian-13.qcow2` still there | H4: per-entry naming, kvm's cache untouched |
| Second Proxy logs no `fetching`, and the server is not hit again | H4 + the image lock: cache hit under the new name |
| `px` goes Provisioning → Running with an IP, without flapping back to Provisioning | H3: the address path, and no spurious demotion |
| `Healthy=True` | Squid answers a real CONNECT probe end to end |
| `virsh list --all` shows two new `proxy-*` domains | Create |
| After deleting both: domains and their `.qcow2`/`-seed.iso` gone, base image still present | Delete, and the cache surviving |

Optionally `curl -x http://<ip>:3128 https://example.com -I` from this box, if it routes to
192.168.0.0/24 — the existing kvm proxies live on that LAN.

**7. Cleanup.** `kubectl delete px proxy-kvm2-1 proxy-kvm2-2` and let the finalizer do the
work, then verify on beelink read-only. **Any leftover domain or volume: I print the exact
`virsh undefine --nvram` / `vol-delete` lines and you run them** — no write reaches the shared
host from me. Then decide whether the `kvm2-home` ConfigMap entry stays (keep it if you want
kvm2 running for real; `git checkout config/manager/providers_config.yaml` otherwise).

**Rollback at any point:**

```bash
kubectl -n egress-proxies-operator-system set image \
  deploy/egress-proxies-operator-controller-manager manager=egress-proxies-operator:kvm-dev
```

### Who runs what

I drive steps 0-6 and stop at gate 4 for a go. Every beelink write is yours. Nothing here
touches `proxy-beelink-1/2`, `base-debian-13.qcow2`, your unrelated VMs, or the five
pre-existing orphan volumes the review found.

---

## Step 6 — Route klog into the structured logger

**Not a review finding.** Found while watching the Step 5 validation: some lines ignore the
zap encoder entirely, so no amount of `--zap-encoder=json` makes the stream parseable.

```
I0917 19:50:02.580097       1 leaderelection.go:258] "Attempting to acquire leader lease..." lock="..."
```

That is klog's own text sink. client-go's leader elector does `logger := klog.FromContext(ctx)`
(`leaderelection.go:257`), and controller-runtime hands the elector a context with no logr in it
(`pkg/manager/internal.go:461-473` — `mgr.Start` gets `ctrl.SetupSignalHandler()`, which carries
no logger). `klog.FromContext` falls back to `Background()`, and with no logger registered that
is klog's stderr writer. Nothing in this repo imports or configures klog; it is an indirect
dependency (`k8s.io/klog/v2 v2.140.0`) that ~60 linked packages use, including
`client-go/tools/{leaderelection,cache,record}`, `client-go/transport` and `client-go/rest`.
controller-runtime does not route it either — its only `klog.SetLogger` call is in its own test
suite.

### 6a. `cmd/main.go` — one line and an import

```go
logger := zap.New(zap.UseFlagOptions(&opts))
ctrl.SetLogger(logger)
klog.SetLogger(logger)
```

**Plain `SetLogger`, deliberately not `SetLoggerWithOptions(logger, klog.ContextualLogger(true))`.**
The contextual form is what klog's own docs recommend, because it skips a klogr round trip — but
it also disables klog's verbosity gate ("Such a logger cannot rely on verbosity checking in
klog", `contextual.go:83-86`), handing the decision to zap's level. With `Development: true`
that level is Debug, which enables V(1), so client-go's V(1) chatter that klog currently
suppresses at its default `-v=0` would start appearing. Plain `SetLogger` keeps klog's own check
in front ("klog will do its own verbosity checks before calling logger.V().Info",
`contextual.go:43-44`) and still redirects everything it does emit ("all log lines will be
suppressed from the regular output, and redirected to the logr implementation", `:47-48`).
Same fix, no new volume. Performance is irrelevant at this scale.

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

### 6b. `internal/discovery/server.go` — the other stderr bypass

The discovery API's `http.Server` (`:113-119`) sets no `ErrorLog`, so `net/http`'s own errors —
`Accept` failures, handler panics that escape — go to `log.Default()` and out to stderr in
stdlib format. `s.log` is already in scope two lines above:

```go
srv := &http.Server{
	Handler:           s.handler(),
	ErrorLog:          slog.NewLogLogger(logr.ToSlogHandler(s.log), slog.LevelError),
	ReadHeaderTimeout: readHeadTimeout,
	...
}
```

`logr.ToSlogHandler` is the pattern the repo already uses for the same purpose in
`internal/provider/gcp/wirelog.go:31-36`. This server is plain HTTP with no TLS, so `ErrorLog`
carries genuine faults rather than handshake noise from scanners — Error level is right.

### 6c. `config/manager/manager.yaml` — make the deployed stream parseable

```yaml
args:
  - --leader-elect
  - --health-probe-bind-address=:8081
  - --providers-config=/etc/proxy-operator/providers.yaml
  - --zap-encoder=json
  - --zap-time-encoding=rfc3339
```

Verified that the flag wins over the in-code default: `addDefaults` only fills `NewEncoder`
`if o.NewEncoder == nil` (`zap.go:176-191`), and `BindFlags` sets it from `--zap-encoder`.
`--zap-time-encoding=rfc3339` matters because the JSON encoder defaults to `epoch`, which would
give `1758138422.31` instead of a timestamp anyone can read. Verbosity is left at debug, so the
per-RPC lines stay — that was the choice.

### Deliberately not doing

`opts := zap.Options{Development: true}` (`cmd/main.go:154-155`) stays. Flipping it to a
production default would change the format for every existing deployment, not just this one, and
the flags in 6c get the same result here without that blast radius.

### What stays unformatted regardless, and why that is fine

Flag-parse errors and `--version` (`cmd/main.go:160-163`) print before any logger exists, and
`utilruntime.Must` panics in `init()` run before `main`. controller-runtime's own metrics and
health servers also set no `ErrorLog` (`pkg/internal/httpserver/server.go:11-19`) and expose no
option to set one. All three are outside reach; none can fire during steady-state operation.

### Tests

None. All three changes are wiring: two struct fields and one manifest key. `cmd/main.go` has no
test coverage at all, and pinning `klog.SetLogger` hermetically means mutating global state that
would leak into every other test in the package. Verified by observation instead — see below.
Same call as H1's: a test that costs more than it protects does not earn its place.

### Verification

Rebuild, redeploy, and watch the rollout produce the line that started this:

```bash
kubectl -n egress-proxies-operator-system logs deploy/egress-proxies-operator-controller-manager \
  | grep -i 'leader' | head -3
```

It must be JSON with an rfc3339 `ts`, not `I0917 ...`. Then confirm the whole stream parses:

```bash
kubectl -n egress-proxies-operator-system logs deploy/egress-proxies-operator-controller-manager \
  | jq -e -s 'length > 0' >/dev/null && echo "every line is JSON"
```

That second command is the real check — it fails if *any* line is not JSON, which is exactly the
property being fixed.

---

## Ordering

**H2 → H3 → H4**, three commits.

H3 is orthogonal: it touches only `get.go` and `get_test.go`.

H2 and H4 both touch `image.go` but at different heights, and the order matters. H2's `image.go`
edit is a deletion — `discardVolume` moves out of `uploadImage` and up into `fetchImage` — while
H4 edits `imageSource.volumeName()`, `resolveImage`, and `cachedImage`'s digest branch ten lines
below. Landing H2 first means H4 rebases cleanly; the reverse means H4 is written against a
failure path H2 is about to restructure. Keeping H2's guard in `backend.go` rather than in
`countingReader` is part of the same separation.

Semantically, H2 makes `fetchImage` delete the base after the transfer connection is closed, and
that delete is only unambiguously safe once names are per-entry — so H2's commit message should
say the discard is still unfenced against a second entry sharing the name and that H4 closes it,
and H4's should point back. The deferred atomic-publish work then lands exactly where H2 put the
discard, on a volume this call created under a name no other entry can produce.

## Verification

```bash
go build ./... && go vet ./...
go test -race -count=1 ./internal/provider/kvm2/
make test
```

Per step, the check that actually matters is that the new test fails against the unfixed code —
stash the source edit, keep the test, watch it fail, restore. For H2 that means asserting
`eofReader` converts before the fix exists; for H3, that the dead-link subtest returns a
successful Provisioning instance today; for H4, that two entries produce one name today.

After H4, run the kvm2 integration test on beelink. **It has never run** — Step 11 is unchecked
in `docs/plans-executions/2026-09-16-1333-kvm2-provider.md`, every `base-*` on the host predates
kvm2, and both live domains carry the kvm v1 metadata namespace. It is the first end-to-end
exercise of the new name and does a real cold download beside kvm's untouched base.

Each step: its own commit on `feat/kvm2-provider` with the execution-file entry and its checklist
line ticked. `CHANGELOG.md` lands with a "validate and record" step once the user confirms the
work, as in the kvm campaign. Trailer:
`Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>`.