From 19b4a82ae9d1cf0feeea08e2bbe41e4793b854ed Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 20:28:09 +0000 Subject: [PATCH] Document the provider health checks Architecture gets a provider-health section, component rows and four decisions; the KVM provider doc gets a health-check section with the reason table and the rotated-Secret gap; the testing guide lists the new suites. CLAUDE.md's Architecture TODO and stale "only go.mod exists" gotcha now point at the real docs, plus the RBAC-marker gotcha found while wiring the monitor. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 19 +++-- docs/architecture.md | 85 ++++++++++++++++++- docs/kvm-provider.md | 54 +++++++++++- .../2026-09-11-1526-provider-health-checks.md | 33 ++++++- docs/testing.md | 24 +++++- 5 files changed, 197 insertions(+), 18 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 57e257e..08b04a9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,10 +25,11 @@ entrypoint yet, so there's nothing to run locally. ## Architecture -Not yet built. TODO: once the controller-runtime manager and reconcile loop exist, -document the data-flow diagram and package/role table here, following the standard -reconcile-loop shape: fetch → resolve → list related → compute desired state → merge -→ patch only on diff. +Documented in [docs/architecture.md](docs/architecture.md). It holds the component +table, the reconcile flow, and one section per runnable: health engine, discovery and +lease API, orphan GC, metrics, tracing, and provider health checks. Provider-specific +detail is in `docs/kvm-provider.md` and the GCP docs; the test layout is in +`docs/testing.md`. ## Conventions @@ -214,7 +215,9 @@ A lightweight `check` job (`go vet` / `go build` / `go test -short`) gates the b ## Gotchas -- This repo currently contains only `go.mod` (module - `gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator`) — no controller code, - `main.go`, Dockerfile, k8s manifests, Makefile, or CI config exist yet. Nothing in - this file describes working software; it's the convention scaffold to build against. +- The "What this is" and "Commands" sections above predate the code and are stale. + The Makefile targets (`make test`, `make manifests generate`, `make test-e2e`, + `make test-integration-kvm`) and `docs/testing.md` are authoritative. +- `+kubebuilder:rbac` markers are package-level. Keep them in their own comment block, + never inside a type's or function's doc comment: controller-gen silently ignores + them there. After `make manifests`, check `git diff config/rbac/role.yaml`. diff --git a/docs/architecture.md b/docs/architecture.md index 3f7daee..fc3fded 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -4,9 +4,9 @@ | Component | Package | Runs as | Leader-elected | Role | |---|---|---|---|---| -| Proxy CRD + helpers | `api/v1alpha1` | types | — | `Proxy` spec/status, CEL validation, defaulting, pure helpers | +| Proxy + ProviderHealth CRDs | `api/v1alpha1` | types | — | `Proxy` spec/status, CEL validation, defaulting, pure helpers; the status-only, cluster-scoped `ProviderHealth` | | Reconciler | `internal/controller` | controller | yes (with the manager) | the state machine: provision, replace, delete, represent health | -| Provider contract | `internal/provider` | library | — | `Provider` interface, error taxonomy, deterministic naming, config, metrics decorator | +| Provider contract | `internal/provider` | library | — | `Provider` interface, optional `Checker`, error taxonomy, deterministic naming, config, metrics decorator | | kubernetes provider | `internal/provider/kubernetes` | library | — | real Squid pods in this cluster (local dev/CI) | | gcp provider | `internal/provider/gcp` | library | — | Compute Engine VMs, four API calls, fire-and-forget ops | | kvm provider | `internal/provider/kvm` | library | — | VMs on libvirt hosts over SSH-forwarded sockets; image cache, overlay disks, NoCloud seeds | @@ -14,6 +14,7 @@ | Lease store | `internal/lease` | Runnable (expiry sweep) | no | in-memory leases + cooldowns, single mutex | | Discovery API | `internal/discovery` | Runnable | no | HTTP list/lease/release/report on `:8090` | | Orphan GC | `internal/gc` | Runnable | yes | deletes tagged instances whose CR is gone | +| Provider health monitor | `internal/providerhealth` | Runnable | yes | periodic provider checks → one `ProviderHealth` per provider, target gauges, transition logs | | Metrics | `internal/metrics` | library | — | explicit registration, scrape-time collectors | | Composition root | `cmd/main.go` | binary | — | flags, provider registry, wires everything onto one manager | @@ -306,6 +307,8 @@ registries). Two kinds: - **Scrape-time collectors** — `proxy_operator_proxies{phase}` and `proxy_operator_leases_active` read the cache / lease store at every scrape; reconcile-incremented gauges inevitably drift and leak series. + The provider-check gauges (§11) read the health monitor's snapshot the + same way. - **Fed vectors** — `healthcheck_duration_seconds{proxy}` and `healthcheck_failures_total{proxy}` observe EVERY probe (status writes are transition-only; metrics carry the high-frequency signal), and the @@ -348,7 +351,8 @@ incoming trace context): - Discovery API: one server span per request (named from the route pattern), incoming W3C `traceparent` honored so clients' traces continue into the operator; `/healthz` excluded. -- `gc.sweep` per GC pass; `health.probe` per probe **only** with +- `gc.sweep` per GC pass; `providerhealth.check` per round of provider + checks; `health.probe` per probe **only** with `--trace-health-probes` (default off — probes run ~1/s per proxy). Log correlation: logr sinks never see a context, so trace IDs ride on the @@ -359,6 +363,62 @@ Re-deriving from the base rather than layering keeps zap from emitting duplicate keys on nested spans. GCP V(5) wire logs get the same keys from the slog handler's ctx. +### 11. Provider health checks (`internal/providerhealth/`) + +Answers "can this cluster reach each provider's backend right now?" +without waiting for a Proxy to fail. A provider opts in by implementing +`provider.Checker`: `Check(ctx) []TargetStatus` returns one verdict per +target and never fails as a whole. All three providers do: + +| Provider | Target | What one check does | +|---|---|---| +| kvm | each `kvm.hosts` entry | over the pooled SSH+libvirt connection: libvirt version and hostname, then the configured pool must exist and be running; reports pool capacity and free space ([details](kvm-provider.md#health-check)) | +| kubernetes | `apiserver` | lists at most one managed Pod cluster-wide: the verb and scope `ListByTag` and GC rely on | +| gcp | the project | one page of an aggregated instance list with `maxResults=1`: credentials, Compute API enabled, `compute.instances.list` | + +```text +Monitor.Start(ctx) ── leader-only; --provider-check-interval (1m; 0 = off, + │ otherwise ≥ 10s), --provider-check-timeout (15s) + ├─ once: delete managed-by-labelled ProviderHealth objects whose provider + │ is not configured (all of them when checks are off) — the providers + │ config is read once per process + └─ at start, then every Interval: checkOnce + │ every provider's Check concurrently, under one context.WithTimeout + │ snapshot → the scrape-time gauges + │ log transitions: down or reason changed → "WARNING: provider target + │ not ready"; back up → "Provider target ready"; steady → silent + └ per provider: APIReader.Get → Create if missing → status merge patch +``` + +- **One status write per provider per round**, since `lastCheckTime` + moves every round. At the 10s minimum interval that is still far below + leader-election renewals. Proxy status avoids writes like this only + because its probes run once a second per proxy. +- **Reads go through the API reader**, not the cache. The objects are + cluster-scoped; a cached Get would start a cluster-wide informer (and + need `watch`) even with `--proxy-namespace`. +- **Checks bypass the provider decorators.** `cmd/main.go` collects the + Checkers before wrapping the providers in `WithTracing(WithMetrics(...))`, + so a check never counts as a provider request. KVM RPCs still appear in + the V(1) `libvirt call` trace, and each round is one + `providerhealth.check` span. +- **Not wired to `/readyz`.** A dead hypervisor must not make the manager + unready while the other providers keep working. +- **Gauges** `proxy_operator_provider_target_up`, + `proxy_operator_provider_check_duration_seconds` and + `proxy_operator_provider_check_timestamp_seconds`, each labelled + `{provider,target}`. Only the leader exports them. + +Illustrative output: + +```sh +$ kubectl get providerhealth # short name: ph +NAME TYPE READY TARGETS LAST CHECK AGE +kubernetes kubernetes True 1/1 12s 3d +kvm-home kvm False 0/1 12s 3d +$ kubectl get ph kvm-home -o yaml # per target: reason, message, details +``` + ## Decisions Judgment calls the spec left open, and deliberate deviations — recorded so @@ -528,3 +588,22 @@ they read as choices, not accidents. Chronological by build step. external sites. The kube-apiserver ignores incoming `traceparent` by design (public endpoint), so k8s client spans are leaves — in-process traces, not cross-process ones. +- **Provider health is a status-only, cluster-scoped CRD** (user + decision). `kubectl get providerhealth` works without Prometheus, which + the deployment does not ship (its ServiceMonitor is commented out). There + is no spec: the providers config file stays the one place providers are + configured, and the object only reports on it. The gauges carry the same + verdicts for alerting. +- **KVM checks reuse the pooled connection instead of dialing fresh.** A + fresh dial would not pick up a rotated Secret anyway, since the SSH + material is cached. It would also open an sshd session per host per + minute. And a working cached connection is exactly what reconciles use. + The cost is that a check against a hung host drops the shared + connection, the same rule every cancelled reconcile already follows. +- **Provider transitions are logs, not Events**, in line with the + EventRecorder decision above. They log at Info with a `WARNING:` prefix, + and each target's `lastTransitionTime` in the object records when it + flipped. +- **A provider name that is not a valid object name** gets metrics but no + `ProviderHealth` object, with a warning at start. Tightening config + validation instead would crash-loop existing deployments on upgrade. diff --git a/docs/kvm-provider.md b/docs/kvm-provider.md index f58e4e5..0f75c27 100644 --- a/docs/kvm-provider.md +++ b/docs/kvm-provider.md @@ -265,12 +265,60 @@ The provider logs through controller-runtime's logger, named `kvm`, with only `length` and `duration`. There is no raw wire tier like the GCP provider's V(5); go-libvirt exposes no hook for it. +## Health check + +The operator checks every configured host every `--provider-check-interval` +(default 1m). It publishes the result in two places: the `ProviderHealth` +object named after the provider (`kubectl get ph kvm-home -o yaml`), and the +`proxy_operator_provider_target_up{provider,target}` gauge. + +All hosts are checked concurrently, each over the same pooled connection +that Create and Get use: + +1. The SSH and libvirt handshakes. These only happen on the first check, or + after the link dropped. +2. `virConnectGetLibVersion` and `virConnectGetHostname`. +3. `virStoragePoolLookupByName` and `virStoragePoolGetInfo` on the host's + `pool`. + +A passing target's `details` carry `hostname`, `libvirtVersion`, `pool`, +`poolState`, `poolCapacity` and `poolAvailable` (in GiB). The free space is +the closest thing this provider has to a capacity signal. Every target also +shows its SSH `user`. A failing target says where the check broke: + +| Reason | Meaning | Look at | +|---|---|---| +| `SSHMaterialUnavailable` | the key or known_hosts file is missing or does not parse | the `kvm-ssh` Secret (see *Cluster-side setup*) | +| `Unreachable` | the TCP connect failed, or the link died mid-check | DNS, Tailscale or a firewall between the cluster and `address` | +| `SSHHandshakeFailed` | the host key is missing from known_hosts or has changed, or `user` rejected the key | the known_hosts entry; `user`'s `authorized_keys` | +| `LibvirtConnectFailed` | SSH worked, the libvirt socket did not | `user` in the `libvirt` group, `libvirtd` running, the `socket` path | +| `Timeout` | no answer within `--provider-check-timeout` (default 15s) | a hung `libvirtd` or a saturated link | +| `PoolNotFound` | there is no pool named `pool` | `virsh pool-list --all` | +| `PoolNotActive` | the pool exists but is not running | `virsh pool-start ` and `virsh pool-autostart ` | +| `LibvirtError` | any other libvirt error | the message carries libvirt's own text | + +Sharing the connection has two consequences: + +- The leader now keeps one SSH connection per host open from startup, with a + keepalive every 15s. Before, the first connection opened with the first KVM + reconcile. +- A check that hits its timeout drops the host's connection, just as a + cancelled reconcile already does. Calls in flight on a hung link then fail + fast, and the next call redials. + ## Known gaps - **No quota or capacity detection.** libvirt reports an out-of-space pool or an over-committed host as an ordinary error; the reconciler treats it as transient and retries with backoff rather than the slower - quota backoff. + quota backoff. The health check does report each pool's free space + (`poolAvailable`), so a pool that is filling up shows before Creates + start failing. +- **A rotated `kvm-ssh` Secret needs a manager restart.** The key and + known_hosts files are read on first use and cached for the life of the + process. Replacing them in the Secret therefore changes nothing for + later connections, health checks included. After a host key change, the + check keeps reporting `SSHHandshakeFailed` until the manager restarts. - **Orphan volumes without a domain are invisible to GC.** A crash between creating the volumes and defining the domain leaves an overlay and seed that `ListByTag` cannot see (it lists domains). They are @@ -315,4 +363,6 @@ Unit tests run over an in-memory fake of the libvirt API. The optional integration test against a real host is described in [testing.md](testing.md#the-kvm-integration-test-real-hypervisor); its `-v` output includes the `libvirt call` trace above, at the verbosity -`KVM_TEST_LOG_LEVEL` selects (default 1). +`KVM_TEST_LOG_LEVEL` selects (default 1). `TestKVMCheck_realHost` runs the +health check against the same host and creates nothing: +`go test -tags=integration -count=1 -v -run TestKVMCheck ./internal/provider/kvm/`. diff --git a/docs/plans-executions/2026-09-11-1526-provider-health-checks.md b/docs/plans-executions/2026-09-11-1526-provider-health-checks.md index c87761b..dc4adf2 100644 --- a/docs/plans-executions/2026-09-11-1526-provider-health-checks.md +++ b/docs/plans-executions/2026-09-11-1526-provider-health-checks.md @@ -7,7 +7,7 @@ Plan: [docs/plans/2026-09-11-1526-provider-health-checks.md](../plans/2026-09-11 - [x] Step 3 — KVM check - [x] Step 4 — Monitor - [x] Step 5 — Metrics and wiring -- [ ] Step 6 — Docs +- [x] Step 6 — Docs - [ ] Step 7 — Validate, record, push, open the PR ## Step 0 — Save the plan @@ -235,3 +235,34 @@ Worth noting: config changes. - **Manifest:** `config/manager/manager.yaml` only gained a comment about the two flags. The defaults need no args. + +## Step 6 — Docs + +- **`docs/architecture.md`:** + - component rows for the CRD, the `Checker` contract and the monitor; + - a new §11 covering the provider table, the loop diagram, the write, read and + decorator choices, the gauges, and sample `kubectl get ph` output; + - cross-references from the metrics (§9) and tracing (§10) sections; + - four Decisions entries. +- **`docs/kvm-provider.md`:** + - a "Health check" section with the reason → meaning → where-to-look table and the + two consequences of sharing the connection; + - under Known gaps, the capacity bullet now points at `poolAvailable`, and a new + bullet says a rotated `kvm-ssh` Secret needs a restart; + - the Testing section names `TestKVMCheck_realHost`. +- **`docs/testing.md`:** + - suite rows for the three `check_test.go` files, `internal/providerhealth` (unit and + envtest) and the metrics gauges; + - "17 libvirt calls" is now 20; + - the integration and e2e sections describe the new tests. +- **`CLAUDE.md`:** + - the Architecture TODO now points at `docs/architecture.md`; + - the stale "only `go.mod` exists" gotcha is replaced by a note that the "What this + is" and "Commands" sections predate the code, plus Step 4's RBAC-marker gotcha. + +Worth noting: +- `docs/api.md` is untouched. It documents the discovery HTTP API, which this change + doesn't alter. +- `CLAUDE.md`'s "What this is" and "Commands" sections were left stale on purpose. + Rewriting them is outside this plan, so the new gotcha flags them instead of leaving + them silently wrong. diff --git a/docs/testing.md b/docs/testing.md index c4d79d1..2eb97d6 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -53,18 +53,23 @@ manual runs too. | | `config_test.go` | providers-config parsing + fail-fast validation | pure | | | `metrics_test.go` | `WithMetrics` decorator: classified result labels, error passthrough | fake recorder + static provider | | `internal/provider/kubernetes` | `kubernetes_test.go`, `pod_test.go` | Create/Get/Delete/ListByTag over real Pod objects; pure `buildPod`/`squidConf` incl. the `max_filedescriptors` OOM-regression assertion | `client/fake` (real `corev1.Pod`s, no kubelet) | +| | `check_test.go` | health check: a single cluster-wide list with `Limit=1` and the managed-label selector; the reason for every API error class | `client/fake` + interceptor | | `internal/provider/gcp` | `insert_test.go`, `errors_test.go`, `gcp_test.go` | field-by-field `buildInsertRequest`; HTTP-code classification table; zone-qualified IDs, 409-is-success, 9-row state mapping, ListByTag filter + `ReturnPartialSuccess` | pure builder needs **no fake**; the rest uses the flattened `instancesAPI` seam | +| | `check_test.go` | health check: request shape and the reason per HTTP code or network error; the **real SDK client** makes exactly one request with `maxResults=1`, even when a next page exists | fake seam + `httptest` | | `internal/provider/kvm` | `domainxml_test.go`, `volume_test.go`, `seed_test.go`, `id_test.go`, `metadata_test.go` | pure builders: domain/volume XML unmarshalled back and checked field by field, seed ISO reopened with the same library, providerID round-trip, metadata in both namespace spellings | pure | | | `errors_test.go`, `conn_test.go` | libvirt-code classification table; connection pool (reuse, redial, drop on transport error, cancel releases a blocked RPC, one dial per host under concurrency); SSH dial, host-key policy, keepalive | fake dialer; a **real in-process `x/crypto/ssh` server** forwarding a unix socket | | | `image_test.go`, `kvm_test.go` | image cache (fresh fetch, ready, marker missing, checksum change/mismatch, 4xx/5xx, concurrent callers); Create resume after every partial step, leftover truncated seed, state/IP mapping, every Delete already-gone case, ListByTag across hosts / one down / unreadable domain; logging tiers (V0 silent, V1 one line per RPC, V2 XML) with cloud-init never logged | `httptest` image server + in-memory `fakeAPI` behind the `libvirtAPI` seam | | | `trace_test.go`, `conn_test.go`, `errors_test.go` | every `libvirtAPI` method traced exactly once (guarded by reflection on the interface), results and errors pass through, libvirt error fields, detail keys only at V2, upload content never logged; pool connect/drop lines, no wrapping below V1 | `fakeAPI` + `funcr` capture | -| | `kvm_integration_test.go` (`-tags=integration`) | one real VM: image fetch, Create, idempotent re-Create, Running with IP, ListByTag, Delete, pool state afterwards | a **real hypervisor** — see below | +| | `check_test.go`, `conn_test.go` | health check: details, per-host verdicts in config order, hosts checked concurrently, timeout, the reason table; an inactive or missing pool and daemon errors keep the connection; every dial stage tagged through the real SSH server, with classification unchanged | `fakeAPI`, fake dialers, in-process SSH server | +| | `kvm_integration_test.go` (`-tags=integration`) | one real VM: image fetch, Create, idempotent re-Create, Running with IP, ListByTag, Delete, pool state afterwards; `TestKVMCheck_realHost` runs the health check against the host and creates nothing | a **real hypervisor** — see below | | `internal/health` | `probe_test.go` | probes through a **real CONNECT-capable proxy stub** to a real TLS server: tunnel success, refused CONNECT, wrong status, dead proxy, plain-http forward | `httptest` + hijacked bidirectional tunnel | | | `engine_test.go` | thresholds, first-verdict, latency suppression matrix, dropped-event retry, stale-probe UID guard, tick scheduling/seeding/pruning, `Start` end-to-end | fake `client.Reader` + injected `probeFn` | | `internal/lease` | `store_test.go` | capacity, `MaxLeases=0`, all three selection tie-breaks, target-scoped vs global cooldowns, expiry via fake clock, report-on-retained-lease, idempotent release, **40 concurrent acquires never exceeding capacity** | injectable clock; the concurrency case is why `-race` matters | | `internal/discovery` | `server_test.go` | auth matrix (incl. `/healthz` exemption), list filtering, grant shape, full 409 arithmetic, invalid TTL/body/result, idempotent release, report→cooldown→409 round trip, `Start`/shutdown | `httptest` over the real handler chain, fake cache reader, **real** `lease.Store` | | `internal/gc` | `gc_test.go` | the true-orphan matrix (live/deleting/young/unlabelled all kept), broken-provider isolation, list-failure skips sweep, namespace guard, sweep loop | fake reader + canned-list provider | -| `internal/metrics` | `metrics_test.go` | scrape-time collectors (`GatherAndCompare`), per-proxy series cleanup via `ForgetProxy`, label counts, fresh-registry-per-test property | `prometheus/client_golang/testutil` | +| `internal/providerhealth` | `status_test.go`, `monitor_test.go` | `buildStatus` (the Ready condition, per-target transition times, truncation); objects created with the label and patched, the round timeout, no-checker Unknown, invalid names, transition-only logging, cleanup of labelled unconfigured objects, the checks-off path, `ValidateSchedule` | fake client with a cluster-scoped RESTMapper + `funcr` capture | +| | `envtest_test.go` | the generated `ProviderHealth` schema against a real API server: cluster scope, status subresource, required fields, list-map keys, the 1024-character message limit | envtest (skipped under `-short`) | +| `internal/metrics` | `metrics_test.go` | scrape-time collectors (`GatherAndCompare`) incl. the provider-target gauges, per-proxy series cleanup via `ForgetProxy`, label counts, fresh-registry-per-test property | `prometheus/client_golang/testutil` | Conventions (from `CLAUDE.md`): stdlib `testing`, table-driven by default, `t.Parallel()` where safe, tests next to source, ginkgo only in the @@ -89,7 +94,7 @@ envtest suite the scaffold generated. - **Live GCP**: no test talks to Google. The seam boundary (`buildInsertRequest` + classification) is tested exhaustively instead. - **Live libvirt, by default**: the KVM unit tests run over an in-memory - fake of the 17 libvirt calls the provider uses. Whether libvirt accepts + fake of the 20 libvirt calls the provider uses. Whether libvirt accepts the domain XML, whether a raw qcow2 upload plus pool refresh yields a usable backing image, and whether the guest actually boots and reports an address are provable only on a real host — that is the opt-in @@ -139,13 +144,24 @@ but no per-VM overlay or seed. It also tries a TCP connect to port 22 on the guest, but only logs the outcome — the machine running the test may not route to the bridge network. +`TestKVMCheck_realHost` runs the provider health check +([kvm-provider.md](kvm-provider.md#health-check)) against the same host. +It must find the host reachable and the pool running, and it creates +nothing. It needs the same variables; to run only this test: + +```sh +go test -tags=integration -count=1 -v -run TestKVMCheck ./internal/provider/kvm/ +``` + The test is not part of `make test` and not run in CI. Keep it compiling with `go vet -tags=integration ./...` when the provider's internals move. ## The scaffolded `make test-e2e` suite `test/e2e/` is the kubebuilder-generated smoke suite (build image → kind -cluster → deploy → assert the manager pod runs and serves metrics). It +cluster → deploy → assert the manager pod runs, publishes a Ready +`ProviderHealth` for the `kubernetes` provider, and serves metrics that +include that provider's `proxy_operator_provider_target_up` series). It compiles only under `-tags=e2e`, manages its own kind cluster (`make test-e2e` / `make cleanup-test-e2e`), and has been kept compiling (`go vet -tags=e2e ./...` is part of the routine) but is **not part of