kacerr/egress-proxies-operator
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
54525bf67a9f205a762a40a8d211b6959fe941cb
egress-proxies-operator/review/scope.md
T
kacerrandClaude Opus 5 41fbd5e7e6 kvm2: record the review validation, enable the provider entry, changelog 
Closes out the review-fix campaign. The kvm2 provider was deployed
in-cluster and validated against beelink -- the first time any of this
code had run against real libvirt rather than the in-memory fake.

config/manager/providers_config.yaml ships the kvm2-home entry enabled,
since the operator is now running it for real.

review/ carries the code review the four H-commits reference, so H1..H4
resolve to something rather than dangling: scope, the seven-chunk sweep,
the escalations, the ten deep dives and the final report. .claude/agents
carries the two reviewer definitions that produced them.

What the validation did not prove is recorded next to what it did: H2's
error paths need a body that fails part way through, and a healthy 433 MB
download never finishes short, so the unverified libvirtd short-stream
behaviour is still open. The kvm2 integration test has also still never
been run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-17 20:50:45 +00:00

134 lines
11 KiB
Markdown
Raw Blame History

# kvm2 provider review: scope
Branch `feat/kvm2-provider` at `2510842`. Seed: `grep -ril 'kvm2\|libvirt'` excluding vendor,
generated, testdata, docs. The minikube layout hints in the brief do not apply; this repo is
the egress-proxies operator and kvm2 is `internal/provider/kvm2/`, a pure-Go provider
(`digitalocean/go-libvirt` RPC over an SSH-tunnelled socket, no cgo, no driver plugin binary).
Checklist items with no counterpart here, so reviewers can answer them quickly rather than hunt:
- libvirt **network** lifecycle / DHCP / static IP: kvm2 creates no networks; it attaches to a
configured bridge and reads the address via `DomainInterfaceAddresses`. The live questions are
IP detection, timeouts and stale addresses.
- GPU/PCI/NUMA passthrough: none.
- driver plugin RPC boundary: none. Closest analogue is the libvirt RPC link over SSH
(`link`, `call`, budgets, dead-link detection, libvirt version differences).
- cgo/build tags: no cgo. Only tag is `//go:build integration`.
- Handle leaks: go-libvirt handles are plain values (no `Free`); the leakable resources are
SSH connections, the libvirt link, goroutines (keepalive, `await`, `call`), HTTP bodies,
temp dirs and half-written storage volumes.
## CORE (kvm2-owned)
`internal/provider/kvm2/` — 18 source files (~3.2k lines), 22 test files (~7.3k lines).
| File | Lines | What |
|---|---|---|
| kvm2.go | 243 | `Provider`, `New`, options, `withDefaults`, `host`, `wrapErr`, `lockName`, `keyedLocks` |
| ssh.go | 216 | `dialStage`/`dialError`, `loadCredentials`, `sshSocket` + keepalive, `dialSSHSocket`, `await` |
| hosts.go | 162 | `dialer`, `hostTable` (per-host cached backend, `get`/`dialFresh`), `libvirtDialer` |
| backend.go | 369 | `backend` interface, `budgets`, `link` (dead marking), `call`/`run`, `libvirtBackend` |
| observe.go | 263 | `observed` backend decorator: span + log per libvirt call |
| create.go | 280 | `Create`, `create`, `ensureOverlay`, `writeSeed`, `ensureRunning`, `waitRunning` |
| delete.go | 98 | `Delete`, `removeDomain`, `removeVolumes` |
| get.go | 119 | `Get`, `get`, `readMetadata`, `address` |
| list.go | 56 | `ListByTag` |
| state.go | 49 | `mapState`, `isActive`, `firstIPv4` |
| id.go | 28 | `formatProviderID`, `parseProviderID` |
| metadata.go | 78 | GC mark in domain `<metadata>`: marshal/parse, `managed`, `createdAt` |
| errors.go | 97 | `permanentf`/`transientf`, `classify`, `classifyLibvirt`, `libvirtCode`, `hasCode` |
| image.go | 467 | `resolveImage`, `lockImage`, `ensureImage`, `cachedImage`, marker read/write, `fetchImage`, `uploadImage`, `do`, `checkResponse`, pool space checks |
| volume.go | 95 | volume naming, `urlImageKey`, `buildVolumeXML` |
| xml.go | 256 | `domainParams`, `buildDomainXML` and the XML structs |
| seed.go | 124 | `buildSeedISO` (temp dir, iso9660), `buildVendorData` |
| check.go | 189 | `Check`, `checkHost`, `probeHost`, `checkReason`, formatting helpers |
Tests (each reviewed with its source file): `*_test.go` next to every file above, plus
`provider_test.go` (contract tests), `fake_test.go` (fake backend), `sshserver_test.go`
(in-process SSH server), `kvm2_integration_test.go` (`//go:build integration`, `TestKVM2*`).
Other kvm2-owned bits:
- `cmd/main.go:62,206` — import and `"kvm2": kvm2.New` registration
- `Makefile:73-75` — `test-integration-kvm2`
- `config/samples/proxy_kvm2.yaml`, `config/samples/kustomization.yaml:8`
- `config/samples/providers-config.yaml:58-`, `config/manager/providers_config.yaml:54-` (commented kvm2 entries)
Reference only, not under review: `docs/kvm2-provider-spec.md`, `docs/kvm2-provider.md`,
`docs/provider-contract.md`. Reviewers may read them to judge code against stated intent.
## SHARED (on the kvm2 call path; listed symbols only)
Called by kvm2:
- `internal/provider/config_kvm.go` — `KVMConfig`, `KVMImage`, `KVMMachineType`, `KVMHost`,
`KVMFormat*`/`KVMFirmware*` consts, `IsHTTPURL`, `validateKVM`, `validateKVMBlock`
(the validator is what stands between user YAML and kvm2's XML/URL/volume-name building)
- `internal/provider/config.go` — `ProviderConfig.KVM2`, `setBlocks`, the `kvm2` arms of `Config.validate`
- `internal/provider/provider.go` — `Provider` interface, `CreateRequest`, `Placement`, `Instance`,
`InstanceState` + `State*` consts, `LabelManagedYes`
- `internal/provider/errors.go` — `Error`, `Wrap`, `Class`, `ErrTransient`/`ErrPermanent`/`ErrNotFound`/`ErrQuotaExceeded`
- `internal/provider/check.go` — `Checker`, `TargetStatus`, `Reason*` consts
- `internal/provider/name.go` — `NameFromUID` (produces the domain name kvm2 puts in XML and volume names)
- `internal/tracing` — `StartSpan`, `Tracer`, `Option`
Calling into kvm2:
- `internal/controller/proxy_controller.go` — `reconcileManaged` (Create/Get/Delete calls at
156/197/218), `replaceInstance`, `reconcileDelete`, `providerFailure`, `deletionFailure`,
`resolveCloudInit`, `placementFrom`
- `internal/gc/gc.go` — `Sweeper.sweep` (`ListByTag` at 105, `Delete` at 125)
- `internal/providerhealth/monitor.go` — `Monitor.checkOnce`, `Monitor.run` (`Check` at 214; the timeout it passes)
- `internal/provider/registry/registry.go` — `Constructor`, `Build`
- `internal/provider/metrics.go`, `internal/provider/tracing.go` — `WithMetrics`/`WithTracing`
decorators (`instrumented`, `traced`): error passthrough, and that they hide `Checker`
- `cmd/main.go:196-225` — registry build, checker extraction from unwrapped providers, wrapping
- `config/manager/manager.yaml:150-178` — `kvm-ssh` Secret mount, `/tmp` emptyDir (64Mi) that `buildSeedISO` uses
## EXCLUDED but considered
- `internal/provider/kvm/` — the older kvm provider; kvm2 is a clean-room rewrite and imports nothing from it.
- `internal/provider/config_kvm_test.go`, `config_test.go` — reviewed only for kvm2 cases, inside chunk 7; rest is kvm.
- `internal/provider/gcp/`, `internal/provider/kubernetes/` — other providers, not on a kvm2 path.
- `internal/health`, `internal/discovery`, `internal/lease` — consume `Proxy` status, never call the provider.
- `internal/metrics`, `internal/tracing` internals — only the three entry points above are on the path.
- `internal/providerhealth/envtest_test.go` — mentions kvm2 only as a type string in a fixture.
- `api/v1alpha1/providerhealth_types.go:71`, `config/crd/bases/...providerhealths.yaml` — comment-only / generated.
- `go.mod`, `go.sum` — dependency pins (`go-libvirt` 20260814 pseudo-version, `iso9660` v0.4.0, `x/crypto` v0.54.0); noted, not reviewed. Third-party source may be read to understand a call, not reported on.
- `config/samples/proxy_kvm.yaml`, `Makefile:69-71,185-196` (`test-integration-kvm`, `kvm-ssh-secret`) — kvm-only or shared tooling with no kvm2 logic. `kvm-ssh-secret` is mentioned in chunk 7 for context only.
- `README.md`, `CHANGELOG.md`, `AGENTS.md`, `.claude/settings.json`, `docs/` — prose / tooling.
- `.gitea/workflows/build.yaml`, `Dockerfile` — no kvm2-specific content; integration test is not run in CI (worth stating in chunk 7, not a file to review).
## Proposed phase 1 chunking (7 chunks)
1. **Connection layer (SSH + libvirt link)**
Files: `ssh.go`, `hosts.go`, `backend.go`, + `ssh_test.go`, `sshserver_test.go`, `hosts_test.go`, `backend_test.go`.
SHARED in play: `KVMConfig` (SSHKeyFile, KnownHostsFile, InsecureIgnoreHostKey), `KVMHost`.
Focus: connection/goroutine leaks on error paths, dead-link detection and redial races in `hostTable`, budgets vs caller context, keepalive, host key handling.
2. **Domain lifecycle (Create / Delete)**
Files: `kvm2.go`, `create.go`, `delete.go`, `id.go`, + `kvm2_test.go`, `create_test.go`, `delete_test.go`, `id_test.go`, `provider_test.go`.
SHARED in play: `CreateRequest`, `Placement`, `NameFromUID`; `reconcileManaged`, `replaceInstance`, `reconcileDelete` (how Create/Delete are retried).
Focus: partial-create cleanup, idempotency, foreign-domain refusal, half-deleted domains, `keyedLocks` correctness, `waitRunning` timeout/cancellation.
3. **Observation, state and error classification (Get / ListByTag)**
Files: `get.go`, `list.go`, `state.go`, `errors.go`, + `get_test.go`, `list_test.go`, `state_test.go`, `errors_test.go`.
SHARED in play: `Instance`, `InstanceState`, `Error`/`Wrap`/`Class` and the `Err*` classes; `Sweeper.sweep`, `providerFailure`, `deletionFailure`.
Focus: libvirt state mapping (paused/crashed/pmsuspended/shutoff), IP detection and stale addresses, NotFound vs transient, one dead host failing the whole list and what GC does with a partial list.
4. **Base image cache and volumes**
Files: `image.go`, `volume.go`, + `image_test.go`, `volume_test.go`.
SHARED in play: `KVMImage`, `KVMFormat*`, `IsHTTPURL`, `validateKVMBlock` (image key charset).
Focus: download/upload streaming, sha256 verify + marker protocol, cross-replica and cross-Create races on the same base volume, cleanup of partial volumes, URL handling (SSRF, redirects, size caps), pool space check, volume-name injection via URL/key.
5. **XML and cloud-init seed templating**
Files: `xml.go`, `seed.go`, `metadata.go`, (`buildVolumeXML` in `volume.go`), + `xml_test.go`, `seed_test.go`, `metadata_test.go`.
SHARED in play: `KVMMachineType`, `KVMHost` (bridge, pool), `KVMFirmware*`, `CreateRequest.CloudInit`, `NameFromUID`; `resolveCloudInit`; `manager.yaml` `/tmp` emptyDir.
Focus: escaping of config- and user-supplied values, validity across libvirt versions, metadata parse robustness (GC trusts it), temp-dir cleanup and size limit, vendor-data merging.
6. **Health check and observability**
Files: `check.go`, `observe.go`, + `check_test.go`, `observe_test.go`.
SHARED in play: `Checker`, `TargetStatus`, `Reason*`; `Monitor.checkOnce`/`run`; `tracing.StartSpan`/`Tracer`; `WithMetrics`/`WithTracing` decorators; `cmd/main.go:213-225`.
Focus: dedicated check connections closed on every path, check timeout vs dial budget, reason mapping, secrets or cloud-init leaking into logs/spans, span end on every path.
7. **Config, registration, deployment and test gating**
Files: `internal/provider/config.go` + `config_kvm.go` (kvm2 arms), their tests' kvm2 cases, `internal/provider/registry/registry.go`, `cmd/main.go:196-225`, `config/manager/manager.yaml:150-178`, `config/manager/providers_config.yaml`, `config/samples/providers-config.yaml`, `config/samples/proxy_kvm2.yaml`, `Makefile:73-75`, `kvm2_integration_test.go`, `fake_test.go`.
SHARED in play: `validateKVMBlock`, `setBlocks`, `Config.validate`, `registry.Build`, `Constructor`.
Focus: validation gaps that let bad values reach chunks 4/5, `New` behaviour with a nil block or unreadable key, Secret mount perms, integration test gating/cleanup and whether the fake backend diverges from real libvirt in ways that hide bugs.
Reference in New Issue View Git Blame Copy Permalink