Add KVM provider methods: Create/Get/Delete/ListByTag over the libvirt seam
Create looks the domain up first and only starts it when it exists; otherwise it caches the image, reuses or creates the overlay, rebuilds the seed ISO unconditionally (a cancelled upload can leave a truncated seed that no later attempt would repair), defines and starts the domain. Get maps libvirt state plus the first routable IPv4 from ARP or the guest agent onto the provider states. Delete destroys, undefines and removes the per-VM volumes by name, tolerating every already-gone case. ListByTag keeps managed domains across all hosts, skipping domains it cannot read and failing the sweep only for an unreachable host. Also corrects the Step 2 seed determinism test: the iso9660 writer stamps the build time into the volume descriptor, so builds differ by a few bytes; the test now compares label, files and size. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
@@ -7,7 +7,7 @@ Plan: [2026-09-04-1404-kvm-provider.md](../plans/2026-09-04-1404-kvm-provider.md
|
||||
- [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`)
|
||||
- [ ] Step 4 — Provider methods (`kvm.go`) over a fake API
|
||||
- [x] Step 4 — Provider methods (`kvm.go`) over a fake API
|
||||
- [ ] Step 5 — Wiring, manifests, samples, docs, CHANGELOG
|
||||
- [ ] Step 6 — Integration test against a real hypervisor
|
||||
- [ ] Step 7 — Manual validation + MR
|
||||
@@ -196,3 +196,90 @@ 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.
|
||||
|
||||
Reference in New Issue
Block a user