2026-09-17 23:12:46 +02:00
1 changed files with 301 additions and 0 deletions
@@ -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 `<disk type="file">` with a
+  `<source file>` 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 (`<os firmware="efi">`, 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.<Call>` 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 `<instance>` 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/<ts>-kvm2-provider.md`
+  with the real timestamp, commit alone, create the branch. Execution summary
+  `docs/plans-executions/<ts>-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/<host>/domains/<name>`; 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).