diff --git a/docs/plans/2026-09-16-1333-kvm2-provider.md b/docs/plans/2026-09-16-1333-kvm2-provider.md new file mode 100644 index 0000000..968ef2b --- /dev/null +++ b/docs/plans/2026-09-16-1333-kvm2-provider.md @@ -0,0 +1,301 @@ +# Plan: kvm2, a clean-room libvirt/KVM provider built from the provider contract + +**Created:** 2026-09-16 13:33 + +## Context + +The provider documentation was just split into a generic contract +(`docs/provider-contract.md`), a spec template (`docs/provider-spec-template.md`) +and an appendix on the existing providers (`docs/providers-existing.md`), so that +a new provider can be specified and built from those files alone. Before using +them for Proxmox, the user wants a dry run on a known backend: rebuild the +libvirt/KVM provider from scratch as a second type, `kvm2`, following the +documents, without touching or reading the existing `internal/provider/kvm/` +code. The result is a validated workflow (spec first, then implementation by +fresh agents against the contract) and a v2 provider that can replace v1 once it +passes on the real hypervisor. + +User decisions: + +- **Placement:** new package `internal/provider/kvm2`, type `kvm2`, config block + `kvm2:`, side by side with `kvm`. The Go config struct reuses + `provider.KVMConfig` (`internal/provider/config_kvm.go` is shared code, not + the kvm package), so an entry ports by changing `type` and the block name. +- **Clean room:** implementers never open, grep or import + `internal/provider/kvm/`. Docs (`docs/kvm-provider.md`) and the operational + lessons from the first build's execution log are allowed. +- **Implementer:** the orchestrator (this session) writes the spec and the step + briefs; each implementation step runs in a fresh general-purpose subagent + given only the three docs, the filled spec, the backend facts and its brief. + The orchestrator reviews each result against the contract, runs the tests and + commits. +- **Profiles:** `create-return` blocking, `provisioning-state` create-blocks, + `child-spans` span-decorator, `quota` detected (pool free space precheck), + `credentials` mounted-files (the existing `kvm-ssh` Secret), `client-kind` + rpc-over-ssh, `gc-mark` written-metadata, `created-at` written-at-create, + `ip-source` discovered (agent, then ARP), `list-partial` fail-whole per host + with unreadable domains skipped, `not-found` error-code, `delete-return` + waits, `secondary-resources` explicit, `timeouts` per-call, `logging` tiered, + `backend-test` integration-tag. + +## Backend facts the spec carries (allowed lessons) + +- Real host `beelink`: libvirt 11.6, QEMU 10.1, pool `default` at + `/srv/vms/images`, bridge `br0`, `beelink.tail8c69c.ts.net:22`, user `novakj`, + key `~/.ssh/proxy-operator_ed25519`, known_hosts `~/.ssh/kvm_known_hosts`. + The Debian trixie qcow2 is about 435 MB. +- AppArmor confines QEMU from the domain XML: only `` with a + `` path gets a rule, so volume paths are resolved with + `StorageVolGetPath` and emitted as file disks. +- Debian 13 hangs in GRUB under BIOS with a serial console and no video: UEFI + by default (``, secure boot feature disabled), `firmware: + bios` per image; OVMF is a host prerequisite; undefine needs the NVRAM flag. +- libvirt hands the metadata element back without its namespace declaration: + parse by local name. +- Vendor-data must be a shell script, not cloud-config, to survive cloud-init's + merge with user-data; it installs `qemu-guest-agent`. The agent answered after + about 20 s where ARP never did. +- Read-only rootfs: `iso9660.NewWriter` needs a temp dir under `/tmp` (64Mi). +- go-libvirt (`github.com/digitalocean/go-libvirt`, in go.mod): every method + blocks and takes no ctx; `libvirt.Error{Code uint32, Message}` has a value + receiver; needed `ErrorNumber` constants exist (`ErrNoDomain`, + `ErrNoStorageVol`, `ErrNoStoragePool`, `ErrOperationInvalid`, + `ErrNoDomainMetadata`, `ErrStorageVolExist`, `ErrXMLError`, `ErrNoSupport`, + `ErrInvalidArg`, `ErrAuthFailed`, `ErrAccessDenied`). `libvirt.New(net.Conn)` + then `Connect()`. No new dependencies: `x/crypto/ssh`, `kdomanski/iso9660` + are already direct requires; use `slices`, never `sort` (depguard). + +## Design decisions recorded in the spec (step 1) + +- **Package layout** (`internal/provider/kvm2/`): `kvm2.go` (type, `New`, + logger, per-name keyed lock), `id.go`, `state.go`, `errors.go`, `xml.go`, + `seed.go`, `backend.go` (ctx-first seam over the 20 go-libvirt calls, adapter + with a per-RPC deadline), `ssh.go`, `hosts.go` (one cached connection per + host, lazy dial, dialer seam), `observe.go` (the single decorator: one span + `kvm2.` per RPC plus the V(1)/V(2) log line), `image.go`, `create.go`, + `get.go`, `delete.go`, `list.go`, `check.go`, plus `fake_test.go`, + `sshserver_test.go`, `kvm2_integration_test.go`. Names are suggestions for + the briefs; the implementer may choose others by responsibility. +- **Seam:** every seam method takes `ctx`; the adapter runs the blocking call in + a goroutine and selects on result, `ctx.Done()` and a timer (dial 10s, RPC + 30s, `DomainCreate` 90s, upload/download ctx plus a 20m cap). On abandon it + closes the connection and marks it dead; the next call redials. The image + download runs on a dedicated connection so a cancelled poll cannot cut it off. +- **Decorator wrapped once:** applied inside the real dialer where a connection + is born, so provider code never sees an undecorated backend. Spans `kvm2.dial` + (in the dialer) and `kvm2.download` (in `image.go`) are the two non-RPC spans. + A reflection test asserts every seam method is instrumented. +- **Metadata:** own namespace + `https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/kvm2`, + element `` with `managed`, `uid`, `createdAt`. kvm2 never lists or + adopts v1 domains; a `kvm` and a `kvm2` entry may share a host. +- **Adoption guard:** an existing domain whose metadata `uid` is non-empty and + differs from `req.UID` makes `Create` return `ErrPermanent`. +- **Quota precheck:** before creating volumes, `StoragePoolGetInfo`; needed = + image `Content-Length` (if not cached) + `diskSizeGb` + 16 MiB; below that + returns `ErrQuotaExceeded` naming pool, available and needed. +- **Download** requires `Content-Length` (Permanent otherwise), streams through + `sha256` into `StorageVolUpload`, refreshes the pool, writes the marker + volume last; a failed attempt deletes the partial volume. +- **Classify:** `ErrNoDomain`, `ErrNoStorageVol` NotFound, but Transient inside + `Create`; `ErrNoStoragePool`, `ErrXMLError`, `ErrNoSupport`, `ErrInvalidArg`, + `ErrAuthFailed`, `ErrAccessDenied` Permanent; SSH authentication failures and + host-key mismatches or unknown host keys Permanent; missing key or + known_hosts file Transient (Secret pending), unparsable key Permanent; + cancellation, deadlines, `net.OpError`, `io.EOF` and anything else Transient. +- **Same-name concurrency:** a keyed mutex per instance name serialises Create + and Delete for one name. +- **State:** nostate Provisioning; running or blocked without IP Provisioning, + with IP Running; everything else, including unknown codes, Stopped. IP from + the guest agent first, then ARP, first non-loopback non-link-local IPv4. +- **Checker:** per host concurrently on a fresh dedicated connection (dial, + `ConnectGetLibVersion`, `ConnectGetHostname`, pool lookup and info, close), so + it never touches the pooled connection. Reasons: shared ones plus + `PoolNotFound`, `PoolNotActive`, `CredentialsUnavailable`. Details `user`, + `hostname`, `libvirtVersion`, `pool`, `poolState`, `poolCapacityGiB`, + `poolAvailableGiB`. +- **Rotation:** SSH material is read on every dial, so Secret rotation needs no + restart. +- **Config validation messages:** `validateKVM` is refactored into + `validateKVMBlock(prefix, block string, c *KVMConfig)` so messages say + `kvm2.` for the new block; `validateKVM` stays as a wrapper for `kvm`. + +## Steps (one commit each, on `feat/kvm2-provider` off `main`) + +PR #7 is merged, so `main` carries the three docs. Step 0 begins with +`git fetch origin && git switch main && git pull --ff-only`. + +Every subagent brief opens and closes with: "Do not open, grep, list or import +anything under `internal/provider/kvm/`. If a detail seems to need that code, +stop and report the question instead." Each brief names the files it may create +or edit and nothing else, lists the inputs (the three docs, the spec, +`docs/kvm-provider.md` for backend facts, CLAUDE.md conventions), the +acceptance tests by name and the commands to run, and asks the agent to end +with "files read outside the allowed set: none". + +- **Step 0 (orchestrator):** copy this plan to `docs/plans/-kvm2-provider.md` + with the real timestamp, commit alone, create the branch. Execution summary + `docs/plans-executions/-kvm2-provider.md` with the checklist after step 1. +- **Step 1 (orchestrator): the spec.** Copy `docs/provider-spec-template.md` to + `docs/kvm2-provider-spec.md` and answer all 24 entries with the profiles and + decisions above; every Evidence line cites the user guide, a go-libvirt symbol, + or "verified on beelink in step 11". Entry 21's checklist lists the concrete + test names from steps 4 to 7. Verify: no blank Answer lines, every axis code + used matches contract §4.20, no reference to `internal/provider/kvm/`. +- **Step 2 (subagent): config wiring, IDs, state, classify.** Edits + `internal/provider/config.go` (`KVM2 *KVMConfig` field, `setBlocks`, + allow-list, dispatch), `config_kvm.go` (`validateKVMBlock`), + `config_test.go` and `config_kvm_test.go` rows. Creates `kvm2.go` (type, + assertions, `New` that never dials and warns on unreadable SSH files, stub + methods), `id.go`, `state.go`, `errors.go` with tests: + `TestFormatProviderID_roundTrip`, `TestParseProviderID_malformed`, + `TestMapState_table`, `TestFirstIPv4_filters`, `TestClassify_table`. Also + registers `"kvm2": kvm2.New` in `cmd/main.go` so the two type lists change in + one commit. Verify: three `"kvm2"` sites in `config.go`, no import of + `provider/kvm`, `go test -race ./internal/provider/... && go vet ./...`. +- **Step 3 (subagent): pure builders.** `xml.go`, `seed.go` and tests: + `TestDomainXML_fileDisksWithResolvedPaths`, `TestDomainXML_uefiDefaultSecureBootOff`, + `TestDomainXML_biosEscape`, `TestDomainXML_metadataElement`, + `TestDomainXML_guestAgentChannel`, `TestParseMetadata_withNamespace`, + `TestParseMetadata_withoutNamespaceDeclaration`, + `TestParseMetadata_absentGivesEmptyUID`, `TestVolumeXML_overlayBackingStore`, + `TestSeedISO_filesAndLabel`, `TestSeedISO_userDataByteForByte`, + `TestSeedISO_tempDirRemoved`, `TestVendorData_isShellScriptNotCloudConfig`, + `TestVendorData_multipartWhenExtraProvided`. Verify: no `type="volume"` in + `xml.go`; temp dir under `/tmp` with a deferred removal. +- **Step 4 (subagent): transport, seam, decorator, fakes.** `backend.go`, + `ssh.go`, `hosts.go`, `observe.go`, `fake_test.go`, `sshserver_test.go` and + tests: `TestDial_forwardsUnixSocket`, `TestDial_unknownHostKeyRejected`, + `TestDial_mismatchedHostKeyRejected`, `TestDial_insecureIgnoreHostKeyAccepts`, + `TestDial_missingKeyFileIsTransient`, `TestDial_unparsableKeyIsPermanent`, + `TestDial_handshakeTimeout`, `TestCall_deadlineClosesConnectionAndReturnsTransient`, + `TestCall_cancelUnblocksRPC`, `TestHosts_oneDialPerHostUnderConcurrency`, + `TestHosts_redialsAfterDrop`, `TestHosts_unconfiguredHostIsPermanent`, + `TestObserve_everySeamMethodInstrumented`, `TestObserve_childSpansUnderParent`, + `TestObserve_logsOneLinePerCallAtV1`, `TestObserve_detailOnlyAtV2`, + `TestObserve_uploadBodyNeverLogged`, `TestSSH_keyMaterialNeverInErrorsOrLogs`. + Verify: no `context.Background()` outside tests and the keepalive goroutine; + `tracing.Start` in the decorator and the dialer; `go test -race -count=2`. +- **Step 5 (subagent): image cache and pool-space precheck.** `image.go` and + tests: `TestEnsureImage_downloadsAndWritesMarkerLast`, + `TestEnsureImage_cachedWithMarkerSkipsDownload`, + `TestEnsureImage_missingMarkerRedownloads`, + `TestEnsureImage_sha256ChangeRedownloads`, + `TestEnsureImage_sha256MismatchIsPermanentAndRemovesVolume`, + `TestEnsureImage_noContentLengthIsPermanent`, `TestEnsureImage_http404IsPermanent`, + `TestEnsureImage_http503IsTransient`, `TestEnsureImage_concurrentCallersFetchOnce`, + `TestEnsureImage_cancelledMidStreamReturnsPromptly`, + `TestEnsureImage_streamsWithoutBuffering`, `TestCheckPoolSpace_belowNeededIsQuota`. + Verify: no `io.ReadAll` or `bytes.Buffer` on the download path; download span. +- **Step 6 (subagent): the four contract methods.** `create.go`, `get.go`, + `delete.go`, `list.go`, `kvm2_test.go`, finishing `kvm2.go`. Tests map 1:1 to + contract §11: `TestCreate_repeatReturnsSameIDAndCreatesNothing`, + `TestCreate_resumesAfterEachPartialStep`, `TestCreate_returnsOnlyWhenRunning`, + `TestCreate_rejectsUnknownPlacement`, `TestCreate_adoptionWithForeignUIDIsPermanent`, + `TestCreate_poolOutOfSpaceIsQuotaExceeded`, + `TestCreate_libvirtNotFoundIsTransientNotNotFound`, + `TestCreate_cloudInitSentinelNeverLoggedOrInErrors`, + `TestGet_afterDeleteReturnsNotFound`, `TestGet_duringDeleteStillReturnsInstance`, + `TestGet_backendBusyIsTransientNeverNotFound`, `TestGet_runningWithoutIPIsProvisioning`, + `TestGet_runningWithIPIsRunning`, `TestGet_agentPreferredOverARP`, + `TestGet_addressErrorIsProvisioning`, `TestGet_readsUIDAndCreatedAtFromBackend`, + `TestGet_malformedIDIsPermanent`, `TestGet_unconfiguredHostIsPermanent`, + `TestDelete_malformedIDIsPermanent`, `TestDelete_twiceSucceeds`, + `TestDelete_removesOverlayAndSeed`, `TestDelete_runningDomainDestroyedFirst`, + `TestDelete_toleratesMissingVolumesAndPool`, `TestDelete_unconfiguredHostIsNoOp`, + `TestDelete_concurrentSameNameSafe`, `TestListByTag_excludesUnmarkedDomains`, + `TestListByTag_emptyUIDStillListed`, `TestListByTag_acrossHostsInConfigOrder`, + `TestListByTag_unreachableHostFailsWhole`, `TestListByTag_unreadableDomainSkipped`, + `TestProvider_cancelledContextReturnsPromptly`, + `TestProvider_everyErrorIsWrappedWithConfiguredName`, + `TestLogging_v0SilentV1OutcomeV2Detail`. Verify: `CloudInit` appears in + `create.go` only as `len(req.CloudInit)`; stub strings gone; `Wrap` always + gets the configured name; `go test -race -count=2 ./internal/provider/kvm2/`. +- **Step 7 (subagent): Checker and the CRD comment.** `check.go`, + `check_test.go`: `TestCheck_oneEntryPerHostInConfigOrder`, + `TestCheck_hostsCheckedConcurrently`, `TestCheck_expiredContextReturnsPromptly`, + `TestCheck_reasonsTable`, `TestCheck_neverUsesPooledConnection`, + `TestCheck_detailsKeys`. Edit `api/v1alpha1/providerhealth_types.go` + (`status.type` comment) and run `make manifests`. Verify: only the CRD + description line changes under `config/`; `go test ./internal/providerhealth/`. +- **Step 8 (subagent): deployment, Makefile, integration test.** Edit + `config/manager/manager.yaml` (comment: the `kvm-ssh` mount serves kvm and + kvm2), `config/manager/providers_config.yaml` and + `config/samples/providers-config.yaml` (commented `kvm2` entry), + `config/samples/kustomization.yaml`, `Makefile` (`test-integration-kvm2`, + `-run TestKVM2`, `-timeout 40m`), `docs/testing.md` (suite rows and a kvm2 + integration section). Create `config/samples/proxy_kvm2.yaml` (zone + `beelink`, provider `kvm2-home`) and `kvm2_integration_test.go` + (`KVM2_TEST_HOST`, `_USER`, `_SSH_KEY`, `_KNOWN_HOSTS`, `_BRIDGE`, + `_IMAGE_URL`; optional `_IMAGE_SHA256`, `_POOL`, `_SOCKET`, `_TIMEOUT`, + `_KEEP`, `_LOG_LEVEL`) asserting in order: `New` does not dial; `Check` + Ready with details; `Create` returns `hosts//domains/`; second + `Create` same ID, domain count unchanged; `Get` reaches Running with an IPv4 + and the written UID and CreatedAt; `ListByTag` contains it; `Delete` nil; + `Get` NotFound; `Delete` again nil; pool holds base and marker, no per-VM + volumes; with an in-memory span recorder, `kvm2.DomainDefineXML` and + `kvm2.dial` spans are children of `provider.create`. Verify: + `go vet -tags=integration ./... && go vet -tags=e2e ./...`, + `bin/kustomize build config/default` and `config/samples` parse, + `make -n test-integration-kvm2`. +- **Step 9 (subagent): documentation.** Create `docs/kvm2-provider.md` + (prerequisites incl. OVMF, cluster setup reusing `kvm-ssh`, config reference + pointing at the shared `KVMConfig` table, placement mapping and the + add-a-key-never-edit warning, what lands on the host incl. the metadata + namespace, image cache and quota precheck, health-check reasons, logging and + span names, migration from `kvm`, known gaps). Edit + `docs/providers-existing.md` (fourth column in the three tables and a kvm2 + section), `docs/architecture.md` (components row, §5 block, §10 span + topology, §11 row, Decisions bullets), `README.md` row, `docs/providers.md` + guide link. Verify with the link and symbol sweep from the docs-split + execution log; no em-dashes in new prose. +- **Step 10 (orchestrator): milestone review.** `go-operator-reviewer` on the + branch with the spec and contract as inputs, then `/code-review high`, + `make lint`, `make test` (full, with envtest). Fix accepted findings with + regression tests in one commit; record rejected ones in the summary. +- **Step 11 (orchestrator with the user): real-host validation, changelog, MR.** + Hand the user a host checklist (leftover `proxy-*` domains and volumes from + earlier runs, OVMF installed, pool running with free space, `br0` up, the + known_hosts entry) and the `KVM2_TEST_*` export lines; do not run remote + probes unprompted. With the user's go-ahead run `make test-integration-kvm2` + (a permission prompt is expected; the allow-list entry + `make test-integration-kvm *` does not match) and, if wanted, the in-cluster + pass on kind: build and load the image, deploy, a scratch ConfigMap with the + `kvm2-home` entry, apply `proxy_kvm2.yaml`, wait for `Ready`, + `kubectl get ph kvm2-home -o yaml`, a `curl -x` through the proxy, delete + and confirm the domain and volumes are gone; scratch overrides are not + committed. Record cold download duration and time to IP. CHANGELOG entry + after the user confirms; push with the token helper; MR via the Gitea API + with base `main`; never merge. + +## Verification + +- Per step: the checks listed above, plus `go build ./... && go test -short ./...` + before every commit and `git diff --stat` reviewed for files outside the + step's allowed set. +- Clean-room check on every step's diff: `grep -rn 'provider/kvm"' internal/provider/kvm2/` + empty; the agent's closing line "files read outside the allowed set: none"; + identifiers that coincide with v1's names from the appendix (`libvirtAPI`, + `hostPool`, `imageCache`, `tracingAPI`, `lazySSH`) are treated as a smell and + questioned. +- Contract mapping: after step 7, every rule of contract §2 and §8 has a named + test in the package; the §11 list maps 1:1 to test names (recorded in the + spec's entry 21). +- End to end: the integration run's ordered assertions (step 8) on beelink, and + the optional kind pass (step 11). + +## Risks + +- Subagent scope creep into `internal/provider/kvm/`: instruction-only guard, + mitigated by the closing attestation and the diff greps. +- `.claude/settings.json` is locally modified and uncommitted; it stays out of + feature commits, and the permission prompt for the new make target is + accepted rather than pre-approved. +- `Content-Length` is required for downloads; the Debian and Ubuntu mirrors send + it. +- The cancelled-call policy still drops other in-flight RPCs on that host once; + the dedicated download connection removes the expensive case and the spec + states the rest. +- `go-operator-reviewer` hunts over-abstraction; the seam, the decorator and + the dialer interface each have one production implementation and one fake, + which the contract requires (§4.15, §11).