kacerr/egress-proxies-operator
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

kvm2: name the cached base image after the provider entry (H4)

Browse Source 
baseVolumeName was base-<key>.<format>, byte-identical to what the kvm
provider builds in the same pool -- for URL images too, since kvm takes
hex(sum[:8]) and kvm2 takes hex(sum[:])[:16], the same eight bytes -- with
a completely separate lock table. The shipped config has a live kvm-home
entry and a commented-out kvm2-home entry on the same host, the same key
debian-13 and the same default pool, and the comment invites the operator
to enable it. Both integration tests use key test-image. On beelink both
live overlays back onto kvm's base-debian-13.qcow2.

Since cachedImage accepts any 64-hex marker when no sha256 is configured
and never re-hashes the base, that meant two entries pointing one key at
different URLs silently shared a file; a concurrent cold fetch let one
entry delete the other's half-uploaded base and boot a guest from the
remains; and a sha256 edit deleted the base in place under live overlays,
which libvirt permits without complaint.

The name now carries the provider entry: base-<entry>-<key>.<format>.
imageSource gained an Entry field and resolveImage takes the provider
name, so every production call site follows from imageSource.volumeName().
That closes the cross-entry sharing outright, kvm<->kvm2 and kvm2<->kvm2
alike, and turns the per-Provider imageLocks from a latent bug into a
correct one: with names unique per entry, two entries cannot contend for a
file, so no process-wide lock table is needed.

The entry name now reaches a file name, so New checks it against the
existing kvmIdentifier rule, exported as provider.IsKVMIdentifier rather
than duplicated. Provider names are otherwise validated for all types at
once and only for being non-empty and unique, so a name with a slash would
have produced a volume nothing could address.

One companion fix, because the naming alone does not stop it:
cachedImage's digest-mismatch branch deleted the marker and the base and
refetched. It now returns ErrPermanent naming both digests and telling the
operator what to do. That branch was the remaining path that unlinks a
good file under running domains, and the contract already said "add a new
key instead" -- the code was contradicting it. The no-marker and
non-hex-marker branches keep deleting, since there the base is debris.

Not fixed, and said so in the docs rather than left to look closed:
editing images.<key>.url is still a silent no-op, since nothing records
the URL; and cross-process fencing still needs the deferred atomic publish
(upload to a temp name, verify, publish), which lands where H2 put the
discard.

Migration is a non-event. The base-debian-13.qcow2 on beelink is kvm's,
kvm keeps using it untouched, and kvm2 downloads its own copy once per
host. Nothing is replaced under a running guest; the pool needs room for
both, which the prerequisites now say.

The eviction recipe in the guide named base-debian-13.qcow2, which after
this change is kvm's file -- following it would have deleted the wrong
provider's base. It now greps for the prefix and deletes the marker first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
kacerr
2026-09-17 17:57:02 +00:00
co-authored by Claude Opus 5
parent 9682737c69
commit c10f98a352
17 changed files with 258 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md
+96 -1
View File
@@ -5,7 +5,7 @@ Plan: [docs/plans/2026-09-17-1515-kvm2-review-fixes.md](../plans/2026-09-17-1515
- [x] Step 1 — H1: a stalled write must not delay the teardown
- [x] Step 2 — H2: a source error must not reach go-libvirt's stream sender
- [x] Step 3 — H3: an unreadable address must not look like a missing one
- [ ] Step 4 — H4: the base image cache must not be shared across provider entries
- [x] Step 4 — H4: the base image cache must not be shared across provider entries
Remaining after those: the 48 mediums and lows in `review/REPORT.md`. Each gets its own
checklist line here when it is planned, so this list stays a record of decided work rather
@@ -283,3 +283,98 @@ fix's clothes.
now returns Transient rather than a fake Provisioning — better, and self-correcting on the
next poll. Returning NotFound one poll earlier is a two-line addition at the same spot, left
for when L6 comes up so this commit stays one idea.
## Step 4 — H4: the base image cache must not be shared across provider entries
`baseVolumeName` was `base-<key>.<format>` — byte-identical to what
`internal/provider/kvm/image.go` builds in the same pool, for URL images too (kvm takes
`hex(sum[:8])`, kvm2 takes `hex(sum[:])[:16]`, the same eight bytes), with a completely
separate lock table. The shipped config has a live `kvm-home` entry and a commented-out
`kvm2-home` entry on the same host, the same key `debian-13` and the same default pool, and
the comment invites the operator to enable it. Both integration tests use key `test-image`.
On beelink both live overlays back onto kvm's `base-debian-13.qcow2`.
Since `cachedImage` accepts any 64-hex marker when no `sha256` is configured and never
re-hashes the base, that meant: two entries pointing one key at different URLs silently share
a file; a concurrent cold fetch lets one entry delete the other's half-uploaded base and boot
a guest from the remains; and a `sha256` edit deletes the base in place under live overlays,
which libvirt permits without complaint.
The name now carries the provider entry: `base-<entry>-<key>.<format>`. `imageSource` gained
an `Entry` field and `resolveImage` takes the provider name, so the four production call
sites all follow from `imageSource.volumeName()`. That closes the cross-entry sharing
outright, kvm↔kvm2 and kvm2↔kvm2 alike, and it turns the existing per-`Provider` `imageLocks`
from a latent bug into a correct one — with names unique per entry, no two entries can
contend for a file, so a process-wide lock table is not needed.
**The entry name reaches a file name, so it now has to be a usable one.** Provider names are
validated for all types at once and only for being non-empty and unique
(`internal/provider/config.go`), so `New` checks its own against the existing `kvmIdentifier`
rule, exported as `provider.IsKVMIdentifier` rather than duplicated. A name with a slash
fails at startup instead of producing a volume nothing can address.
**One companion fix, because the naming alone does not stop it.** `cachedImage`'s
digest-mismatch branch deleted the marker and the base and refetched. It now returns
`ErrPermanent` naming both digests and telling the operator what to do. That branch was the
remaining path that unlinks a good file under running domains, and the contract already said
"add a new key instead" — the code was contradicting it. The no-marker and non-hex-marker
branches keep deleting, since there the base is genuine debris.
### What this does NOT fix
Stated plainly rather than left to look closed:
- **Editing `images.<key>.url` is still a silent no-op.** The cache is keyed by entry and
key, and nothing records the URL, so the new one is never fetched. Pre-existing, already
documented as "add a new key"; the eviction section now says it outright.
- **Cross-process fencing.** Two managers, or a manager plus a hand-run `virsh`, can still
meet on one name. E4 refuted the rollout and leader-handover triggers for the shipped
manifests, so this is narrow, and its fix is the deferred atomic publish — upload to a temp
name, verify, publish — which lands where H2 put the discard.
### Migration
Nothing to do. The `base-debian-13.qcow2` on beelink is kvm's, kvm keeps using it untouched,
and kvm2 downloads its own copy once per host when `kvm2-home` is enabled. No window in which
anything is replaced under a running guest. The pool needs room for both, which the
prerequisites now say.
### Tests
Mostly literals becoming helpers, plus a new `fx.baseName()` in `provider_test.go` for the
three fixtures that asserted `"base-debian-13.qcow2"` by hand. The log and span assertions in
`TestEnsureImage_downloadsAndWritesMarkerLast` keep asserting a full name rather than loosening
to a prefix, so they still pin that the key stays readable in operator-facing output.
One test changed behaviour rather than spelling: `TestEnsureImage_sha256ChangeRedownloads`
became `TestEnsureImage_sha256ChangeRefusesInsteadOfReplacing`, asserting Permanent, zero
server hits, zero deletes, and both volumes untouched. Two added:
`TestNew_rejectsUnsafeEntryName`, and a `TestVolumeNames` row for a second entry with the same
key.
Both negative checks were run:
```bash
# baseVolumeName reverted to base-<key>.<format>:
go test -count=1 -run TestVolumeNames ./internal/provider/kvm2/
# volume_test.go:135: got "base-debian-13.qcow2", want "base-kvm2-lab-debian-13.qcow2"
# the digest branch reverted to delete-and-refetch:
go test -count=1 -run TestEnsureImage_sha256ChangeRefuses ./internal/provider/kvm2/
# image_test.go:412: ensureImage() error = <nil>, want ErrPermanent telling the operator what to do
```
`go vet -tags=integration` was run too, since `kvm2_integration_test.go:484` builds the base
name and only compiles under that tag.
### Worth noting
**The integration test has still never run on beelink.** 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. After this change it is also
the first end-to-end exercise of the new name, and it will do a real cold download under
`base-kvm2-integration-test-image.qcow2` beside kvm's untouched `base-test-image.qcow2`.
**Docs touched beyond the obvious:** the eviction recipe named `base-debian-13.qcow2`, which
after this change is *kvm's* file — following it would have deleted the wrong provider's base.
It now greps for the prefix and deletes the marker first.