From 18a3378507ed82e71e0d002d2e0743afddd24d5d Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 18:08:59 +0000 Subject: [PATCH 01/12] 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. -- 2.54.0 From 1c773c24ad85c271e26fb3f0c562b8834b338650 Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 18:30:31 +0000 Subject: [PATCH 02/12] Add the cluster-scoped ProviderHealth CRD One operator-owned object per configured provider will carry the periodic check result: a Ready condition, per-target verdicts with their own transition times, and a ready/total count for kubectl get. There is no spec; only a viewer role is shipped since the operator is the sole writer. Written by hand (kubebuilder is not installed), registered in the existing AddKnownTypes call rather than a second init(). Co-Authored-By: Claude Opus 5 (1M context) --- PROJECT | 7 + api/v1alpha1/providerhealth_types.go | 127 ++++++++++++ api/v1alpha1/proxy_types.go | 2 +- api/v1alpha1/zz_generated.deepcopy.go | 114 +++++++++++ .../crawl.example.com_providerhealths.yaml | 193 ++++++++++++++++++ config/crd/kustomization.yaml | 1 + config/rbac/kustomization.yaml | 3 + config/rbac/providerhealth_viewer_role.yaml | 29 +++ .../2026-09-11-1526-provider-health-checks.md | 60 ++++++ 9 files changed, 535 insertions(+), 1 deletion(-) create mode 100644 api/v1alpha1/providerhealth_types.go create mode 100644 config/crd/bases/crawl.example.com_providerhealths.yaml create mode 100644 config/rbac/providerhealth_viewer_role.yaml create mode 100644 docs/plans-executions/2026-09-11-1526-provider-health-checks.md diff --git a/PROJECT b/PROJECT index 721151d..e176257 100644 --- a/PROJECT +++ b/PROJECT @@ -18,4 +18,11 @@ resources: kind: Proxy path: gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1 version: v1alpha1 +- api: + crdVersion: v1 + domain: example.com + group: crawl + kind: ProviderHealth + path: gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1 + version: v1alpha1 version: "3" diff --git a/api/v1alpha1/providerhealth_types.go b/api/v1alpha1/providerhealth_types.go new file mode 100644 index 0000000..329463f --- /dev/null +++ b/api/v1alpha1/providerhealth_types.go @@ -0,0 +1,127 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// ConditionReady summarizes a ProviderHealth's targets: True when every +// target passed its last check. +const ConditionReady = "Ready" + +// ProviderTarget is the latest check verdict for one thing a provider +// depends on: a KVM hypervisor, the Kubernetes API server, a GCP project. +type ProviderTarget struct { + // name identifies the target within its provider: the KVM host name, + // "apiserver", or the GCP project. + // +kubebuilder:validation:MinLength=1 + // +required + Name string `json:"name"` + + // address is where the check connected to. + // +optional + Address string `json:"address,omitempty"` + + // ready is true when the last check succeeded. + // +required + Ready bool `json:"ready"` + + // reason is a CamelCase verdict such as Reachable, Unreachable, + // Forbidden, SSHHandshakeFailed or PoolNotActive. + // +optional + Reason string `json:"reason,omitempty"` + + // message is human-readable detail, usually the error of a failed check. + // +kubebuilder:validation:MaxLength=1024 + // +optional + Message string `json:"message,omitempty"` + + // latencyMillis is how long the last check took. + // +optional + LatencyMillis int64 `json:"latencyMillis,omitempty"` + + // lastTransitionTime is when ready last changed. + // +required + LastTransitionTime metav1.Time `json:"lastTransitionTime"` + + // details are provider-specific facts from the last check, e.g. the + // libvirt version and free storage-pool space of a KVM host. + // +optional + Details map[string]string `json:"details,omitempty"` +} + +// ProviderHealthStatus is the result of the latest check of one provider. +type ProviderHealthStatus struct { + // type is the provider's type from the providers config: kubernetes, + // gcp or kvm. + // +optional + Type string `json:"type,omitempty"` + + // conditions hold Ready, which summarizes the targets. + // +listType=map + // +listMapKey=type + // +optional + Conditions []metav1.Condition `json:"conditions,omitempty"` + + // targets hold one verdict per checked target. + // +listType=map + // +listMapKey=name + // +optional + Targets []ProviderTarget `json:"targets,omitempty"` + + // targetsReady counts the ready targets as "ready/total". + // +optional + TargetsReady string `json:"targetsReady,omitempty"` + + // lastCheckTime is when the latest check finished. + // +optional + LastCheckTime *metav1.Time `json:"lastCheckTime,omitempty"` +} + +// +kubebuilder:object:root=true +// +kubebuilder:subresource:status +// +kubebuilder:resource:scope=Cluster,shortName=ph +// +kubebuilder:printcolumn:name="Type",type=string,JSONPath=`.status.type` +// +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status` +// +kubebuilder:printcolumn:name="Targets",type=string,JSONPath=`.status.targetsReady` +// +kubebuilder:printcolumn:name="Last Check",type=date,JSONPath=`.status.lastCheckTime` +// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` + +// ProviderHealth reports the periodic health check of one configured +// provider. It is named after the provider, and the operator creates, +// updates and deletes it itself; there is no spec to edit. +type ProviderHealth struct { + metav1.TypeMeta `json:",inline"` + + // metadata is a standard object metadata + // +optional + metav1.ObjectMeta `json:"metadata,omitzero"` + + // status is the result of the latest check + // +optional + Status ProviderHealthStatus `json:"status,omitzero"` +} + +// +kubebuilder:object:root=true + +// ProviderHealthList contains a list of ProviderHealth +type ProviderHealthList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitzero"` + Items []ProviderHealth `json:"items"` +} diff --git a/api/v1alpha1/proxy_types.go b/api/v1alpha1/proxy_types.go index bd17201..e7419d8 100644 --- a/api/v1alpha1/proxy_types.go +++ b/api/v1alpha1/proxy_types.go @@ -361,7 +361,7 @@ type ProxyList struct { func init() { SchemeBuilder.Register(func(s *runtime.Scheme) error { - s.AddKnownTypes(SchemeGroupVersion, &Proxy{}, &ProxyList{}) + s.AddKnownTypes(SchemeGroupVersion, &Proxy{}, &ProxyList{}, &ProviderHealth{}, &ProviderHealthList{}) return nil }) } diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index da378cb..e6d3b09 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -95,6 +95,120 @@ func (in *PlacementSpec) DeepCopy() *PlacementSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ProviderHealth) DeepCopyInto(out *ProviderHealth) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) + in.Status.DeepCopyInto(&out.Status) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProviderHealth. +func (in *ProviderHealth) DeepCopy() *ProviderHealth { + if in == nil { + return nil + } + out := new(ProviderHealth) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *ProviderHealth) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ProviderHealthList) DeepCopyInto(out *ProviderHealthList) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ListMeta.DeepCopyInto(&out.ListMeta) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]ProviderHealth, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProviderHealthList. +func (in *ProviderHealthList) DeepCopy() *ProviderHealthList { + if in == nil { + return nil + } + out := new(ProviderHealthList) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *ProviderHealthList) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ProviderHealthStatus) DeepCopyInto(out *ProviderHealthStatus) { + *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]v1.Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.Targets != nil { + in, out := &in.Targets, &out.Targets + *out = make([]ProviderTarget, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.LastCheckTime != nil { + in, out := &in.LastCheckTime, &out.LastCheckTime + *out = (*in).DeepCopy() + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProviderHealthStatus. +func (in *ProviderHealthStatus) DeepCopy() *ProviderHealthStatus { + if in == nil { + return nil + } + out := new(ProviderHealthStatus) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ProviderTarget) DeepCopyInto(out *ProviderTarget) { + *out = *in + in.LastTransitionTime.DeepCopyInto(&out.LastTransitionTime) + if in.Details != nil { + in, out := &in.Details, &out.Details + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProviderTarget. +func (in *ProviderTarget) DeepCopy() *ProviderTarget { + if in == nil { + return nil + } + out := new(ProviderTarget) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Proxy) DeepCopyInto(out *Proxy) { *out = *in diff --git a/config/crd/bases/crawl.example.com_providerhealths.yaml b/config/crd/bases/crawl.example.com_providerhealths.yaml new file mode 100644 index 0000000..838d3f8 --- /dev/null +++ b/config/crd/bases/crawl.example.com_providerhealths.yaml @@ -0,0 +1,193 @@ +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.21.0 + name: providerhealths.crawl.example.com +spec: + group: crawl.example.com + names: + kind: ProviderHealth + listKind: ProviderHealthList + plural: providerhealths + shortNames: + - ph + singular: providerhealth + scope: Cluster + versions: + - additionalPrinterColumns: + - jsonPath: .status.type + name: Type + type: string + - jsonPath: .status.conditions[?(@.type=="Ready")].status + name: Ready + type: string + - jsonPath: .status.targetsReady + name: Targets + type: string + - jsonPath: .status.lastCheckTime + name: Last Check + type: date + - jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 + schema: + openAPIV3Schema: + description: |- + ProviderHealth reports the periodic health check of one configured + provider. It is named after the provider, and the operator creates, + updates and deletes it itself; there is no spec to edit. + properties: + apiVersion: + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources + type: string + kind: + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string + metadata: + type: object + status: + description: status is the result of the latest check + properties: + conditions: + description: conditions hold Ready, which summarizes the targets. + items: + description: Condition contains details for one aspect of the current + state of this API Resource. + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map + lastCheckTime: + description: lastCheckTime is when the latest check finished. + format: date-time + type: string + targets: + description: targets hold one verdict per checked target. + items: + description: |- + ProviderTarget is the latest check verdict for one thing a provider + depends on: a KVM hypervisor, the Kubernetes API server, a GCP project. + properties: + address: + description: address is where the check connected to. + type: string + details: + additionalProperties: + type: string + description: |- + details are provider-specific facts from the last check, e.g. the + libvirt version and free storage-pool space of a KVM host. + type: object + lastTransitionTime: + description: lastTransitionTime is when ready last changed. + format: date-time + type: string + latencyMillis: + description: latencyMillis is how long the last check took. + format: int64 + type: integer + message: + description: message is human-readable detail, usually the error + of a failed check. + maxLength: 1024 + type: string + name: + description: |- + name identifies the target within its provider: the KVM host name, + "apiserver", or the GCP project. + minLength: 1 + type: string + ready: + description: ready is true when the last check succeeded. + type: boolean + reason: + description: |- + reason is a CamelCase verdict such as Reachable, Unreachable, + Forbidden, SSHHandshakeFailed or PoolNotActive. + type: string + required: + - lastTransitionTime + - name + - ready + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + targetsReady: + description: targetsReady counts the ready targets as "ready/total". + type: string + type: + description: |- + type is the provider's type from the providers config: kubernetes, + gcp or kvm. + type: string + type: object + type: object + served: true + storage: true + subresources: + status: {} diff --git a/config/crd/kustomization.yaml b/config/crd/kustomization.yaml index 7ab3951..7204749 100644 --- a/config/crd/kustomization.yaml +++ b/config/crd/kustomization.yaml @@ -3,4 +3,5 @@ # It should be run by config/default resources: - bases/crawl.example.com_proxies.yaml +- bases/crawl.example.com_providerhealths.yaml # +kubebuilder:scaffold:crdkustomizeresource diff --git a/config/rbac/kustomization.yaml b/config/rbac/kustomization.yaml index 9ff20da..5a2908a 100644 --- a/config/rbac/kustomization.yaml +++ b/config/rbac/kustomization.yaml @@ -25,4 +25,7 @@ resources: - proxy_admin_role.yaml - proxy_editor_role.yaml - proxy_viewer_role.yaml +# ProviderHealth objects are written only by the operator, so they get a +# viewer role and nothing else. +- providerhealth_viewer_role.yaml diff --git a/config/rbac/providerhealth_viewer_role.yaml b/config/rbac/providerhealth_viewer_role.yaml new file mode 100644 index 0000000..43978ae --- /dev/null +++ b/config/rbac/providerhealth_viewer_role.yaml @@ -0,0 +1,29 @@ +# This rule is not used by the project egress-proxies-operator itself. +# It is provided to allow the cluster admin to help manage permissions for users. +# +# Grants read-only access to ProviderHealth objects, the results of the +# operator's periodic provider checks. The operator creates, updates and +# deletes them itself, so no admin or editor role is provided. + +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + labels: + app.kubernetes.io/name: egress-proxies-operator + app.kubernetes.io/managed-by: kustomize + name: providerhealth-viewer-role +rules: +- apiGroups: + - crawl.example.com + resources: + - providerhealths + verbs: + - get + - list + - watch +- apiGroups: + - crawl.example.com + resources: + - providerhealths/status + verbs: + - get 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 new file mode 100644 index 0000000..67362fa --- /dev/null +++ b/docs/plans-executions/2026-09-11-1526-provider-health-checks.md @@ -0,0 +1,60 @@ +# Execution: provider health checks +Plan: [docs/plans/2026-09-11-1526-provider-health-checks.md](../plans/2026-09-11-1526-provider-health-checks.md) + +- [x] Step 0 — Save the plan +- [x] Step 1 — ProviderHealth API and CRD +- [ ] Step 2 — Contract plus the kubernetes and GCP checks +- [ ] Step 3 — KVM check +- [ ] Step 4 — Monitor +- [ ] Step 5 — Metrics and wiring +- [ ] Step 6 — Docs +- [ ] Step 7 — Validate, record, push, open the PR + +## Step 0 — Save the plan + +Branched off `main` (at the PR #5 merge, `eb8f075`). The plan was copied out of +`~/.claude/plans/` and committed on its own, before any code, as the repo's plan rules +require: + +```bash +git checkout -b feat/provider-health main +cp ~/.claude/plans/now-i-would-like-flickering-wall.md docs/plans/2026-09-11-1526-provider-health-checks.md +``` + +The uncommitted `.claude/settings.json` change was left unstaged and stays out of this +branch. + +## Step 1 — ProviderHealth API and CRD + +`kubebuilder` isn't installed here (only `bin/controller-gen`, `bin/kustomize` and +`bin/setup-envtest` are), so `api/v1alpha1/providerhealth_types.go` and the `PROJECT` +entry were written by hand instead of with `kubebuilder create api`. For a cluster-scoped +resource `PROJECT` simply omits `namespaced`, which is what kubebuilder itself writes. + +The new types are registered in the existing `AddKnownTypes` call in `proxy_types.go`, +not in a second `init()`. + +The deepcopy code, the CRD and the RBAC come from the usual generators: + +```bash +make generate manifests +go build ./... && go vet ./api/... && go test -short ./api/... +``` + +To confirm the new CRD and the viewer role actually reach the rendered install, I built +the kustomization and grepped for them: + +```bash +bin/kustomize build config/default | grep -n providerhealth +# providerhealths.crawl.example.com (scope: Cluster, shortName ph) +# egress-proxies-operator-providerhealth-viewer-role +``` + +Worth noting: +- The only role added is the viewer role. The operator is the object's only writer, so + the admin and editor roles kubebuilder scaffolds for Proxy were not added. +- The type file has no unit test, since it has no logic. The schema, including the + required `ready`/`lastTransitionTime` and the list-map keys, is proven by the envtest + round-trip in Step 4. +- `config/rbac/role.yaml` is unchanged so far. The manager's own permissions arrive in + Step 4 with the Monitor's RBAC markers. -- 2.54.0 From 1962365f447f54c24124edccb9e37aa0aaef677e Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 20:08:21 +0000 Subject: [PATCH 03/12] Add the provider Checker contract and the kubernetes and GCP checks Checker is an optional interface next to Provider: Check returns one verdict per target and never fails as a whole. The kubernetes provider lists at most one managed Pod cluster-wide (the verb and scope GC relies on); the GCP provider fetches a single page of at most one managed instance through a new Probe seam method, since the SDK treats MaxResults as a page size and the existing AggregatedList drains every page. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-11-1526-provider-health-checks.md | 39 ++++- internal/provider/check.go | 46 +++++ internal/provider/gcp/check.go | 78 +++++++++ internal/provider/gcp/check_test.go | 165 ++++++++++++++++++ internal/provider/gcp/gcp.go | 7 +- internal/provider/gcp/gcp_test.go | 8 + internal/provider/kubernetes/check.go | 53 ++++++ internal/provider/kubernetes/check_test.go | 114 ++++++++++++ internal/provider/kubernetes/kubernetes.go | 6 +- 9 files changed, 511 insertions(+), 5 deletions(-) create mode 100644 internal/provider/check.go create mode 100644 internal/provider/gcp/check.go create mode 100644 internal/provider/gcp/check_test.go create mode 100644 internal/provider/kubernetes/check.go create mode 100644 internal/provider/kubernetes/check_test.go 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 67362fa..9ba5a38 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 @@ -3,7 +3,7 @@ Plan: [docs/plans/2026-09-11-1526-provider-health-checks.md](../plans/2026-09-11 - [x] Step 0 — Save the plan - [x] Step 1 — ProviderHealth API and CRD -- [ ] Step 2 — Contract plus the kubernetes and GCP checks +- [x] Step 2 — Contract plus the kubernetes and GCP checks - [ ] Step 3 — KVM check - [ ] Step 4 — Monitor - [ ] Step 5 — Metrics and wiring @@ -58,3 +58,40 @@ Worth noting: round-trip in Step 4. - `config/rbac/role.yaml` is unchanged so far. The manager's own permissions arrive in Step 4 with the Monitor's RBAC markers. + +## Step 2 — Contract plus the kubernetes and GCP checks + +`provider.Checker` is an optional interface in `internal/provider/check.go`, kept out of +`Provider`. Each implementation has a compile-time assertion, because the monitor's +type assertion would otherwise fail silently. + +- **kubernetes:** lists at most one managed Pod cluster-wide, using the same client, verb + and scope as `ListByTag`. The target is `apiserver`, at the `restCfg.Host` now stored + in the new `apiHost` field. +- **GCP:** calls a new seam method, `Probe`. `realInstances.Probe` lives in `check.go`, + next to its only caller. `ListByTag`'s `Sprintf` filter became the shared + `managedFilter` constant, so the check and the GC sweep can't drift apart. + +Before relying on "MaxResults is only a page size", I checked the SDK's REST +`AggregatedList` in the module cache: + +```bash +grep -n "func (c \*instancesRESTClient) AggregatedList" -A80 \ + "$(go list -m -f '{{.Dir}}' cloud.google.com/go/compute)"/apiv1/instances_client.go +# InternalFetch: req.MaxResults = pageSize ... return elems, resp.GetNextPageToken(), nil +``` + +```bash +go build ./... && go vet ./internal/provider/... && go test -race -count=1 ./internal/provider/... +``` + +Worth noting: +- The planned `TestRealInstances_Probe_fetchesOnePage` became a three-case + `TestRealInstances_Probe` table. + - It keeps the first page's `nextPageToken`, and asserts exactly one request with + `maxResults=1` for each case. + - "empty project" covers `iterator.Done`. + - "api error keeps its status" proves the real client returns a `*googleapi.Error`, so + a disabled Compute API is reported as `Forbidden` rather than `Unreachable`. +- Once a context has ended, both checks report `Timeout` whatever the error says. The + monitor's timeout is the usual cause, and the underlying error only repeats it. diff --git a/internal/provider/check.go b/internal/provider/check.go new file mode 100644 index 0000000..b2fe4d3 --- /dev/null +++ b/internal/provider/check.go @@ -0,0 +1,46 @@ +package provider + +import ( + "context" + "time" +) + +// Reasons shared by every Checker, so a listing reads the same across +// providers. Providers add their own for failures only they can tell +// apart (KVM's SSHHandshakeFailed or PoolNotActive, say). +const ( + ReasonReachable = "Reachable" + ReasonTimeout = "Timeout" + ReasonUnreachable = "Unreachable" + ReasonUnauthorized = "Unauthorized" + ReasonForbidden = "Forbidden" + ReasonAPIError = "APIError" +) + +// TargetStatus is one target's verdict from a Checker. +type TargetStatus struct { + // Name identifies the target within its provider: a KVM host name, + // "apiserver", a GCP project. + Name string + // Address is what the check connected to, for humans. + Address string + Ready bool + // Reason is ReasonReachable when Ready, otherwise why not. + Reason string + // Message is human-readable detail, usually the error. + Message string + Latency time.Duration + // Details are provider-specific facts gathered by the check. + Details map[string]string +} + +// Checker is the optional interface of a provider that can verify its +// backend is reachable and usable without touching any instance. The +// provider health monitor type-asserts for it on the unwrapped providers, +// so it stays out of the Provider contract. +type Checker interface { + // Check returns one verdict per target, in a stable order. It never + // fails as a whole — a failure is a verdict — and must return promptly + // once ctx ends. + Check(ctx context.Context) []TargetStatus +} diff --git a/internal/provider/gcp/check.go b/internal/provider/gcp/check.go new file mode 100644 index 0000000..3a5f976 --- /dev/null +++ b/internal/provider/gcp/check.go @@ -0,0 +1,78 @@ +package gcp + +import ( + "context" + "errors" + "net/http" + "time" + + "cloud.google.com/go/compute/apiv1/computepb" + "google.golang.org/api/googleapi" + "google.golang.org/api/iterator" + "google.golang.org/protobuf/proto" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +var _ provider.Checker = (*Provider)(nil) + +// computeAddress is the endpoint the SDK talks to, shown only to humans. +const computeAddress = "compute.googleapis.com" + +// managedFilter selects the instances this operator created. +const managedFilter = "labels." + provider.LabelManaged + " = " + provider.LabelManagedYes + +// Check asks the project for one page holding at most one managed +// instance. That exercises the credentials, the Compute API and the +// compute.instances.list permission orphan GC depends on, in a single +// request whatever the project's size. The target is the project: the +// provider config names no zones. +func (p *Provider) Check(ctx context.Context) []provider.TargetStatus { + start := time.Now() + err := p.api.Probe(ctx, &computepb.AggregatedListInstancesRequest{ + Project: p.cfg.Project, + Filter: proto.String(managedFilter), + MaxResults: proto.Uint32(1), + ReturnPartialSuccess: proto.Bool(true), + }) + ts := provider.TargetStatus{Name: p.cfg.Project, Address: computeAddress, Latency: time.Since(start)} + if err != nil { + logAPIError(p.logger(ctx), "check", err) + ts.Reason = checkReason(ctx, err) + ts.Message = err.Error() + return []provider.TargetStatus{ts} + } + ts.Ready = true + ts.Reason = provider.ReasonReachable + return []provider.TargetStatus{ts} +} + +// Probe fetches the first page of an aggregated list and discards it. The +// SDK treats MaxResults as the page size, so AggregatedList, which drains +// the iterator, would still walk every page of the project. +func (r *realInstances) Probe(ctx context.Context, req *computepb.AggregatedListInstancesRequest) error { + _, err := r.client.AggregatedList(ctx, req).Next() + if err == iterator.Done { + return nil + } + return err +} + +// checkReason maps a failed probe to a reason. A token that cannot be +// obtained never reaches the API, so it has no status code and lands on +// Unreachable, with the detail in the message. +func checkReason(ctx context.Context, err error) string { + var gerr *googleapi.Error + switch { + case ctx.Err() != nil, errors.Is(err, context.DeadlineExceeded): + return provider.ReasonTimeout + case !errors.As(err, &gerr): + return provider.ReasonUnreachable + case gerr.Code == http.StatusUnauthorized: + return provider.ReasonUnauthorized + case gerr.Code == http.StatusForbidden: + return provider.ReasonForbidden + default: + return provider.ReasonAPIError + } +} diff --git a/internal/provider/gcp/check_test.go b/internal/provider/gcp/check_test.go new file mode 100644 index 0000000..faae156 --- /dev/null +++ b/internal/provider/gcp/check_test.go @@ -0,0 +1,165 @@ +package gcp + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "sync" + "syscall" + "testing" + + compute "cloud.google.com/go/compute/apiv1" + "cloud.google.com/go/compute/apiv1/computepb" + "google.golang.org/api/googleapi" + "google.golang.org/api/option" + "google.golang.org/protobuf/proto" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +func TestCheck_reachable(t *testing.T) { + t.Parallel() + api := &fakeAPI{} + p := newTestProvider(api) + + got := p.Check(context.Background()) + + if len(got) != 1 { + t.Fatalf("Check() returned %d targets, want 1", len(got)) + } + ts := got[0] + if !ts.Ready || ts.Reason != provider.ReasonReachable || ts.Message != "" { + t.Errorf("Check() = %+v, want ready and Reachable", ts) + } + if ts.Name != "my-project" || ts.Address != "compute.googleapis.com" { + t.Errorf("target = %q at %q, want my-project at compute.googleapis.com", ts.Name, ts.Address) + } + want := &computepb.AggregatedListInstancesRequest{ + Project: "my-project", + Filter: proto.String("labels.proxy-operator-managed = true"), + MaxResults: proto.Uint32(1), + ReturnPartialSuccess: proto.Bool(true), + } + if !proto.Equal(api.probeReq, want) { + t.Errorf("probe request = %v, want %v", api.probeReq, want) + } +} + +func TestCheck_failureReasons(t *testing.T) { + t.Parallel() + ended, cancel := context.WithCancel(context.Background()) + cancel() + + tests := []struct { + name string + ctx context.Context + err error + want string + }{ + {"unauthorized", context.Background(), gerr(401), provider.ReasonUnauthorized}, + {"forbidden", context.Background(), gerr(403, "forbidden"), provider.ReasonForbidden}, + {"api disabled", context.Background(), gerr(403, "accessNotConfigured"), provider.ReasonForbidden}, + {"server error", context.Background(), gerr(500), provider.ReasonAPIError}, + {"deadline", context.Background(), &url.Error{ + Op: "Get", URL: "https://compute.googleapis.com/", Err: context.DeadlineExceeded, + }, provider.ReasonTimeout}, + {"context ended", ended, context.Canceled, provider.ReasonTimeout}, + {"connection refused", context.Background(), &url.Error{ + Op: "Get", URL: "https://compute.googleapis.com/", Err: syscall.ECONNREFUSED, + }, provider.ReasonUnreachable}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + p := newTestProvider(&fakeAPI{probeErr: tc.err}) + + got := p.Check(tc.ctx) + + if len(got) != 1 { + t.Fatalf("Check() returned %d targets, want 1", len(got)) + } + if got[0].Ready || got[0].Reason != tc.want || got[0].Message != tc.err.Error() { + t.Errorf("Check() = %+v, want not ready, reason %s, message %q", got[0], tc.want, tc.err.Error()) + } + }) + } +} + +// TestRealInstances_Probe runs the real SDK client against a local server, +// since the point of Probe is how many requests the SDK makes. +func TestRealInstances_Probe(t *testing.T) { + t.Parallel() + tests := []struct { + name string + status int + body string + wantCode int + }{ + { + name: "fetches only the first page", + status: http.StatusOK, + body: `{"items":{"zones/europe-west1-b":{"instances":[{"name":"proxy-a"}]}},"nextPageToken":"page-2"}`, + }, + { + name: "empty project", + status: http.StatusOK, + body: `{}`, + }, + { + name: "api error keeps its status", + status: http.StatusForbidden, + body: `{"error":{"code":403,"message":"Compute Engine API has not been used","errors":[{"reason":"accessNotConfigured"}]}}`, + wantCode: http.StatusForbidden, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var ( + mu sync.Mutex + reqs []*url.URL + ) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + reqs = append(reqs, r.URL) + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tc.status) + _, _ = w.Write([]byte(tc.body)) + })) + t.Cleanup(srv.Close) + ctx := context.Background() + c, err := compute.NewInstancesRESTClient(ctx, option.WithEndpoint(srv.URL), option.WithoutAuthentication()) + if err != nil { + t.Fatalf("NewInstancesRESTClient: %v", err) + } + t.Cleanup(func() { _ = c.Close() }) + + err = (&realInstances{client: c}).Probe(ctx, &computepb.AggregatedListInstancesRequest{ + Project: "my-project", + MaxResults: proto.Uint32(1), + }) + + var gerr *googleapi.Error + switch { + case tc.wantCode == 0 && err != nil: + t.Fatalf("Probe() = %v, want nil", err) + case tc.wantCode != 0 && (!errors.As(err, &gerr) || gerr.Code != tc.wantCode): + t.Fatalf("Probe() = %v, want a googleapi error with code %d", err, tc.wantCode) + } + mu.Lock() + defer mu.Unlock() + if len(reqs) != 1 { + t.Fatalf("server saw %d requests, want 1", len(reqs)) + } + if got := reqs[0].Query().Get("maxResults"); got != "1" { + t.Errorf("maxResults = %q, want 1", got) + } + if want := "/compute/v1/projects/my-project/aggregated/instances"; reqs[0].Path != want { + t.Errorf("path = %q, want %q", reqs[0].Path, want) + } + }) + } +} diff --git a/internal/provider/gcp/gcp.go b/internal/provider/gcp/gcp.go index a951f0b..57b8acf 100644 --- a/internal/provider/gcp/gcp.go +++ b/internal/provider/gcp/gcp.go @@ -33,6 +33,8 @@ type instancesAPI interface { Get(ctx context.Context, req *computepb.GetInstanceRequest) (*computepb.Instance, error) Delete(ctx context.Context, req *computepb.DeleteInstanceRequest) (opName string, err error) AggregatedList(ctx context.Context, req *computepb.AggregatedListInstancesRequest) ([]*computepb.Instance, error) + // Probe fetches only the first page of an aggregated list; see Check. + Probe(ctx context.Context, req *computepb.AggregatedListInstancesRequest) error } // realInstances adapts *compute.InstancesClient to the seam. @@ -221,17 +223,16 @@ func (p *Provider) Delete(ctx context.Context, providerID string) error { // entire GC sweep. func (p *Provider) ListByTag(ctx context.Context) ([]provider.Instance, error) { log := p.logger(ctx) - filter := fmt.Sprintf("labels.%s = %s", provider.LabelManaged, provider.LabelManagedYes) instances, err := p.api.AggregatedList(ctx, &computepb.AggregatedListInstancesRequest{ Project: p.cfg.Project, - Filter: proto.String(filter), + Filter: proto.String(managedFilter), ReturnPartialSuccess: proto.Bool(true), }) if err != nil { logAPIError(log, "list", err) return nil, p.wrapErr("list", "", err) } - log.V(1).Info("GCP instances listed", "filter", filter, "count", len(instances)) + log.V(1).Info("GCP instances listed", "filter", managedFilter, "count", len(instances)) out := make([]provider.Instance, 0, len(instances)) for _, inst := range instances { conv := toInstance(inst, lastPathSegment(inst.GetZone())) diff --git a/internal/provider/gcp/gcp_test.go b/internal/provider/gcp/gcp_test.go index 60e526e..a2e3d00 100644 --- a/internal/provider/gcp/gcp_test.go +++ b/internal/provider/gcp/gcp_test.go @@ -31,6 +31,14 @@ type fakeAPI struct { listReq *computepb.AggregatedListInstancesRequest listInsts []*computepb.Instance listErr error + + probeReq *computepb.AggregatedListInstancesRequest + probeErr error +} + +func (f *fakeAPI) Probe(_ context.Context, req *computepb.AggregatedListInstancesRequest) error { + f.probeReq = req + return f.probeErr } func (f *fakeAPI) Insert(_ context.Context, req *computepb.InsertInstanceRequest) (string, error) { diff --git a/internal/provider/kubernetes/check.go b/internal/provider/kubernetes/check.go new file mode 100644 index 0000000..4943b05 --- /dev/null +++ b/internal/provider/kubernetes/check.go @@ -0,0 +1,53 @@ +package kubernetes + +import ( + "context" + "errors" + "time" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +var _ provider.Checker = (*Provider)(nil) + +// Check lists at most one managed Pod across the cluster: the same client, +// verb and scope ListByTag and orphan GC rely on, so a pass means the API +// server answers and the operator's RBAC still covers them. It does not +// prove create or delete rights. +func (p *Provider) Check(ctx context.Context) []provider.TargetStatus { + start := time.Now() + var pods corev1.PodList + err := p.client.List(ctx, &pods, client.MatchingLabels{ + provider.LabelManaged: provider.LabelManagedYes, + }, client.Limit(1)) + ts := provider.TargetStatus{Name: "apiserver", Address: p.apiHost, Latency: time.Since(start)} + if err != nil { + ts.Reason = checkReason(ctx, err) + ts.Message = err.Error() + return []provider.TargetStatus{ts} + } + ts.Ready = true + ts.Reason = provider.ReasonReachable + return []provider.TargetStatus{ts} +} + +func checkReason(ctx context.Context, err error) string { + var status apierrors.APIStatus + switch { + case ctx.Err() != nil, errors.Is(err, context.DeadlineExceeded), + apierrors.IsTimeout(err), apierrors.IsServerTimeout(err): + return provider.ReasonTimeout + case apierrors.IsUnauthorized(err): + return provider.ReasonUnauthorized + case apierrors.IsForbidden(err): + return provider.ReasonForbidden + case errors.As(err, &status): + return provider.ReasonAPIError + default: + return provider.ReasonUnreachable + } +} diff --git a/internal/provider/kubernetes/check_test.go b/internal/provider/kubernetes/check_test.go new file mode 100644 index 0000000..b2e80f0 --- /dev/null +++ b/internal/provider/kubernetes/check_test.go @@ -0,0 +1,114 @@ +package kubernetes + +import ( + "context" + "errors" + "net/url" + "syscall" + "testing" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +type listFunc func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error + +func newCheckProvider(list listFunc) *Provider { + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + panic(err) + } + c := fake.NewClientBuilder().WithScheme(scheme).WithInterceptorFuncs(interceptor.Funcs{List: list}).Build() + p := newWithClient(c, provider.ProviderConfig{Name: "kubernetes"}) + p.apiHost = "https://10.96.0.1:443" + return p +} + +func TestCheck_reachable(t *testing.T) { + t.Parallel() + p := newTestProvider(testPod("ns", "a", "uid-a", corev1.PodRunning, "10.0.0.1")) + p.apiHost = "https://10.96.0.1:443" + + got := p.Check(context.Background()) + + if len(got) != 1 { + t.Fatalf("Check() returned %d targets, want 1", len(got)) + } + ts := got[0] + if !ts.Ready || ts.Reason != provider.ReasonReachable || ts.Message != "" { + t.Errorf("Check() = %+v, want ready and Reachable", ts) + } + if ts.Name != "apiserver" || ts.Address != "https://10.96.0.1:443" { + t.Errorf("target = %q at %q, want apiserver at https://10.96.0.1:443", ts.Name, ts.Address) + } +} + +func TestCheck_listOptions(t *testing.T) { + t.Parallel() + var got client.ListOptions + p := newCheckProvider(func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + got.ApplyOptions(opts) + return c.List(ctx, list, opts...) + }) + + p.Check(context.Background()) + + if got.Limit != 1 { + t.Errorf("Limit = %d, want 1", got.Limit) + } + if got.Namespace != "" { + t.Errorf("Namespace = %q, want cluster-wide", got.Namespace) + } + want := provider.LabelManaged + "=" + provider.LabelManagedYes + if got.LabelSelector == nil || got.LabelSelector.String() != want { + t.Errorf("LabelSelector = %v, want %s", got.LabelSelector, want) + } +} + +func TestCheck_failureReasons(t *testing.T) { + t.Parallel() + pods := schema.GroupResource{Resource: "pods"} + ended, cancel := context.WithCancel(context.Background()) + cancel() + + tests := []struct { + name string + ctx context.Context + err error + want string + }{ + {"forbidden", context.Background(), apierrors.NewForbidden(pods, "", errors.New("rbac denied")), provider.ReasonForbidden}, + {"unauthorized", context.Background(), apierrors.NewUnauthorized("token expired"), provider.ReasonUnauthorized}, + {"api timeout", context.Background(), apierrors.NewTimeoutError("slow etcd", 1), provider.ReasonTimeout}, + {"server timeout", context.Background(), apierrors.NewServerTimeout(pods, "list", 1), provider.ReasonTimeout}, + {"context ended", ended, context.Canceled, provider.ReasonTimeout}, + {"internal error", context.Background(), apierrors.NewInternalError(errors.New("etcd down")), provider.ReasonAPIError}, + {"connection refused", context.Background(), &url.Error{ + Op: "Get", URL: "https://10.96.0.1:443/api/v1/pods", Err: syscall.ECONNREFUSED, + }, provider.ReasonUnreachable}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + p := newCheckProvider(func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return tc.err + }) + + got := p.Check(tc.ctx) + + if len(got) != 1 { + t.Fatalf("Check() returned %d targets, want 1", len(got)) + } + if got[0].Ready || got[0].Reason != tc.want || got[0].Message != tc.err.Error() { + t.Errorf("Check() = %+v, want not ready, reason %s, message %q", got[0], tc.want, tc.err.Error()) + } + }) + } +} diff --git a/internal/provider/kubernetes/kubernetes.go b/internal/provider/kubernetes/kubernetes.go index a2688b0..72c6fd6 100644 --- a/internal/provider/kubernetes/kubernetes.go +++ b/internal/provider/kubernetes/kubernetes.go @@ -36,6 +36,8 @@ const defaultImage = "ubuntu/squid:6.6-24.04_edge" type Provider struct { client client.Client image string + // apiHost is the API server URL, shown by Check; empty in tests. + apiHost string } // New builds a kubernetes Provider from its config block. Satisfies @@ -64,7 +66,9 @@ func NewWithTransportWrapper(_ context.Context, cfg provider.ProviderConfig, wra if err != nil { return nil, fmt.Errorf("kubernetes provider %q: building client: %w", cfg.Name, err) } - return newWithClient(c, cfg), nil + p := newWithClient(c, cfg) + p.apiHost = restCfg.Host + return p, nil } // newWithClient builds a Provider around an already-constructed client, -- 2.54.0 From 04dd2c02842b002a258d110f486439283c23e1f1 Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 20:17:35 +0000 Subject: [PATCH 04/12] Add the KVM health check: reachability and storage pool per host Each configured host is checked concurrently over the pooled SSH+libvirt connection: libvirt version and hostname prove the daemon answers, then the configured pool must exist and be running, with its free space reported. Dial failures are now tagged with their stage (SSH material, TCP, SSH handshake, libvirt connect) by a dialError that keeps the message and unwraps to the cause, so classification is unchanged and the check can say why a host is unreachable. A pool verdict is decided after the call returns so it never drops a working connection. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-11-1526-provider-health-checks.md | 45 ++- internal/provider/kvm/check.go | 164 +++++++++++ internal/provider/kvm/check_test.go | 270 ++++++++++++++++++ internal/provider/kvm/conn.go | 32 ++- internal/provider/kvm/conn_test.go | 80 ++++++ internal/provider/kvm/fake_test.go | 54 ++++ internal/provider/kvm/kvm_integration_test.go | 30 ++ internal/provider/kvm/trace.go | 21 ++ internal/provider/kvm/trace_test.go | 3 + 9 files changed, 694 insertions(+), 5 deletions(-) create mode 100644 internal/provider/kvm/check.go create mode 100644 internal/provider/kvm/check_test.go 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 9ba5a38..8566d90 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 @@ -4,7 +4,7 @@ Plan: [docs/plans/2026-09-11-1526-provider-health-checks.md](../plans/2026-09-11 - [x] Step 0 — Save the plan - [x] Step 1 — ProviderHealth API and CRD - [x] Step 2 — Contract plus the kubernetes and GCP checks -- [ ] Step 3 — KVM check +- [x] Step 3 — KVM check - [ ] Step 4 — Monitor - [ ] Step 5 — Metrics and wiring - [ ] Step 6 — Docs @@ -95,3 +95,46 @@ Worth noting: a disabled Compute API is reported as `Forbidden` rather than `Unreachable`. - Once a context has ended, both checks report `Timeout` whatever the error says. The monitor's timeout is the usual cause, and the underlying error only repeats it. + +## Step 3 — KVM check + +`conn.go` now wraps each of the four dial stages in a `dialError`: ssh-material, tcp, +ssh-handshake and libvirt-connect. `Error()` returns the inner message unchanged and +`Unwrap()` returns the cause, so `classify`/`isClassified` and every existing +message-matching test behave as before. `TestDialError_keepsClassification` pins this +down. + +The three new RPCs went into `libvirtAPI`, `tracingAPI` and `fakeAPI`. The method-count +guard in `TestTracingAPI_logsEveryMethod` fails until a traced row exists for each new +method, which is what forced the three new rows. + +`Check` runs one goroutine per host, over the shared connection, through +`hostPool.call`. The pool verdict is decided only after `call` returns, because +returning it as an error from inside the callback would make `call` drop a perfectly +good connection. + +Build, vet (also with the integration tag, since that file changed) and race tests: + +```bash +go build ./internal/provider/... && go vet ./internal/provider/kvm/ \ + && go vet -tags=integration ./internal/provider/kvm/ \ + && go test -race -count=1 ./internal/provider/kvm/ +``` + +Worth noting: +- **Pool state wording:** `poolState` uses virsh's words (`running`, `inactive`, …) + rather than go-libvirt's constant names, so it reads like `virsh pool-list` output. +- **Details:** each host also reports its SSH `user`. That is the first thing to check + when the reason is `SSHHandshakeFailed`. +- **Tests renamed or merged:** + - The planned `TestSSHAuth_dial_stages` is `TestLazySSH_dial_tagsTheFailingStage`. It + dials through `lazySSH`, so the ssh-material stage is covered as well as the three + stages the in-process SSH server can trigger. + - The pool-inactive and pool-missing cases are merged into `TestCheck_libvirtVerdicts`, + which also covers a daemon error. Each case runs two checks and asserts a single + dial. +- **Tests added beyond the plan:** + - `TestCheck_timeout`: a daemon that stops answering gives `Timeout`. + - `TestDialError_keepsClassification`. +- **Not run yet:** `TestKVMCheck_realHost` in the integration file only compiles here. + It needs beelink, so it goes on the Step 7 checklist for the user to run. diff --git a/internal/provider/kvm/check.go b/internal/provider/kvm/check.go new file mode 100644 index 0000000..a2d7f51 --- /dev/null +++ b/internal/provider/kvm/check.go @@ -0,0 +1,164 @@ +package kvm + +import ( + "context" + "errors" + "fmt" + "sync" + "time" + + libvirt "github.com/digitalocean/go-libvirt" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +var _ provider.Checker = (*Provider)(nil) + +// Reasons only the KVM check can tell apart; the rest are provider-wide. +const ( + reasonSSHMaterialUnavailable = "SSHMaterialUnavailable" + reasonSSHHandshakeFailed = "SSHHandshakeFailed" + reasonLibvirtConnectFailed = "LibvirtConnectFailed" + reasonLibvirtError = "LibvirtError" + reasonPoolNotFound = "PoolNotFound" + reasonPoolNotActive = "PoolNotActive" +) + +// hostFacts is what a successful check learns about a host. +type hostFacts struct { + version uint64 + hostname string + poolState libvirt.StoragePoolState + capacity uint64 + available uint64 +} + +// Check verifies every configured host concurrently, in kvm.hosts order. +// It goes through the same pooled connection Create and Get use, so the +// SSH and libvirt handshakes only happen on the first check or after the +// link dropped; then two cheap RPCs prove the daemon answers, and the +// storage pool VMs are created in must exist and be running. +func (p *Provider) Check(ctx context.Context) []provider.TargetStatus { + out := make([]provider.TargetStatus, len(p.cfg.Hosts)) + var wg sync.WaitGroup + for i, h := range p.cfg.Hosts { + wg.Go(func() { out[i] = p.checkHost(ctx, h) }) + } + wg.Wait() + return out +} + +// checkHost returns the RPC errors from fn untouched: call drops the +// shared connection on anything that is not a libvirt error, and a pool +// that exists but is stopped says nothing about the link. facts is read +// only after call returned nil, since an abandoned fn may still write it. +func (p *Provider) checkHost(ctx context.Context, host provider.KVMHost) provider.TargetStatus { + ts := provider.TargetStatus{ + Name: host.Name, + Address: host.Address, + Details: map[string]string{"user": host.User, "pool": host.Pool}, + } + start := time.Now() + var facts hostFacts + err := p.hosts.call(ctx, p.logger(ctx), host, func(api libvirtAPI) error { + version, err := api.ConnectGetLibVersion() + if err != nil { + return err + } + hostname, err := api.ConnectGetHostname() + if err != nil { + return err + } + pool, err := api.StoragePoolLookupByName(host.Pool) + if err != nil { + return err + } + state, capacity, _, available, err := api.StoragePoolGetInfo(pool) + if err != nil { + return err + } + facts = hostFacts{ + version: version, + hostname: hostname, + poolState: libvirt.StoragePoolState(state), + capacity: capacity, + available: available, + } + return nil + }) + ts.Latency = time.Since(start) + if err != nil { + ts.Reason = checkReason(err) + ts.Message = err.Error() + return ts + } + ts.Details["hostname"] = facts.hostname + ts.Details["libvirtVersion"] = formatLibVersion(facts.version) + ts.Details["poolState"] = poolStateName(facts.poolState) + ts.Details["poolCapacity"] = formatGiB(facts.capacity) + ts.Details["poolAvailable"] = formatGiB(facts.available) + if facts.poolState != libvirt.StoragePoolRunning { + ts.Reason = reasonPoolNotActive + ts.Message = fmt.Sprintf("storage pool %q is %s", host.Pool, poolStateName(facts.poolState)) + return ts + } + ts.Ready = true + ts.Reason = provider.ReasonReachable + return ts +} + +// checkReason names where a failed check broke. The dial stage wins over +// a timeout: "the SSH handshake did not finish" says more than "the check +// ran out of time", and the message still carries the deadline. +func checkReason(err error) string { + var de *dialError + if errors.As(err, &de) { + switch de.stage { + case stageSSHMaterial: + return reasonSSHMaterialUnavailable + case stageSSHHandshake: + return reasonSSHHandshakeFailed + case stageLibvirtConnect: + return reasonLibvirtConnectFailed + default: + return provider.ReasonUnreachable + } + } + switch { + case errors.Is(err, context.DeadlineExceeded), errors.Is(err, context.Canceled): + return provider.ReasonTimeout + case hasCode(err, libvirt.ErrNoStoragePool): + return reasonPoolNotFound + case isLibvirtError(err): + return reasonLibvirtError + default: + return provider.ReasonUnreachable + } +} + +// formatLibVersion decodes libvirt's major*1e6 + minor*1e3 + micro. +func formatLibVersion(v uint64) string { + return fmt.Sprintf("%d.%d.%d", v/1_000_000, v/1_000%1_000, v%1_000) +} + +// poolStateName uses virsh's words for a pool state. +func poolStateName(s libvirt.StoragePoolState) string { + switch s { + case libvirt.StoragePoolInactive: + return "inactive" + case libvirt.StoragePoolBuilding: + return "building" + case libvirt.StoragePoolRunning: + return "running" + case libvirt.StoragePoolDegraded: + return "degraded" + case libvirt.StoragePoolInaccessible: + return "inaccessible" + default: + return fmt.Sprintf("state %d", s) + } +} + +func formatGiB(bytes uint64) string { + return fmt.Sprintf("%.1fGiB", float64(bytes)/(1<<30)) +} diff --git a/internal/provider/kvm/check_test.go b/internal/provider/kvm/check_test.go new file mode 100644 index 0000000..ca99639 --- /dev/null +++ b/internal/provider/kvm/check_test.go @@ -0,0 +1,270 @@ +package kvm + +import ( + "context" + "errors" + "fmt" + "io" + "maps" + "strings" + "sync/atomic" + "testing" + "time" + + libvirt "github.com/digitalocean/go-libvirt" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +// withoutLatency zeroes the one field a test cannot predict. +func withoutLatency(in []provider.TargetStatus) []provider.TargetStatus { + out := make([]provider.TargetStatus, len(in)) + for i, ts := range in { + ts.Latency = 0 + out[i] = ts + } + return out +} + +func TestCheck_reachable(t *testing.T) { + t.Parallel() + api := newFakeAPI("default") + api.poolCapacity = 10 << 30 + api.pools["default"]["disk.qcow2"] = &fakeVol{capacity: 4 << 30} + p := newTestProvider(hostFakes{"nas1": api}, "nas1") + + got := withoutLatency(p.Check(context.Background())) + + want := provider.TargetStatus{ + Name: "nas1", + Address: "nas1:22", + Ready: true, + Reason: provider.ReasonReachable, + Details: map[string]string{ + "user": "virt", + "pool": "default", + "hostname": "fake-host", + "libvirtVersion": "10.2.0", + "poolState": "running", + "poolCapacity": "10.0GiB", + "poolAvailable": "6.0GiB", + }, + } + if len(got) != 1 { + t.Fatalf("Check() returned %d targets, want 1", len(got)) + } + if got[0].Name != want.Name || got[0].Address != want.Address || got[0].Ready != want.Ready || + got[0].Reason != want.Reason || got[0].Message != "" || !maps.Equal(got[0].Details, want.Details) { + t.Errorf("Check() = %+v, want %+v", got[0], want) + } +} + +func TestCheck_perHostVerdicts(t *testing.T) { + t.Parallel() + p := newTestProvider(hostFakes{"nas1": newFakeAPI("default"), "nas3": newFakeAPI("default")}, "nas1", "nas2", "nas3") + + got := p.Check(context.Background()) + + if len(got) != 3 { + t.Fatalf("Check() returned %d targets, want 3", len(got)) + } + for i, want := range []struct { + name string + ready bool + }{{"nas1", true}, {"nas2", false}, {"nas3", true}} { + if got[i].Name != want.name || got[i].Ready != want.ready { + t.Errorf("target %d = %s ready=%v, want %s ready=%v", i, got[i].Name, got[i].Ready, want.name, want.ready) + } + } + if got[1].Reason != provider.ReasonUnreachable || !strings.Contains(got[1].Message, errHostOff.Error()) { + t.Errorf("unreachable host = %s %q, want Unreachable carrying %q", got[1].Reason, got[1].Message, errHostOff) + } + if got[1].Details["pool"] != "default" || got[1].Details["hostname"] != "" { + t.Errorf("unreachable host details = %v, want the configured pool and nothing learned", got[1].Details) + } +} + +// TestCheck_libvirtVerdicts also checks twice per case: none of these +// verdicts is evidence against the link, so the connection must survive. +func TestCheck_libvirtVerdicts(t *testing.T) { + t.Parallel() + tests := []struct { + name string + setup func(api *fakeAPI) + pools []string + wantReason string + wantMessage string + }{ + { + name: "pool inactive", + pools: []string{"default"}, + setup: func(api *fakeAPI) { api.poolStates["default"] = libvirt.StoragePoolInactive }, + wantReason: reasonPoolNotActive, + wantMessage: `storage pool "default" is inactive`, + }, + { + name: "pool missing", + pools: []string{"other"}, + wantReason: reasonPoolNotFound, + wantMessage: "no storage pool with matching name 'default'", + }, + { + name: "daemon error", + pools: []string{"default"}, + setup: func(api *fakeAPI) { api.fail["ConnectGetHostname"] = lvError(libvirt.ErrInternalError, "boom") }, + wantReason: reasonLibvirtError, + wantMessage: "boom", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + api := newFakeAPI(tc.pools...) + if tc.setup != nil { + tc.setup(api) + } + var dials atomic.Int32 + dial := func(ctx context.Context, h provider.KVMHost) (*hostClient, error) { + dials.Add(1) + return hostFakes{"nas1": api}.dial(ctx, h) + } + pc := testConfig("nas1") + p := newWithDialer(pc.Name, *pc.KVM, dial, nil) + + for range 2 { + got := p.Check(context.Background()) + if got[0].Ready || got[0].Reason != tc.wantReason || !strings.Contains(got[0].Message, tc.wantMessage) { + t.Errorf("Check() = %+v, want not ready, %s, message containing %q", got[0], tc.wantReason, tc.wantMessage) + } + } + if n := dials.Load(); n != 1 { + t.Errorf("dialed %d times over two checks, want 1", n) + } + }) + } +} + +func TestCheck_hostsCheckedConcurrently(t *testing.T) { + t.Parallel() + nas2Dialing := make(chan struct{}) + dial := func(ctx context.Context, h provider.KVMHost) (*hostClient, error) { + switch h.Name { + case "nas1": + select { + case <-nas2Dialing: + case <-ctx.Done(): + return nil, ctx.Err() + } + case "nas2": + close(nas2Dialing) + } + return &hostClient{api: newFakeAPI("default"), close: func() {}}, nil + } + pc := testConfig("nas1", "nas2") + p := newWithDialer(pc.Name, *pc.KVM, dial, nil) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + got := p.Check(ctx) + + for _, ts := range got { + if !ts.Ready { + t.Errorf("%s = %s %q, want ready: a sequential check would time out nas1", ts.Name, ts.Reason, ts.Message) + } + } +} + +func TestCheck_timeout(t *testing.T) { + t.Parallel() + api := newFakeAPI("default") + block := make(chan struct{}) + t.Cleanup(func() { close(block) }) + blocked := &blockingAPI{fakeAPI: api, block: block} + dial := func(context.Context, provider.KVMHost) (*hostClient, error) { + return &hostClient{api: blocked, close: func() {}}, nil + } + pc := testConfig("nas1") + p := newWithDialer(pc.Name, *pc.KVM, dial, nil) + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + got := p.Check(ctx) + + if got[0].Ready || got[0].Reason != provider.ReasonTimeout { + t.Errorf("Check() = %+v, want Timeout", got[0]) + } +} + +// blockingAPI hangs in ConnectGetLibVersion until block closes, like a +// daemon that stopped answering. +type blockingAPI struct { + *fakeAPI + block chan struct{} +} + +func (b *blockingAPI) ConnectGetLibVersion() (uint64, error) { + <-b.block + return 0, io.EOF +} + +func TestCheckReason(t *testing.T) { + t.Parallel() + tests := []struct { + name string + err error + want string + }{ + {"ssh material", &dialError{stage: stageSSHMaterial, err: fmt.Errorf("%w: parsing key", provider.ErrPermanent)}, reasonSSHMaterialUnavailable}, + {"tcp", &dialError{stage: stageTCP, err: errors.New("ssh dial: connection refused")}, provider.ReasonUnreachable}, + {"ssh handshake", &dialError{stage: stageSSHHandshake, err: errors.New("ssh handshake: unable to authenticate")}, reasonSSHHandshakeFailed}, + {"libvirt connect", &dialError{stage: stageLibvirtConnect, err: errors.New("libvirt connect via /sock: EOF")}, reasonLibvirtConnectFailed}, + {"stage survives the pool's wrapping", fmt.Errorf("connecting to host nas1 (nas1:22): %w", + &dialError{stage: stageSSHHandshake, err: context.DeadlineExceeded}), reasonSSHHandshakeFailed}, + {"deadline", context.DeadlineExceeded, provider.ReasonTimeout}, + {"cancelled", context.Canceled, provider.ReasonTimeout}, + {"no pool", lvError(libvirt.ErrNoStoragePool, "no pool"), reasonPoolNotFound}, + {"other libvirt error", lvError(libvirt.ErrInternalError, "boom"), reasonLibvirtError}, + {"link died mid-call", io.EOF, provider.ReasonUnreachable}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := checkReason(tc.err); got != tc.want { + t.Errorf("checkReason(%v) = %s, want %s", tc.err, got, tc.want) + } + }) + } +} + +func TestDialError_keepsClassification(t *testing.T) { + t.Parallel() + err := fmt.Errorf("connecting to host nas1: %w", + &dialError{stage: stageSSHMaterial, err: fmt.Errorf("%w: parsing kvm.sshKeyFile: bad", provider.ErrPermanent)}) + + if got := classify(err); !errors.Is(got, provider.ErrPermanent) { + t.Errorf("classify() = %v, want ErrPermanent through the dialError", got) + } + if !isClassified(err) { + t.Error("isClassified() = false, want true") + } + if want := "connecting to host nas1: permanent: parsing kvm.sshKeyFile: bad"; !strings.HasSuffix(err.Error(), "parsing kvm.sshKeyFile: bad") { + t.Errorf("message = %q, want it unchanged, like %q", err.Error(), want) + } +} + +func TestFormatLibVersion(t *testing.T) { + t.Parallel() + tests := []struct { + in uint64 + want string + }{ + {10002000, "10.2.0"}, + {11006001, "11.6.1"}, + {9000000, "9.0.0"}, + } + for _, tc := range tests { + if got := formatLibVersion(tc.in); got != tc.want { + t.Errorf("formatLibVersion(%d) = %s, want %s", tc.in, got, tc.want) + } + } +} diff --git a/internal/provider/kvm/conn.go b/internal/provider/kvm/conn.go index 8f83751..da7ff1a 100644 --- a/internal/provider/kvm/conn.go +++ b/internal/provider/kvm/conn.go @@ -44,6 +44,9 @@ type libvirtAPI interface { StorageVolUpload(vol libvirt.StorageVol, r io.Reader, offset, length uint64, flags libvirt.StorageVolUploadFlags) error StorageVolDownload(vol libvirt.StorageVol, w io.Writer, offset, length uint64, flags libvirt.StorageVolDownloadFlags) error StorageVolDelete(vol libvirt.StorageVol, flags libvirt.StorageVolDeleteFlags) error + ConnectGetLibVersion() (uint64, error) + ConnectGetHostname() (string, error) + StoragePoolGetInfo(pool libvirt.StoragePool) (state uint8, capacity, allocation, available uint64, err error) } // hostClient is one live connection to a hypervisor. @@ -68,6 +71,27 @@ func (c *hostClient) alive() bool { // real one; tests inject their own. type hostDialer func(ctx context.Context, host provider.KVMHost) (*hostClient, error) +// dialStage names the step of a connection attempt that failed, so the +// health check can say why a host is unreachable without parsing messages. +type dialStage string + +const ( + stageSSHMaterial dialStage = "ssh-material" + stageTCP dialStage = "tcp" + stageSSHHandshake dialStage = "ssh-handshake" + stageLibvirtConnect dialStage = "libvirt-connect" +) + +// dialError tags a dial failure with its stage. It keeps the message and +// unwraps to the cause, so classify sees exactly what it saw before. +type dialError struct { + stage dialStage + err error +} + +func (e *dialError) Error() string { return e.err.Error() } +func (e *dialError) Unwrap() error { return e.err } + // hostPool caches one connection per host, dialing lazily and redialing // after the connection drops. Dials for different hosts don't block each // other; concurrent callers for the same host share a single dial. @@ -254,7 +278,7 @@ func (l *lazySSH) load() (*sshAuth, error) { func (l *lazySSH) dial(ctx context.Context, host provider.KVMHost) (*hostClient, error) { a, err := l.load() if err != nil { - return nil, err + return nil, &dialError{stage: stageSSHMaterial, err: err} } return a.dial(ctx, host) } @@ -285,7 +309,7 @@ func (a *sshAuth) dialSSH(ctx context.Context, host provider.KVMHost) (*ssh.Clie var d net.Dialer nc, err := d.DialContext(ctx, "tcp", host.Address) if err != nil { - return nil, fmt.Errorf("ssh dial: %w", err) + return nil, &dialError{stage: stageTCP, err: fmt.Errorf("ssh dial: %w", err)} } _ = nc.SetDeadline(deadline) conn, chans, reqs, err := ssh.NewClientConn(nc, host.Address, &ssh.ClientConfig{ @@ -295,7 +319,7 @@ func (a *sshAuth) dialSSH(ctx context.Context, host provider.KVMHost) (*ssh.Clie }) if err != nil { nc.Close() - return nil, fmt.Errorf("ssh handshake: %w", err) + return nil, &dialError{stage: stageSSHHandshake, err: fmt.Errorf("ssh handshake: %w", err)} } _ = nc.SetDeadline(time.Time{}) return ssh.NewClient(conn, chans, reqs), nil @@ -319,7 +343,7 @@ func (u unixForward) Dial() (net.Conn, error) { func connectLibvirt(ctx context.Context, client *ssh.Client, socket string) (*hostClient, error) { lv := libvirt.NewWithDialer(unixForward{client: client, path: socket}) if err := awaitOrCancel(ctx, lv.Connect, func() { client.Close() }); err != nil { - return nil, fmt.Errorf("libvirt connect via %s: %w", socket, err) + return nil, &dialError{stage: stageLibvirtConnect, err: fmt.Errorf("libvirt connect via %s: %w", socket, err)} } stop := make(chan struct{}) var once sync.Once diff --git a/internal/provider/kvm/conn_test.go b/internal/provider/kvm/conn_test.go index 1340a7b..722c948 100644 --- a/internal/provider/kvm/conn_test.go +++ b/internal/provider/kvm/conn_test.go @@ -614,6 +614,86 @@ func TestNew_missingSSHMaterialStillBuilds(t *testing.T) { } } +// The health check reports where a connection broke, so every stage of a +// real dial must come back tagged. +func TestLazySSH_dial_tagsTheFailingStage(t *testing.T) { + t.Parallel() + tests := []struct { + name string + mutate func(t *testing.T, f *sshFixture) + want dialStage + wantReason string + }{ + { + name: "key material missing", + mutate: func(t *testing.T, f *sshFixture) { f.cfg.SSHKeyFile = filepath.Join(t.TempDir(), "absent") }, + want: stageSSHMaterial, + wantReason: reasonSSHMaterialUnavailable, + }, + { + name: "nothing listening", + mutate: func(t *testing.T, f *sshFixture) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + f.host.Address = ln.Addr().String() + ln.Close() + }, + want: stageTCP, + wantReason: provider.ReasonUnreachable, + }, + { + name: "host key not in known_hosts", + mutate: func(t *testing.T, f *sshFixture) { + impostor, _ := newKeyPair(t) + line := knownhosts.Line([]string{f.server.addr}, impostor.PublicKey()) + if err := os.WriteFile(f.cfg.KnownHostsFile, []byte(line+"\n"), 0o600); err != nil { + t.Fatal(err) + } + }, + want: stageSSHHandshake, + wantReason: reasonSSHHandshakeFailed, + }, + { + name: "client key rejected", + mutate: func(t *testing.T, f *sshFixture) { + _, otherPEM := newKeyPair(t) + if err := os.WriteFile(f.keyFile, otherPEM, 0o600); err != nil { + t.Fatal(err) + } + }, + want: stageSSHHandshake, + wantReason: reasonSSHHandshakeFailed, + }, + { + name: "libvirt socket missing", + mutate: func(_ *testing.T, f *sshFixture) { f.host.Socket = "/nonexistent/libvirt-sock" }, + want: stageLibvirtConnect, + wantReason: reasonLibvirtConnectFailed, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + f := newSSHFixture(t, true) + tc.mutate(t, f) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + _, err := newLazySSH(f.cfg).dial(ctx, f.host) + + var de *dialError + if !errors.As(err, &de) || de.stage != tc.want { + t.Fatalf("dial() error = %v, want a %s dialError", err, tc.want) + } + if got := checkReason(err); got != tc.wantReason { + t.Errorf("checkReason() = %s, want %s", got, tc.wantReason) + } + }) + } +} + func TestKeepalive_closesClientWhenServerStalls(t *testing.T) { t.Parallel() f := newSSHFixture(t, false) diff --git a/internal/provider/kvm/fake_test.go b/internal/provider/kvm/fake_test.go index 9599bb5..a0a74b5 100644 --- a/internal/provider/kvm/fake_test.go +++ b/internal/provider/kvm/fake_test.go @@ -28,6 +28,12 @@ type fakeAPI struct { // uploadHook, when set, sees every StorageVolUpload before the fake // reads the stream; it can return an error to simulate a broken upload. uploadHook func(vol string, r io.Reader) error + // hostname, libVersion, poolCapacity and poolStates answer the health + // check's RPCs. A pool missing from poolStates is running. + hostname string + libVersion uint64 + poolCapacity uint64 + poolStates map[string]libvirt.StoragePoolState } type fakeVol struct { @@ -53,6 +59,11 @@ func newFakeAPI(pools ...string) *fakeAPI { domains: map[string]*fakeDomain{}, fail: map[string]error{}, failOnce: map[string]error{}, + + hostname: "fake-host", + libVersion: 10002000, + poolCapacity: 100 << 30, + poolStates: map[string]libvirt.StoragePoolState{}, } for _, p := range pools { f.pools[p] = map[string]*fakeVol{} @@ -409,3 +420,46 @@ func (f *fakeAPI) StorageVolDelete(vol libvirt.StorageVol, _ libvirt.StorageVolD delete(f.pools[vol.Pool], vol.Name) return nil } + +// --- host --- + +func (f *fakeAPI) ConnectGetLibVersion() (uint64, error) { + f.mu.Lock() + defer f.mu.Unlock() + if err := f.enter("ConnectGetLibVersion"); err != nil { + return 0, err + } + return f.libVersion, nil +} + +func (f *fakeAPI) ConnectGetHostname() (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + if err := f.enter("ConnectGetHostname"); err != nil { + return "", err + } + return f.hostname, nil +} + +// StoragePoolGetInfo reports the pool's state and poolCapacity minus the +// volumes' capacities as available space. +func (f *fakeAPI) StoragePoolGetInfo(pool libvirt.StoragePool) (uint8, uint64, uint64, uint64, error) { + f.mu.Lock() + defer f.mu.Unlock() + if err := f.enter("StoragePoolGetInfo"); err != nil { + return 0, 0, 0, 0, err + } + vols, ok := f.pools[pool.Name] + if !ok { + return 0, 0, 0, 0, lvError(libvirt.ErrNoStoragePool, "no pool %s", pool.Name) + } + var used uint64 + for _, v := range vols { + used += uint64(v.capacity) + } + state := libvirt.StoragePoolRunning + if s, ok := f.poolStates[pool.Name]; ok { + state = s + } + return uint8(state), f.poolCapacity, used, f.poolCapacity - used, nil +} diff --git a/internal/provider/kvm/kvm_integration_test.go b/internal/provider/kvm/kvm_integration_test.go index 6e9e157..2f4070c 100644 --- a/internal/provider/kvm/kvm_integration_test.go +++ b/internal/provider/kvm/kvm_integration_test.go @@ -347,6 +347,36 @@ func TestKVM_lifecycle(t *testing.T) { env.keep = false } +// TestKVMCheck_realHost runs the provider health check against the host. +// It creates nothing: it must reach the host and find the pool running. +func TestKVMCheck_realHost(t *testing.T) { + env := loadIntegrationEnv(t) + log := integrationLogger(t, env.logLevel) + ctx, cancel := context.WithTimeout(logr.NewContext(context.Background(), log), time.Minute) + defer cancel() + + prov, err := New(ctx, env.cfg) + if err != nil { + t.Fatalf("New: %v", err) + } + got := prov.(*Provider).Check(ctx) + + if len(got) != 1 { + t.Fatalf("Check returned %d targets, want 1", len(got)) + } + ts := got[0] + t.Logf("%s (%s): ready=%v reason=%s latency=%s details=%v", + ts.Name, ts.Address, ts.Ready, ts.Reason, ts.Latency.Round(time.Millisecond), ts.Details) + if !ts.Ready { + t.Fatalf("check failed: %s: %s", ts.Reason, ts.Message) + } + for _, key := range []string{"hostname", "libvirtVersion", "poolState", "poolAvailable"} { + if ts.Details[key] == "" { + t.Errorf("details[%s] is empty, want what the host reported", key) + } + } +} + // waitForRunning polls Get until the instance is Running with an IP or ctx // ends. Transient errors are retried; anything else fails the test. func waitForRunning(ctx context.Context, t *testing.T, p *Provider, id string) *provider.Instance { diff --git a/internal/provider/kvm/trace.go b/internal/provider/kvm/trace.go index a19a38d..6700509 100644 --- a/internal/provider/kvm/trace.go +++ b/internal/provider/kvm/trace.go @@ -209,3 +209,24 @@ func (t *tracingAPI) StorageVolDelete(vol libvirt.StorageVol, flags libvirt.Stor c.done(err) return err } + +func (t *tracingAPI) ConnectGetLibVersion() (uint64, error) { + c := t.begin("ConnectGetLibVersion") + v, err := t.inner.ConnectGetLibVersion() + c.done(err, "version", formatLibVersion(v)) + return v, err +} + +func (t *tracingAPI) ConnectGetHostname() (string, error) { + c := t.begin("ConnectGetHostname") + h, err := t.inner.ConnectGetHostname() + c.done(err, "hostname", h) + return h, err +} + +func (t *tracingAPI) StoragePoolGetInfo(pool libvirt.StoragePool) (uint8, uint64, uint64, uint64, error) { + c := t.begin("StoragePoolGetInfo", "pool", pool.Name) + state, capacity, allocation, available, err := t.inner.StoragePoolGetInfo(pool) + c.done(err, "state", poolStateName(libvirt.StoragePoolState(state)), "capacity", capacity, "available", available) + return state, capacity, allocation, available, err +} diff --git a/internal/provider/kvm/trace_test.go b/internal/provider/kvm/trace_test.go index 0a11a30..d81883e 100644 --- a/internal/provider/kvm/trace_test.go +++ b/internal/provider/kvm/trace_test.go @@ -103,6 +103,9 @@ func TestTracingAPI_logsEveryMethod(t *testing.T) { return api.StorageVolDownload(overlay, io.Discard, 0, 0, 0) }}, {"StorageVolDelete", func(api libvirtAPI) error { return api.StorageVolDelete(overlay, 0) }}, + {"ConnectGetLibVersion", func(api libvirtAPI) error { _, err := api.ConnectGetLibVersion(); return err }}, + {"ConnectGetHostname", func(api libvirtAPI) error { _, err := api.ConnectGetHostname(); return err }}, + {"StoragePoolGetInfo", func(api libvirtAPI) error { _, _, _, _, err := api.StoragePoolGetInfo(pool); return err }}, } if n := reflect.TypeOf((*libvirtAPI)(nil)).Elem().NumMethod(); n != len(cases) { t.Fatalf("libvirtAPI has %d methods, this test covers %d; trace the new one", n, len(cases)) -- 2.54.0 From 88a15ecdb54f891f8b587aa5dc1ad64da027dbe5 Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 20:20:52 +0000 Subject: [PATCH 05/12] Add the provider health Monitor A leader-only runnable checks every configured provider concurrently under one timeout per round (first round at start, then every interval), keeps the latest results for the metrics collector, logs targets going down, changing reason or coming back, and publishes one ProviderHealth object per provider: created with a managed-by label when missing, then a status merge patch. Per-target lastTransitionTime survives rounds in which ready does not flip. Objects of providers that are no longer configured are deleted once at start; with checks turned off, all are. Reads go through the API reader, so no cluster-wide informer is needed. Co-Authored-By: Claude Opus 5 (1M context) --- config/rbac/role.yaml | 16 + .../2026-09-11-1526-provider-health-checks.md | 52 ++- internal/providerhealth/envtest_test.go | 100 +++++ internal/providerhealth/monitor.go | 290 ++++++++++++++ internal/providerhealth/monitor_test.go | 354 ++++++++++++++++++ internal/providerhealth/status.go | 98 +++++ internal/providerhealth/status_test.go | 135 +++++++ 7 files changed, 1044 insertions(+), 1 deletion(-) create mode 100644 internal/providerhealth/envtest_test.go create mode 100644 internal/providerhealth/monitor.go create mode 100644 internal/providerhealth/monitor_test.go create mode 100644 internal/providerhealth/status.go create mode 100644 internal/providerhealth/status_test.go diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index ba26c8a..a798bf4 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -22,6 +22,22 @@ rules: - get - list - watch +- apiGroups: + - crawl.example.com + resources: + - providerhealths + verbs: + - create + - delete + - get + - list +- apiGroups: + - crawl.example.com + resources: + - providerhealths/status + verbs: + - get + - patch - apiGroups: - crawl.example.com resources: 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 8566d90..861ce6a 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 @@ -5,7 +5,7 @@ Plan: [docs/plans/2026-09-11-1526-provider-health-checks.md](../plans/2026-09-11 - [x] Step 1 — ProviderHealth API and CRD - [x] Step 2 — Contract plus the kubernetes and GCP checks - [x] Step 3 — KVM check -- [ ] Step 4 — Monitor +- [x] Step 4 — Monitor - [ ] Step 5 — Metrics and wiring - [ ] Step 6 — Docs - [ ] Step 7 — Validate, record, push, open the PR @@ -138,3 +138,53 @@ Worth noting: - `TestDialError_keepsClassification`. - **Not run yet:** `TestKVMCheck_realHost` in the integration file only compiles here. It needs beelink, so it goes on the Step 7 checklist for the user to run. + +## Step 4 — Monitor + +`internal/providerhealth` has the `Monitor` runnable plus the pure `buildStatus`. + +- **Scheduling:** the Monitor is leader-only. It cleans up once at start, checks + immediately, then checks on a ticker. +- **Each round:** providers are checked concurrently under one timeout, then logged and + published one after another. A failed write is logged, and the next round writes + again. +- **Reads:** objects are read through the reader it is given. `main.go` passes the + manager's API reader (Step 5). + +The first `make manifests` left `config/rbac/role.yaml` unchanged, with no error. +I had put the `+kubebuilder:rbac` markers inside `Monitor`'s doc comment, and +controller-gen only reads RBAC markers from package-level comment groups. So they were +silently attached to the type and ignored. An empty `git diff config/rbac/role.yaml` was +the only sign. Moving them into their own comment block fixed it: + +```bash +make manifests && git diff config/rbac/role.yaml +# + providerhealths: create, delete, get, list +# + providerhealths/status: get, patch +``` + +Vet, race tests and the envtest round-trip (using the `bin/k8s/1.36.2-linux-amd64` +binaries `make setup-envtest` had already fetched): + +```bash +go vet ./internal/providerhealth/ && go test -race -count=1 -v ./internal/providerhealth/ +# --- PASS: TestMonitor_envtestRoundTrip (6.45s) ... ok +``` + +Worth noting: +- **Fake client scope:** the fake client needs a RESTMapper that registers + ProviderHealth as `RESTScopeRoot`. Its default mapper treats every kind as namespaced. +- **Why the envtest calls `publish` directly:** `checkOnce` only logs write errors, so a + schema rejection would look like a missing object. Calling `publish` makes the API + server's answer the test failure. + - Its first round writes a 2000-character message; the stored message is truncated to + 1024. + - Its second round proves that only the target that flipped gets a new + `lastTransitionTime`. +- **No guard against a hung checker:** the Monitor trusts `Checker`'s contract to return + once the context ends, and doesn't abandon a checker that ignores it. All three + checkers stop when the context ends: KVM through `awaitOrCancel`, and kubernetes and + GCP through their clients. +- **Test renamed:** the planned `TestMonitor_timeoutBoundsBlockingChecker` is + `TestMonitor_checkOnce_timeoutBoundsEachRound`. It also asserts that a fast provider + still publishes next to a hung one. diff --git a/internal/providerhealth/envtest_test.go b/internal/providerhealth/envtest_test.go new file mode 100644 index 0000000..0fc5f10 --- /dev/null +++ b/internal/providerhealth/envtest_test.go @@ -0,0 +1,100 @@ +package providerhealth + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + "unicode/utf8" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +// TestMonitor_envtestRoundTrip writes through a real API server, which is +// the only thing that validates the generated schema: cluster scope, the +// status subresource, required fields, list-map keys and the message limit. +func TestMonitor_envtestRoundTrip(t *testing.T) { + if testing.Short() { + t.Skip("envtest skipped under -short") + } + env := &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, + ErrorIfCRDPathMissing: true, + BinaryAssetsDirectory: envtestBinaryDir(), + } + cfg, err := env.Start() + if err != nil { + t.Fatalf("starting envtest (run `make setup-envtest` first): %v", err) + } + t.Cleanup(func() { _ = env.Stop() }) + scheme := runtime.NewScheme() + if err := crawlv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + c, err := client.New(cfg, client.Options{Scheme: scheme}) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + m := &Monitor{Client: c, Reader: c} + + first := Result{Provider: "kvm-home", Type: "kvm", Supported: true, CheckedAt: t0, Targets: []provider.TargetStatus{ + {Name: "nas1", Address: "nas1:22", Ready: true, Reason: provider.ReasonReachable, + Latency: 42 * time.Millisecond, Details: map[string]string{"libvirtVersion": "10.2.0"}}, + {Name: "nas2", Address: "nas2:22", Reason: provider.ReasonUnreachable, Message: strings.Repeat("x", 2000)}, + }} + if err := m.publish(ctx, first); err != nil { + t.Fatalf("first publish: %v", err) + } + got := getHealth(t, c, "kvm-home") + if got.Status.TargetsReady != "1/2" || got.Status.Targets[0].Details["libvirtVersion"] != "10.2.0" || + got.Status.Targets[0].LatencyMillis != 42 || utf8.RuneCountInString(got.Status.Targets[1].Message) != maxMessageLen { + t.Errorf("status after the first round = %+v", got.Status) + } + + second := Result{Provider: "kvm-home", Type: "kvm", Supported: true, CheckedAt: t1, + Targets: []provider.TargetStatus{up("nas1"), up("nas2")}} + if err := m.publish(ctx, second); err != nil { + t.Fatalf("second publish: %v", err) + } + got = getHealth(t, c, "kvm-home") + if cond := readyCondition(got); cond == nil || cond.Reason != reasonAllTargetsReady { + t.Errorf("Ready after recovery = %+v, want %s", cond, reasonAllTargetsReady) + } + if lt := got.Status.Targets[0].LastTransitionTime.Time; !lt.Equal(t0) { + t.Errorf("nas1 lastTransitionTime = %s, want %s", lt, t0) + } + if lt := got.Status.Targets[1].LastTransitionTime.Time; !lt.Equal(t1) { + t.Errorf("nas2 lastTransitionTime = %s, want %s", lt, t1) + } + + m.cleanup(ctx, nil) + var gone crawlv1alpha1.ProviderHealth + if err := c.Get(ctx, client.ObjectKey{Name: "kvm-home"}, &gone); !apierrors.IsNotFound(err) { + t.Errorf("Get after cleanup = %v, want NotFound", err) + } +} + +// envtestBinaryDir finds the binaries `make setup-envtest` downloads, so +// the test also runs outside make (which sets KUBEBUILDER_ASSETS instead). +func envtestBinaryDir() string { + base := filepath.Join("..", "..", "bin", "k8s") + entries, err := os.ReadDir(base) + if err != nil { + return "" + } + for _, e := range entries { + if e.IsDir() { + return filepath.Join(base, e.Name()) + } + } + return "" +} diff --git a/internal/providerhealth/monitor.go b/internal/providerhealth/monitor.go new file mode 100644 index 0000000..e001ea7 --- /dev/null +++ b/internal/providerhealth/monitor.go @@ -0,0 +1,290 @@ +// Package providerhealth checks every configured provider on an interval +// and publishes the results: one ProviderHealth object per provider, a log +// line whenever a target goes down or comes back, and a snapshot the +// metrics collector reads at scrape time. +package providerhealth + +import ( + "context" + "fmt" + "slices" + "sync" + "time" + + "github.com/go-logr/logr" + "go.opentelemetry.io/otel/attribute" + "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/tracing" +) + +const ( + // labelManagedBy marks the ProviderHealth objects this operator owns; + // cleanup never deletes an object without it. + labelManagedBy = "app.kubernetes.io/managed-by" + managedByValue = "egress-proxies-operator" + + minInterval = 10 * time.Second + defaultTimeout = 15 * time.Second +) + +// Target is one configured provider. Checker is nil for a provider that +// cannot check itself. +type Target struct { + Name string + Type string + Checker provider.Checker +} + +// Result is the outcome of one provider's latest check. +type Result struct { + Provider string + Type string + // Supported is false when the provider has no Checker. + Supported bool + Targets []provider.TargetStatus + CheckedAt time.Time +} + +// controller-gen reads RBAC markers only at package level, so they stay +// out of Monitor's doc comment. +// +kubebuilder:rbac:groups=crawl.example.com,resources=providerhealths,verbs=get;list;create;delete +// +kubebuilder:rbac:groups=crawl.example.com,resources=providerhealths/status,verbs=get;patch + +// Monitor is the manager Runnable that checks the providers. +type Monitor struct { + // Client writes the ProviderHealth objects. + Client client.Client + // Reader reads them; pass the manager's API reader. The objects are + // cluster-scoped, and reading them through the cache would start a + // cluster-wide informer needing watch rights, whatever --proxy-namespace + // says, for a handful of reads a minute. + Reader client.Reader + // Targets are the configured providers, in config order. + Targets []Target + // Interval between rounds of checks; 0 turns checking off. The first + // round runs at start: nothing races it, and early visibility is the + // point. + Interval time.Duration + // Timeout bounds each round (default 15s). + Timeout time.Duration + + now func() time.Time + + mu sync.Mutex + latest []Result + + // prev holds each target's previous verdict for transition logging. + // Only the loop goroutine touches it. + prev map[string]provider.TargetStatus +} + +// NeedLeaderElection is true: only the leader dials the hypervisors and +// writes the objects, and only its gauges describe the checks. +func (m *Monitor) NeedLeaderElection() bool { return true } + +// ValidateSchedule checks --provider-check-interval and +// --provider-check-timeout. The interval is 0 (off) or at least 10s, which +// is what keeps one status write per provider per round cheap; the timeout +// is positive and no longer than the interval. +func ValidateSchedule(interval, timeout time.Duration) error { + switch { + case interval == 0: + return nil + case interval < minInterval: + return fmt.Errorf("--provider-check-interval must be 0 (off) or at least %s, got %s", minInterval, interval) + case timeout <= 0 || timeout > interval: + return fmt.Errorf("--provider-check-timeout must be positive and at most the interval (%s), got %s", interval, timeout) + } + return nil +} + +// Start deletes the objects of providers that are no longer configured, +// then checks every Interval until ctx ends. The providers config is read +// once per process, so cleaning up once is enough. With checking off, +// every object goes, so none is left claiming a stale Ready. +func (m *Monitor) Start(ctx context.Context) error { + if m.Timeout == 0 { + m.Timeout = defaultTimeout + } + if m.now == nil { + m.now = time.Now + } + log := logf.FromContext(ctx).WithName("provider-health") + ctx = logf.IntoContext(ctx, log) + + keep := map[string]bool{} + if m.Interval > 0 { + for _, t := range m.Targets { + keep[t.Name] = true + } + } + m.cleanup(ctx, keep) + if m.Interval == 0 { + log.Info("Provider checks are off (--provider-check-interval=0)") + return nil + } + for _, t := range m.Targets { + if errs := validation.IsDNS1123Subdomain(t.Name); len(errs) > 0 { + log.Info("WARNING: provider name is not a valid object name; its ProviderHealth object is not written, its metrics are", + "provider", t.Name, "problems", errs) + } + } + + m.checkOnce(ctx) + ticker := time.NewTicker(m.Interval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return nil + case <-ticker.C: + m.checkOnce(ctx) + } + } +} + +// Snapshot returns the latest results in config order. +func (m *Monitor) Snapshot() []Result { + m.mu.Lock() + defer m.mu.Unlock() + out := make([]Result, len(m.latest)) + for i, r := range m.latest { + r.Targets = slices.Clone(r.Targets) + out[i] = r + } + return out +} + +// checkOnce runs every provider's check concurrently under one timeout, +// then records, logs and publishes the results provider by provider. A +// failed write is logged and retried by the next round. +func (m *Monitor) checkOnce(ctx context.Context) { + ctx, span := tracing.Start(ctx, "providerhealth.check") + defer span.End() + log := logf.FromContext(ctx) + + checkCtx, cancel := context.WithTimeout(ctx, m.Timeout) + defer cancel() + results := make([]Result, len(m.Targets)) + var wg sync.WaitGroup + for i, t := range m.Targets { + wg.Go(func() { results[i] = m.run(checkCtx, t) }) + } + wg.Wait() + + m.mu.Lock() + m.latest = results + m.mu.Unlock() + + notReady := 0 + for _, r := range results { + m.logTransitions(log, r) + for _, ts := range r.Targets { + if !ts.Ready { + notReady++ + } + } + if err := m.publish(ctx, r); err != nil { + log.Error(err, "Writing provider health", "provider", r.Provider) + } + } + span.SetAttributes( + attribute.Int("providerhealth.providers", len(results)), + attribute.Int("providerhealth.targets_not_ready", notReady), + ) +} + +func (m *Monitor) run(ctx context.Context, t Target) Result { + r := Result{Provider: t.Name, Type: t.Type, Supported: t.Checker != nil} + if t.Checker != nil { + r.Targets = t.Checker.Check(ctx) + } + r.CheckedAt = m.now() + return r +} + +// logTransitions logs a target going down, staying down for a different +// reason, or coming back. Steady states stay quiet; a target that is ready +// the first time it is seen only logs at V(1). +func (m *Monitor) logTransitions(log logr.Logger, r Result) { + if m.prev == nil { + m.prev = map[string]provider.TargetStatus{} + } + for _, ts := range r.Targets { + key := r.Provider + "/" + ts.Name + prev, seen := m.prev[key] + m.prev[key] = ts + kv := []any{"provider", r.Provider, "target", ts.Name, "address", ts.Address, "reason", ts.Reason} + switch { + case !ts.Ready && (!seen || prev.Ready || prev.Reason != ts.Reason): + log.Info("WARNING: provider target not ready", append(kv, "message", ts.Message)...) + case ts.Ready && seen && !prev.Ready: + log.Info("Provider target ready", append(kv, "latency", ts.Latency.Round(time.Millisecond))...) + case ts.Ready && !seen: + log.V(1).Info("Provider target ready", append(kv, "latency", ts.Latency.Round(time.Millisecond))...) + } + } +} + +// publish creates the provider's ProviderHealth object when missing and +// patches its status. The status changes on every round, since +// lastCheckTime moves; the comparison only skips the patch when a clock +// did not advance. +func (m *Monitor) publish(ctx context.Context, r Result) error { + if len(validation.IsDNS1123Subdomain(r.Provider)) > 0 { + return nil + } + var ph crawlv1alpha1.ProviderHealth + err := m.Reader.Get(ctx, client.ObjectKey{Name: r.Provider}, &ph) + switch { + case apierrors.IsNotFound(err): + ph = crawlv1alpha1.ProviderHealth{ObjectMeta: metav1.ObjectMeta{ + Name: r.Provider, + Labels: map[string]string{labelManagedBy: managedByValue}, + }} + if err := m.Client.Create(ctx, &ph); err != nil { + return fmt.Errorf("creating ProviderHealth %s: %w", r.Provider, err) + } + case err != nil: + return fmt.Errorf("reading ProviderHealth %s: %w", r.Provider, err) + } + base := ph.DeepCopy() + ph.Status = buildStatus(ph.Status, r) + if equality.Semantic.DeepEqual(base.Status, ph.Status) { + return nil + } + if err := m.Client.Status().Patch(ctx, &ph, client.MergeFrom(base)); err != nil { + return fmt.Errorf("patching ProviderHealth %s status: %w", r.Provider, err) + } + return nil +} + +// cleanup deletes this operator's ProviderHealth objects whose name is not +// in keep. +func (m *Monitor) cleanup(ctx context.Context, keep map[string]bool) { + log := logf.FromContext(ctx) + var list crawlv1alpha1.ProviderHealthList + if err := m.Reader.List(ctx, &list, client.MatchingLabels{labelManagedBy: managedByValue}); err != nil { + log.Error(err, "Listing provider health objects; stale ones stay until the next start") + return + } + for i := range list.Items { + ph := &list.Items[i] + if keep[ph.Name] { + continue + } + if err := m.Client.Delete(ctx, ph); client.IgnoreNotFound(err) != nil { + log.Error(err, "Deleting provider health of an unconfigured provider", "name", ph.Name) + continue + } + log.Info("Deleted provider health of an unconfigured provider", "name", ph.Name) + } +} diff --git a/internal/providerhealth/monitor_test.go b/internal/providerhealth/monitor_test.go new file mode 100644 index 0000000..b2616d3 --- /dev/null +++ b/internal/providerhealth/monitor_test.go @@ -0,0 +1,354 @@ +package providerhealth + +import ( + "context" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/go-logr/logr/funcr" + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +var ( + t0 = time.Date(2026, 9, 11, 15, 0, 0, 0, time.UTC) + t1 = t0.Add(time.Minute) +) + +type checkFunc func(ctx context.Context) []provider.TargetStatus + +func (f checkFunc) Check(ctx context.Context) []provider.TargetStatus { return f(ctx) } + +func verdicts(ts ...provider.TargetStatus) checkFunc { + return func(context.Context) []provider.TargetStatus { return ts } +} + +func up(name string) provider.TargetStatus { + return provider.TargetStatus{Name: name, Address: name + ":22", Ready: true, Reason: provider.ReasonReachable} +} + +func down(name, reason string) provider.TargetStatus { + return provider.TargetStatus{Name: name, Address: name + ":22", Reason: reason, Message: name + " is down"} +} + +func managed(name string) *crawlv1alpha1.ProviderHealth { + return &crawlv1alpha1.ProviderHealth{ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: map[string]string{labelManagedBy: managedByValue}, + }} +} + +// newFakeClient registers ProviderHealth as cluster-scoped; the fake's +// default mapper would treat it as namespaced. +func newFakeClient(t *testing.T, objs ...client.Object) client.WithWatch { + t.Helper() + scheme := runtime.NewScheme() + if err := crawlv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + mapper := apimeta.NewDefaultRESTMapper([]schema.GroupVersion{crawlv1alpha1.GroupVersion}) + mapper.Add(crawlv1alpha1.GroupVersion.WithKind("ProviderHealth"), apimeta.RESTScopeRoot) + return fake.NewClientBuilder(). + WithScheme(scheme). + WithRESTMapper(mapper). + WithStatusSubresource(&crawlv1alpha1.ProviderHealth{}). + WithObjects(objs...). + Build() +} + +func newTestMonitor(c client.Client, targets ...Target) *Monitor { + return &Monitor{ + Client: c, + Reader: c, + Targets: targets, + Interval: time.Minute, + Timeout: time.Second, + now: func() time.Time { return t1 }, + } +} + +func getHealth(t *testing.T, c client.Reader, name string) *crawlv1alpha1.ProviderHealth { + t.Helper() + var ph crawlv1alpha1.ProviderHealth + if err := c.Get(context.Background(), client.ObjectKey{Name: name}, &ph); err != nil { + t.Fatalf("Get ProviderHealth %s: %v", name, err) + } + return &ph +} + +func readyCondition(ph *crawlv1alpha1.ProviderHealth) *metav1.Condition { + return apimeta.FindStatusCondition(ph.Status.Conditions, crawlv1alpha1.ConditionReady) +} + +func TestMonitor_checkOnce_createsLabelledObjects(t *testing.T) { + t.Parallel() + c := newFakeClient(t) + m := newTestMonitor(c, + Target{Name: "kvm-home", Type: "kvm", Checker: verdicts(up("nas1"), down("nas2", provider.ReasonUnreachable))}, + Target{Name: "local", Type: "kubernetes", Checker: verdicts(up("apiserver"))}, + ) + + m.checkOnce(context.Background()) + + kvm := getHealth(t, c, "kvm-home") + if kvm.Labels[labelManagedBy] != managedByValue { + t.Errorf("labels = %v, want %s=%s", kvm.Labels, labelManagedBy, managedByValue) + } + if cond := readyCondition(kvm); cond == nil || cond.Status != metav1.ConditionFalse || cond.Reason != reasonTargetsNotReady { + t.Errorf("kvm-home Ready = %+v, want False/%s", cond, reasonTargetsNotReady) + } + if kvm.Status.Type != "kvm" || kvm.Status.TargetsReady != "1/2" || kvm.Status.LastCheckTime == nil || !kvm.Status.LastCheckTime.Time.Equal(t1) { + t.Errorf("kvm-home status = %+v, want type kvm, 1/2 ready, checked at %s", kvm.Status, t1) + } + local := getHealth(t, c, "local") + if cond := readyCondition(local); cond == nil || cond.Status != metav1.ConditionTrue { + t.Errorf("local Ready = %+v, want True", cond) + } +} + +func TestMonitor_checkOnce_patchesExisting(t *testing.T) { + t.Parallel() + existing := managed("local") + existing.Status = crawlv1alpha1.ProviderHealthStatus{ + Type: "kubernetes", + Targets: []crawlv1alpha1.ProviderTarget{readyAt("apiserver", t0)}, + } + c := newFakeClient(t, existing) + m := newTestMonitor(c, Target{Name: "local", Type: "kubernetes", Checker: verdicts(up("apiserver"))}) + + m.checkOnce(context.Background()) + + got := getHealth(t, c, "local") + if got.Status.LastCheckTime == nil || !got.Status.LastCheckTime.Time.Equal(t1) { + t.Errorf("lastCheckTime = %v, want %s", got.Status.LastCheckTime, t1) + } + if lt := got.Status.Targets[0].LastTransitionTime.Time; !lt.Equal(t0) { + t.Errorf("apiserver lastTransitionTime = %s, want %s kept: it was ready before too", lt, t0) + } +} + +func TestMonitor_checkOnce_timeoutBoundsEachRound(t *testing.T) { + t.Parallel() + c := newFakeClient(t) + var hadDeadline atomic.Bool + hanging := checkFunc(func(ctx context.Context) []provider.TargetStatus { + _, ok := ctx.Deadline() + hadDeadline.Store(ok) + <-ctx.Done() + return []provider.TargetStatus{down("nas1", provider.ReasonTimeout)} + }) + m := newTestMonitor(c, + Target{Name: "kvm-home", Type: "kvm", Checker: hanging}, + Target{Name: "local", Type: "kubernetes", Checker: verdicts(up("apiserver"))}, + ) + m.Timeout = 50 * time.Millisecond + + done := make(chan struct{}) + go func() { + m.checkOnce(context.Background()) + close(done) + }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("checkOnce still running 5s after a 50ms timeout") + } + + if !hadDeadline.Load() { + t.Error("the checker got a context without a deadline") + } + if cond := readyCondition(getHealth(t, c, "kvm-home")); cond == nil || cond.Status != metav1.ConditionFalse { + t.Errorf("kvm-home Ready = %+v, want False", cond) + } + if cond := readyCondition(getHealth(t, c, "local")); cond == nil || cond.Status != metav1.ConditionTrue { + t.Errorf("local Ready = %+v, want True despite the slow neighbour", cond) + } +} + +func TestMonitor_checkOnce_noCheckerIsUnknown(t *testing.T) { + t.Parallel() + c := newFakeClient(t) + m := newTestMonitor(c, Target{Name: "gcp-eu", Type: "gcp"}) + + m.checkOnce(context.Background()) + + got := getHealth(t, c, "gcp-eu") + if cond := readyCondition(got); cond == nil || cond.Status != metav1.ConditionUnknown || cond.Reason != reasonChecksNotImplemented { + t.Errorf("Ready = %+v, want Unknown/%s", cond, reasonChecksNotImplemented) + } +} + +func TestMonitor_checkOnce_invalidNameSkipsTheObject(t *testing.T) { + t.Parallel() + c := newFakeClient(t) + m := newTestMonitor(c, Target{Name: "Home_KVM", Type: "kvm", Checker: verdicts(up("nas1"))}) + + m.checkOnce(context.Background()) + + var list crawlv1alpha1.ProviderHealthList + if err := c.List(context.Background(), &list); err != nil { + t.Fatal(err) + } + if len(list.Items) != 0 { + t.Errorf("wrote %d objects, want none for an invalid name", len(list.Items)) + } + snap := m.Snapshot() + if len(snap) != 1 || snap[0].Provider != "Home_KVM" || !snap[0].Targets[0].Ready { + t.Errorf("Snapshot() = %+v, want the result kept for metrics", snap) + } +} + +func TestMonitor_logsOnlyTransitions(t *testing.T) { + t.Parallel() + var mu sync.Mutex + var lines []string + log := funcr.New(func(_, args string) { + mu.Lock() + defer mu.Unlock() + lines = append(lines, args) + }, funcr.Options{}) + ctx := logr.NewContext(context.Background(), log) + rounds := [][]provider.TargetStatus{ + {up("nas1")}, + {up("nas1")}, + {down("nas1", provider.ReasonUnreachable)}, + {down("nas1", provider.ReasonUnreachable)}, + {down("nas1", provider.ReasonTimeout)}, + {up("nas1")}, + } + round := 0 + m := newTestMonitor(newFakeClient(t), Target{Name: "kvm-home", Type: "kvm", Checker: checkFunc( + func(context.Context) []provider.TargetStatus { return rounds[round] })}) + + for round = range rounds { + m.checkOnce(ctx) + } + + mu.Lock() + defer mu.Unlock() + want := [][]string{ + {`"msg"="WARNING: provider target not ready"`, `"reason"="Unreachable"`}, + {`"msg"="WARNING: provider target not ready"`, `"reason"="Timeout"`}, + {`"msg"="Provider target ready"`}, + } + if len(lines) != len(want) { + t.Fatalf("logged %d lines, want %d:\n%s", len(lines), len(want), strings.Join(lines, "\n")) + } + for i, subs := range want { + for _, sub := range subs { + if !strings.Contains(lines[i], sub) { + t.Errorf("line %d missing %s:\n%s", i, sub, lines[i]) + } + } + } +} + +func TestMonitor_cleanup_deletesOnlyLabelledUnconfigured(t *testing.T) { + t.Parallel() + foreign := &crawlv1alpha1.ProviderHealth{ObjectMeta: metav1.ObjectMeta{Name: "foreign"}} + c := newFakeClient(t, managed("kvm-home"), managed("retired"), foreign) + m := newTestMonitor(c) + + m.cleanup(context.Background(), map[string]bool{"kvm-home": true}) + + getHealth(t, c, "kvm-home") + getHealth(t, c, "foreign") + var gone crawlv1alpha1.ProviderHealth + if err := c.Get(context.Background(), client.ObjectKey{Name: "retired"}, &gone); !apierrors.IsNotFound(err) { + t.Errorf("Get retired = %v, want NotFound", err) + } +} + +func TestMonitor_Start_offCleansUpAndReturns(t *testing.T) { + t.Parallel() + c := newFakeClient(t, managed("kvm-home")) + var checked atomic.Bool + m := newTestMonitor(c, Target{Name: "kvm-home", Type: "kvm", Checker: checkFunc( + func(context.Context) []provider.TargetStatus { checked.Store(true); return nil })}) + m.Interval = 0 + + errc := make(chan error, 1) + go func() { errc <- m.Start(context.Background()) }() + select { + case err := <-errc: + if err != nil { + t.Fatalf("Start() = %v, want nil", err) + } + case <-time.After(5 * time.Second): + t.Fatal("Start did not return with checking off") + } + + if checked.Load() { + t.Error("a check ran with checking off") + } + var gone crawlv1alpha1.ProviderHealth + if err := c.Get(context.Background(), client.ObjectKey{Name: "kvm-home"}, &gone); !apierrors.IsNotFound(err) { + t.Errorf("Get kvm-home = %v, want NotFound so no stale Ready is left behind", err) + } +} + +func TestMonitor_Start_checksAtOnceAndStops(t *testing.T) { + t.Parallel() + c := newFakeClient(t) + m := newTestMonitor(c, Target{Name: "local", Type: "kubernetes", Checker: verdicts(up("apiserver"))}) + m.Interval = time.Hour + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + errc := make(chan error, 1) + go func() { errc <- m.Start(ctx) }() + deadline := time.Now().Add(5 * time.Second) + for len(m.Snapshot()) == 0 { + if time.Now().After(deadline) { + t.Fatal("no check within 5s of Start, want one right away") + } + time.Sleep(10 * time.Millisecond) + } + cancel() + + select { + case err := <-errc: + if err != nil { + t.Errorf("Start() = %v, want nil", err) + } + case <-time.After(5 * time.Second): + t.Fatal("Start did not return after its context ended") + } +} + +func TestValidateSchedule(t *testing.T) { + t.Parallel() + tests := []struct { + name string + interval, timeout time.Duration + wantErr bool + }{ + {"off", 0, 0, false}, + {"defaults", time.Minute, 15 * time.Second, false}, + {"shortest interval", 10 * time.Second, 10 * time.Second, false}, + {"too frequent", 5 * time.Second, time.Second, true}, + {"negative interval", -time.Minute, time.Second, true}, + {"no timeout", time.Minute, 0, true}, + {"timeout longer than interval", time.Minute, 2 * time.Minute, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if err := ValidateSchedule(tc.interval, tc.timeout); (err != nil) != tc.wantErr { + t.Errorf("ValidateSchedule(%s, %s) = %v, want error %v", tc.interval, tc.timeout, err, tc.wantErr) + } + }) + } +} diff --git a/internal/providerhealth/status.go b/internal/providerhealth/status.go new file mode 100644 index 0000000..6dcaeec --- /dev/null +++ b/internal/providerhealth/status.go @@ -0,0 +1,98 @@ +package providerhealth + +import ( + "fmt" + "slices" + "strings" + "unicode/utf8" + + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" +) + +// Reasons on the Ready condition. +const ( + reasonAllTargetsReady = "AllTargetsReady" + reasonTargetsNotReady = "TargetsNotReady" + reasonChecksNotImplemented = "ChecksNotImplemented" + reasonNoTargets = "NoTargets" +) + +// maxMessageLen matches the CRD's limit on a target's message. +const maxMessageLen = 1024 + +// buildStatus derives a ProviderHealth status from the previous one and a +// fresh result. Every timestamp comes from r.CheckedAt, and prev is left +// untouched. A target keeps its lastTransitionTime while ready does not +// flip, which is how several hosts each show when they went down. +func buildStatus(prev crawlv1alpha1.ProviderHealthStatus, r Result) crawlv1alpha1.ProviderHealthStatus { + now := metav1.NewTime(r.CheckedAt) + out := crawlv1alpha1.ProviderHealthStatus{ + Type: r.Type, + Conditions: slices.Clone(prev.Conditions), + LastCheckTime: &now, + } + before := make(map[string]crawlv1alpha1.ProviderTarget, len(prev.Targets)) + for _, t := range prev.Targets { + before[t.Name] = t + } + ready := 0 + var failing []string + for _, ts := range r.Targets { + t := crawlv1alpha1.ProviderTarget{ + Name: ts.Name, + Address: ts.Address, + Ready: ts.Ready, + Reason: ts.Reason, + Message: truncate(ts.Message, maxMessageLen), + LatencyMillis: ts.Latency.Milliseconds(), + LastTransitionTime: now, + Details: ts.Details, + } + if b, ok := before[ts.Name]; ok && b.Ready == ts.Ready { + t.LastTransitionTime = b.LastTransitionTime + } + if ts.Ready { + ready++ + } else { + failing = append(failing, ts.Name+": "+ts.Reason) + } + out.Targets = append(out.Targets, t) + } + if r.Supported { + out.TargetsReady = fmt.Sprintf("%d/%d", ready, len(r.Targets)) + } + + cond := metav1.Condition{Type: crawlv1alpha1.ConditionReady, LastTransitionTime: now} + switch { + case !r.Supported: + cond.Status = metav1.ConditionUnknown + cond.Reason = reasonChecksNotImplemented + cond.Message = fmt.Sprintf("%s providers have no health check", r.Type) + case len(r.Targets) == 0: + cond.Status = metav1.ConditionUnknown + cond.Reason = reasonNoTargets + cond.Message = "the provider has nothing to check" + case len(failing) == 0: + cond.Status = metav1.ConditionTrue + cond.Reason = reasonAllTargetsReady + cond.Message = fmt.Sprintf("%d of %d targets ready", ready, len(r.Targets)) + default: + cond.Status = metav1.ConditionFalse + cond.Reason = reasonTargetsNotReady + cond.Message = truncate(strings.Join(failing, "; "), maxMessageLen) + } + apimeta.SetStatusCondition(&out.Conditions, cond) + return out +} + +// truncate cuts s to at most n characters, which is how the API server +// counts maxLength. +func truncate(s string, n int) string { + if utf8.RuneCountInString(s) <= n { + return s + } + return string([]rune(s)[:n-1]) + "…" +} diff --git a/internal/providerhealth/status_test.go b/internal/providerhealth/status_test.go new file mode 100644 index 0000000..6e3ac0e --- /dev/null +++ b/internal/providerhealth/status_test.go @@ -0,0 +1,135 @@ +package providerhealth + +import ( + "strings" + "testing" + "time" + "unicode/utf8" + + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +func readyAt(name string, at time.Time) crawlv1alpha1.ProviderTarget { + return crawlv1alpha1.ProviderTarget{Name: name, Ready: true, Reason: provider.ReasonReachable, LastTransitionTime: metav1.NewTime(at)} +} + +func TestBuildStatus(t *testing.T) { + t.Parallel() + tests := []struct { + name string + prev crawlv1alpha1.ProviderHealthStatus + result Result + wantStatus metav1.ConditionStatus + wantReason string + wantMessage string + wantTargetsReady string + // wantTransitions is each target's lastTransitionTime, in order. + wantTransitions []time.Time + }{ + { + name: "all ready", + result: Result{Type: "kvm", Supported: true, Targets: []provider.TargetStatus{up("nas1"), up("nas2")}, CheckedAt: t1}, + wantStatus: metav1.ConditionTrue, + wantReason: reasonAllTargetsReady, + wantMessage: "2 of 2 targets ready", + wantTargetsReady: "2/2", + wantTransitions: []time.Time{t1, t1}, + }, + { + name: "one target down", + result: Result{Type: "kvm", Supported: true, Targets: []provider.TargetStatus{up("nas1"), down("nas2", provider.ReasonUnreachable)}, CheckedAt: t1}, + wantStatus: metav1.ConditionFalse, + wantReason: reasonTargetsNotReady, + wantMessage: "nas2: Unreachable", + wantTargetsReady: "1/2", + wantTransitions: []time.Time{t1, t1}, + }, + { + name: "only a flip moves a target's transition time", + prev: crawlv1alpha1.ProviderHealthStatus{Targets: []crawlv1alpha1.ProviderTarget{readyAt("nas1", t0), readyAt("nas2", t0)}}, + result: Result{Type: "kvm", Supported: true, CheckedAt: t1, + Targets: []provider.TargetStatus{up("nas1"), down("nas2", provider.ReasonTimeout)}}, + wantStatus: metav1.ConditionFalse, + wantReason: reasonTargetsNotReady, + wantMessage: "nas2: Timeout", + wantTargetsReady: "1/2", + wantTransitions: []time.Time{t0, t1}, + }, + { + name: "no checker", + result: Result{Type: "gcp", CheckedAt: t1}, + wantStatus: metav1.ConditionUnknown, + wantReason: reasonChecksNotImplemented, + }, + { + name: "nothing to check", + result: Result{Type: "kvm", Supported: true, CheckedAt: t1}, + wantStatus: metav1.ConditionUnknown, + wantReason: reasonNoTargets, + wantTargetsReady: "0/0", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := buildStatus(tc.prev, tc.result) + + cond := apimeta.FindStatusCondition(got.Conditions, crawlv1alpha1.ConditionReady) + if cond == nil || cond.Status != tc.wantStatus || cond.Reason != tc.wantReason { + t.Fatalf("Ready = %+v, want %s/%s", cond, tc.wantStatus, tc.wantReason) + } + if tc.wantMessage != "" && cond.Message != tc.wantMessage { + t.Errorf("Ready message = %q, want %q", cond.Message, tc.wantMessage) + } + if got.TargetsReady != tc.wantTargetsReady { + t.Errorf("targetsReady = %q, want %q", got.TargetsReady, tc.wantTargetsReady) + } + if got.Type != tc.result.Type || got.LastCheckTime == nil || !got.LastCheckTime.Time.Equal(t1) { + t.Errorf("type/lastCheckTime = %q/%v, want %q/%s", got.Type, got.LastCheckTime, tc.result.Type, t1) + } + if len(got.Targets) != len(tc.wantTransitions) { + t.Fatalf("got %d targets, want %d", len(got.Targets), len(tc.wantTransitions)) + } + for i, want := range tc.wantTransitions { + if lt := got.Targets[i].LastTransitionTime.Time; !lt.Equal(want) { + t.Errorf("target %s lastTransitionTime = %s, want %s", got.Targets[i].Name, lt, want) + } + } + }) + } +} + +func TestBuildStatus_conditionKeepsTransitionWhileUnchanged(t *testing.T) { + t.Parallel() + first := buildStatus(crawlv1alpha1.ProviderHealthStatus{}, + Result{Type: "kvm", Supported: true, Targets: []provider.TargetStatus{up("nas1")}, CheckedAt: t0}) + firstCond := apimeta.FindStatusCondition(first.Conditions, crawlv1alpha1.ConditionReady) + + second := buildStatus(first, Result{Type: "kvm", Supported: true, Targets: []provider.TargetStatus{up("nas1")}, CheckedAt: t1}) + + cond := apimeta.FindStatusCondition(second.Conditions, crawlv1alpha1.ConditionReady) + if !cond.LastTransitionTime.Time.Equal(t0) { + t.Errorf("Ready lastTransitionTime = %s, want %s from the first round", cond.LastTransitionTime, t0) + } + if !firstCond.LastTransitionTime.Time.Equal(t0) || len(first.Conditions) != 1 { + t.Errorf("buildStatus modified the previous status: %+v", first.Conditions) + } +} + +func TestBuildStatus_truncatesLongMessages(t *testing.T) { + t.Parallel() + long := strings.Repeat("é", 3000) + got := buildStatus(crawlv1alpha1.ProviderHealthStatus{}, Result{Type: "kvm", Supported: true, CheckedAt: t1, + Targets: []provider.TargetStatus{{Name: "nas1", Reason: provider.ReasonUnreachable, Message: long}}}) + + if n := utf8.RuneCountInString(got.Targets[0].Message); n != maxMessageLen { + t.Errorf("message has %d characters, want %d", n, maxMessageLen) + } + if !strings.HasSuffix(got.Targets[0].Message, "…") { + t.Error("truncated message does not end with an ellipsis") + } +} -- 2.54.0 From 9a3ee963a0e3dec174cbd58731aef846b851b801 Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 20:23:55 +0000 Subject: [PATCH 06/12] Wire the provider health monitor into the manager, with gauges --provider-check-interval (1m; 0 = off, else at least 10s) and --provider-check-timeout (15s) are validated before the providers config loads. Checkers are collected from the unwrapped providers in config order, so checks never count as provider requests. Three scrape-time gauges per {provider,target} read the monitor's snapshot: up, check duration and check timestamp. The e2e suite waits for the kubernetes ProviderHealth to turn Ready and asserts its gauge in the scrape. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/main.go | 60 ++++++++++++++++++- config/manager/manager.yaml | 3 + .../2026-09-11-1526-provider-health-checks.md | 49 ++++++++++++++- internal/metrics/metrics.go | 48 +++++++++++++++ internal/metrics/metrics_test.go | 42 +++++++++++++ test/e2e/e2e_test.go | 16 +++++ 6 files changed, 215 insertions(+), 3 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 18d76cf..cf32fa5 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -16,8 +16,8 @@ limitations under the License. // Package main is the composition root: it loads the provider config (fail // fast), assembles the provider registry, and wires the reconciler, health -// engine, lease store, discovery API, orphan GC, and metrics onto one -// controller-runtime manager. +// engine, lease store, discovery API, orphan GC, provider health checks and +// metrics onto one controller-runtime manager. package main import ( @@ -60,6 +60,7 @@ import ( "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider/kubernetes" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider/kvm" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider/registry" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/providerhealth" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/tracing" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/version" // +kubebuilder:scaffold:imports @@ -97,6 +98,7 @@ func main() { var showVersion bool var gcpWireFullPayloads bool var traceHealthProbes bool + var providerCheckInterval, providerCheckTimeout time.Duration flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ "Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.") @@ -140,6 +142,12 @@ func main() { flag.BoolVar(&traceHealthProbes, "trace-health-probes", false, "Emit one trace span per health probe. Off by default: probes run about "+ "once per second per proxy and would dominate trace volume.") + flag.DurationVar(&providerCheckInterval, "provider-check-interval", time.Minute, + "Interval between provider health checks (can the cluster reach each hypervisor, API "+ + "and account), published as ProviderHealth objects and metrics. 0 turns them off; "+ + "otherwise at least 10s.") + flag.DurationVar(&providerCheckTimeout, "provider-check-timeout", 15*time.Second, + "Time limit for one round of provider health checks; at most the interval.") opts := zap.Options{ Development: true, @@ -174,6 +182,10 @@ func main() { setupLog.Error(nil, "--providers-config is required") os.Exit(1) } + if err := providerhealth.ValidateSchedule(providerCheckInterval, providerCheckTimeout); err != nil { + setupLog.Error(err, "Invalid provider check flags") + os.Exit(1) + } cfg, err := provider.LoadConfigFile(providersConfig) if err != nil { setupLog.Error(err, "Failed to load providers config", "path", providersConfig) @@ -195,6 +207,14 @@ func main() { os.Exit(1) } m := metrics.New() + // Checkers come from the unwrapped providers, in config order: the + // metrics and tracing wrappers implement only Provider, and a check is + // not a provider request to be counted anyway. + checkTargets := make([]providerhealth.Target, 0, len(cfg.Providers)) + for _, pc := range cfg.Providers { + checker, _ := providers[pc.Name].(provider.Checker) + checkTargets = append(checkTargets, providerhealth.Target{Name: pc.Name, Type: pc.Type, Checker: checker}) + } for name, p := range providers { // Tracing outermost: the span covers the metrics recording too. providers[name] = provider.WithTracing(name, provider.WithMetrics(name, p, m)) @@ -314,6 +334,18 @@ func main() { os.Exit(1) } + monitor := &providerhealth.Monitor{ + Client: mgr.GetClient(), + Reader: mgr.GetAPIReader(), + Targets: checkTargets, + Interval: providerCheckInterval, + Timeout: providerCheckTimeout, + } + if err := mgr.Add(monitor); err != nil { + setupLog.Error(err, "Failed to add provider health monitor") + os.Exit(1) + } + if err := m.Register(ctrlmetrics.Registry, proxyPhaseCounts(mgr.GetClient()), func() int { @@ -327,6 +359,10 @@ func main() { setupLog.Error(err, "Failed to register metrics") os.Exit(1) } + if err := metrics.RegisterProviderTargets(ctrlmetrics.Registry, providerTargetSamples(monitor)); err != nil { + setupLog.Error(err, "Failed to register provider check metrics") + os.Exit(1) + } if err := (&controller.ProxyReconciler{ Client: mgr.GetClient(), @@ -389,3 +425,23 @@ func proxyPhaseCounts(c client.Reader) func() map[string]int { return counts } } + +// providerTargetSamples flattens the monitor's latest results for the +// scrape-time provider-check gauges. +func providerTargetSamples(mon *providerhealth.Monitor) func() []metrics.ProviderTarget { + return func() []metrics.ProviderTarget { + var out []metrics.ProviderTarget + for _, r := range mon.Snapshot() { + for _, ts := range r.Targets { + out = append(out, metrics.ProviderTarget{ + Provider: r.Provider, + Target: ts.Name, + Up: ts.Ready, + Duration: ts.Latency, + CheckedAt: r.CheckedAt, + }) + } + } + return out + } +} diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 0f9a83a..1e21b85 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -66,6 +66,9 @@ spec: - --providers-config=/etc/proxy-operator/providers.yaml # Add --trace-health-probes to emit one span per health probe # (high volume: ~1/s per proxy). + # Provider health checks (kubectl get providerhealth) run every + # minute; tune with --provider-check-interval (0 turns them off, + # otherwise at least 10s) and --provider-check-timeout (15s). env: # Bearer token for the discovery API. Optional: without the # Secret the API serves unauthenticated (with a loud warning). 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 861ce6a..c87761b 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 @@ -6,7 +6,7 @@ Plan: [docs/plans/2026-09-11-1526-provider-health-checks.md](../plans/2026-09-11 - [x] Step 2 — Contract plus the kubernetes and GCP checks - [x] Step 3 — KVM check - [x] Step 4 — Monitor -- [ ] Step 5 — Metrics and wiring +- [x] Step 5 — Metrics and wiring - [ ] Step 6 — Docs - [ ] Step 7 — Validate, record, push, open the PR @@ -188,3 +188,50 @@ Worth noting: - **Test renamed:** the planned `TestMonitor_timeoutBoundsBlockingChecker` is `TestMonitor_checkOnce_timeoutBoundsEachRound`. It also asserts that a fast provider still publishes next to a hung one. + +## Step 5 — Metrics and wiring + +`metrics.RegisterProviderTargets` registers a scrape-time collector for three gauges, +each labelled `{provider,target}`: `up`, check duration and check timestamp. +`Register`'s signature is unchanged. + +`cmd/main.go`: +- gains `--provider-check-interval` (1m) and `--provider-check-timeout` (15s); +- validates them before even loading the providers config; +- collects the `Checker`s from the unwrapped providers in config order, just before the + metrics and tracing wrap loop; +- adds the Monitor with the manager's API reader; +- registers the collector through a small `providerTargetSamples` closure next to + `proxyPhaseCounts`. + +Build, vet and metrics tests: + +```bash +go build ./... && go vet ./cmd/... ./internal/metrics/ && go test -race -count=1 ./internal/metrics/ +``` + +To confirm bad flags fail fast, before anything touches the cluster, I ran the binary +with a 5s interval. zap's development mode prints a stack trace after the error, hence +the grep: + +```bash +go run ./cmd --providers-config=/dev/null --provider-check-interval=5s 2>&1 | grep -m1 "Invalid provider check flags" +# ERROR setup Invalid provider check flags {"error": "--provider-check-interval must be 0 (off) or at least 10s, got 5s"} +go vet -tags=e2e ./test/e2e/ +``` + +The e2e suite gets a new ordered spec, "should publish provider health". It waits for +`providerhealth kubernetes` to report `Ready=True`. It runs before the existing metrics +spec, which now also asserts +`proxy_operator_provider_target_up{provider="kubernetes",target="apiserver"} 1`. By then +the first round is known to be done, so the series is certain to exist. + +Worth noting: +- **Main-only wiring:** `providerTargetSamples` is an 8-line loop in `package main`, + which has no test file. The e2e metrics assertion is its only coverage. +- **The KVM entry isn't asserted in e2e:** the default providers config also ships + `kvm-home` (beelink), which would report `SSHMaterialUnavailable` in kind. That entry + is the user's own host, and asserting it would break the suite as soon as the default + config changes. +- **Manifest:** `config/manager/manager.yaml` only gained a comment about the two flags. + The defaults need no args. diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index b827395..c1b07c1 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -117,3 +117,51 @@ func (c *constCollector) Collect(ch chan<- prometheus.Metric) { ch <- prometheus.MustNewConstMetric(c.desc, prometheus.GaugeValue, float64(value), label) } } + +// ProviderTarget is one provider health-check target as the gauges see it. +type ProviderTarget struct { + Provider, Target string + Up bool + Duration time.Duration + CheckedAt time.Time +} + +// RegisterProviderTargets registers the provider-check gauges. Like the +// other scrape-time collectors they are read from the source at every +// scrape, so a target that goes away stops being reported rather than +// leaving a stale series behind. +func RegisterProviderTargets(reg prometheus.Registerer, read func() []ProviderTarget) error { + labels := []string{"provider", "target"} + return reg.Register(&providerTargetCollector{ + up: prometheus.NewDesc("proxy_operator_provider_target_up", + "Whether the target passed its latest provider health check (1) or not (0).", labels, nil), + duration: prometheus.NewDesc("proxy_operator_provider_check_duration_seconds", + "Duration of the target's latest provider health check.", labels, nil), + checked: prometheus.NewDesc("proxy_operator_provider_check_timestamp_seconds", + "Unix time of the target's latest provider health check.", labels, nil), + read: read, + }) +} + +type providerTargetCollector struct { + up, duration, checked *prometheus.Desc + read func() []ProviderTarget +} + +func (c *providerTargetCollector) Describe(ch chan<- *prometheus.Desc) { + ch <- c.up + ch <- c.duration + ch <- c.checked +} + +func (c *providerTargetCollector) Collect(ch chan<- prometheus.Metric) { + for _, t := range c.read() { + up := 0.0 + if t.Up { + up = 1 + } + ch <- prometheus.MustNewConstMetric(c.up, prometheus.GaugeValue, up, t.Provider, t.Target) + ch <- prometheus.MustNewConstMetric(c.duration, prometheus.GaugeValue, t.Duration.Seconds(), t.Provider, t.Target) + ch <- prometheus.MustNewConstMetric(c.checked, prometheus.GaugeValue, float64(t.CheckedAt.UnixNano())/1e9, t.Provider, t.Target) + } +} diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go index d6e87a7..d55e3ac 100644 --- a/internal/metrics/metrics_test.go +++ b/internal/metrics/metrics_test.go @@ -111,3 +111,45 @@ func TestRegister_freshRegistryPerTest(t *testing.T) { register(t, m1, nil, 0) register(t, m2, nil, 0) } + +func TestRegisterProviderTargets_scrape(t *testing.T) { + t.Parallel() + checked := time.Unix(1757602800, 0) + targets := []ProviderTarget{ + {Provider: "kvm-home", Target: "nas1", Up: true, Duration: 84 * time.Millisecond, CheckedAt: checked}, + {Provider: "kvm-home", Target: "nas2", Up: false, Duration: 15 * time.Second, CheckedAt: checked}, + } + reg := prometheus.NewRegistry() + if err := RegisterProviderTargets(reg, func() []ProviderTarget { return targets }); err != nil { + t.Fatalf("RegisterProviderTargets: %v", err) + } + + expected := ` +# HELP proxy_operator_provider_check_duration_seconds Duration of the target's latest provider health check. +# TYPE proxy_operator_provider_check_duration_seconds gauge +proxy_operator_provider_check_duration_seconds{provider="kvm-home",target="nas1"} 0.084 +proxy_operator_provider_check_duration_seconds{provider="kvm-home",target="nas2"} 15 +# HELP proxy_operator_provider_check_timestamp_seconds Unix time of the target's latest provider health check. +# TYPE proxy_operator_provider_check_timestamp_seconds gauge +proxy_operator_provider_check_timestamp_seconds{provider="kvm-home",target="nas1"} 1.7576028e+09 +proxy_operator_provider_check_timestamp_seconds{provider="kvm-home",target="nas2"} 1.7576028e+09 +# HELP proxy_operator_provider_target_up Whether the target passed its latest provider health check (1) or not (0). +# TYPE proxy_operator_provider_target_up gauge +proxy_operator_provider_target_up{provider="kvm-home",target="nas1"} 1 +proxy_operator_provider_target_up{provider="kvm-home",target="nas2"} 0 +` + if err := testutil.GatherAndCompare(reg, strings.NewReader(expected)); err != nil { + t.Error(err) + } +} + +func TestRegisterProviderTargets_emptySnapshotHasNoSeries(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + if err := RegisterProviderTargets(reg, func() []ProviderTarget { return nil }); err != nil { + t.Fatalf("RegisterProviderTargets: %v", err) + } + if n, err := testutil.GatherAndCount(reg); err != nil || n != 0 { + t.Errorf("series before the first check = %d (%v), want 0", n, err) + } +} diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 06244f3..abc9f79 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -173,6 +173,20 @@ var _ = Describe("Manager", Ordered, func() { Eventually(verifyControllerUp).Should(Succeed()) }) + // Runs before the metrics spec on purpose: once a ProviderHealth is + // Ready, the first round of checks is done and its gauges exist. + It("should publish provider health", func() { + By("waiting for the kubernetes provider's ProviderHealth to report Ready") + verifyProviderReady := func(g Gomega) { + cmd := exec.Command("kubectl", "get", "providerhealth", "kubernetes", + "-o", `jsonpath={.status.conditions[?(@.type=="Ready")].status}`) + output, err := utils.Run(cmd) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(output).To(Equal("True"), "kubernetes provider not Ready") + } + Eventually(verifyProviderReady).Should(Succeed()) + }) + It("should ensure the metrics endpoint is serving metrics", func() { By("creating a ClusterRoleBinding for the service account to allow access to metrics") cmd := exec.Command("kubectl", "create", "clusterrolebinding", metricsRoleBindingName, @@ -264,6 +278,8 @@ var _ = Describe("Manager", Ordered, func() { g.Expect(err).NotTo(HaveOccurred(), "Failed to retrieve logs from curl pod") g.Expect(metricsOutput).NotTo(BeEmpty()) g.Expect(metricsOutput).To(ContainSubstring("< HTTP/1.1 200 OK")) + g.Expect(metricsOutput).To(ContainSubstring( + `proxy_operator_provider_target_up{provider="kubernetes",target="apiserver"} 1`)) } Eventually(verifyMetricsAvailable, 2*time.Minute).Should(Succeed()) }) -- 2.54.0 From 19b4a82ae9d1cf0feeea08e2bbe41e4793b854ed Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 20:28:09 +0000 Subject: [PATCH 07/12] 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 -- 2.54.0 From 0e456affdd19c7ebacb93661c3ad0b34a5a888b3 Mon Sep 17 00:00:00 2001 From: kacerr Date: Fri, 11 Sep 2026 20:31:15 +0000 Subject: [PATCH 08/12] Record the automated validation of the provider health checks make test (envtest, -race) and the kind e2e suite pass; the e2e ProviderHealth for the kubernetes provider turned Ready within a second of the manager starting and its gauge appeared in the scrape. make lint is blocked by the local go1.27 toolchain crashing staticcheck on the standard library, unrelated to this branch. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-11-1526-provider-health-checks.md | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) 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 dc4adf2..550e397 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 @@ -266,3 +266,49 @@ Worth noting: - `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. + +## Step 7 — Validate, record, push, open the PR + +The full suite runs codegen, `go fmt` and `go vet`, then `go test -race` on every +package, with both envtest suites included: + +```bash +make test +# every package ok; internal/providerhealth 92.5 %, internal/provider/kvm 92.0 % coverage +``` + +`go fmt` rewrote nothing, and `git status` showed only the Step 6 docs. + +`make lint` could not run, for a reason unrelated to this branch. The Makefile rebuilt +the custom golangci-lint, which reports "built with go1.27.0", and its bundled +staticcheck (honnef.co/go/tools v0.7.0) panics on the standard library before it +reaches any repo package: + +```bash +make lint +# buildir: package "poll" (isInitialPkg: false, needAnalyzeSource: true): unexpected expr: *ast.KeyValueExpr +# make: *** [Makefile:107: lint] Error 3 +``` + +The local toolchain is go1.27.0 while `go.mod` says 1.26.0; the otel-tracing plan's +log shows `make lint` running normally before the toolchain moved. Retrying with +`GOTOOLCHAIN=go1.26.0` would have meant downloading a toolchain, and the user declined +that. `go vet` is clean in all three configurations: plain inside `make test`, plus +`-tags=integration` and `-tags=e2e`. + +The kind e2e suite creates and deletes its own `egress-proxies-operator-test-e2e` +cluster. I checked `KIND_CLUSTER` first, because the two clusters already on the +machine (`kind`, `proxy-operator-demo`) must not be torn down by `cleanup-test-e2e`: + +```bash +make test-e2e > /e2e.log 2>&1 +# Ran 3 of 5 Specs in 86.210 seconds +# SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 2 Skipped +``` + +In the deployed manager, the `kubernetes` ProviderHealth reported `Ready=True` about a +second after the pod was up (20:30:05.6 → 20:30:06.7). The metrics scrape that followed +contained `proxy_operator_provider_target_up{provider="kubernetes",target="apiserver"} 1`. +That exercises the generated RBAC, the API-reader path and the `main.go` gauge wiring +end to end. The two skipped specs are the Tempo-gated tracing specs; `TEMPO_URL` was not +set. -- 2.54.0 From 4e5b5d62df979ea01bf13deeea7ec12211044120 Mon Sep 17 00:00:00 2001 From: kacerr Date: Sat, 12 Sep 2026 05:04:45 +0000 Subject: [PATCH 09/12] Probe hosts for health instead of risking the shared connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The health check shares each host's libvirt connection with the reconciles, so it must not be able to take that connection away from them. - A timed-out check no longer drops the link. The check goes through the new hostPool.probe, which abandons its RPCs when the context ends and leaves the connection alone, so an image upload or a Create running on the same link is not cut off. A link that really died is closed by the keepalive. One abandoned probe per host may stay waiting; until it returns, the next check reports Timeout at once (errProbeBusy). - Waiting for a host's connection slot now ends with the caller's context. hostEntry used a mutex, so a check could wait out a reconcile redialing a dead host — minutes — past its own timeout. The slot is a one-slot channel and acquire bounds only the wait, never the uncontended take. - --provider-check-timeout defaults to 0 and derives min(15s, interval). The flat 15s default contradicted the validation that the timeout may not exceed the interval, so --provider-check-interval=12s alone failed to start. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/main.go | 5 +- config/manager/manager.yaml | 3 +- docs/architecture.md | 12 ++- docs/kvm-provider.md | 13 ++- .../2026-09-11-1526-provider-health-checks.md | 51 ++++++++++ docs/testing.md | 2 +- internal/provider/kvm/check.go | 14 +-- internal/provider/kvm/check_test.go | 53 ++++++++++- internal/provider/kvm/conn.go | 95 ++++++++++++++++--- internal/provider/kvm/conn_test.go | 91 ++++++++++++++++++ internal/providerhealth/monitor.go | 28 +++--- internal/providerhealth/monitor_test.go | 25 ++++- 12 files changed, 349 insertions(+), 43 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index cf32fa5..acbae79 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -146,8 +146,9 @@ func main() { "Interval between provider health checks (can the cluster reach each hypervisor, API "+ "and account), published as ProviderHealth objects and metrics. 0 turns them off; "+ "otherwise at least 10s.") - flag.DurationVar(&providerCheckTimeout, "provider-check-timeout", 15*time.Second, - "Time limit for one round of provider health checks; at most the interval.") + flag.DurationVar(&providerCheckTimeout, "provider-check-timeout", 0, + "Time limit for one round of provider health checks: 15s, or the interval when that is "+ + "shorter. If set, at most the interval.") opts := zap.Options{ Development: true, diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 1e21b85..8b3c5ee 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -68,7 +68,8 @@ spec: # (high volume: ~1/s per proxy). # Provider health checks (kubectl get providerhealth) run every # minute; tune with --provider-check-interval (0 turns them off, - # otherwise at least 10s) and --provider-check-timeout (15s). + # otherwise at least 10s) and --provider-check-timeout (15s, or + # the interval if that is shorter). env: # Bearer token for the discovery API. Optional: without the # Secret the API serves unauthenticated (with a loud warning). diff --git a/docs/architecture.md b/docs/architecture.md index fc3fded..c847d55 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -378,7 +378,8 @@ target and never fails as a whole. All three providers do: ```text Monitor.Start(ctx) ── leader-only; --provider-check-interval (1m; 0 = off, - │ otherwise ≥ 10s), --provider-check-timeout (15s) + │ otherwise ≥ 10s), --provider-check-timeout (15s, or + │ the interval if that is shorter) ├─ 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 @@ -598,8 +599,13 @@ they read as choices, not accidents. Chronological by build step. 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. + Sharing must not cost the reconciles anything, so the check calls + through `hostPool.probe`. A check that runs out of time abandons its + RPCs instead of closing the connection, which would kill an upload or a + Create running on it. At most one abandoned check per host is left + waiting, and until it returns, the next check reports `Timeout` at once. + Waiting for a host's connection slot also ends with the caller's + deadline, so a reconcile redialing a dead host cannot stall the round. - **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 diff --git a/docs/kvm-provider.md b/docs/kvm-provider.md index 0f75c27..feab3f5 100644 --- a/docs/kvm-provider.md +++ b/docs/kvm-provider.md @@ -292,7 +292,7 @@ shows its SSH `user`. A failing target says where the check broke: | `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 | +| `Timeout` | no answer within `--provider-check-timeout` (default 15s), or the previous check of the host is still waiting | a hung `libvirtd`, a saturated link, or a reconcile stuck redialing the host | | `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 | @@ -302,9 +302,14 @@ 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. +- A check that runs out of time gives up on its RPCs but leaves the + connection alone, so an image upload or a Create running on it is not cut + off. At most one abandoned check per host keeps waiting; until it + returns, the next check reports `Timeout` straight away. A link that is + really dead is closed by the keepalive within about two keepalive + intervals. +- A check never waits longer than its timeout for the host's connection, + even while a reconcile holds it to redial a host that is down. ## Known gaps 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 550e397..60b0a0c 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 @@ -312,3 +312,54 @@ contained `proxy_operator_provider_target_up{provider="kubernetes",target="apise That exercises the generated RBAC, the API-reader path and the `main.go` gauge wiring end to end. The two skipped specs are the Tempo-gated tracing specs; `TEMPO_URL` was not set. + +### Follow-up — the check must not cost the reconciles their connection + +Reviewing the shared-connection decision after the automated validation turned up two +ways the health check could hurt the work it shares the link with, both of which the +plan's "a check against a hung host drops the shared connection" line accepted too +easily: + +- A check that timed out went through `hostPool.call`, whose cancel path drops the + connection. A `libvirtd` slow to answer one `ConnectGetLibVersion` would therefore + cut off an image upload or a `Create` running on the same link. +- `hostEntry` was guarded by a `sync.Mutex`, so waiting for a host's slot could not be + interrupted. A reconcile redialing a host that is down holds the slot for as long as + the SSH dial takes — minutes on a blackholed address — and the whole check round + would wait behind it, past its own timeout. + +So the check now calls `hostPool.probe` instead of `call`: on `ctx` ending it abandons +the RPC and leaves the connection alone (a link that is really dead is closed by the +keepalive anyway, which ends the abandoned `fn` too). One abandoned probe per host is +allowed to stay waiting — `hostEntry.probing` is an `atomic.Bool`, not a counter, so a +daemon that never answers cannot pile up goroutines; further probes get the new +`errProbeBusy`, which `checkReason` maps to `Timeout`. `hostEntry.mu` became a one-slot +channel with an `acquire(ctx)` that only bounds *waiting*: an uncontended caller takes a +free slot even with an expired context, so `call`'s behaviour is unchanged where nothing +is contended. + +The second change came out of the same pass: `--provider-check-timeout` defaulted to a +flat 15s while the flag validation rejected a timeout longer than the interval, so +`--provider-check-interval=12s` alone failed to start. The flag now defaults to 0 and +`Monitor.setDefaults` derives `min(15s, Interval)`; `ValidateSchedule` accepts 0 and +rejects only negatives and timeouts past the interval. + +Re-verified with the same commands as above: + +```bash +go vet ./... +go test -race -count=1 ./internal/provider/kvm/ ./internal/providerhealth/ +make test +# every package ok; internal/provider/kvm 92.2 %, internal/providerhealth 93.7 % coverage +``` + +Worth noting: +- `make lint` still cannot run, for the toolchain reason recorded above; unchanged by + this follow-up. +- The two new `kvm` tests are timing-shaped by necessity — they assert that a caller + with a 50ms deadline does not wait out a dial that is blocked on a channel. The + assertions are one-sided (`> 1s` fails) so a slow machine does not make them flaky. +- `docs/architecture.md`, `docs/kvm-provider.md` and `docs/testing.md` were corrected in + the same commit: the old text promised the drop-on-timeout behaviour that is now gone. +- The real-hypervisor commands from the plan's Verification section are still the user's + to run; nothing here replaces them. diff --git a/docs/testing.md b/docs/testing.md index 2eb97d6..e7ac883 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -60,7 +60,7 @@ manual runs too. | | `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 | -| | `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 | +| | `check_test.go`, `conn_test.go` | health check: details, per-host verdicts in config order, hosts checked concurrently, a timeout that keeps the connection and refuses a second check while the first still waits, no waiting behind a reconcile's dial, the reason table; `probe` abandons without closing, but still drops on a transport error; waiting for a host's slot ends with the caller's deadline; 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` | diff --git a/internal/provider/kvm/check.go b/internal/provider/kvm/check.go index a2d7f51..a700731 100644 --- a/internal/provider/kvm/check.go +++ b/internal/provider/kvm/check.go @@ -48,10 +48,12 @@ func (p *Provider) Check(ctx context.Context) []provider.TargetStatus { return out } -// checkHost returns the RPC errors from fn untouched: call drops the -// shared connection on anything that is not a libvirt error, and a pool -// that exists but is stopped says nothing about the link. facts is read -// only after call returned nil, since an abandoned fn may still write it. +// checkHost goes through probe, so a check that runs out of time abandons +// its RPCs rather than closing the connection the reconciles share. fn +// returns the RPC errors untouched, since a transport error still drops the +// link; a pool that exists but is stopped says nothing about the link, so +// that verdict is reached afterwards. facts is read only after probe +// returned nil, since an abandoned fn may still write it. func (p *Provider) checkHost(ctx context.Context, host provider.KVMHost) provider.TargetStatus { ts := provider.TargetStatus{ Name: host.Name, @@ -60,7 +62,7 @@ func (p *Provider) checkHost(ctx context.Context, host provider.KVMHost) provide } start := time.Now() var facts hostFacts - err := p.hosts.call(ctx, p.logger(ctx), host, func(api libvirtAPI) error { + err := p.hosts.probe(ctx, p.logger(ctx), host, func(api libvirtAPI) error { version, err := api.ConnectGetLibVersion() if err != nil { return err @@ -125,7 +127,7 @@ func checkReason(err error) string { } } switch { - case errors.Is(err, context.DeadlineExceeded), errors.Is(err, context.Canceled): + case errors.Is(err, context.DeadlineExceeded), errors.Is(err, context.Canceled), errors.Is(err, errProbeBusy): return provider.ReasonTimeout case hasCode(err, libvirt.ErrNoStoragePool): return reasonPoolNotFound diff --git a/internal/provider/kvm/check_test.go b/internal/provider/kvm/check_test.go index ca99639..022790d 100644 --- a/internal/provider/kvm/check_test.go +++ b/internal/provider/kvm/check_test.go @@ -7,6 +7,7 @@ import ( "io" "maps" "strings" + "sync" "sync/atomic" "testing" "time" @@ -174,14 +175,18 @@ func TestCheck_hostsCheckedConcurrently(t *testing.T) { } } -func TestCheck_timeout(t *testing.T) { +// A daemon that stopped answering times the check out, but the connection +// the reconciles share stays up, and the next check does not stack a +// second hung RPC on top of the first. +func TestCheck_timeoutKeepsTheConnection(t *testing.T) { t.Parallel() api := newFakeAPI("default") block := make(chan struct{}) t.Cleanup(func() { close(block) }) blocked := &blockingAPI{fakeAPI: api, block: block} + var closed atomic.Bool dial := func(context.Context, provider.KVMHost) (*hostClient, error) { - return &hostClient{api: blocked, close: func() {}}, nil + return &hostClient{api: blocked, close: func() { closed.Store(true) }}, nil } pc := testConfig("nas1") p := newWithDialer(pc.Name, *pc.KVM, dial, nil) @@ -193,6 +198,49 @@ func TestCheck_timeout(t *testing.T) { if got[0].Ready || got[0].Reason != provider.ReasonTimeout { t.Errorf("Check() = %+v, want Timeout", got[0]) } + if closed.Load() { + t.Error("the timed-out check closed the connection the reconciles share") + } + again := p.Check(context.Background()) + if again[0].Reason != provider.ReasonTimeout || !strings.Contains(again[0].Message, "still waiting") { + t.Errorf("second Check() = %+v, want an immediate Timeout while the first still waits", again[0]) + } +} + +// A reconcile redialing a dead host holds the host's slot for minutes; the +// check must give up at its own deadline rather than wait that out. +func TestCheck_doesNotWaitBehindAReconcileDial(t *testing.T) { + t.Parallel() + release := make(chan struct{}) + dialing := make(chan struct{}) + var once sync.Once + dial := func(context.Context, provider.KVMHost) (*hostClient, error) { + once.Do(func() { close(dialing) }) + <-release + return nil, errHostOff + } + pc := testConfig("nas1") + p := newWithDialer(pc.Name, *pc.KVM, dial, nil) + reconcile := make(chan error, 1) + go func() { + _, err := p.Get(context.Background(), formatProviderID("nas1", testName)) + reconcile <- err + }() + <-dialing + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + start := time.Now() + + got := p.Check(ctx) + + if waited := time.Since(start); waited > time.Second { + t.Errorf("Check() took %s behind the reconcile's dial, want about 50ms", waited) + } + if got[0].Reason != provider.ReasonTimeout { + t.Errorf("Check() = %+v, want Timeout", got[0]) + } + close(release) + <-reconcile } // blockingAPI hangs in ConnectGetLibVersion until block closes, like a @@ -222,6 +270,7 @@ func TestCheckReason(t *testing.T) { &dialError{stage: stageSSHHandshake, err: context.DeadlineExceeded}), reasonSSHHandshakeFailed}, {"deadline", context.DeadlineExceeded, provider.ReasonTimeout}, {"cancelled", context.Canceled, provider.ReasonTimeout}, + {"previous check still waiting", errProbeBusy, provider.ReasonTimeout}, {"no pool", lvError(libvirt.ErrNoStoragePool, "no pool"), reasonPoolNotFound}, {"other libvirt error", lvError(libvirt.ErrInternalError, "boom"), reasonLibvirtError}, {"link died mid-call", io.EOF, provider.ReasonUnreachable}, diff --git a/internal/provider/kvm/conn.go b/internal/provider/kvm/conn.go index da7ff1a..5b962c0 100644 --- a/internal/provider/kvm/conn.go +++ b/internal/provider/kvm/conn.go @@ -9,6 +9,7 @@ import ( "net" "os" "sync" + "sync/atomic" "time" libvirt "github.com/digitalocean/go-libvirt" @@ -102,9 +103,16 @@ type hostPool struct { entries map[string]*hostEntry } +// hostEntry is one host's slot in the pool. lock is a one-slot semaphore +// rather than a mutex so that waiting for it can end with the caller's +// context: whoever holds it may be dialing a dead host for as long as the +// dial takes. type hostEntry struct { - mu sync.Mutex + lock chan struct{} client *hostClient + // probing is set while a probe's fn runs, including one whose caller + // has already given up on it. + probing atomic.Bool } func newHostPool(dial hostDialer) *hostPool { @@ -116,16 +124,37 @@ func (p *hostPool) entry(name string) *hostEntry { defer p.mu.Unlock() e, ok := p.entries[name] if !ok { - e = &hostEntry{} + e = &hostEntry{lock: make(chan struct{}, 1)} p.entries[name] = e } return e } +// acquire takes e.lock. Only waiting is bounded by ctx: a free slot is +// taken even when ctx has already ended, so an uncontended caller behaves +// as it did with a mutex, and a contended one gives up with ctx's error. +func (e *hostEntry) acquire(ctx context.Context) error { + select { + case e.lock <- struct{}{}: + return nil + default: + } + select { + case e.lock <- struct{}{}: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} + +func (e *hostEntry) release() { <-e.lock } + func (p *hostPool) get(ctx context.Context, log logr.Logger, host provider.KVMHost) (*hostClient, error) { e := p.entry(host.Name) - e.mu.Lock() - defer e.mu.Unlock() + if err := e.acquire(ctx); err != nil { + return nil, fmt.Errorf("waiting for the connection to host %s (%s): %w", host.Name, host.Address, err) + } + defer e.release() if e.client != nil { if e.client.alive() { return e.client, nil @@ -149,8 +178,8 @@ func (p *hostPool) get(ctx context.Context, log logr.Logger, host provider.KVMHo // connection that was already replaced by a redial is left alone. func (p *hostPool) drop(host string, c *hostClient) { e := p.entry(host) - e.mu.Lock() - defer e.mu.Unlock() + e.lock <- struct{}{} + defer e.release() if e.client == c { e.client = nil } @@ -173,19 +202,61 @@ func (p *hostPool) call(ctx context.Context, log logr.Logger, host provider.KVMH if err != nil { return err } - api := c.api - if log.V(1).Enabled() { - api = &tracingAPI{inner: c.api, log: log} - } - err = awaitOrCancel(ctx, func() error { return fn(api) }, func() { + err = awaitOrCancel(ctx, func() error { return fn(traced(c, log)) }, func() { log.V(1).Info("libvirt connection dropped", "reason", "context cancelled", "error", ctx.Err().Error()) p.drop(host.Name, c) }) + p.dropOnTransportError(ctx, log, host, c, err) + return err +} + +// errProbeBusy refuses a probe while the previous one on the same host has +// not come back yet. +var errProbeBusy = errors.New("the previous check of this host is still waiting for libvirt to answer") + +// probe is call for the health check, which must never cost the reconciles +// their connection: when ctx ends first, fn is abandoned and the connection +// left alone. A link that is really dead is closed by the keepalive, which +// ends fn too. At most one probe per host runs at a time, an abandoned one +// included, so a daemon that stopped answering cannot pile up goroutines; +// until it answers, further probes get errProbeBusy. +func (p *hostPool) probe(ctx context.Context, log logr.Logger, host provider.KVMHost, fn func(api libvirtAPI) error) error { + log = log.WithName("libvirt") + e := p.entry(host.Name) + if !e.probing.CompareAndSwap(false, true) { + return errProbeBusy + } + c, err := p.get(ctx, log, host) + if err != nil { + e.probing.Store(false) + return err + } + err = awaitOrCancel(ctx, func() error { + defer e.probing.Store(false) + return fn(traced(c, log)) + }, func() { + log.V(1).Info("libvirt probe abandoned, connection kept", "error", ctx.Err().Error()) + }) + p.dropOnTransportError(ctx, log, host, c, err) + return err +} + +// traced returns c's API, logging every RPC when log has V(1) enabled. +func traced(c *hostClient, log logr.Logger) libvirtAPI { + if log.V(1).Enabled() { + return &tracingAPI{inner: c.api, log: log} + } + return c.api +} + +// dropOnTransportError drops c when err is evidence against the link: +// neither a libvirt error, nor a verdict already classified, nor the +// caller's own context ending. +func (p *hostPool) dropOnTransportError(ctx context.Context, log logr.Logger, host provider.KVMHost, c *hostClient, err error) { if err != nil && !isLibvirtError(err) && !isClassified(err) && !errors.Is(err, ctx.Err()) { log.V(1).Info("libvirt connection dropped", "reason", "transport error", "error", err.Error()) p.drop(host.Name, c) } - return err } // awaitOrCancel runs fn in the background and returns its result, unless ctx diff --git a/internal/provider/kvm/conn_test.go b/internal/provider/kvm/conn_test.go index 722c948..b3a777b 100644 --- a/internal/provider/kvm/conn_test.go +++ b/internal/provider/kvm/conn_test.go @@ -232,6 +232,97 @@ func TestHostPool_call_hostsAreIndependent(t *testing.T) { } } +// A dial to a dead host holds the host's slot for as long as it takes; a +// caller with a deadline must not wait past it. +func TestHostPool_get_waitingRespectsContext(t *testing.T) { + t.Parallel() + release := make(chan struct{}) + dialing := make(chan struct{}) + var once sync.Once + dial := func(context.Context, provider.KVMHost) (*hostClient, error) { + once.Do(func() { close(dialing) }) + <-release + return nil, errors.New("ssh dial: connection timed out") + } + p := newHostPool(dial) + slow := make(chan error, 1) + go func() { slow <- p.call(context.Background(), discard, testHost, noop) }() + <-dialing + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + start := time.Now() + + err := p.call(ctx, discard, testHost, noop) + + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("call() error = %v, want the caller's deadline", err) + } + if waited := time.Since(start); waited > time.Second { + t.Errorf("call() waited %s behind another caller's dial, want about 50ms", waited) + } + close(release) + if err := <-slow; err == nil { + t.Error("the blocked dial succeeded, want its error") + } +} + +func TestHostPool_probe_abandonsWithoutClosing(t *testing.T) { + t.Parallel() + d := &fakeDialer{} + p := newHostPool(d.dial) + unblock := make(chan struct{}) + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond) + defer cancel() + + err := p.probe(ctx, discard, testHost, func(libvirtAPI) error { + <-unblock // a daemon that stopped answering + return nil + }) + + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("probe() error = %v, want DeadlineExceeded", err) + } + if d.clients[0].closed.Load() { + t.Error("probe closed the shared connection when it gave up") + } + if err := p.probe(context.Background(), discard, testHost, noop); !errors.Is(err, errProbeBusy) { + t.Errorf("probe() while the abandoned one still waits = %v, want errProbeBusy", err) + } + if err := p.call(context.Background(), discard, testHost, noop); err != nil { + t.Errorf("call() on the kept connection = %v", err) + } + close(unblock) + deadline := time.Now().Add(time.Second) + for { + err := p.probe(context.Background(), discard, testHost, noop) + if err == nil { + break + } + if !errors.Is(err, errProbeBusy) || time.Now().After(deadline) { + t.Fatalf("probe() after the abandoned one returned = %v, want nil", err) + } + time.Sleep(time.Millisecond) + } + if d.dialCount() != 1 { + t.Errorf("dials = %d, want 1: nothing should have redialed", d.dialCount()) + } +} + +func TestHostPool_probe_dropsConnectionOnTransportError(t *testing.T) { + t.Parallel() + d := &fakeDialer{} + p := newHostPool(d.dial) + + err := p.probe(context.Background(), discard, testHost, func(libvirtAPI) error { return io.ErrUnexpectedEOF }) + + if !errors.Is(err, io.ErrUnexpectedEOF) { + t.Fatalf("probe() error = %v, want io.ErrUnexpectedEOF passed through", err) + } + if !d.clients[0].closed.Load() { + t.Error("a transport error during a probe left the connection cached") + } +} + // --- SSH dial against an in-process server --- func newKeyPair(t *testing.T) (ssh.Signer, []byte) { diff --git a/internal/providerhealth/monitor.go b/internal/providerhealth/monitor.go index e001ea7..133a6bc 100644 --- a/internal/providerhealth/monitor.go +++ b/internal/providerhealth/monitor.go @@ -73,7 +73,8 @@ type Monitor struct { // round runs at start: nothing races it, and early visibility is the // point. Interval time.Duration - // Timeout bounds each round (default 15s). + // Timeout bounds each round; 0 means 15s, or the interval when that is + // shorter. Timeout time.Duration now func() time.Time @@ -92,31 +93,36 @@ func (m *Monitor) NeedLeaderElection() bool { return true } // ValidateSchedule checks --provider-check-interval and // --provider-check-timeout. The interval is 0 (off) or at least 10s, which -// is what keeps one status write per provider per round cheap; the timeout -// is positive and no longer than the interval. +// is what keeps one status write per provider per round cheap. The timeout +// is 0 (derived from the interval, see Monitor.Timeout) or positive and no +// longer than the interval. func ValidateSchedule(interval, timeout time.Duration) error { switch { case interval == 0: return nil case interval < minInterval: return fmt.Errorf("--provider-check-interval must be 0 (off) or at least %s, got %s", minInterval, interval) - case timeout <= 0 || timeout > interval: - return fmt.Errorf("--provider-check-timeout must be positive and at most the interval (%s), got %s", interval, timeout) + case timeout < 0 || timeout > interval: + return fmt.Errorf("--provider-check-timeout must be at most the interval (%s), got %s", interval, timeout) } return nil } +func (m *Monitor) setDefaults() { + if m.Timeout == 0 { + m.Timeout = min(defaultTimeout, m.Interval) + } + if m.now == nil { + m.now = time.Now + } +} + // Start deletes the objects of providers that are no longer configured, // then checks every Interval until ctx ends. The providers config is read // once per process, so cleaning up once is enough. With checking off, // every object goes, so none is left claiming a stale Ready. func (m *Monitor) Start(ctx context.Context) error { - if m.Timeout == 0 { - m.Timeout = defaultTimeout - } - if m.now == nil { - m.now = time.Now - } + m.setDefaults() log := logf.FromContext(ctx).WithName("provider-health") ctx = logf.IntoContext(ctx, log) diff --git a/internal/providerhealth/monitor_test.go b/internal/providerhealth/monitor_test.go index b2616d3..5e2d402 100644 --- a/internal/providerhealth/monitor_test.go +++ b/internal/providerhealth/monitor_test.go @@ -340,7 +340,8 @@ func TestValidateSchedule(t *testing.T) { {"shortest interval", 10 * time.Second, 10 * time.Second, false}, {"too frequent", 5 * time.Second, time.Second, true}, {"negative interval", -time.Minute, time.Second, true}, - {"no timeout", time.Minute, 0, true}, + {"timeout left to derive, short interval", 12 * time.Second, 0, false}, + {"negative timeout", time.Minute, -time.Second, true}, {"timeout longer than interval", time.Minute, 2 * time.Minute, true}, } for _, tc := range tests { @@ -352,3 +353,25 @@ func TestValidateSchedule(t *testing.T) { }) } } + +func TestMonitor_setDefaults_timeout(t *testing.T) { + t.Parallel() + tests := []struct { + name string + interval, timeout, want time.Duration + }{ + {"default", time.Minute, 0, 15 * time.Second}, + {"interval shorter than the default", 12 * time.Second, 0, 12 * time.Second}, + {"explicit", time.Minute, 20 * time.Second, 20 * time.Second}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := &Monitor{Interval: tc.interval, Timeout: tc.timeout} + m.setDefaults() + if m.Timeout != tc.want { + t.Errorf("Timeout = %s, want %s", m.Timeout, tc.want) + } + }) + } +} -- 2.54.0 From 4e23347760772d9714484b7a72c46d0ede7d78fd Mon Sep 17 00:00:00 2001 From: kacerr Date: Sat, 12 Sep 2026 05:59:22 +0000 Subject: [PATCH 10/12] Point the KVM integration tests at a dedicated known_hosts Leaving KVM_TEST_KNOWN_HOSTS unset silently disables host-key verification for the run, so document it as part of the normal setup rather than as an optional extra, and suggest a kvm_known_hosts holding only the hypervisors. Co-Authored-By: Claude Opus 5 (1M context) --- docs/testing.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/testing.md b/docs/testing.md index e7ac883..cfa94d9 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -114,13 +114,19 @@ had. export KVM_TEST_HOST=nas1.home.lan:22 # SSH endpoint export KVM_TEST_USER=virt # must be in the libvirt group export KVM_TEST_SSH_KEY=$HOME/.ssh/id_ed25519 # private key for that user +export KVM_TEST_KNOWN_HOSTS=$HOME/.ssh/kvm_known_hosts # the hypervisors' host keys export KVM_TEST_BRIDGE=br0 # bridge to a network with DHCP export KVM_TEST_IMAGE_URL=https://cloud.debian.org/images/cloud/trixie/latest/debian-13-generic-amd64.qcow2 make test-integration-kvm ``` -Optional: `KVM_TEST_KNOWN_HOSTS` (without it host keys are **not** -verified — the log says so), `KVM_TEST_IMAGE_SHA256`, `KVM_TEST_POOL` +`KVM_TEST_KNOWN_HOSTS` is technically optional, but leaving it out turns +host-key verification **off** for the run (the log says so); keep a +`kvm_known_hosts` holding just the hypervisors rather than pointing the +test at your everyday `known_hosts`, so a re-keyed hypervisor is a +one-line fix in a file nothing else uses. + +Also optional: `KVM_TEST_IMAGE_SHA256`, `KVM_TEST_POOL` (default `default`), `KVM_TEST_SOCKET`, `KVM_TEST_TIMEOUT` (default `20m`, must cover the first-time image download), `KVM_TEST_KEEP` to leave the VM behind for `virsh console` when something goes wrong, -- 2.54.0 From 80b2e9c85ebfd2da75d8ea298587956276cc53cf Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 08:41:42 +0000 Subject: [PATCH 11/12] Plan the provider blueprint document Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-16-0841-provider-blueprint.md | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 docs/plans/2026-09-16-0841-provider-blueprint.md diff --git a/docs/plans/2026-09-16-0841-provider-blueprint.md b/docs/plans/2026-09-16-0841-provider-blueprint.md new file mode 100644 index 0000000..1c1ba5c --- /dev/null +++ b/docs/plans/2026-09-16-0841-provider-blueprint.md @@ -0,0 +1,119 @@ +# Plan: Provider blueprint (docs/providers.md) +**Created:** 2026-09-16 08:41 + +## Context + +The provider contract is spread across `docs/prompts/__initial-prompt.md` §5, +the comments in `internal/provider/{provider,errors,check}.go`, `docs/architecture.md` +§5 and §11, `docs/kvm-provider.md`, and plan Step 2/3/8. Many rules exist only in +code: providerID must be self-contained, UID/CreatedAt must be read back, Create +must survive a crash partway through, calls must stop when ctx is cancelled, +decorators are wired in `cmd/main.go`, and so on. With "~5 providers soon", the +next provider author needs one blueprint. It should say what a provider MUST, +SHOULD or MAY do, show which features each existing provider (kubernetes, gcp, +kvm) actually implements (with file:line evidence), give a checklist for adding +one, and list the gaps. + +Decisions (user): store in `docs/providers.md`; scope = current state plus a +separate "Gaps and proposed features" section (nothing implemented); wait for +`feat/provider-health` to merge, then branch off `main`. + +## Step 0 — Save the plan; wait for the merge + +- Right after ExitPlanMode, copy this plan to `docs/plans/2026-09-16-0841-provider-blueprint.md` + and commit it on its own. The current branch is `feat/provider-health`, so the + plan reaches `main` with that merge. CLAUDE.md requires the plan commit to + come before any branching. +- Stop and wait for the user to confirm that `feat/provider-health` is merged. + Then `git switch main && git pull`, and `git switch -c feat/provider-blueprint`. + +## Step 1 — Collect evidence (read-only, in parallel) + +Run 3 Explore agents at the same time, one each for `internal/provider/kubernetes`, +`internal/provider/gcp` and `internal/provider/kvm`, including their tests. Each +answers the **same questionnaire**, citing `file:line` for every answer and writing +"not implemented" explicitly when that's the case: + +1. **Identity**: providerID format and parsing; what it carries (zone, host, namespace). +2. **Create**: how it stays idempotent (name lookup, 409, DomainLookupByName); + whether it returns before the VM is up or waits; what happens when an earlier + attempt crashed partway; which Placement fields are used or ignored; how + cloud-init is delivered; how the port is used; how GC labels/UID are attached. +3. **Get**: native state → InstanceState mapping; IP discovery rules; UID and + CreatedAt readback; how NotFound is detected. +4. **Delete**: idempotency; what gets removed (disks, volumes, seeds); returns + before or after the resource is gone; unknown/malformed providerID behaviour. +5. **ListByTag**: scope (namespaces, zones, hosts); partial-failure behaviour; how + managed resources are recognised. +6. **Error classification**: table of backend error → NotFound/Quota/Transient/ + Permanent; whether quota is detected at all. +7. **Health check** (`Checker`): targets, provider-specific Reasons, Details keys. +8. **Config**: YAML section, defaults, validation (`internal/provider/config*.go`), + credentials/Secrets, which settings are read once per process. +9. **Connections and cancellation**: pooling, reconnects, timeouts, ctx honouring. +10. **Observability**: log keys and verbosity levels, spans (`kvm/trace.go`, gcp + `wirelog.go`), anything beyond the shared metrics/tracing decorators. +11. **Testing**: fakes, envtest/kind, `//go:build integration` tests, what is + not covered. +12. **Known gaps**: TODOs, stale comments, documented limitations. + +Meanwhile, read the shared layer myself: `internal/provider/{provider,errors,check, +name,config,metrics,tracing}.go`, `internal/provider/registry/registry.go`, +`cmd/main.go` (registry wiring, decorator order, Checker collection), and the +places that consume the contract, so every MUST rule traces to a caller that depends on it: +`internal/controller` (error-class branching, ErrNotFound → replace/adopt), +`internal/gc` (UID, CreatedAt age skip, ListByTag errors), +`internal/providerhealth`. Then cross-check against the existing docs listed in Context. + +## Step 2 — Write `docs/providers.md` + +Sections: + +1. **What a provider is**: a few sentences plus links to architecture.md §5/§11. +2. **The contract**: per method (Create/Get/Delete/ListByTag), MUST/SHOULD/MAY + rules, each followed by *why*: the reconciler or GC behaviour it protects, + linked to code. +3. **Error taxonomy**: what each class makes the reconciler do; how to map backend + errors; "unclassified = transient" pitfall. +4. **Identity and GC labels**: providerID guidance, `LabelManaged`/`LabelUID`, + CreatedAt, deterministic `NameFromUID`. +5. **Optional interfaces**: `Checker` (shared reasons, never fails as a whole, + collected before decorators). +6. **Feature matrix**: one row per questionnaire item, columns kubernetes/gcp/kvm, + cells `yes`/`partial`/`no` plus a short note. Put the file:line evidence in a + compact per-provider notes subsection under the table, not in the cells. +7. **Adding a provider: checklist**: package layout, config section and validation, + registry constructor, `cmd/main.go` wiring, Checker, error mapping with tests, + `name`/label tests, integration test behind a build tag, provider doc page, + architecture.md rows, demo script (optional). +8. **Gaps and proposed features**: current-state gaps per provider (e.g. no kvm + quota detection, orphan volumes invisible to GC, Secret rotation needs a + restart, ListByTag partial-failure inconsistency, stale `mock` references in + `provider.go` comments), plus proposals that are clearly marked as not + implemented. Only list them; don't design them. + +Also: link it from `docs/architecture.md` (Components table → provider contract row) +and from the CLAUDE.md "Architecture" section. Leave per-provider docs where they are. + +Follow the plan-execution convention: create `docs/plans-executions/2026-09-16-0841-provider-blueprint.md` +with the status checklist and step entries, and commit it with each step. + +## Step 3 — Verify, then open the MR + +- A verification agent (Explore) re-checks each matrix cell and each MUST rule + against the cited file:line and reports any mismatch. Fix what it finds. +- Check that every relative link in the new and edited docs resolves (grep the + `](` targets and `ls` each one). +- `go build ./... && go test -short ./...` as a sanity check (docs-only change, + expected no-op). +- Push `feat/provider-blueprint` and open the MR against `main` via the Gitea API, + following the project's push/MR memory note. After the user confirms, add a CHANGELOG.md entry. + +## Critical files + +- New: `docs/providers.md`, `docs/plans/2026-09-16-0841-provider-blueprint.md`, + `docs/plans-executions/2026-09-16-0841-provider-blueprint.md` +- Edited: `docs/architecture.md` (link), `CLAUDE.md` (link) +- Read-only sources: `internal/provider/**`, `cmd/main.go`, `internal/controller`, + `internal/gc`, `internal/providerhealth`, `docs/prompts/__initial-prompt.md`, + `docs/kvm-provider.md`, `docs/gcp-vm-validation.md` -- 2.54.0 From e6ed46fd7cee9d913891bdb95da019da67609c58 Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 08:45:58 +0000 Subject: [PATCH 12/12] Record the provider health validation and add the changelog entry The user ran the real-hypervisor checks against beelink and confirmed they passed, which completes Step 7 of the provider health plan. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 21 +++++++++++++++++++ .../2026-09-11-1526-provider-health-checks.md | 15 ++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8248f9..c84bf86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,26 @@ # Changelog +## 2026-09-16 08:45 UTC — Provider health checks: ProviderHealth CRD, per-provider checks, gauges + +- **Each configured provider is now checked on a schedule** and reported in a + cluster-scoped `ProviderHealth` object (`kubectl get ph`), one per provider, with a + `Ready` condition, a reason, and per-target verdicts. +- **Checks:** kubernetes lists one object from the API server; GCP fetches a single + instance page; KVM checks SSH/libvirt reachability and the storage pool on every + host, in parallel. Failures map to fixed reasons (`Unreachable`, + `SSHHandshakeFailed`, `PoolNotFound`, `Timeout`, …). +- **The KVM check can't hurt reconciles:** it uses `hostPool.probe`, which gives up on + a slow RPC without dropping the shared connection, and waiting for a host slot can + be interrupted. +- **Metrics:** `proxy_operator_provider_target_up`, `_check_duration_seconds` and + `_check_timestamp_seconds`. The flags are `--provider-check-interval` and + `--provider-check-timeout` (defaults to `min(15s, interval)`). +- Validated with `make test`, `make test-e2e` and the real-hypervisor KVM run against + beelink. `make lint` is blocked by a Go 1.27 toolchain/staticcheck panic that has + nothing to do with this change. +- Key files: `api/v1alpha1/providerhealth_types.go`, `internal/provider/check.go`, + `internal/provider/kvm/`, `internal/providerhealth/`, `docs/architecture.md`. + ## 2026-09-10 22:58 UTC — Code-review fixes on the KVM branch: deletion, validation, lazy SSH, guest agent Five findings from a review of `feat/kvm-provider`, all fixed on the branch. 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 60b0a0c..a58dade 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 @@ -8,7 +8,7 @@ Plan: [docs/plans/2026-09-11-1526-provider-health-checks.md](../plans/2026-09-11 - [x] Step 4 — Monitor - [x] Step 5 — Metrics and wiring - [x] Step 6 — Docs -- [ ] Step 7 — Validate, record, push, open the PR +- [x] Step 7 — Validate, record, push, open the PR ## Step 0 — Save the plan @@ -363,3 +363,16 @@ Worth noting: the same commit: the old text promised the drop-on-timeout behaviour that is now gone. - The real-hypervisor commands from the plan's Verification section are still the user's to run; nothing here replaces them. + +### Real-hypervisor validation and PR + +The user ran the plan's real-hypervisor checks against beelink (the `TestKVMCheck` +integration test plus the manual `ProviderHealth` and metrics checks) and confirmed +they passed. After that the CHANGELOG entry went in, and the branch was pushed with +the token-file credential helper. The PR against `main` was opened through the Gitea +API because `tea` isn't installed. + +Worth noting: +- The branch also carries `docs/plans/2026-09-16-0841-provider-blueprint.md`, on + purpose: that plan reaches `main` with this merge, and the blueprint work starts on a + new branch afterwards. -- 2.54.0