In the kind cluster two KVM Proxies with real cloud-configs booted fine but never left Provisioning: the guest agent was not installed, so the provider could not learn the IP. cloud-init merges vendor-data under user-data with its default mergers, which keep the user-data's value for any key both define and never append lists, so a cloud-config with its own `packages:` silently dropped the vendor-data's packages and runcmd. The integration test passed only because its user-data was empty. withGuestAgent now appends qemu-guest-agent to `packages` and the enable command to `runcmd` of the Proxy's own cloud-config (skipping entries already present) and re-renders it. Vendor-data is no longer defaulted; `vendorData` is plain extra data. User-data that is not a cloud-config document (script, #include, MIME) still gets the bootstrap as vendor-data. A cloud-config that is not a YAML mapping, or whose packages/runcmd is not a list, fails the Proxy as ErrPermanent. Validated in-cluster: both Proxies Ready and healthy in two minutes, 204 through squid from the LAN. Docs, sample comment, CHANGELOG and the plan execution log (Run 7) updated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
633 lines
34 KiB
Markdown
633 lines
34 KiB
Markdown
# Execution: KVM/libvirt provider
|
|
|
|
Plan: [2026-09-04-1404-kvm-provider.md](../plans/2026-09-04-1404-kvm-provider.md)
|
|
|
|
- [x] Step 0 — Save plan + branch `feat/kvm-provider`
|
|
- [x] Step 1 — Config (`KVMConfig` & friends, `validate()` case, tests)
|
|
- [x] Step 2 — Pure builders (`domainxml.go`, `volume.go`, `seed.go`, `id.go`, `metadata.go`)
|
|
- [x] Step 3 — Transport + errors (`conn.go`, `errors.go`)
|
|
- [x] Step 3b — Image cache (`image.go`)
|
|
- [x] Step 4 — Provider methods (`kvm.go`) over a fake API
|
|
- [x] Step 5 — Wiring, manifests, samples, docs, CHANGELOG
|
|
- [x] Step 6 — Integration test against a real hypervisor (written and compiled; first real run is part of Step 7)
|
|
- [ ] Step 7 — Manual validation + MR
|
|
|
|
## Step 0 — Save plan + branch
|
|
|
|
Plan committed alone on `feat/proxy-operator`, then branched:
|
|
|
|
```bash
|
|
git checkout -b feat/kvm-provider
|
|
```
|
|
|
|
## Step 1 — Config
|
|
|
|
The new structs (`KVMConfig`, `KVMHost`, `KVMMachineType`, `KVMImage`) and
|
|
`validateKVM` went into their own file, `internal/provider/config_kvm.go`,
|
|
rather than being appended to `config.go` as the plan said: the KVM block has
|
|
far more fields and rules than the other two providers, and keeping it
|
|
separate leaves `config.go` readable. Tests likewise live in
|
|
`config_kvm_test.go` instead of extending the table in `config_test.go`; the
|
|
invalid-config table covers every required field, duplicate host names, image
|
|
URL scheme, sha256 shape, and the disk format enum.
|
|
|
|
While adding the `kvm` case to `validate()`, the pairwise "type is X but a Y
|
|
block is set" checks (which would have needed six branches with three types)
|
|
were replaced by a `setBlocks()` helper that lists the non-nil blocks and
|
|
rejects any that don't match `type`. The error strings are unchanged, so the
|
|
existing GCP/kubernetes tests still pass.
|
|
|
|
Dependencies were fetched up front so the API signatures in the plan could be
|
|
checked with `go doc`; they stay `// indirect` in `go.mod` until Step 2 imports
|
|
them:
|
|
|
|
```bash
|
|
go get github.com/digitalocean/go-libvirt@latest github.com/kdomanski/iso9660@v0.4.0
|
|
# go-libvirt resolved to v0.0.0-20260814190004-1a83157e1858 (no tagged release)
|
|
# side effect: golang.org/x/crypto 0.54.0 → 0.56.0, x/mod, x/text, x/tools bumped
|
|
```
|
|
|
|
Worth noting: `go doc` showed that go-libvirt's `New(net.Conn)` is deprecated
|
|
in favour of `NewWithDialer(socket.Dialer)`, where `Dialer` is a one-method
|
|
`Dial() (net.Conn, error)` interface. Step 3 should wrap the SSH client's unix
|
|
forward in that interface instead of using `New`; the plan's transport design
|
|
is otherwise unaffected.
|
|
|
|
## Step 2 — Pure builders
|
|
|
|
`internal/provider/kvm/` now has the five builders from the plan plus
|
|
`kvm.go` holding the package doc and `withDefaults` (disk size 10 GiB, the
|
|
default vendor-data, `/var/run/libvirt/libvirt-sock`, pool `default`, image
|
|
format `qcow2`). Everything is a pure function over `encoding/xml` structs or
|
|
strings, unit-tested by unmarshalling the output back and checking fields.
|
|
|
|
Two design choices differ from the plan text:
|
|
|
|
- **Domain disks reference pool volumes, not host paths.** The plan had
|
|
`buildDomainXML` take the overlay and seed paths (needing two
|
|
`StorageVolGetPath` round-trips per Create). The domain XML now uses
|
|
`<disk type="volume"><source pool=... volume=.../>`, so only the *base*
|
|
image path is ever looked up (the overlay's `<backingStore>` needs a real
|
|
path). Fewer RPCs and the builder's inputs are all names.
|
|
**Reverted in Step 7**: volume-referenced disks cannot start on an
|
|
AppArmor host, see there. The domain XML is back to `type="file"` with
|
|
the two `StorageVolGetPath` calls the plan had.
|
|
- **Capacities are always `unit="bytes"`.** libvirt's `G` means GiB but
|
|
`GB` means 10^9; spelling the byte count out removes the ambiguity from
|
|
both the overlay and the seed/base volumes.
|
|
|
|
The metadata element is written with a default namespace
|
|
(`<instance xmlns="…/kvm">`) because `encoding/xml` cannot emit prefixes on
|
|
marshal. libvirt may hand it back prefixed; `parseInstanceMetadata` is
|
|
tested against both spellings since `encoding/xml` matches on the namespace
|
|
URI. Whether libvirt accepts the default-namespace form on `DomainDefineXML`
|
|
is confirmed in Step 6 against a real host, not here. (Step 7: it does,
|
|
but `DomainGetMetadata` returns the element with no namespace at all; the
|
|
parser now matches on local name only.)
|
|
|
|
Checked before relying on `github.com/kdomanski/iso9660` for the seed: its
|
|
filename mangling lowercases and keeps `-`, so `user-data`/`meta-data`/
|
|
`vendor-data` come out as `user-data;1` etc. in the directory record, which
|
|
the Linux isofs driver (and therefore cloud-init's NoCloud mount) presents as
|
|
`user-data`. The seed test opens the ISO with the same library's reader and
|
|
asserts label `cidata`, all three files, and byte-identical output for
|
|
identical inputs. `NewWriter()` stages files in a temp directory, so
|
|
`Cleanup()` is deferred.
|
|
|
|
`go mod tidy` ran as part of this step; it promoted `kdomanski/iso9660` to a
|
|
direct dependency and dropped `go-libvirt` again because nothing imports it
|
|
yet — Step 3 brings it back.
|
|
|
|
Worth noting: `withDefaults` copies the `Images` map and `Hosts` slice
|
|
instead of mutating the config in place, so the registry's shared
|
|
`ProviderConfig` stays untouched. The default vendor-data installs
|
|
`qemu-guest-agent` via `packages:` + `runcmd`, which means the first boot
|
|
needs network access to the distro mirrors; a Proxy on an isolated bridge
|
|
would still work (ARP discovery), just without the agent.
|
|
|
|
## Step 3 — Transport + errors
|
|
|
|
`conn.go` holds the `libvirtAPI` seam (17 methods, signatures as in the
|
|
plan), a per-host connection pool, and the SSH dialer; `errors.go` holds
|
|
`classify`, `isLibvirtError`, `hasCode` and `logAPIError`. Both are
|
|
unit-tested; the whole package still runs in about a second under `-race`.
|
|
|
|
The one thing the plan did not anticipate: **go-libvirt RPCs take no
|
|
context and never time out**. A hung hypervisor would pin a reconcile worker
|
|
forever. So the pool exposes `call(ctx, host, fn)` rather than handing out
|
|
raw clients: `fn` runs in a goroutine, and if `ctx` ends first the pool
|
|
closes the SSH connection out from under it (which makes the blocked RPC
|
|
return) and reports `ctx.Err()`. The same helper (`awaitOrCancel`) bounds
|
|
`lv.Connect()` during dial. Two more liveness measures for the same reason:
|
|
the SSH handshake gets a deadline (ctx's, or 30 s), and each connection runs
|
|
an OpenSSH `keepalive@openssh.com` loop every 15 s that closes the client
|
|
when a request errors or goes unanswered for one interval.
|
|
|
|
Connection-drop policy in `call`: any error that is *not* a `libvirt.Error`
|
|
(so io.EOF, "connection closed", SSH failures) drops the cached connection
|
|
and the next call redials; a daemon-reported `libvirt.Error` keeps it. Dials
|
|
are serialised per host (concurrent Creates on one host share a dial) but
|
|
hosts don't block each other.
|
|
|
|
As Step 1 noted, the dialer uses `libvirt.NewWithDialer` with a
|
|
`socket.Dialer` that wraps `ssh.Client.Dial("unix", socket)` rather than the
|
|
deprecated `New(net.Conn)`.
|
|
|
|
Tests: the pool is exercised through a fake dialer (reuse, redial after
|
|
`Disconnected`, drop on transport error, keep on libvirt error, cancel
|
|
releases a blocked call, concurrent callers share one dial, hosts
|
|
independent). The SSH layer is tested against an in-process
|
|
`x/crypto/ssh` server that serves `direct-streamlocal@openssh.com` channels
|
|
by dialing the requested unix path (an echo socket standing in for
|
|
`libvirt-sock`), with keys generated per test and a real `known_hosts` file
|
|
written via `knownhosts.Line`. Covered: forward round-trip and the socket
|
|
path the server saw, unknown host key rejected with `knownhosts.KeyError`,
|
|
`insecureIgnoreHostKey`, wrong client key, cancelled ctx, missing key or
|
|
known_hosts file, and keepalive both closing a stalled client and leaving a
|
|
healthy one alone. The libvirt handshake (`Connect()`) itself is *not*
|
|
covered here — faking libvirt's XDR RPC just for that is not worth it; the
|
|
Step 6 integration test covers it against a real host.
|
|
|
|
`go mod tidy` promoted `go-libvirt` and `golang.org/x/crypto` to direct
|
|
dependencies.
|
|
|
|
Worth noting: `classify` maps `ErrOperationInvalid` (e.g. destroying a
|
|
domain that is already off) to Transient; Delete in Step 4 must check that
|
|
code itself via `hasCode` and treat it as success rather than relying on
|
|
the classifier. Nothing maps to `ErrQuotaExceeded` — libvirt has no quota
|
|
concept; an out-of-space pool surfaces as a Transient `ErrInternalError`.
|
|
|
|
## Step 3b — Image cache
|
|
|
|
`image.go` follows the plan closely: `resolveImage` maps `placement.image`
|
|
to a configured key or a direct http(s) URL (else Permanent);
|
|
`imageCache.ensure` checks base volume + marker (+ marker content when a
|
|
checksum is configured), tears down and re-fetches when anything is off,
|
|
streams the HTTP body through `StorageVolUpload` with a `sha256` tee,
|
|
refreshes the pool, then writes the marker last. A per-`(host, volume)`
|
|
mutex serialises concurrent callers; the pool itself is the only cache
|
|
state.
|
|
|
|
Errors from this file are pre-tagged with the taxonomy sentinel via
|
|
`fmt.Errorf("%w: …", provider.ErrPermanent, …)` and `classify` now honours
|
|
such tags before looking at libvirt codes. That keeps one `wrapErr` path in
|
|
the provider instead of special-casing image errors.
|
|
|
|
`fake_test.go` landed with this step rather than Step 4: an in-memory
|
|
`libvirtAPI` with pools, volumes (content, format, capacity, backing path),
|
|
domains (state, metadata, per-source interface addresses), per-method error
|
|
injection (`fail` always / `failOnce`) and an `uploadHook` to break a stream
|
|
mid-way. It models the libvirt error codes the provider branches on
|
|
(`ErrStorageVolExist`, `ErrNoStorageVol`, `ErrNoDomain`,
|
|
`ErrOperationInvalid`, `ErrNoDomainMetadata`, `ErrAgentUnresponsive`).
|
|
|
|
Tests cover every case in the plan plus: no checksum configured (marker is
|
|
still written with the computed digest; second call needs no marker read),
|
|
403/500 alongside 404/503, missing pool → Permanent, cancelled ctx →
|
|
Transient with nothing created, and different hosts fetching independently.
|
|
|
|
One test-harness lesson worth keeping: `httptest`'s server drops
|
|
`Content-Length` and switches to chunked encoding for bodies larger than
|
|
its 2 KiB write buffer, so the fake mirror sets the header explicitly.
|
|
Real distro mirrors send it; a server that doesn't is reported as
|
|
Permanent with a message naming the header.
|
|
|
|
Worth noting: the base volume is created with `capacity = Content-Length`
|
|
and the *image's* format (`qcow2`), and the raw bytes are uploaded over it.
|
|
That relies on libvirt's upload writing the file verbatim (which is how
|
|
`virsh vol-upload` populates volumes); the following `StoragePoolRefresh`
|
|
makes libvirt re-read the real virtual size from the qcow2 header. Step 6
|
|
confirms this on a real host. Also, the whole download runs inside one
|
|
`pool.call`, so cancellation aborts the HTTP body (ctx-bound) and the
|
|
upload stream fails; the half-written base volume is deleted on the spot
|
|
and, failing that, on the next attempt via the missing marker.
|
|
|
|
## Step 4 — Provider methods
|
|
|
|
`kvm.go` now holds `Provider`, `New` (loads the SSH key and known_hosts
|
|
up front via `newSSHAuth`, so a bad path fails at startup), the
|
|
`newWithDialer` seam, and `Create`/`Get`/`Delete`/`ListByTag`. Each
|
|
method resolves its host from config, then runs the whole libvirt
|
|
conversation inside one `hostPool.call`, so a cancelled reconcile drops
|
|
the connection and the blocked RPC returns. Errors pass through `classify`
|
|
in one `wrapErr`, as the Step 3b note intended.
|
|
|
|
Behaviour that goes beyond the plan text, or that a future reader should
|
|
know before touching this file:
|
|
|
|
- **Create looks up the domain first.** If it exists, only the running
|
|
check runs and the volumes are never touched. If not, nothing can be
|
|
using the volumes yet: the overlay is reused when it exists (its
|
|
creation is atomic; `ErrStorageVolExist` → reuse) but the **seed is
|
|
always deleted and rebuilt**. The plan said "exists → skip upload"; the
|
|
reviewer pointed out that a reconcile cancelled mid-upload leaves a
|
|
truncated seed that the compensating delete cannot remove, because
|
|
`hostPool.call` has already closed the connection the delete would go
|
|
over. Skipping such a seed would boot the VM without cloud-init and no
|
|
retry would ever repair it. Rebuilding costs one ~40 KiB upload per
|
|
attempt before the domain exists, nothing afterwards. The table test
|
|
breaks Create after each step and re-runs it, and a separate test
|
|
starts from a leftover truncated seed.
|
|
- **Get swallows address-source errors.** `DomainInterfaceAddresses` from
|
|
ARP returns nothing until the guest has sent traffic and the agent
|
|
source errors with `ErrAgentUnresponsive` until `qemu-guest-agent` is
|
|
up, so both are logged at V(2) and mean "no IP yet" → `Provisioning`.
|
|
Loopback, link-local and unspecified addresses are skipped when
|
|
choosing the first IPv4 (the agent reports `lo` too).
|
|
- **State mapping:** `running`/`blocked` are active; `nostate` →
|
|
Provisioning (the reconciler answers Stopped with delete-and-recreate,
|
|
too drastic for a state libvirt could not determine); everything else
|
|
(paused, pmsuspended, shutdown, shutoff, crashed) → Stopped.
|
|
- **Delete treats a missing pool as "nothing to clean"** rather than
|
|
Permanent: failing there would pin the Proxy's finalizer on a config
|
|
edit that renamed the pool. Domain removal is destroy (`ErrOperationInvalid`
|
|
on a stopped domain is fine) → undefine with managed-save, snapshot
|
|
metadata and nvram flags → overlay and seed deleted by name.
|
|
- **ListByTag** reads each domain's metadata once (passed into the same
|
|
`inspect` that Get uses, so no second `DomainGetMetadata` round-trip)
|
|
and skips domains without the operator's element. A domain that cannot
|
|
be read at all — vanished mid-sweep, corrupt metadata, state call
|
|
failing — is *skipped*, not fatal: GC never deletes what it does not
|
|
see, so leaving one out is safe, whereas one bad domain blocking GC for
|
|
every other one (the first version's behaviour, caught in review) is
|
|
not. An unreachable host still fails the whole sweep.
|
|
`ConnectListAllDomains` is called with flags 0 (active and inactive).
|
|
- A host named in a providerID but no longer in `kvm.hosts` is Permanent
|
|
for Get and Delete: the operator cannot reach it, and retrying forever
|
|
would hide the config problem.
|
|
|
|
Tests run the provider over a `hostDialer` that maps host names to
|
|
`fakeAPI` instances (an unmapped host is "unreachable"). The image is
|
|
pre-seeded in the pool for most tests; one test runs the full first-use
|
|
download through `httptest` and confirms a second Proxy on the same host
|
|
does not download again.
|
|
|
|
Found while running the package: `TestBuildSeedISO_deterministic` from
|
|
Step 2 was wrong. `iso9660.ImageWriter` stamps `time.Now()` into the
|
|
volume descriptor, so two builds in different seconds differ by six
|
|
timestamp bytes; the test had only passed because both builds landed in
|
|
the same second. Byte identity is nothing the provider depends on (an
|
|
existing seed is never re-uploaded), so the test now compares label,
|
|
files and size, and the comment on `buildSeedISO` no longer claims
|
|
identical output.
|
|
|
|
The `go-operator-reviewer` agent ran on the first version of this step
|
|
and found the two bugs above (truncated seed, wedged GC sweep) plus the
|
|
double metadata read and the `nostate` mapping; all were fixed before
|
|
committing. One finding it raised is deliberately **not** fixed here
|
|
because it lives in Step 3b's `image.go` and changes the volume naming
|
|
the plan chose: re-fetching a base image (checksum edited in config, or
|
|
upstream content changed) deletes `base-<key>.<fmt>` while existing
|
|
overlays still back onto it, breaking those VMs' disks. Content-addressed
|
|
base names would avoid it. Flagged for the user; until decided, Step 5's
|
|
docs must warn that an image's `sha256`/`url` must not change while
|
|
Proxies using it exist.
|
|
|
|
Worth noting: `go test -cover` reports 88.2 % for the package; the
|
|
uncovered lines are the real SSH/libvirt dial path and log-only
|
|
branches. `go vet` and `-race` (three runs) are clean. Wiring into
|
|
`cmd/main.go` is Step 5, so `New` is not yet reachable from a running
|
|
operator.
|
|
|
|
## Step 6 — Integration test
|
|
|
|
Done before Step 5, at the user's request, after a discussion of what the
|
|
unit tests can and cannot prove: they run over an in-memory fake of the
|
|
libvirt calls, so every assumption about libvirt's real behaviour (domain
|
|
XML accepted, raw qcow2 upload + pool refresh usable as a backing image,
|
|
guest boots and reports an address) needed a test against a real host.
|
|
The wiring in Step 5 is not a prerequisite — the test drives the provider
|
|
directly.
|
|
|
|
`internal/provider/kvm/kvm_integration_test.go` is behind
|
|
`//go:build integration` and skips itself under `-short` or when any of
|
|
`KVM_TEST_HOST`, `KVM_TEST_USER`, `KVM_TEST_SSH_KEY`, `KVM_TEST_BRIDGE`,
|
|
`KVM_TEST_IMAGE_URL` is unset. Optional knobs beyond the plan's list:
|
|
`KVM_TEST_KNOWN_HOSTS` (without it the run disables host-key checking
|
|
and logs that), `KVM_TEST_IMAGE_SHA256`, `KVM_TEST_POOL`,
|
|
`KVM_TEST_SOCKET`, `KVM_TEST_TIMEOUT` (default 20 m, must cover a cold
|
|
image download) and `KVM_TEST_KEEP` to leave a failed run's VM behind for
|
|
`virsh console`.
|
|
|
|
One sequential test, `TestKVM_lifecycle`, not subtests: each stage
|
|
depends on hypervisor state the previous one left. Create (timed, so the
|
|
log shows the image fetch), re-Create returns the same ID, poll Get to
|
|
Running with an IPv4 (Stopped fails fast with a hint to use the console),
|
|
UID and CreatedAt survive the metadata round-trip, ListByTag includes it,
|
|
Delete, Get → NotFound, second Delete is a no-op, ListByTag excludes it,
|
|
and the pool is inspected through the provider's own connection pool
|
|
(`p.hosts.call`, reachable because the test is in package `kvm`): overlay
|
|
and seed gone, base image and marker kept. A TCP connect to the guest's
|
|
port 22 is attempted but only logged, since the test machine may not
|
|
route to the bridge network. Cleanup deletes the VM on failure with its
|
|
own two-minute budget, because the test context may already be spent.
|
|
|
|
Makefile target, so the invocation is not something to remember:
|
|
|
|
```bash
|
|
make test-integration-kvm
|
|
# = go test -race -tags=integration -count=1 -v -timeout 40m -run TestKVM ./internal/provider/kvm/
|
|
```
|
|
|
|
`docs/testing.md` gained the kvm package rows in the suite table, a
|
|
"live libvirt" bullet under the deliberately-not-covered list, and a
|
|
section on running this test, with the deviation from the testcontainers
|
|
convention called out as the plan asked.
|
|
|
|
Worth noting: **the test has not been executed yet.** This session had
|
|
no hypervisor credentials; it was verified with
|
|
`go vet -tags=integration ./...` and by running it without the
|
|
environment (self-skip). Its first real run is the opening act of
|
|
Step 7's manual validation, and it is the point at which the Step 2 and
|
|
Step 3b "confirm on a real host" notes get answered. The test's config
|
|
is built directly rather than through the provider package's validation
|
|
(unexported), so a typo in an env var surfaces from `New` or from
|
|
libvirt, not from a validation message.
|
|
|
|
## Step 5 — Wiring, manifests, samples, docs, CHANGELOG
|
|
|
|
Done after Step 6 (see that entry for why the order flipped). Everything
|
|
the plan listed, plus a few things it did not:
|
|
|
|
- `cmd/main.go`: `"kvm": kvm.New` in the builtins map. No RBAC change —
|
|
the provider talks to hypervisors, not the API server.
|
|
- `config/manager/manager.yaml`: `kvm-ssh` Secret mounted read-only at
|
|
`/etc/proxy-operator/kvm` with `optional: true`, so deployments without
|
|
a kvm provider are unaffected. It nests under the providers-config
|
|
mount at `/etc/proxy-operator`; kustomize renders it (`kubectl
|
|
kustomize config/default` checked). The container runs as uid 65532
|
|
and Secret files default to 0644, so the key is readable without an
|
|
`fsGroup`; the `kubectl create secret` recipe is in a comment next to
|
|
the mount, in `docs/kvm-provider.md` and in the README.
|
|
- `config/samples/providers-config.yaml`: `kvm-home` block with Debian 13
|
|
and Ubuntu 24.04 official image URLs, two machine types, one host, and
|
|
every optional field shown commented out. Parsed through
|
|
`provider.LoadConfigFile` in a throwaway test to make sure the sample
|
|
validates. `config/samples/proxy_kvm.yaml` uses the GCP sample's squid
|
|
cloud-init and shows `ssh_authorized_keys` commented out rather than
|
|
set — a sample with a placeholder key would either fail cloud-init or
|
|
invite pasting a real key into a committed file. Added to the samples
|
|
kustomization; `config/manager/providers_config.yaml`'s comment now
|
|
mentions kvm.
|
|
- `docs/kvm-provider.md`: prerequisites with a three-line preflight
|
|
(`virsh list`, `pool-info`, `ip link` over ssh), cluster setup, the full
|
|
config reference table, placement table, a table of what lands on the
|
|
host and its lifetime, how the image cache works and how to evict, the
|
|
**warning not to change or evict an image while Proxies use it** (the
|
|
reviewer's finding from Step 4, carried as a documented rule since the
|
|
content-addressed-naming alternative was not chosen), how to log in,
|
|
and the known gaps.
|
|
- README: providers bullet and a "KVM setup" section mirroring the GCP
|
|
one. `docs/architecture.md`: components row, a §5 block for the kvm
|
|
call flow in the same style as the other two, and five decision-log
|
|
entries (SSH transport, operator-fetched images, seed always rebuilt,
|
|
`nostate` → Provisioning, skip-vs-fail in the GC sweep).
|
|
- `CHANGELOG.md` entry at the top, stating explicitly that the provider
|
|
is not yet validated on a real host.
|
|
|
|
Verification: `go build ./...`, `go vet ./...`, `go test -short` on
|
|
`cmd` and `internal/provider/...`, and both kustomizations rendered.
|
|
`hack/providers-dev.yaml` untouched, as planned.
|
|
|
|
Worth noting: the CRD has no rule requiring all three placement fields
|
|
(that is GCP's own check), so the single-host "zone may be omitted"
|
|
behaviour works without an API change. And with this step the branch is
|
|
feature-complete but **unproven**: Step 7 runs the integration test and
|
|
the end-to-end checklist against a real hypervisor before the MR.
|
|
|
|
## Step 7 — Manual validation + MR (in progress)
|
|
|
|
Host: `beelink` (Ubuntu, libvirt 11.6.0, QEMU 10.1.0, pool `default` at
|
|
`/srv/vms/images` on local ext4, bridge `br0`), reached over Tailscale as
|
|
`beelink.tail8c69c.ts.net`, SSH user `novakj`. Not the `nas1.home.lan`
|
|
the plan and samples name; that host does not resolve from the dev box.
|
|
|
|
```bash
|
|
export KVM_TEST_HOST=beelink.tail8c69c.ts.net:22
|
|
export KVM_TEST_USER=novakj
|
|
export KVM_TEST_SSH_KEY=$HOME/.ssh/proxy-operator_ed25519
|
|
export KVM_TEST_KNOWN_HOSTS=$HOME/.ssh/kvm_known_hosts
|
|
export KVM_TEST_BRIDGE=br0
|
|
export KVM_TEST_IMAGE_URL=https://cloud.debian.org/images/cloud/trixie/latest/debian-13-generic-amd64.qcow2
|
|
make test-integration-kvm
|
|
```
|
|
|
|
### Run 1 and 2: `Could not open '/srv/vms/images/base-test-image.qcow2': Permission denied`
|
|
|
|
Everything up to `DomainCreate` worked on the first try against a real
|
|
host, which answers the open notes from Steps 2 and 3b: the SSH-forwarded
|
|
libvirt handshake, the raw qcow2 upload plus pool refresh as a backing
|
|
image, the default-namespace metadata element, and `DomainDefineXML` for
|
|
the generated XML were all accepted by libvirt 11.6. QEMU then exited at
|
|
start with `Permission denied` on the base image.
|
|
|
|
Ownership was ruled out the slow way: the file was `root:root 0600` (the
|
|
dir-pool default; libvirt's DAC driver chowns the chain at start and
|
|
restores it after a failed start, so that observation meant nothing),
|
|
`qemu.conf` was all defaults (`libvirt-qemu`, `dynamic_ownership=1`), and
|
|
after a manual `chown libvirt-qemu:kvm` the second run failed identically.
|
|
|
|
The actual cause is the Step 2 choice of `<disk type="volume">`.
|
|
libvirt's AppArmor helper builds each domain's profile from the domain XML
|
|
without consulting the storage driver, and its `add_file_path` only emits a
|
|
rule when the source has a `path`; a volume-referenced disk has pool and
|
|
volume names instead, so neither the overlay nor its backing image gets a
|
|
rule and QEMU is denied on the first one it opens. Every stock Debian and
|
|
Ubuntu hypervisor confines QEMU this way. The unit tests could not have
|
|
caught it: the fake accepts either form.
|
|
|
|
Fix, in this step: `buildDomainXML` takes `DiskPath`/`SeedPath` and emits
|
|
`type="file"` disks; `ensureOverlay` and `writeSeed` return the volume
|
|
handles (the exists-already branch now does a `StorageVolLookupByName`),
|
|
and Create resolves both paths with `StorageVolGetPath` before defining
|
|
the domain. Two extra cheap RPCs per Create. `TestBuildDomainXML_fieldByField`
|
|
now fails on any `type="volume"` or `pool=` attribute so the design cannot
|
|
drift back, and `TestCreate_freshHost` asserts the disk sources equal the
|
|
paths the fake reports.
|
|
|
|
Worth noting: the first two runs left the base image and its marker in
|
|
the pool as designed, so later runs skip the 435 MB download.
|
|
|
|
### Run 3: `parsing domain metadata: expected element <instance> in name space … but have no name space`
|
|
|
|
With file-backed disks, Create returned in 1 s (image cached) and the
|
|
domain started. Get then failed to parse the metadata: libvirt 11.6 finds
|
|
the element by our URI but returns it as a bare `<instance>` with no
|
|
namespace declaration at all. Step 2 had tested the default-namespace and
|
|
prefixed spellings and left "does libvirt accept the default-namespace
|
|
form" for the real host; it does, it just does not give it back that way.
|
|
|
|
Fix: `parseInstanceMetadata` matches on the local name only, since the
|
|
lookup already selected the element by URI. The fake now strips the
|
|
declaration when storing metadata, so the provider's Get and ListByTag
|
|
tests parse the shape libvirt really returns, and a new
|
|
`TestParseInstanceMetadata_libvirtBareForm` pins it.
|
|
|
|
### Run 4: guest hangs at GRUB's "Booting" line
|
|
|
|
Create and start passed; Get polled `Provisioning` with no IP while the
|
|
VM sat at 100 % CPU after GRUB printed "Booting `Debian GNU/Linux`" and
|
|
before the kernel wrote anything to the serial console. The domain XML
|
|
matches a working VM on the same host (q35, BIOS, host-passthrough,
|
|
virtio disk, SATA cdrom, br0), so the difference is the disk content:
|
|
the cached image was downloaded without a checksum and never verified.
|
|
Diagnosis was handed to the user (sha512 against Debian's published
|
|
sums, `qemu-img check`, and a `virt-install --import` of the same image
|
|
outside the operator). Because the provider's own logs were invisible in
|
|
the test output, this run also produced a side plan:
|
|
[2026-09-10-0805-kvm-libvirt-tracing](2026-09-10-0805-kvm-libvirt-tracing.md),
|
|
which traces every libvirt RPC at V(1) and routes it into `-v` output.
|
|
|
|
### Run 5: the guest hang is Debian 13's BIOS boot path; UEFI fixes it
|
|
|
|
The cached image's sha512 matched Debian's published sum, and
|
|
`qemu-img info` showed a sound 3 GiB qcow2, which also confirms that
|
|
the raw upload over a libvirt-created volume plus pool refresh yields a
|
|
byte-identical file (the Step 3b note is answered). Bisected by hand on
|
|
beelink with `virt-install --import` on a fresh overlay of the same
|
|
image:
|
|
|
|
| variant | seed | CPU | firmware | result |
|
|
|---|---|---|---|---|
|
|
| operator's domain | ours | host-passthrough | SeaBIOS | hangs after GRUB "Booting", 100 % CPU |
|
|
| bare | none | host-passthrough | SeaBIOS | same hang |
|
|
| bare | none | host-model | SeaBIOS | same hang |
|
|
| bare | none | host-passthrough | **OVMF (`--boot uefi`)** | **boots to login** |
|
|
|
|
So neither the seed ISO, the domain XML nor the CPU model was the
|
|
problem: the image's legacy GRUB → kernel handoff does not work on this
|
|
host, while its EFI stub path does. Not diagnosed further; it is
|
|
Debian's chain, not ours, and every current cloud image boots UEFI.
|
|
|
|
Change: `images.<key>.firmware` (`uefi` default, `bios` escape),
|
|
threaded through `withDefaults` → `imageSource` → `domainParams`, and
|
|
`buildDomainXML` emits `<os firmware="efi"><firmware><feature
|
|
enabled="no" name="secure-boot"/></firmware>` so libvirt selects the
|
|
OVMF binary and creates the NVRAM file; the `nvram` undefine flag Delete
|
|
already passes removes it (seen on the host: `virsh undefine` without
|
|
`--nvram` refuses such a domain). Direct-URL images default to UEFI
|
|
too. Docs: OVMF is a new hypervisor prerequisite, config table row,
|
|
sample comment, known-gaps entry, decision-log entry.
|
|
|
|
Worth noting: the first image download went through with no checksum
|
|
configured, and the cache reuses a base image by key, so switching
|
|
`KVM_TEST_IMAGE_URL` requires deleting `base-test-image.qcow2` and its
|
|
marker by hand first. That is the reviewer's Step 4 finding showing up
|
|
in practice; still documented rather than fixed.
|
|
|
|
Follow-up, same day: the cause is known upstream. [Debian bug
|
|
#1111240](https://bugs.debian.org/1111240) (filed 2025-08-16 against
|
|
the cloud image, reassigned to `grub2`, still open as of 2026-09-10;
|
|
trixie ships grub2 2.12-9+deb13u2) reports exactly this with a domain
|
|
XML almost identical to ours: BIOS, virtio disk, SATA cdrom seed, bridge
|
|
NIC, `<console><target type='serial'/>`, no `<graphics>`/`<video>`.
|
|
The cloud team reproduced it: trixie's grub.cfg has `terminal_output
|
|
gfxterm serial`, and with a serial port present but no video adapter
|
|
GRUB never hands over to the kernel. Their fix in the image
|
|
(`terminal_output serial`) is not something an operator can apply;
|
|
the reporter's workaround (console `target type='virtio'`) boots but
|
|
takes the kernel's `console=ttyS0` output with it. UEFI sidesteps the
|
|
gfxterm path and keeps the serial console, so the decision stands, but
|
|
the docs now name the bug rather than blaming the host, and note that
|
|
bookworm images are unaffected. `latest` on cloud.debian.org was the
|
|
2026-08-31 build when this was hit, so every trixie point release to
|
|
date carries it.
|
|
|
|
### Run 6: pass
|
|
|
|
With UEFI the full lifecycle passed in 23 s on a warm cache: Create 2 s
|
|
(domain start 0.9 s), guest agent answering with `192.168.0.89` after
|
|
20 s (ARP never did; the host had not talked to the guest), TCP 22
|
|
reachable from the dev machine, ListByTag seeing it, Delete leaving only
|
|
the base image and marker in the pool, second Delete a no-op. Trace
|
|
excerpt worth keeping for the record:
|
|
|
|
```
|
|
09:44:28.603 libvirt virDomainDefineXML xmlBytes=1509 domain=proxy-hk5oavoqk45n3twq duration=13.51ms
|
|
09:44:28.603 kvm domain defined uid=9bbceb41-… firmware=uefi
|
|
09:44:29.542 libvirt virDomainCreate domain=proxy-hk5oavoqk45n3twq duration=938.705ms
|
|
09:44:49.573 libvirt virDomainInterfaceAddresses domain=proxy-hk5oavoqk45n3twq source=DomainInterfaceAddressesSrcAgent interfaces=2 duration=1.742ms
|
|
09:44:49.573 kvm KVM domain fetched state=Running ip=192.168.0.89 uid=9bbceb41-…
|
|
```
|
|
|
|
Worth noting: the sweep listed a second managed domain,
|
|
`proxy-qxge3dbm2mnpc3ih`, shut off, left behind by an earlier failed
|
|
run whose cleanup never ran. That is exactly the orphan GC exists for;
|
|
on the host it needs `virsh undefine --nvram` plus its two volumes by
|
|
hand. The `ListByTag` sweep costs one `virDomainGetMetadata` per
|
|
domain on the host (21 here, ~0.4 ms each over the tunnel), which is
|
|
fine at this scale but is the call to watch on a busy hypervisor.
|
|
|
|
### Run 7: in-cluster, two defects the integration test could not see
|
|
|
|
The e2e pass ran the operator inside the kind cluster
|
|
(`egress-proxies-operator:kvm-dev`, built with `make docker-build`,
|
|
loaded with `kind load docker-image`, deployed via the kustomize
|
|
`config/manager` with the `kvm-home` provider pointing at beelink) and
|
|
two Proxies with real cloud-configs: one plain squid, one with a debug
|
|
login user. Both defects come from the difference between the test
|
|
harness and the real thing, not from libvirt.
|
|
|
|
**Read-only root filesystem.** The manager runs with
|
|
`readOnlyRootFilesystem: true`, and `iso9660.NewWriter` stages the seed
|
|
in a temp directory, so the first Create failed with `creating iso
|
|
writer: read-only file system`. Fix: a 64 MiB `emptyDir` on `/tmp` in
|
|
`config/manager/manager.yaml`. Side finding: the error is classified
|
|
`ErrPermanent` by `writeSeed`, yet `hostPool.call` treated any
|
|
non-libvirt error as a transport failure and dropped the SSH connection,
|
|
so every retry redialled. `call` now leaves the connection alone for
|
|
errors already carrying a provider sentinel (`isClassified`), with a
|
|
test.
|
|
|
|
**The guest agent never got installed.** Both VMs booted (confirmed on
|
|
the console by hand) but sat in `Provisioning` for 25 minutes: the
|
|
trace showed `virDomainInterfaceAddresses` from the agent source failing
|
|
with `ErrAgentUnresponsive` on every poll and the ARP source empty. The
|
|
integration test had passed with `CloudInit: "#cloud-config\n"`; the
|
|
real Proxies have `packages: [squid]` and `runcmd:`. cloud-init merges
|
|
vendor-data under user-data with its default mergers, which keep the
|
|
user-data's value for any key both define and never append lists, so the
|
|
vendor-data's `packages`/`runcmd` were dropped wholesale. Confirmed
|
|
locally with the dev box's cloud-init 26.1 before changing anything:
|
|
|
|
```sh
|
|
python3 - <<'EOF'
|
|
from cloudinit import mergers
|
|
user = {"packages": ["squid"], "runcmd": ["systemctl restart squid"]}
|
|
vendor = {"packages": ["qemu-guest-agent"], "runcmd": [["systemctl", "enable", "--now", "qemu-guest-agent"]]}
|
|
print(mergers.construct(mergers.default_mergers()).merge(dict(user), dict(vendor)))
|
|
EOF
|
|
# {'packages': ['squid'], 'runcmd': ['systemctl restart squid']}
|
|
```
|
|
|
|
Fix: `withGuestAgent` in `seed.go` merges the bootstrap into the Proxy's
|
|
cloud-config itself (append to `packages` and `runcmd`, skip if present,
|
|
re-render with `sigs.k8s.io/yaml`), so vendor-data is no longer defaulted
|
|
and `vendorData` is plain extra data. User-data that is not a cloud-config
|
|
document (script, `#include`, MIME) still gets the bootstrap as
|
|
vendor-data since there is nothing to collide with; a cloud-config that
|
|
does not parse as a mapping fails the Proxy as `ErrPermanent`. The
|
|
rendered document for the squid Proxy passed `cloud-init schema
|
|
--config-file`. The logging test's fixture (`#cloud-config` + a bare
|
|
sentinel line) is not a mapping and had to become `hostname: <sentinel>`.
|
|
|
|
Rebuilt, reloaded, restarted, deleted and recreated both Proxies: both
|
|
`Ready`/`HEALTHY True` in 2 min 1 s from `kubectl apply`, IP via the
|
|
agent 110 s after `virDomainCreate` (the guest installs squid and the
|
|
agent from the mirrors first), and `curl -x http://<ip>:3128
|
|
https://www.gstatic.com/generate_204` from the dev box returns 204
|
|
through both.
|
|
|
|
Worth noting: the `#cloud-config` round-trip drops comments and reorders
|
|
keys alphabetically; semantics are preserved (numbers, booleans and
|
|
block scalars survive, checked in tests) but a user diffing the seed
|
|
against their CR will see a different document. Vendor-data supplied via
|
|
`vendorData` is subject to the same cloud-init merge rule and is now
|
|
documented as such. The kind pod also restarted twice during the session
|
|
from "Pod sandbox changed" (the node briefly lost the API server), which
|
|
is unrelated to the operator. Still uncommitted on purpose: the
|
|
`kustomization.yaml` image override (`egress-proxies-operator:kvm-dev`)
|
|
and the `beelink` host entries in `config/manager/providers_config.yaml`
|
|
and `config/samples/providers-config.yaml`, which are this machine's dev
|
|
setup rather than the repo's defaults.
|