diff --git a/config/manager/providers_config.yaml b/config/manager/providers_config.yaml index 4ae1361..0602819 100644 --- a/config/manager/providers_config.yaml +++ b/config/manager/providers_config.yaml @@ -53,7 +53,9 @@ data: # pool: default # kvm2 takes the same block shape as kvm: an entry ports by changing # type to kvm2 and the block name to kvm2. kvm2 does not adopt domains - # made by kvm, so delete those Proxies first. See docs/kvm2-provider.md. + # made by kvm, so delete those Proxies first. It caches its base images + # under base--., so it downloads its own copy rather + # than sharing kvm's; budget the pool space. See docs/kvm2-provider.md. # - name: kvm2-home # type: kvm2 # kvm2: diff --git a/config/samples/providers-config.yaml b/config/samples/providers-config.yaml index 14a0bfb..4dc272d 100644 --- a/config/samples/providers-config.yaml +++ b/config/samples/providers-config.yaml @@ -57,7 +57,9 @@ providers: # pool: default # kvm2 takes the same block shape as kvm: an entry ports by changing type # to kvm2 and the block name to kvm2. kvm2 does not adopt domains made by - # kvm, so delete those Proxies first. See docs/kvm2-provider.md. + # kvm, so delete those Proxies first. It caches its base images under + # base--., so it downloads its own copy rather than + # sharing kvm's; budget the pool space. See docs/kvm2-provider.md. # - name: kvm2-home # type: kvm2 # kvm2: diff --git a/docs/kvm2-provider-spec.md b/docs/kvm2-provider-spec.md index f853534..d8821b8 100644 --- a/docs/kvm2-provider-spec.md +++ b/docs/kvm2-provider-spec.md @@ -74,11 +74,18 @@ field and that catalogue edits affect only new VMs. `sha256`, `format` qcow2 or raw, `firmware` uefi or bios) or a URL given directly in `placement.image`. The provider downloads the image into the host's storage pool on first use and caches it there as a base volume named -`base-.` (for a URL: `base-.`), -with a sidecar marker volume `base-..sha256` holding the digest of -the verified upload, written last. A base volume without a marker is +`base--.` (for a URL: `` is the first 16 hex of +sha256(url)), where `` is the provider entry's name, with a sidecar +marker volume `base--..sha256` holding the digest of +the verified upload, written last. The entry is in the name because the `kvm` +provider builds `base-.` in the same pool from the same catalogue +shape, and two entries pointing one key at different URLs would otherwise share +a file; the cost is one copy per entry per host, and the entry's name is checked +against `kvmIdentifier` at construction. A base volume without a marker is incomplete and is deleted and redone. When `sha256` is configured and differs -from the marker's content, the image is re-downloaded. The download streams +from the marker's content, the image is **not** replaced: that is `ErrPermanent` +naming both digests, because overlays back onto the base file and libvirt +unlinks it under running domains without complaint. The download streams from the URL through the operator into `StorageVolUpload` with a running sha256; nothing is buffered. `Content-Length` is required because the volume's capacity must be declared before the upload; a response without it is @@ -89,8 +96,9 @@ Proxy cannot cut it off. After a successful upload `StoragePoolRefresh` makes the file usable as a backing image. Catalogue edit warning for the guide: overlays back onto the base file, so -replacing or evicting a base volume while Proxies use it corrupts those VMs; -add a new key instead. +evicting a base volume while Proxies use it corrupts those VMs. A `sha256` +change is refused for that reason; a `url` change is a silent no-op, since the +cache is keyed by entry and key. Add a new key for either. **Evidence:** kvm-provider.md "The image cache" and "Known gaps"; execution log of 2026-09-04 (raw qcow2 upload plus pool refresh accepted by libvirt 11.6; @@ -381,7 +389,7 @@ read `kvm2.hosts` and not `kvm.hosts`. | `diskSizeGb` | | 10 | not negative | overlay capacity, quota precheck | | `vendorData` | | | | seed `vendor-data` | | `installGuestAgent` | | true | | seed `vendor-data` | -| `images.` | | | key matches `kvmIdentifier` | volume name `base-.` | +| `images.` | | | key matches `kvmIdentifier` | volume name `base--.` | | `images..url` | yes per entry | | http(s) URL | download | | `images..sha256` | | | 64 hex | marker comparison | | `images..format` | | qcow2 | qcow2 or raw | base volume XML, overlay backing format | diff --git a/docs/kvm2-provider.md b/docs/kvm2-provider.md index a55604a..287297f 100644 --- a/docs/kvm2-provider.md +++ b/docs/kvm2-provider.md @@ -37,7 +37,8 @@ Per host, once: API. Nothing on the host filters the port; the provider does not use `spec.port` at all. 4. **A storage pool** (default `default`, usually `/var/lib/libvirt/images`) - with room for one base image per distro (a few hundred MB) plus the + with room for one base image per distro and provider entry (a few + hundred MB each) plus the per-VM overlays (`diskSizeGb` each, thin-provisioned). The pool must be running: a stopped pool fails the health check with `PoolNotActive` and fails `Create` permanently. @@ -202,13 +203,22 @@ host's pool: | Object | Name | Lifetime | |---|---|---| -| base image | `base-.` (for a URL image, `` is the first 16 hex characters of the URL's sha256) | cached; survives Proxy deletion | -| image marker | `base-..sha256`, a 64-byte raw volume holding the digest | with the base image | +| base image | `base--.`, where `` is the provider entry's name (for a URL image, `` is the first 16 hex characters of the URL's sha256) | cached; survives Proxy deletion | +| image marker | `base--..sha256`, a 64-byte raw volume holding the digest | with the base image | | overlay disk | `proxy-abc….qcow2`, qcow2 backed by the base file, `diskSizeGb` capacity | with the Proxy | | seed ISO | `proxy-abc…-seed.iso`, raw | with the Proxy | | domain | `proxy-abc…` | with the Proxy | -The names are built in `internal/provider/kvm2/volume.go`. +The names are built in `internal/provider/kvm2/volume.go`. The base image +carries the **provider entry's name** because the file is shared by every +Proxy on that host but must not be shared by two entries: the `kvm` +provider builds `base-.` in the same pool from the same +catalogue shape, and two kvm2 entries pointing one key at different URLs +would otherwise agree on the name and disagree on the contents. The cost +is one copy of each image per entry per host. For that reason the entry's +name must itself be a usable file name — letters, digits, dots, dashes and +underscores — which `New` checks at startup, since provider names are +otherwise only required to be non-empty and unique. The domain (`buildDomainXML` in `xml.go`) is a `q35` machine with a host-passthrough CPU, ACPI and APIC, memory in MiB and vcpus from the @@ -318,16 +328,27 @@ the rest find the marker. To evict manually: ```sh -ssh novakj@beelink virsh -c qemu:///system vol-delete --pool default base-debian-13.qcow2.sha256 -ssh novakj@beelink virsh -c qemu:///system vol-delete --pool default base-debian-13.qcow2 +ssh novakj@beelink virsh -c qemu:///system vol-list --pool default | grep base-kvm2-home- +ssh novakj@beelink virsh -c qemu:///system vol-delete --pool default base-kvm2-home-debian-13.qcow2.sha256 +ssh novakj@beelink virsh -c qemu:///system vol-delete --pool default base-kvm2-home-debian-13.qcow2 ``` -**Warning: do not change an image entry's `url` or `sha256`, and do not -evict its base volume, while Proxies using that image exist on the -host.** The overlays back onto the base file; deleting or replacing it -corrupts every VM that uses it. Delete those Proxies first, or add the -new image under a new key and move Proxies over by changing their -`placement.image` (which replaces the VM). +Marker first: a base without its marker is refetched, a marker without its +base is cleaned up, and neither leaves a Proxy pointing at nothing. + +**Warning: do not evict a base volume while Proxies using that image exist +on the host.** The overlays back onto the base file, and libvirt will +unlink it under running domains without complaint; the guests keep running +on the open file descriptor and fail at their next start. Delete those +Proxies first. + +Changing `sha256` on an entry whose base is already cached is **refused**, +permanently and by name, rather than resolved by replacing the file — that +replacement was the same corruption, just automated. Add the image under a +new key and move Proxies over by changing their `placement.image` (which +replaces the VM), or delete the Proxies and evict the base by hand. +Changing `url` alone does nothing at all: the cache is keyed by entry and +key, so the new URL is never fetched. Add a new key for that too. ## Quota: the pool-space precheck @@ -381,6 +402,13 @@ A `kvm` domain left behind on a host is invisible to kvm2 forever; clean it up on the host with `virsh undefine --nvram ` and `virsh vol-delete` for its `.qcow2` and `-seed.iso`. +The base image cache is **not** shared between the two types, and that is +deliberate: kvm names its copy `base-.` while kvm2 names its +own `base--.`, so kvm2 downloads the image once more +per host rather than adopting a file another provider maintains. Nothing +needs deleting; the `kvm` base stays where it is and keeps serving the +overlays that back onto it. Budget the extra few hundred MB in the pool. + ## Logging The provider logs through controller-runtime's logger, named `kvm2`, with diff --git a/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md b/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md index 3be2fb1..910d0b7 100644 --- a/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md +++ b/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md @@ -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-.` — 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--.`. `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..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-.: +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 = , 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. diff --git a/internal/provider/config_kvm.go b/internal/provider/config_kvm.go index 95f5271..2efa33f 100644 --- a/internal/provider/config_kvm.go +++ b/internal/provider/config_kvm.go @@ -108,11 +108,17 @@ var sha256Hex = regexp.MustCompile(`^[0-9a-fA-F]{64}$`) // kvmIdentifier constrains the config names that are pasted into // identifiers elsewhere: a host name goes into the providerID // hosts//domains/, which is split on "/" to parse, and an image -// key becomes the file name of the cached base volume. A slash in either -// produces an ID or a path that never resolves again, so the VM leaks with -// nothing able to address it. +// key becomes part of the file name of the cached base volume. A slash in +// either produces an ID or a path that never resolves again, so the VM +// leaks with nothing able to address it. var kvmIdentifier = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]*$`) +// IsKVMIdentifier reports whether s is safe to paste into a providerID or a +// volume name. The provider entry's own name reaches a volume name too, and +// that name is validated for every provider type at once (only non-empty +// and unique), so the kvm2 provider checks its own at construction. +func IsKVMIdentifier(s string) bool { return kvmIdentifier.MatchString(s) } + func validateKVM(prefix string, c *KVMConfig) error { return validateKVMBlock(prefix, "kvm", c) } @@ -137,7 +143,7 @@ func validateKVMBlock(prefix, block string, c *KVMConfig) error { p := fmt.Sprintf("%s: %s.images[%q]", prefix, block, name) if !kvmIdentifier.MatchString(name) { return fmt.Errorf("%s: key must be letters, digits, dots, dashes or underscores"+ - " — it becomes the cached volume name base-.", p) + " — it becomes part of the cached base volume's name", p) } if !IsHTTPURL(img.URL) { return fmt.Errorf("%s.url must be an http(s) URL (got %q)", p, img.URL) diff --git a/internal/provider/kvm2/create.go b/internal/provider/kvm2/create.go index 5751343..0de7e45 100644 --- a/internal/provider/kvm2/create.go +++ b/internal/provider/kvm2/create.go @@ -38,7 +38,7 @@ func (p *Provider) Create(ctx context.Context, req provider.CreateRequest) (stri if !ok { return "", p.wrapErr("create", "", permanentf("placement.machineType %q is not in kvm2.machineTypes", req.Placement.MachineType)) } - src, err := resolveImage(p.cfg, req.Placement.Image) + src, err := resolveImage(p.name, p.cfg, req.Placement.Image) if err != nil { return "", p.wrapErr("create", "", err) } diff --git a/internal/provider/kvm2/create_test.go b/internal/provider/kvm2/create_test.go index 49e862f..66fb26c 100644 --- a/internal/provider/kvm2/create_test.go +++ b/internal/provider/kvm2/create_test.go @@ -61,13 +61,13 @@ func TestCreate_resumesAfterEachPartialStep(t *testing.T) { { name: "base without marker is refetched", setup: func(fx *fixture) { - fx.putVol("base-debian-13.qcow2", []byte("truncated")) + fx.putVol(fx.baseName(), []byte("truncated")) }, check: func(t *testing.T, fx *fixture) { - if got := fx.fake.vol("base-debian-13.qcow2"); got == nil || !bytes.Equal(got.content, fx.image) { + if got := fx.fake.vol(fx.baseName()); got == nil || !bytes.Equal(got.content, fx.image) { t.Errorf("base volume not refetched") } - if fx.fake.vol("base-debian-13.qcow2.sha256") == nil { + if fx.fake.vol(fx.baseName()+".sha256") == nil { t.Errorf("marker not written") } }, diff --git a/internal/provider/kvm2/delete_test.go b/internal/provider/kvm2/delete_test.go index 250937d..e3eda54 100644 --- a/internal/provider/kvm2/delete_test.go +++ b/internal/provider/kvm2/delete_test.go @@ -64,7 +64,8 @@ func TestDelete_removesOverlayAndSeed(t *testing.T) { t.Errorf("volume %s still present after Delete", name) } } - for _, name := range []string{"base-debian-13.qcow2", "base-debian-13.qcow2.sha256"} { + base := "base-" + testProvider + "-debian-13.qcow2" + for _, name := range []string{base, base + ".sha256"} { if fx.fake.vol(name) == nil { t.Errorf("shared base volume %s removed by Delete", name) } diff --git a/internal/provider/kvm2/image.go b/internal/provider/kvm2/image.go index 6a6a695..f0d6664 100644 --- a/internal/provider/kvm2/image.go +++ b/internal/provider/kvm2/image.go @@ -42,22 +42,25 @@ var hexDigest = regexp.MustCompile(`^[0-9a-f]{64}$`) // catalogue entry, or a URL given directly with the catalogue defaults, plus // the key its base volume is named after. type imageSource struct { - Key string + Entry string + Key string provider.KVMImage } -func (s imageSource) volumeName() string { return baseVolumeName(s.Key, s.Format) } +func (s imageSource) volumeName() string { return baseVolumeName(s.Entry, s.Key, s.Format) } func (s imageSource) markerName() string { return markerVolumeName(s.volumeName()) } // resolveImage maps placement.image to a source. cfg is already defaulted, // so a catalogue entry carries its format and firmware; a URL gets qcow2 -// and uefi. -func resolveImage(cfg provider.KVMConfig, image string) (imageSource, error) { +// and uefi. entry is the provider entry's name, which the base volume is +// named after so two entries never share one file. +func resolveImage(entry string, cfg provider.KVMConfig, image string) (imageSource, error) { if img, ok := cfg.Images[image]; ok { - return imageSource{Key: image, KVMImage: img}, nil + return imageSource{Entry: entry, Key: image, KVMImage: img}, nil } if provider.IsHTTPURL(image) { return imageSource{ + Entry: entry, Key: urlImageKey(image), KVMImage: provider.KVMImage{URL: image, Format: provider.KVMFormatQcow2, Firmware: provider.KVMFirmwareUEFI}, }, nil @@ -127,7 +130,11 @@ func cachedImage(ctx context.Context, log logr.Logger, pool libvirt.StoragePool, case !hexDigest.MatchString(digest): log.V(1).Info("base image marker holds no digest; refetching") case src.SHA256 != "" && !strings.EqualFold(digest, src.SHA256): - log.V(1).Info("base image digest differs from the configured sha256; refetching", "cached", digest, "configured", strings.ToLower(src.SHA256)) + // Refusing rather than replacing: every overlay on this host names + // the base as its backing file, and libvirt will unlink it under + // running domains without a word. + return "", permanentf("base image %s is cached with sha256 %s but %s is configured; add the image under a new key and move Proxies to it, or delete the Proxies using it and evict the base by hand", + name, digest, strings.ToLower(src.SHA256)) default: base, err := be.StorageVolLookupByName(ctx, pool, name) switch { diff --git a/internal/provider/kvm2/image_test.go b/internal/provider/kvm2/image_test.go index 47395d0..af8dd75 100644 --- a/internal/provider/kvm2/image_test.go +++ b/internal/provider/kvm2/image_test.go @@ -100,7 +100,7 @@ func (fx *imageFixture) sameConnection() { func (fx *imageFixture) src() imageSource { fx.t.Helper() - src, err := resolveImage(fx.p.cfg, imageKey) + src, err := resolveImage(fx.p.name, fx.p.cfg, imageKey) if err != nil { fx.t.Fatalf("resolveImage(%q) error = %v", imageKey, err) } @@ -180,20 +180,20 @@ func TestResolveImage_table(t *testing.T) { { name: "catalogue key with defaults applied", image: "debian-13", - want: imageSource{Key: "debian-13", KVMImage: provider.KVMImage{URL: "https://cloud.debian.org/debian-13.qcow2", SHA256: "ABCD", Format: "qcow2", Firmware: "uefi"}}, - wantVolume: "base-debian-13.qcow2", + want: imageSource{Entry: testProvider, Key: "debian-13", KVMImage: provider.KVMImage{URL: "https://cloud.debian.org/debian-13.qcow2", SHA256: "ABCD", Format: "qcow2", Firmware: "uefi"}}, + wantVolume: "base-" + testProvider + "-debian-13.qcow2", }, { name: "catalogue key with explicit format and firmware", image: "alpine", - want: imageSource{Key: "alpine", KVMImage: provider.KVMImage{URL: "https://alpine.example/alpine.img", Format: "raw", Firmware: "bios"}}, - wantVolume: "base-alpine.raw", + want: imageSource{Entry: testProvider, Key: "alpine", KVMImage: provider.KVMImage{URL: "https://alpine.example/alpine.img", Format: "raw", Firmware: "bios"}}, + wantVolume: "base-" + testProvider + "-alpine.raw", }, { name: "http URL used directly with defaults", image: rawURL, - want: imageSource{Key: urlImageKey(rawURL), KVMImage: provider.KVMImage{URL: rawURL, Format: "qcow2", Firmware: "uefi"}}, - wantVolume: "base-" + urlImageKey(rawURL) + ".qcow2", + want: imageSource{Entry: testProvider, Key: urlImageKey(rawURL), KVMImage: provider.KVMImage{URL: rawURL, Format: "qcow2", Firmware: "uefi"}}, + wantVolume: "base-" + testProvider + "-" + urlImageKey(rawURL) + ".qcow2", }, { name: "unknown key is permanent and names the value", @@ -214,7 +214,7 @@ func TestResolveImage_table(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got, err := resolveImage(cfg, tc.image) + got, err := resolveImage(testProvider, cfg, tc.image) if tc.wantErr != "" { if err == nil || !errors.Is(err, provider.ErrPermanent) || !strings.Contains(err.Error(), tc.wantErr) { t.Fatalf("resolveImage(%q) = %+v, %v; want ErrPermanent containing %q", tc.image, got, err, tc.wantErr) @@ -278,7 +278,7 @@ func TestEnsureImage_downloadsAndWritesMarkerLast(t *testing.T) { t.Errorf("got %d lines with %s, want 1:\n%s", len(lines), want, strings.Join(fx.logs.all(), "\n")) continue } - for _, kv := range []string{`"provider"="kvm2-home"`, `"host"="beelink"`, `"volume"="base-debian-13.qcow2"`, `"bytes"=` + strconv.Itoa(len(body)), `"traceID"=`} { + for _, kv := range []string{`"provider"="kvm2-home"`, `"host"="beelink"`, `"volume"="base-` + testProvider + `-debian-13.qcow2"`, `"bytes"=` + strconv.Itoa(len(body)), `"traceID"=`} { if !strings.Contains(lines[0], kv) { t.Errorf("line lacks %s: %s", kv, lines[0]) } @@ -295,7 +295,7 @@ func TestEnsureImage_downloadsAndWritesMarkerLast(t *testing.T) { attrs := spans[0].Attributes() for _, want := range []attribute.KeyValue{ attribute.String("kvm2.host", "beelink"), - attribute.String("kvm2.volume", "base-debian-13.qcow2"), + attribute.String("kvm2.volume", "base-"+testProvider+"-debian-13.qcow2"), attribute.String("kvm2.url_host", strings.TrimPrefix(fx.srv.URL, "http://")), attribute.Int64("kvm2.bytes", int64(len(body))), attribute.Bool("kvm2.sha256_configured", false), @@ -391,7 +391,13 @@ func TestEnsureImage_missingMarkerRedownloads(t *testing.T) { } } -func TestEnsureImage_sha256ChangeRedownloads(t *testing.T) { +// TestEnsureImage_sha256ChangeRefusesInsteadOfReplacing pins the rule the +// contract already stated and the code contradicted. Every overlay on the +// host names the base as its backing file, and libvirt unlinks it under +// running domains without complaint, so a digest that no longer matches is +// for the operator to resolve, not for the next Create to resolve by +// deleting. +func TestEnsureImage_sha256ChangeRefusesInsteadOfReplacing(t *testing.T) { t.Parallel() body := randomBody(t, 8192) fx := newImageFixture(t, serveBytes(body)) @@ -401,25 +407,21 @@ func TestEnsureImage_sha256ChangeRedownloads(t *testing.T) { fx.putVol(src.volumeName(), []byte("the old image")) fx.putVol(src.markerName(), []byte(digestA)) - if _, err := fx.ensure(fx.ctx, src); err != nil { - t.Fatalf("ensureImage() error = %v", err) + _, err := fx.ensure(fx.ctx, src) + if !errors.Is(err, provider.ErrPermanent) || !strings.Contains(err.Error(), "add the image under a new key") { + t.Fatalf("ensureImage() error = %v, want ErrPermanent telling the operator what to do", err) } - if fx.hits.Load() != 1 { - t.Errorf("server hits = %d, want 1", fx.hits.Load()) + if fx.hits.Load() != 0 { + t.Errorf("server hits = %d, want 0: nothing should be refetched", fx.hits.Load()) } - if n := fx.pooled.callCount("StorageVolDelete"); n != 2 { - t.Errorf("StorageVolDelete called %d times, want 2 (marker and base)", n) + if n := fx.pooled.callCount("StorageVolDelete"); n != 0 { + t.Errorf("StorageVolDelete called %d times, want 0: the base backs live overlays", n) } - base := fx.pooled.vol(src.volumeName()) - if base == nil || !bytes.Equal(base.content, body) { - t.Errorf("base volume was not replaced by the download") + if base := fx.pooled.vol(src.volumeName()); base == nil || !bytes.Equal(base.content, []byte("the old image")) { + t.Errorf("base volume = %+v, want the cached one untouched", base) } - marker := fx.pooled.vol(src.markerName()) - if marker == nil || string(marker.content) != digestOf(body) { - t.Errorf("marker = %+v, want the new digest %s", marker, digestOf(body)) - } - if lines := fx.logs.matching("differs from the configured sha256"); len(lines) != 1 { - t.Errorf("got %d refetch lines, want 1", len(lines)) + if marker := fx.pooled.vol(src.markerName()); marker == nil || string(marker.content) != digestA { + t.Errorf("marker = %+v, want the cached digest untouched", marker) } } diff --git a/internal/provider/kvm2/kvm2.go b/internal/provider/kvm2/kvm2.go index 6d11039..11090b8 100644 --- a/internal/provider/kvm2/kvm2.go +++ b/internal/provider/kvm2/kvm2.go @@ -74,6 +74,9 @@ var _ provider.Provider = (*Provider)(nil) // files are only checked for existence, and a missing one is a warning, not // an error, because the Secret mount is optional and may land later. func New(ctx context.Context, pc provider.ProviderConfig) (provider.Provider, error) { + if !provider.IsKVMIdentifier(pc.Name) { + return nil, permanentf("provider name %q: kvm2 names its cached base images after the entry, so the name must be letters, digits, dots, dashes or underscores", pc.Name) + } cfg := provider.KVMConfig{} if pc.KVM2 != nil { cfg = *pc.KVM2 diff --git a/internal/provider/kvm2/kvm2_integration_test.go b/internal/provider/kvm2/kvm2_integration_test.go index 625f16e..0aa39ef 100644 --- a/internal/provider/kvm2/kvm2_integration_test.go +++ b/internal/provider/kvm2/kvm2_integration_test.go @@ -481,7 +481,7 @@ func TestKVM2_lifecycle(t *testing.T) { // 10. The pool keeps the shared base image and its marker, and holds // no per-VM volume. start = time.Now() - base := baseVolumeName(integrationImage, provider.KVMFormatQcow2) + base := baseVolumeName(integrationProvider, integrationImage, provider.KVMFormatQcow2) marker := markerVolumeName(base) overlay, seed := overlayVolumeName(name), seedVolumeName(name) present := poolVolumes(ctx, t, p, host, base, marker, overlay, seed) diff --git a/internal/provider/kvm2/kvm2_test.go b/internal/provider/kvm2/kvm2_test.go index 51abb94..909d00d 100644 --- a/internal/provider/kvm2/kvm2_test.go +++ b/internal/provider/kvm2/kvm2_test.go @@ -110,6 +110,18 @@ func TestNew_nilBlockDoesNotPanic(t *testing.T) { } } +// TestNew_rejectsUnsafeEntryName guards the one config value that reaches a +// volume name without passing the kvm validator: the provider entry's own +// name, which is checked only for being non-empty and unique. +func TestNew_rejectsUnsafeEntryName(t *testing.T) { + t.Parallel() + ctx, _ := captureContext() + _, err := New(ctx, provider.ProviderConfig{Name: "kvm2/home", Type: "kvm2"}) + if !errors.Is(err, provider.ErrPermanent) || !strings.Contains(err.Error(), "kvm2/home") { + t.Fatalf("New() error = %v, want ErrPermanent naming the entry", err) + } +} + func TestHost_table(t *testing.T) { t.Parallel() tests := []struct { diff --git a/internal/provider/kvm2/provider_test.go b/internal/provider/kvm2/provider_test.go index c08b897..47be166 100644 --- a/internal/provider/kvm2/provider_test.go +++ b/internal/provider/kvm2/provider_test.go @@ -129,10 +129,21 @@ func (fx *fixture) putVol(name string, content []byte) *fakeVol { return v } +// baseName is the cached base image's volume name for this fixture's +// provider entry and image key. +func (fx *fixture) baseName() string { + fx.t.Helper() + src, err := resolveImage(fx.p.name, fx.p.cfg, imageKey) + if err != nil { + fx.t.Fatal(err) + } + return src.volumeName() +} + // cacheImage places a complete base image and its marker, as a previous // Create on the host would have. func (fx *fixture) cacheImage() { - src, err := resolveImage(fx.p.cfg, imageKey) + src, err := resolveImage(fx.p.name, fx.p.cfg, imageKey) if err != nil { fx.t.Fatal(err) } @@ -406,7 +417,7 @@ func TestLogging_v0SilentV1OutcomeV2Detail(t *testing.T) { if len(resolved) != 1 { t.Fatalf("V(2) lines matching resolved = %d, want 1", len(resolved)) } - for _, key := range []string{`"cloudInitBytes"=`, `"image"="base-debian-13.qcow2"`, `"firmware"="uefi"`, `"vcpus"=1`, `"memoryMiB"=1024`, `"machineType"="small"`, `"bridge"="br0"`, `"pool"="default"`} { + for _, key := range []string{`"cloudInitBytes"=`, `"image"="base-` + testProvider + `-debian-13.qcow2"`, `"firmware"="uefi"`, `"vcpus"=1`, `"memoryMiB"=1024`, `"machineType"="small"`, `"bridge"="br0"`, `"pool"="default"`} { if !strings.Contains(resolved[0], key) { t.Errorf("resolved line lacks %s: %s", key, resolved[0]) } diff --git a/internal/provider/kvm2/volume.go b/internal/provider/kvm2/volume.go index ddeaf24..3576cb1 100644 --- a/internal/provider/kvm2/volume.go +++ b/internal/provider/kvm2/volume.go @@ -14,7 +14,14 @@ import ( func overlayVolumeName(name string) string { return name + ".qcow2" } func seedVolumeName(name string) string { return name + "-seed.iso" } -func baseVolumeName(key, format string) string { return "base-" + key + "." + format } +// baseVolumeName carries the provider entry because the base is shared by +// every Proxy on a host but must not be shared by two entries: the kvm +// provider builds base-. in the same pool from the same +// catalogue shape, and two kvm2 entries pointing one key at different URLs +// would otherwise agree on the name and disagree on the contents. +func baseVolumeName(entry, key, format string) string { + return "base-" + entry + "-" + key + "." + format +} func markerVolumeName(baseName string) string { return baseName + ".sha256" } // urlImageKey stands in for the catalogue key of an image given as a URL: diff --git a/internal/provider/kvm2/volume_test.go b/internal/provider/kvm2/volume_test.go index 01030dd..1ca0948 100644 --- a/internal/provider/kvm2/volume_test.go +++ b/internal/provider/kvm2/volume_test.go @@ -119,10 +119,14 @@ func TestVolumeNames(t *testing.T) { }{ {name: "overlay", got: overlayVolumeName("proxy-abc"), want: "proxy-abc.qcow2"}, {name: "seed", got: seedVolumeName("proxy-abc"), want: "proxy-abc-seed.iso"}, - {name: "base qcow2", got: baseVolumeName("debian-13", provider.KVMFormatQcow2), want: "base-debian-13.qcow2"}, - {name: "base raw", got: baseVolumeName("alpine", provider.KVMFormatRaw), want: "base-alpine.raw"}, + {name: "base qcow2", got: baseVolumeName("kvm2-home", "debian-13", provider.KVMFormatQcow2), want: "base-kvm2-home-debian-13.qcow2"}, + {name: "base raw", got: baseVolumeName("kvm2-home", "alpine", provider.KVMFormatRaw), want: "base-kvm2-home-alpine.raw"}, {name: "marker", got: markerVolumeName("base-debian-13.qcow2"), want: "base-debian-13.qcow2.sha256"}, - {name: "marker of url base", got: markerVolumeName(baseVolumeName(urlImageKey("https://x/y.img"), "qcow2")), want: "base-" + urlImageKey("https://x/y.img") + ".qcow2.sha256"}, + {name: "marker of url base", got: markerVolumeName(baseVolumeName("kvm2-home", urlImageKey("https://x/y.img"), "qcow2")), want: "base-kvm2-home-" + urlImageKey("https://x/y.img") + ".qcow2.sha256"}, + // Two entries pointing one key at different images must not agree + // on a file name, and neither may collide with the kvm provider's + // base-. in the same pool. + {name: "another entry, same key", got: baseVolumeName("kvm2-lab", "debian-13", provider.KVMFormatQcow2), want: "base-kvm2-lab-debian-13.qcow2"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) {