agent-substrate / agent-substrate/substrate
ateapi: a control plane that cannot reach its store serves 200 on /healthz and /readyz, stays Ready, and fails every RPC
- Ngôn ngữ chính
- Go
- Star
- 1.8k
- Fork
- 316
- Merge trung bình
- 2 ngày 43 phút
- Pull request đã merge (30 ngày)
- 287
Mô tả
### Expected Behavior
Readiness should mean "this process can serve". When `ate-api-server` cannot reach its store it can
serve nothing — every RPC fails — so it should fail `/readyz`, leave the Service endpoints, and
surface a signal that something above it can act on: an alert, a metric, a Condition, anything.
Liveness should keep succeeding throughout. The process is alive and will recover on its own once
the store returns; restarting it would make things worse, not better (see #).
### Actual Behavior
The two probes are indistinguishable, and both are green while the control plane serves nothing.
Store scaled to zero, sampled every 15 s for six minutes:
| | |
|---|---|
| Baseline | `healthz=200 readyz=200 Ready=True restarts=0 rpc=OK` |
| T+15s → T+360s — **24 consecutive samples** | `healthz=200 readyz=200 Ready=True restarts=0 rpc=FAIL` |
Six minutes in which the pod is 200 on `/healthz`, 200 on `/readyz`, `Ready=True` to both the kubelet
and its Service, has never restarted, is still receiving traffic — and fails **every** RPC. Nothing
in Kubernetes has any way to know, and nothing in the control plane says so either.
This is not a slow-detection problem. There is no detection: `/readyz` does not consult the store
under any circumstances, so the interval is infinite.
What it costs, concretely: on 27 Aug we spent about three hours on an outage of this shape, most of
it looking in the wrong place, because every signal at the top of the stack was green and the only
component visibly misbehaving was the api-server crash loop from # — which is a *symptom* of
the store being gone, not the cause. A store outage presents as an api-server bug.
### Steps to Reproduce the Problem
1. Install on GKE with the PostgreSQL backend and confirm `ate-api-server` is serving:
`kubectl-ate get atespaces` returns.
1. Take the store away: `kubectl -n ate-system scale sts/postgres --replicas=0`.
1. Poll all four signals every 15 s for six minutes:
```
kubectl -n ate-system get pod -o jsonpath='{.status.containerStatuses[0].ready} {.status.containerStatuses[0].restartCount}'
curl -s -o /dev/null -w '%{http_code}' localhost:9090/healthz # via port-forward
curl -s -o /dev/null -w '%{http_code}' localhost:9090/readyz
kubectl-ate get atespaces # the actual RPC
```
Every sample reads `Ready=True restarts=0 healthz=200 readyz=200` while the RPC fails. It does
not change with time — we held it for six minutes; there is nothing to wait for.
1. Confirm the endpoint is still in service:
`kubectl -n ate-system get endpointslices -l kubernetes.io/service-name=ate-api-server`. The pod
is still listed, so it is still being sent traffic.
1. Bring the store back: `kubectl -n ate-system scale sts/postgres --replicas=1`. RPCs succeed again
~15 s later with `restarts=0` — the process recovers by itself, which is exactly why liveness
must not be involved in the fix.
### Specifications
- **Version:** `4c1b37d` (base of `release-0.1-rc`) plus three cherry-picks. **Confirmed still
present on `main` at `ea3bdc32`** (2026-09-02) — `internal/serverboot/` has one commit since the
pin (`66300d5c`, #1243) and it does not touch the health surface. Store-agnostic: unchanged by the
valkey → Postgres migration (`60073ecd`, #940).
- **Platform:** GKE, `us-central1-c`, `--store-backend=postgres`, two `ate-api-server` replicas.
Reproduced 2026-08-27.
- **Evidence:** the 24-sample polling log above. (One sample at T+195s read `healthz=000 readyz=000`
— that was the port-forward blipping, not the server; it recovered on the next sample with the
restart count unchanged.)
---
### Root cause
`internal/serverboot/serverboot.go` on `main` at `ea3bdc32`.
**1. `/healthz` is unconditional, by construction.** `metricsMux` at `:356`:
```go
if opts.EnableHealthz {
mux.HandleFunc("/healthz", func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte("ok"))
})
}
```
The doc comment at `:319` says so outright: *"EnableHealthz adds an always-200 /healthz for liveness
probes, which must keep succeeding while a draining server fails /readyz."* That rationale is
correct and this half is arguably working as designed — see the design constraint below.
**2. `/readyz` consults a drain flag and nothing else.** `readinessHandler` (`:367`) reads
`Readiness.Ready()`, and `Readiness` (`:299`) is flipped only by `MarkNotReady`, called only from
`drainOnShutdown` (`cmd/ateapi/main.go:242`). Nothing else in the process can ever influence it. A
store outage is invisible to it because no code path connects the two.
**3. `Readiness` is a one-way latch, so the obvious fix does not fit the existing type.** Its doc
comment (`:297`): *"Calling MarkNotReady flips it **permanently** to not ready."* That is right for
draining and wrong for a store outage, which must be able to recover. Gating readiness on store
reachability needs a reversible predicate — either a new type, or `Readiness` gaining a separate
recoverable condition alongside the terminal drain latch. **This is the reason the fix is not a
one-liner**, and worth knowing before someone starts.
**4. Nothing else reports it either.** There is no store-reachability metric, no log line above
per-RPC errors, and no Condition. The failure is only visible to a caller who tries an RPC and reads
the error.
### The repo already argues this position, for a different binary
`StartReadinessServer`, twenty lines further down at `:334`, exits the process if its readiness
endpoint cannot bind, and explains why:
> *a worker whose readiness endpoint cannot come up never turns Ready and never registers, so dying
> loudly lets the kubelet restart it instead of **leaving a pod that looks alive but can never
> receive work**.*
That is precisely the state `ate-api-server` sits in for the entire duration of a store outage. The
principle is already written down and already accepted; it just was not applied to the component
where the blast radius is the whole cluster.
### Why this is a priority even though it breaks nothing by itself
It is a force multiplier rather than a fault. It does not cause outages — it makes every other
outage cost hours instead of minutes, and it does that without leaving a trace to grep for
afterwards. Two consequences that matter for anyone operating this without the maintainers on hand:
- **Every store-side failure presents as an api-server bug**, because the api-server crash loop
(#) is the only visible symptom while the actual cause is silent.
- **Nothing can be alerted on.** A pod that is `Ready=True restarts=0` with no failing probe and no
metric produces no page, no dashboard change and no event. The first report comes from a user.
Unlike most items of this kind, it also cannot be resolved by documentation. There is no release
note that makes a green health check informative.
### The design constraint the two fixes share
The naive fix — make `/healthz` reflect store reachability — makes # strictly worse: the
kubelet would then kill a pod that is correctly waiting for its store to come back, and a *running*
replica that today rides out a six-minute outage with `restarts=0` would instead crash-loop through
it. The split has to be:
- **`/healthz` = the process is alive.** Never store-gated. Starts *before* the store connect
(that is #'s fix).
- **`/readyz` = this process can serve.** Store-gated, reversibly, in both boot and steady state.
Stated once here because the two issues will otherwise be fixed by two people in two files with
incompatible assumptions.
### Candidate fixes
1. **Gate `/readyz` on store reachability with a reversible predicate.** A background health check
against the store (the pgx pool already exposes what is needed) flipping a condition that
`readinessHandler` ANDs with the existing drain latch. Removes the pod from Service endpoints
while it cannot serve, and puts it back automatically. This is the core fix.
1. **Export a store-reachability metric and log the transitions.** Cheap, independent of 1, and the
only one of these that helps someone diagnosing a cluster after the fact rather than during.
Worth doing even if 1 slips.
1. **Leave `/healthz` exactly as it is.** Explicitly listed as a fix so that it is a decision on the
record rather than an omission — see the constraint above.
1. **Apply the same treatment to the other binaries before the pattern spreads.** #844
(*atecontroller: serve health probes*, open and unreviewed since 11 Aug) adds
`HealthProbeBindAddress` plus liveness on `/healthz` and readiness on `/readyz` with **no**
`AddHealthzCheck` or `AddReadyzCheck` registered — controller-runtime then serves both as
unconditional 200, reproducing this issue in a second component. It is two lines to fix while the
PR is still open.
Related: [#1394 ] (the same health surface killing a *booting* process, and the fix that must land
compatibly with this one), #844 (about to copy the pattern), #636 (the valkey outage this made
unreadable, closed by deleting the backend rather than by a fix).
Hướng dẫn đóng góp
Đánh giá
Issue này chưa được đánh giá.