Add orphan GC sweeper and Prometheus metrics with explicit registration
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -1,10 +1,10 @@
|
||||
# Architecture
|
||||
|
||||
> **Status:** the operator is built through Step 8 (GCP provider) of
|
||||
> **Status:** the operator is built through Step 9 (orphan GC + metrics) of
|
||||
> [docs/plans/2026-08-07-1747-proxy-operator.md](plans/2026-08-07-1747-proxy-operator.md).
|
||||
> This document currently covers the event/reconcile flow and the
|
||||
> HTTP-driven lease/discovery path; the components table and the Decisions
|
||||
> section arrive with Step 10, and the orphan-GC flow lands with Step 9.
|
||||
> This document covers the event/reconcile flow, the HTTP-driven
|
||||
> lease/discovery path, and the GC sweep; the components table and the
|
||||
> Decisions section arrive with Step 10.
|
||||
|
||||
## Event flow: cluster events → reconciler functions
|
||||
|
||||
@@ -240,3 +240,50 @@ Store.Start(ctx) ── manager Runnable, NOT leader-elected: sweeps expired
|
||||
leases + cooldowns; correctness never depends on the
|
||||
sweep (every read checks ExpiresAt against the clock)
|
||||
```
|
||||
|
||||
### 8. Orphan GC (`internal/gc/`) — the crash-safety net
|
||||
|
||||
Timer-driven, leader-elected (destructive ⇒ single writer). Exists for the
|
||||
one gap the reconciler cannot close alone: a crash after a provider Create
|
||||
but before the status write that records the instance.
|
||||
|
||||
```text
|
||||
Sweeper.Start(ctx) ── refuses to run when the cache is namespace-
|
||||
│ restricted unless --gc-allow-namespaced is explicit
|
||||
│ (an incomplete live set would "orphan" live VMs)
|
||||
└─ every Interval (10m; first sweep a full interval after start):
|
||||
sweep(ctx)
|
||||
│ Reader.List(Proxies) → live UID set
|
||||
│ List fails → skip the whole sweep (never guess)
|
||||
│ a CR with deletionTimestamp still counts as LIVE — its
|
||||
│ finalizer owns that deletion; GC racing it double-deletes
|
||||
└ per provider: ListByTag
|
||||
│ error → log, continue with the next provider
|
||||
└ delete only when ALL hold:
|
||||
has the proxy-operator-uid label (ownership proof)
|
||||
older than MinAge (10m) (not mid-create)
|
||||
UID matches no existing CR (truly orphaned)
|
||||
each kill logged loudly with provider, providerID, UID
|
||||
```
|
||||
|
||||
### 9. Metrics (`internal/metrics/`)
|
||||
|
||||
Registered explicitly from `cmd/main.go` (no `init()`; tests use fresh
|
||||
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.
|
||||
- **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
|
||||
health engine deletes a proxy's series when it prunes its state;
|
||||
`lease_requests_total{outcome}` from the discovery handlers;
|
||||
`provider_requests_total{provider,op,result}` from the
|
||||
`provider.WithMetrics` decorator — the one place `Class()` is called
|
||||
purely for observability.
|
||||
|
||||
Each consuming package defines its own small recorder interface
|
||||
(`health.ProbeMetrics`, `discovery.LeaseMetrics`, `provider.RequestRecorder`);
|
||||
`metrics.Metrics` satisfies all of them structurally, so no package other
|
||||
than `cmd/main.go` imports the metrics package.
|
||||
|
||||
@@ -13,7 +13,7 @@ Pairs with [docs/plans/2026-08-07-1747-proxy-operator.md](../plans/2026-08-07-17
|
||||
- [x] Step 6 — Lease store (`internal/lease/`)
|
||||
- [x] Step 7 — Discovery API (`internal/discovery/`)
|
||||
- [x] Step 8 — GCP provider (`internal/provider/gcp/`)
|
||||
- [ ] Step 9 — Orphan GC + metrics
|
||||
- [x] Step 9 — Orphan GC + metrics
|
||||
- [ ] Step 10 — Wiring, config, docs
|
||||
- [ ] Step 11 — Tests
|
||||
- [ ] Verification (vet/test/kind e2e) + commit, push, open MR
|
||||
@@ -875,3 +875,67 @@ protobuf-construction idiom and matches every example in the SDK docs.
|
||||
Registry wiring (`"gcp": gcp.New`) happens at the composition root in
|
||||
Step 10, as designed in Step 2. `docs/architecture.md` §5 now shows both
|
||||
providers' call mappings.
|
||||
|
||||
## Step 9 — Orphan GC + metrics
|
||||
|
||||
Implemented `internal/gc/gc.go` (the `Sweeper` Runnable) and
|
||||
`internal/metrics/metrics.go` (explicit-registration metric set), plus the
|
||||
`provider.WithMetrics` decorator deferred from Step 2 into
|
||||
`internal/provider/metrics.go`, and the observation hooks in the health
|
||||
engine and discovery server.
|
||||
|
||||
**GC**, per the plan: `NeedLeaderElection() = true` (destructive ⇒ single
|
||||
writer), 10 min ticker with the first sweep one full interval after start,
|
||||
per-provider `ListByTag` with log-and-continue on provider errors, and a
|
||||
kill requires all three of: our UID label present, older than `MinAge`
|
||||
(10 min), and the UID matching no existing CR — where a CR with a
|
||||
`deletionTimestamp` still counts as live (its finalizer owns that
|
||||
deletion; GC racing it would double-delete). Two safety behaviors worth
|
||||
naming: a failed Proxy `List` skips the whole sweep (an unreadable live
|
||||
set proves nothing orphaned), and the namespace-scope guard makes `Start`
|
||||
refuse to run against a namespace-restricted cache unless
|
||||
`--gc-allow-namespaced` is explicit, with the flag named in the error.
|
||||
One deviation of record: the plan says kills log "at Warn", but logr has
|
||||
no Warn level — kills log at Info with a `WARNING:` prefix carrying
|
||||
provider, providerID, and UID, same convention as the discovery server's
|
||||
empty-token warning.
|
||||
|
||||
**Metrics**, per the plan: no `init()` — `Metrics.Register(reg, phases,
|
||||
leases)` is called explicitly by the composition root (Step 10), which is
|
||||
also what lets every test use a fresh registry (asserted by a test that
|
||||
registers two sets on two registries). `proxy_operator_proxies{phase}`
|
||||
and `proxy_operator_leases_active` are scrape-time collectors fed by
|
||||
closures — a reconcile-incremented gauge drifts and leaks series on
|
||||
delete; reading the source of truth at scrape time cannot. The
|
||||
probe vectors observe **every** probe (status stays transition-only;
|
||||
metrics carry the high-frequency signal), and the health engine calls
|
||||
`ForgetProxy` when it prunes a state entry so per-proxy series don't leak.
|
||||
`provider_requests_total{provider,op,result}` comes from the
|
||||
`WithMetrics` decorator — the one place `Class()` is called purely for
|
||||
observability, with results labelled ok / not_found / quota_exceeded /
|
||||
permanent / transient.
|
||||
|
||||
**Decoupling shape:** each consuming package defines its own small
|
||||
recorder interface (`health.ProbeMetrics`, `discovery.LeaseMetrics`,
|
||||
`provider.RequestRecorder`); `metrics.Metrics` satisfies all of them
|
||||
structurally. Only `cmd/main.go` will import `internal/metrics`.
|
||||
|
||||
Tests (`-race -count=2` clean): GC's true-orphan matrix in one sweep
|
||||
(live kept, deleting-CR kept, young kept, unlabelled kept, orphan
|
||||
deleted), broken-provider isolation, list-failure skips sweep, the
|
||||
namespace guard both ways, and the Start loop sweeping then stopping;
|
||||
metrics via `prometheus/testutil` — `GatherAndCompare` on the scrape-time
|
||||
collectors, ForgetProxy dropping series, outcome/result label counts;
|
||||
the decorator's five-way classification table with error passthrough
|
||||
asserted. Coverage: gc 86.1%, metrics 95.0%, provider up to 97.1%;
|
||||
health/discovery re-ran green with the hooks in place.
|
||||
|
||||
```bash
|
||||
go test -race -count=2 ./internal/gc/ ./internal/metrics/ ./internal/provider/ ./internal/health/ ./internal/discovery/
|
||||
make test # whole repo green
|
||||
```
|
||||
|
||||
Worth noting: `prometheus/client_golang` was already in the module via
|
||||
controller-runtime's metrics server, so no new dependency — `go mod tidy`
|
||||
just promoted it to direct. `docs/architecture.md` gained §8 (GC sweep)
|
||||
and §9 (metrics shape).
|
||||
|
||||
Reference in New Issue
Block a user