From 18a3378507ed82e71e0d002d2e0743afddd24d5d Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 18:08:59 +0000 Subject: [PATCH] Add plan: provider health checks Periodic, leader-only checks of every configured provider (KVM per host over the pooled SSH+libvirt connection including the storage pool, kubernetes apiserver/RBAC, GCP credentials/API), surfaced through a new cluster-scoped ProviderHealth CRD, Prometheus gauges and transition logs. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-11-1526-provider-health-checks.md | 354 ++++++++++++++++++ 1 file changed, 354 insertions(+) create mode 100644 docs/plans/2026-09-11-1526-provider-health-checks.md diff --git a/docs/plans/2026-09-11-1526-provider-health-checks.md b/docs/plans/2026-09-11-1526-provider-health-checks.md new file mode 100644 index 0000000..489bebc --- /dev/null +++ b/docs/plans/2026-09-11-1526-provider-health-checks.md @@ -0,0 +1,354 @@ +# Plan: Provider health checks — see whether the cluster can reach each provider +**Created:** 2026-09-11 15:26 UTC + +## Context + +Today the operator can't say whether a provider's backend is reachable. The `Provider` +interface ([provider.go:89-109](internal/provider/provider.go#L89-L109)) only has +Create/Get/Delete/ListByTag, and `/healthz` and `/readyz` are bare `healthz.Ping`. A +broken KVM path only shows up later: as a Proxy stuck in Provisioning, or as an orphan-GC +sweep logging an error. Several failures stay invisible until a Proxy needs that host: + +- a missing `kvm-ssh` Secret; +- a rotated host key; +- a Tailscale route that is down; +- a storage pool that is inactive or full. + +The goal is a periodic sanity check of every configured provider from inside the cluster, +with results you can see through `kubectl` and Prometheus. + +Decisions made by the user during planning: + +1. **Surface:** a new operator-owned, read-only, cluster-scoped **`ProviderHealth`** CRD, + with one object per configured provider, a `Ready` condition and per-target detail. + Also Prometheus gauges, and a log line whenever a target goes up or down. No + EventRecorder, matching the earlier decision. +2. **Scope:** real checks for all three providers. + - **kvm:** checked per host. + - **kubernetes:** confirms the API server and RBAC work. + - **gcp:** confirms credentials and the API work. +3. **KVM depth:** reachability plus the storage pool. + - Over the existing pooled SSH+libvirt connection, call the libvirt version and + hostname RPCs. + - Confirm the configured pool is running and report its free space. + - No bridge check. + +Not affected: the Proxy reconcile behaviour, and the manager's `/healthz` and `/readyz`. +A dead hypervisor must never make the manager unready. + +## Design + +### Contract: `internal/provider/check.go` + +```go +type TargetStatus struct { + Name, Address string + Ready bool + Reason, Message string + Latency time.Duration + Details map[string]string +} +// Check never errors; each target carries its own verdict. Must return promptly once ctx ends. +type Checker interface{ Check(ctx context.Context) []TargetStatus } +``` + +- **Shared reasons:** `Reachable`, `Timeout`, `Unreachable`, `Unauthorized`, `Forbidden`, + `APIError`. +- **Compile-time checks:** each provider's `check.go` has `var _ provider.Checker = (*Provider)(nil)`. +- **Bypass the wrappers.** `cmd/main.go:197-201` wraps every provider in + `WithTracing(WithMetrics(...))`, so the Checkers are collected from the **raw** providers + map, before that loop. As a result, checks don't inflate `provider_requests_total`. + +### The checks + +**kvm** (`internal/provider/kvm/check.go`) +- **Concurrency:** one goroutine per `cfg.Hosts` entry (`wg.Go`), writing to `out[i]` so + results stay in config order. +- **Inside `p.hosts.call`:** `ConnectGetLibVersion`, `ConnectGetHostname`, + `StoragePoolLookupByName(host.Pool)`, then `StoragePoolGetInfo`. +- **Rules for `fn`:** + - It returns only raw RPC errors. `call` drops the shared connection on any other error + ([conn.go:160](internal/provider/kvm/conn.go#L160)), so "pool not running" is decided + after `call` returns, never returned from `fn`. + - It fills a captured result struct only after all four calls succeed. That struct is + read only when `call` returned nil, which avoids a race with an abandoned goroutine. +- **Details:** `hostname`, `libvirtVersion` (decoded as major.minor.patch), `pool`, + `poolState`, `poolCapacity` and `poolAvailable` (in GiB). +- **New RPCs:** add the three to `libvirtAPI` ([conn.go:29-47](internal/provider/kvm/conn.go#L29-L47)), + `tracingAPI` (`trace.go`) and `fakeAPI` (`fake_test.go`). `fakeAPI` gets a hostname, a + fixed version, and a per-pool state map that defaults to Running. +- **Telling failure stages apart without matching strings:** add + `type dialError struct{ stage dialStage; err error }` in `conn.go`, with `Error()` + returning the inner message unchanged and `Unwrap()`. It wraps at four places: + - `lazySSH.dial`, when loading the key material fails (`ssh-material`) + - `dialSSH`, at the dial step (`tcp`) + - `dialSSH`, at the handshake (`ssh-handshake`) + - `connectLibvirt` (`libvirt-connect`) + + Because `Unwrap` is kept, `classify` and `isClassified` still see `ErrPermanent` and + `fs.ErrNotExist`, so reconcile behaviour doesn't change. +- **`checkReason(err)`, tested in order:** + + | Condition | Reason | + |---|---| + | stage ssh-material | `SSHMaterialUnavailable` | + | stage tcp | `Unreachable` | + | stage ssh-handshake | `SSHHandshakeFailed` | + | stage libvirt-connect | `LibvirtConnectFailed` | + | ctx deadline or cancel | `Timeout` | + | `ErrNoStoragePool` | `PoolNotFound` | + | any other libvirt error | `LibvirtError` | + | anything else | `Unreachable` | + | success, pool not running | `PoolNotActive` | + | success | `Reachable` | +- **Accepted behaviour:** + - A check that hits its timeout drops the shared connection. That already happens for + any cancelled reconcile context, and it only fires when the host isn't answering. + - The leader now keeps one SSH connection per host open from startup, with keepalives + every 15s. Before, the connection only opened with the first KVM reconcile. + - No forced periodic redial. `lazySSH` caches the key material forever, so a redial + would not pick up a rotated Secret anyway; that gap goes into the docs under + "Known gaps". + +**kubernetes** (`internal/provider/kubernetes/check.go`) +- **Call:** `List` pods with the managed-label selector and `client.Limit(1)`, + cluster-wide. It uses the same uncached client, verb and RBAC as `ListByTag` and GC. + There is no namespace setting in the kubernetes config. +- **Target:** one target named `apiserver`, with the address from a new `apiHost` field + set in `NewWithTransportWrapper`. +- **Reasons:** + - `Timeout`: the context ended, or the error is `IsTimeout`/`IsServerTimeout`. + - `Unauthorized`, `Forbidden`. + - `APIError`: any other API status. + - `Unreachable`: anything else. + +**gcp** (`internal/provider/gcp/check.go`, plus a seam change in `gcp.go`) +- **Why a new seam method:** the existing `AggregatedList` seam reads every page + ([gcp.go:63-78](internal/provider/gcp/gcp.go#L63-L78)), and MaxResults is only the page + size. So add `Probe(ctx, *computepb.AggregatedListInstancesRequest) error`, which makes + one `it.Next()` call and treats `iterator.Done` as nil. +- **Request:** `Project`, `MaxResults: 1`, `ReturnPartialSuccess: true`, and the same + managed-label filter `ListByTag` uses. +- **Target:** one per project, `Name: cfg.Project`, `Address: compute.googleapis.com`. + GCP config has no zones, and reading them from Proxy placements would tie the check to + the Proxy cache. +- **Reasons:** + - `Timeout`: the context ended. + - For `*googleapi.Error`: 401 is `Unauthorized`, 403 is `Forbidden` (this also covers a + disabled API), anything else is `APIError`. + - `Unreachable`: anything else, including a failure to get a token. + +### The `ProviderHealth` CRD: `api/v1alpha1/providerhealth_types.go` + +- **Written by hand**, since kubebuilder isn't installed; `PROJECT` is edited by hand too. +- **Scope and markers:** cluster-scoped, `shortName=ph`, status subresource, no spec. +- **Scheme registration:** add the type to the existing `AddKnownTypes` call in + [proxy_types.go:362](api/v1alpha1/proxy_types.go#L362), not a second `init()`. +- **Status:** + - `type` + - `conditions` (`Ready`; `ConditionReady` constant) + - `targets[]`, as a list-map keyed by `name`, each with: `name`, `address`, `ready`, + `reason`, `message` (at most 1024 characters), `latencyMillis`, + `lastTransitionTime`, `details` + - `targetsReady` (for example `"1/2"`, used by a printer column) + - `lastCheckTime` +- **Printer columns:** Type, Ready, Targets, Last Check, Age. +- **Viewer role:** `config/rbac/providerhealth_viewer_role.yaml`. No editor role, since the + operator owns the object. + +### Monitor: `internal/providerhealth` + +- **Fields:** `Monitor{Client, Reader, Targets []Target{Name, Type, Checker}, Interval, Timeout}`. +- **Scheduling:** + - `NeedLeaderElection() true`. + - Checks once immediately, then on a ticker. Copy the ticker loop from + [gc.go:54-80](internal/gc/gc.go#L54-L80). +- **Each cycle:** + - Runs every provider concurrently under `context.WithTimeout(Timeout)`, inside one + `tracing.Start` span. + - Stores the snapshot under a mutex; `Snapshot()` returns a deep copy. + - Logs changes. Going down logs `WARNING: provider target not ready` with provider, + target, address, reason and message. Coming back logs `Provider target ready`. A + target that is ready on first sight logs at V(1) only. +- **Writes:** + - Get through `mgr.GetAPIReader()`, not the cache. That avoids a cluster-wide informer + and the watch permission it would need, whatever `--proxy-namespace` is set to. + - Create the object if it is missing, labelled + `app.kubernetes.io/managed-by=egress-proxies-operator`. + - Then `Status().Patch(MergeFrom(base))` when the status changed, following + [status.go:114-121](internal/controller/status.go#L114-L121). + - This means one write per provider per interval, because `lastCheckTime` moves each + time. That's acceptable given the 10s minimum interval. +- **`buildStatus(prev, result, now)` is a pure function:** + - A target keeps its `lastTransitionTime` unless `ready` flipped. + - The `Ready` condition is set through `apimeta.SetStatusCondition`: + - `True/AllTargetsReady` when every target is ready + - `False/TargetsNotReady` otherwise, with the failing targets in the message + - `Unknown/ChecksNotImplemented` when the provider has no Checker + - `Unknown/NoTargets` when there are zero targets +- **Cleanup, once at `Start`:** delete labelled objects whose name isn't in the config. + The config is loaded once per process, and unlabelled objects are never touched. +- **Interval 0 (disabled):** run the cleanup with nothing kept, then return nil, so no + object is left showing a stale `Ready=True`. +- **Invalid provider names:** provider names aren't validated as DNS-1123 names. For an + invalid name, warn once and skip the object writes; metrics are still emitted. +- **`ValidateSchedule(interval, timeout)`** is called in `main.go` before the manager + starts. Interval must be 0 or at least 10s; timeout must be in (0, interval]. +- **RBAC markers on `Monitor`:** + - `providerhealths`: get, list, create, delete + - `providerhealths/status`: get, patch + - `make manifests` regenerates `config/rbac/role.yaml`. + +### Metrics and wiring + +- **New function:** `metrics.RegisterProviderTargets(reg, read func() []ProviderTarget) error`. + `Register`'s signature is left alone. +- **Gauges:** a scrape-time collector, in the style of `constCollector`, reads the Monitor + snapshot and emits three gauges labelled `{provider,target}`: + - `proxy_operator_provider_target_up` + - `proxy_operator_provider_check_duration_seconds` + - `proxy_operator_provider_check_timestamp_seconds` +- **Conversion closure:** the one that turns the Monitor snapshot into gauges lives in + `main.go`, next to `proxyPhaseCounts`. So `providerhealth` doesn't import `metrics`. +- **No pool-bytes gauge:** that value is in status, and node-exporter is the right source + for disk alerts. +- **`cmd/main.go`:** + - New flags `--provider-check-interval` (1m) and `--provider-check-timeout` (15s). + - Build the `Targets` from `cfg.Providers` in config order, with + `c, _ := raw[pc.Name].(provider.Checker)`. + - `mgr.Add(&providerhealth.Monitor{Client: mgr.GetClient(), Reader: mgr.GetAPIReader(), ...})`, + then register the collector. + +## Steps (one commit each, execution-log entry per step) + +**Step 0 — Save the plan.** +- Create branch `feat/provider-health` off `main`. +- Copy this plan to `docs/plans/2026-09-11-1526-provider-health-checks.md` and commit it + alone. +- The uncommitted `.claude/settings.json` change stays out of every commit. + +**Step 1 — ProviderHealth API and CRD.** +- `api/v1alpha1/providerhealth_types.go` +- `zz_generated.deepcopy.go`, from `make generate` +- `config/crd/bases/crawl.example.com_providerhealths.yaml`, from `make manifests` +- the CRD and RBAC `kustomization.yaml` files, plus the viewer role +- `PROJECT` + +**Step 2 — Contract plus the kubernetes and GCP checks.** +- `internal/provider/check.go` +- `internal/provider/kubernetes/{kubernetes.go,check.go,check_test.go}` +- `internal/provider/gcp/{gcp.go,check.go,check_test.go,gcp_test.go}` + +**Step 3 — KVM check.** +- `internal/provider/kvm/{conn.go,trace.go,check.go,check_test.go,fake_test.go,conn_test.go,trace_test.go,kvm_integration_test.go}` + +**Step 4 — Monitor.** +- `internal/providerhealth/{monitor.go,status.go,monitor_test.go,status_test.go,envtest_test.go}` +- `config/rbac/role.yaml`, from `make manifests` + +**Step 5 — Metrics and wiring.** +- `internal/metrics/{metrics.go,metrics_test.go}` +- `cmd/main.go` +- `config/manager/manager.yaml`: a comment for the new flags +- `test/e2e/e2e_test.go` + +**Step 6 — Docs.** +- `docs/architecture.md`: a component row plus a provider-health section +- `docs/kvm-provider.md`: the reason table, the shared-connection behaviour, and the + Secret-rotation gap under "Known gaps" +- `docs/testing.md`: the new tests and the commands +- `CLAUDE.md`: its Architecture and Gotchas sections still say "empty scaffold"; point + them at `docs/architecture.md` + +**Step 7 — Validate, record, push, open the PR.** +- Run `go vet ./...`, `go test -short ./...`, `make test` and `make test-e2e`. +- Hand the user the KVM commands below, and record their results in the execution log. +- Add a CHANGELOG entry once the user confirms. +- Push, using the token-file credential helper from memory. +- Open the PR against `main` through the Gitea API (`tea` isn't installed). +- Don't merge. + +## Tests + +**Provider checks** (table-driven, `Test_`): +- kubernetes: + - reachable + - list options: `Limit==1`, the label selector, no namespace + - failure reasons: forbidden, unauthorized, timeout, cancelled, internal, and a + network `*url.Error` +- gcp: + - reachable, asserting the request fields + - failure reasons: 401, 403, 500, deadline, network + - `TestRealInstances_Probe_fetchesOnePage` against `httptest`: exactly one request with + `maxResults=1`, even though the page has a `nextPageToken` +- kvm (using `hostFakes`/`newTestProvider` from `kvm_test.go`): + - reachable, with details filled in + - per-host verdicts, order preserved + - pool inactive, pool missing + - hosts checked concurrently (a sequential check would deadlock) + - a pool verdict keeps the connection (the dial count stays 1) + - a `checkReason` table + - `TestSSHAuth_dial_stages`, using the in-process SSH server in `conn_test.go` + - three new rows in the tracing table + +**Monitor:** +- Setup: the fake client is built with a status subresource and a RESTMapper that + registers ProviderHealth as cluster-scoped. +- Tests: + - `buildStatus` table + - objects are created with the label, and existing ones are patched + - the timeout bounds a checker that blocks + - a nil checker gives Unknown + - only transitions are logged (`funcr` logger) + - cleanup deletes only labelled objects that aren't configured + - the disabled path cleans up and returns + - an invalid name skips the writes + - `ValidateSchedule` table +- An envtest round-trip, skipped under `-short`, which proves the generated schema is + accepted by a real apiserver. + +**Metrics:** `TestRegisterProviderTargets_scrape`, using a fresh registry and +`GatherAndCompare`; an empty snapshot produces no series. + +**e2e (kind):** `providerhealth kubernetes` reaches `Ready=True`, which proves the +generated RBAC works end to end. + +## Verification + +**Automated (Claude runs):** `go vet ./...`, `go test -short ./...`, `make test` (envtest +with `-race`) and `make test-e2e`. + +**Against the real hypervisor (handed to the user, not run by Claude)**, with beelink as +the host: + +```bash +KVM_TEST_HOST=beelink.tail8c69c.ts.net:22 KVM_TEST_USER=novakj KVM_TEST_SSH_KEY=~/.ssh/id_ed25519 \ +KVM_TEST_KNOWN_HOSTS=~/.ssh/known_hosts KVM_TEST_BRIDGE= KVM_TEST_IMAGE_URL= \ + go test -tags=integration -count=1 -v -run TestKVMCheck ./internal/provider/kvm/ + +make install && go run ./cmd/main.go --providers-config \ + --metrics-bind-address :8080 --metrics-secure=false --provider-check-interval=15s +kubectl get ph -w +kubectl get ph kvm-home -o yaml +curl -s localhost:8080/metrics | grep -E 'proxy_operator_provider_(target_up|check_)' +``` + +Each negative case is a one-line edit to the scratch config; nothing changes on the host. + +| Edit | Expected reason | +|---|---| +| `pool: nope` | `PoolNotFound` | +| `address: …:2` | `Unreachable` | +| `knownHostsFile: /dev/null` or `user: nobody` | `SSHHandshakeFailed` | +| `sshKeyFile: /nonexistent` | `SSHMaterialUnavailable` | +| `socket: /nonexistent` | `LibvirtConnectFailed` | + +## Out of scope + +- Gating Proxy creates on a host that is known to be down. +- Tying `/readyz` to provider health. +- Bridge checks. +- SelfSubjectAccessReview checks of create/delete rights. +- Hot-reloading the providers config. +- Picking up a rotated `kvm-ssh` Secret without a restart. This is an existing gap; it + gets documented.