diff --git a/README.md b/README.md index 03ad4ca..344f012 100644 --- a/README.md +++ b/README.md @@ -230,26 +230,32 @@ not answering. still works. - **A refused probe is not a sick backend.** Only evidence about the *backend* takes it out of the pool: a transport failure (connection refused, DNS, TLS, - timeout) or a `5xx` — `503` included, since trapperkeeper answers `503` exactly - when its services are not nominal, so it stays a real failure. A reply that - refuses the *probe request* is evidence about the probe instead: every `4xx` is - the backend answering that our request is the problem (`404`/`410` the path is - not there, `405` it does not take a `GET`, `401`/`403` we are not allowed to - ask, `429` we asked too often), and `501` says it does not implement the + timeout), a `5xx` — `503` included, since trapperkeeper answers `503` exactly + when its services are not nominal — or a `429`, which is the backend reporting + its own capacity rather than judging the request, so an overloaded backend gets + backed off instead of kept at full traffic. A reply that refuses the *probe + request* is evidence about the probe instead: the other `4xx` are the backend + answering that our request is the problem (`404`/`410` the path is not there, + `405` it does not take a `GET`, `401`/`403` we are not allowed to ask), and + `501` says it does not implement the endpoint. A backend answering that way is **left in service** — still queried, still contributing records — and reported as `probe_unsupported` rather than `healthy`, so an operator can tell "verified healthy" from "not actually being checked". A probe path that is wrong for one backend alone can therefore never strand a working backend. The misconfiguration is logged once per transition, naming the backend, the probe path and the status. -- **Thresholds** — a healthy backend goes down after `health_probe_failures` - (default 3) **consecutive** failures; a down one comes back after - `health_probe_successes` (default 2) consecutive successes, and the same - thresholds gate `probe_unsupported` in and out. One blip cannot flap a backend - out, and one lucky reply cannot flap it back in. A run only counts while the - replies keep their kind, so a stretch of `404`s and a stretch of `503`s never - add up to one threshold. A down backend keeps being probed, so recovery is - automatic. +- **Thresholds** — a healthy backend leaves the pool after + `health_probe_failures` (default 3) **consecutive** unsuccessful probes; a down + one comes back after `health_probe_successes` (default 2) consecutive + successes, and the same thresholds gate `probe_unsupported` in and out. One + blip cannot flap a backend out, and one lucky reply cannot flap it back in. + The run counts every unsuccessful probe whatever its kind, so a backend that + fails every probe in mixed ways — a `503`, then a `404`, then a timeout — + still trips the threshold; only a success resets the run. The kinds decide + *which* state the run enters: a real failure anywhere in the run outranks a + refusal, so the backend goes `unhealthy` and is skipped, and only a run of + nothing but refusals enters `probe_unsupported` and stays in service. A down + backend keeps being probed, so recovery is automatic. - **Fails open** — if the prober has marked **every** backend down, `pdbmux` queries them all anyway. A wrong `health_probe_path`, a broken prober or a partition that only the prober sees can therefore never black-hole traffic; @@ -269,9 +275,14 @@ not answering. `consecutive_failures`, `consecutive_successes`, `last_probe` and `last_error`, alongside the `reachable` check `/healthz` runs itself — which always asks **every** - backend, so a backend queries are skipping is still reported. A `query` object - reports the last merged fan-out: `partial`, `contributed`, `configured`, - `partial_rounds` and `last_partial`. + backend, so a backend queries are skipping is still reported. Read the two + together: `state` is the prober's verdict and `probe_unsupported` means "not + being verified", *not* "well", so `reachable` is the field that says whether + the backend is answering right now. A proxy that `404`s everything because the + backend behind it is dead shows `state: probe_unsupported` with `reachable` + carrying the query error, and the overall `status` drops to `degraded` or + `down` accordingly. A `query` object reports the last merged fan-out: + `partial`, `contributed`, `configured`, `partial_rounds` and `last_partial`. - **`health_probe_enabled: false`** turns the whole thing off: no probing goroutines, no backend ever skipped, every backend queried on every request. `X-Backends` still reports how many answered. diff --git a/health.go b/health.go index 23c1268..23d9b47 100644 --- a/health.go +++ b/health.go @@ -42,11 +42,13 @@ type probeState struct { healthy bool unsupported bool probed bool - outcome probeOutcome - failures int - successes int - lastProbe time.Time - lastErr string + // runFailed records whether the current run of not-OK probes contains at + // least one real failure, which decides which non-healthy state it enters. + runFailed bool + failures int + successes int + lastProbe time.Time + lastErr string } // backendHealth is a copy of one backend's probe state, safe to read outside the @@ -86,11 +88,16 @@ func (e *probeRejectedError) Error() string { } // probeRejects reports whether a status code refuses the probe request rather -// than reporting the service unhealthy. Every 4xx is the backend answering that -// our request is the problem, and 501 says it does not implement the endpoint. -// Other 5xx stay real evidence: trapperkeeper answers 503 exactly when its -// services are not nominal. +// than reporting the service unhealthy. A 4xx is the backend answering that our +// request is the problem, and 501 says it does not implement the endpoint. +// 429 is excluded: it is the backend reporting its own capacity, not a verdict +// on the request, so an overloaded backend must get backed off rather than kept +// at full traffic. Other 5xx stay real evidence too: trapperkeeper answers 503 +// exactly when its services are not nominal. func probeRejects(code int) bool { + if code == http.StatusTooManyRequests { + return false + } return (code >= 400 && code < 500) || code == http.StatusNotImplemented } @@ -235,7 +242,9 @@ func (p *prober) probeOne(ctx context.Context, b Backend) { // record applies one probe outcome, flipping state only once the matching // threshold is reached so a single blip cannot flap a backend in either -// direction. +// direction. One run of consecutive not-OK probes makes the down decision, no +// matter how the kinds are mixed; the kinds only pick which non-healthy state +// the run enters, and a real failure anywhere in the run outranks a rejection. func (p *prober) record(name string, err error) { p.mu.Lock() st, ok := p.states[name] @@ -244,19 +253,13 @@ func (p *prober) record(name string, err error) { return } outcome := classifyProbe(err) - // A run only counts while the outcome keeps its kind, so a rejected-probe run - // and a real-failure run never add up to one threshold. - if outcome != st.outcome { - st.failures, st.successes = 0, 0 - } - st.outcome = outcome st.probed = true st.lastProbe = p.now() var transition string - switch outcome { - case outcomeOK: + if outcome == outcomeOK { st.lastErr = "" + st.failures, st.runFailed = 0, false st.successes++ if st.successes >= p.successes { switch { @@ -268,23 +271,26 @@ func (p *prober) record(name string, err error) { transition = fmt.Sprintf("info: backend %q is up after %d consecutive probe successes", name, st.successes) } } - case outcomeRejected: - st.lastErr = err.Error() - st.failures++ - if !st.unsupported && st.failures >= p.failures { - // The backend answered and refused the probe request itself, so this says - // nothing about its health: it keeps serving queries, unverified. - st.unsupported, st.healthy = true, true - transition = fmt.Sprintf("warning: backend %q rejected the health probe %d times: %v; leaving it in service but unverified, fix health_probe_path or its authorization", name, st.failures, err) - } - default: + } else { st.lastErr = err.Error() + st.successes = 0 st.failures++ + st.runFailed = st.runFailed || outcome == outcomeFailure if st.failures >= p.failures { - st.unsupported = false - if st.healthy { - st.healthy = false - transition = fmt.Sprintf("warning: backend %q is down after %d consecutive probe failures: %v", name, st.failures, err) + switch { + case st.runFailed: + // Some probe in this run was evidence about the backend itself, so the + // run is a failure run whatever else it also contains. + st.unsupported = false + if st.healthy { + st.healthy = false + transition = fmt.Sprintf("warning: backend %q is down after %d consecutive failed probes: %v", name, st.failures, err) + } + case !st.unsupported: + // Nothing but refusals of the probe request, which says nothing about the + // backend's health: it keeps serving queries, unverified. + st.unsupported, st.healthy = true, true + transition = fmt.Sprintf("warning: backend %q rejected the health probe %d times: %v; leaving it in service but unverified, fix health_probe_path or its authorization", name, st.failures, err) } } } diff --git a/health_test.go b/health_test.go index 7b0c18d..4a3e040 100644 --- a/health_test.go +++ b/health_test.go @@ -86,10 +86,14 @@ func markDown(t *testing.T, p *prober, name string) { } } +func rejectedProbe(p *prober) error { + return &probeRejectedError{status: http.StatusNotFound, path: p.path} +} + func markUnsupported(t *testing.T, p *prober, name string) { t.Helper() for i := 0; i < p.failures; i++ { - p.record(name, &probeRejectedError{status: http.StatusNotFound, path: p.path}) + p.record(name, rejectedProbe(p)) } if !p.healthy(name) { t.Fatalf("backend %q was excluded by a rejected probe", name) @@ -507,7 +511,7 @@ func TestProber_RejectedProbeKeepsBackendInService(t *testing.T) { codes := []int{ http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound, http.StatusMethodNotAllowed, http.StatusGone, - http.StatusTooManyRequests, http.StatusNotImplemented, + http.StatusNotImplemented, } for _, code := range codes { sb := newStatusBackend(t) @@ -537,7 +541,12 @@ func TestProber_RejectedProbeKeepsBackendInService(t *testing.T) { // 503 is trapperkeeper's answer when its services are not nominal, so it stays a // real failure; so does anything that never got an HTTP reply at all. func TestProber_ServerErrorsAndTransportFailuresStillMarkDown(t *testing.T) { - for _, code := range []int{http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout} { + // 429 says the backend is out of capacity, which is evidence about the backend. + for _, code := range []int{ + http.StatusInternalServerError, http.StatusBadGateway, + http.StatusServiceUnavailable, http.StatusGatewayTimeout, + http.StatusTooManyRequests, + } { sb := newStatusBackend(t) sb.reply(code, degradedStatusBody("error")) p := newTestProber(t, healthConfig(sb.srv.URL, "http://b.invalid")) @@ -573,29 +582,121 @@ func TestProber_ServerErrorsAndTransportFailuresStillMarkDown(t *testing.T) { } } -func TestProber_RejectedRunAndFailureRunDoNotAddUp(t *testing.T) { +// A real failure landing in a rejected run outranks the rejections: the run is +// one run of unsuccessful probes, and its worst evidence decides the state. +func TestProber_RealFailureInARejectedRunMarksDown(t *testing.T) { p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) markUnsupported(t, p, "a") for i := 0; i < 20; i++ { - p.record("a", &probeRejectedError{status: http.StatusNotFound, path: p.path}) + p.record("a", rejectedProbe(p)) } - // One real failure on top of a long rejected run must not trip the threshold. p.record("a", errors.New("boom")) - if !p.healthy("a") { - t.Fatal("a single real failure after a rejected run marked the backend down") - } - for i := 1; i < p.failures; i++ { - p.record("a", errors.New("boom")) - } if p.healthy("a") { - t.Fatal("a full run of real failures should mark the backend down") + t.Fatal("a real failure inside a rejected run left the backend in service") } if got := p.snapshot()["a"].stateName(); got != stateUnhealthy { t.Errorf("state = %q, want %q", got, stateUnhealthy) } } +// The regression this guards: a backend failing every probe but flipping the +// kind of failure — a fronting proxy alternating 404 and 503, say — never +// answers a probe successfully, so it must go down at the threshold like any +// other permanently failing backend. +func TestProber_AlternatingOutcomeKindsStillMarkDown(t *testing.T) { + rejected := func(p *prober) error { return rejectedProbe(p) } + failed := func(*prober) error { return errors.New("boom") } + ok := func(*prober) error { return nil } + + cycles := map[string][]func(*prober) error{ + "rejected/failure": {rejected, failed}, + "failure/rejected": {failed, rejected}, + "failure-heavy 3-cycle": {rejected, failed, failed}, + "reject-heavy 3-cycle": {rejected, rejected, failed}, + } + for name, cycle := range cycles { + t.Run(name, func(t *testing.T) { + p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) + for i := 0; i < 200; i++ { + p.record("a", cycle[i%len(cycle)](p)) + } + if p.healthy("a") { + t.Fatal("a backend that failed 200 probes is still in service") + } + if got := p.snapshot()["a"].stateName(); got != stateUnhealthy { + t.Errorf("state = %q, want %q", got, stateUnhealthy) + } + }) + } + + // The same kinds never trip while a success keeps ending the run. + p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) + interleaved := []func(*prober) error{rejected, ok, failed, ok} + for i := 0; i < 200; i++ { + p.record("a", interleaved[i%len(interleaved)](p)) + if !p.healthy("a") { + t.Fatal("a success between every unsuccessful probe still tripped the threshold") + } + } +} + +// Reaching the threshold on nothing but refusals is the one case that stays in +// service: it is evidence about the probe request, not about the backend. +func TestProber_PurelyRejectedRunStaysInService(t *testing.T) { + p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) + for i := 0; i < p.failures*10; i++ { + p.record("a", rejectedProbe(p)) + if !p.healthy("a") { + t.Fatalf("a purely rejected run excluded the backend after %d probes", i+1) + } + } + if got := p.snapshot()["a"].stateName(); got != stateProbeUnsupported { + t.Fatalf("state = %q, want %q", got, stateProbeUnsupported) + } +} + +// A single success resets the run whatever it was made of. +func TestProber_SuccessResetsTheRunInEveryDirection(t *testing.T) { + for _, name := range []string{"rejected", "failure", "mixed"} { + t.Run(name, func(t *testing.T) { + p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) + cfgFailures := p.failures + next := func(i int) error { + switch name { + case "rejected": + return rejectedProbe(p) + case "failure": + return errors.New("boom") + default: + if i%2 == 0 { + return rejectedProbe(p) + } + return errors.New("boom") + } + } + for i := 0; i < cfgFailures-1; i++ { + p.record("a", next(i)) + } + p.record("a", nil) + snap := p.snapshot()["a"] + if snap.Failures != 0 { + t.Fatalf("consecutive_failures = %d after a success, want 0", snap.Failures) + } + // One short of the threshold again, so the pre-success run is gone. + for i := 0; i < cfgFailures-1; i++ { + p.record("a", next(i)) + } + if !p.healthy("a") { + t.Fatal("the run survived a success") + } + if got := p.snapshot()["a"].stateName(); got == stateProbeUnsupported { + t.Fatal("a sub-threshold run reached probe_unsupported") + } + }) + } +} + func TestProber_RecoversFromProbeUnsupported(t *testing.T) { p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) markUnsupported(t, p, "a") @@ -611,11 +712,25 @@ func TestProber_RecoversFromProbeUnsupported(t *testing.T) { } } -// A rejected probe re-admits a backend the prober had already marked down: the -// evidence that condemned it has been replaced by no evidence at all. -func TestProber_RejectedProbeReadmitsADownBackend(t *testing.T) { +// Rejections cannot talk a down backend back into the pool while its failure run +// is unbroken — the failures are still the run's best evidence. A successful +// probe ends the run, and rejections after that mean the probe stopped working +// rather than the backend. +func TestProber_RejectedProbeReadmitsADownBackendOnlyAfterASuccess(t *testing.T) { p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) markDown(t, p, "a") + + for i := 0; i < p.failures*5; i++ { + p.record("a", rejectedProbe(p)) + if p.healthy("a") { + t.Fatal("rejections re-admitted a backend whose failure run never ended") + } + } + if got := p.snapshot()["a"].stateName(); got != stateUnhealthy { + t.Fatalf("state = %q, want %q", got, stateUnhealthy) + } + + p.record("a", nil) markUnsupported(t, p, "a") } @@ -710,6 +825,38 @@ func TestHealthz_ReportsProbeUnsupported(t *testing.T) { } } +// probe_unsupported alone cannot say whether a backend is alive: a proxy that +// 404s everything because the backend behind it is dead looks exactly like a +// misconfigured probe path. reachable is the field that separates them. +func TestHealthz_ProbeUnsupportedSeparatesReachableFromDead(t *testing.T) { + a := newFakeBackend(t, `[]`, `[]`) + b := newFakeBackend(t, `[]`, `[]`) + srv := newTestServer(healthConfig(a.srv.URL, b.srv.URL)) + markUnsupported(t, srv.health, "a") + markUnsupported(t, srv.health, "b") + b.fail = true + + rec := doGet(t, srv.Handler(), "/healthz", "") + var hr healthReport + if err := json.Unmarshal(rec.Body.Bytes(), &hr); err != nil { + t.Fatal(err) + } + for name, got := range map[string]string{"a": hr.Backends["a"].State, "b": hr.Backends["b"].State} { + if got != stateProbeUnsupported { + t.Fatalf("%s state = %q, want %q", name, got, stateProbeUnsupported) + } + } + if hr.Backends["a"].Reachable != "ok" { + t.Errorf("a reachable = %q, want ok", hr.Backends["a"].Reachable) + } + if hr.Backends["b"].Reachable == "ok" { + t.Error("a dead backend behind an unusable probe path still reports reachable=ok") + } + if hr.Status != "degraded" { + t.Errorf("status = %q, want degraded", hr.Status) + } +} + func TestStatusBodyHealthy_IgnoresBodiesWithoutStates(t *testing.T) { for _, body := range []string{`{}`, `[]`, `null`, `{"svc":{}}`, `not json`} { if err := statusBodyHealthy([]byte(body)); err != nil { diff --git a/server.go b/server.go index c977525..7bc4447 100644 --- a/server.go +++ b/server.go @@ -673,10 +673,18 @@ type healthReport struct { } // backendReport pairs this request's own reachability check with the background -// prober's running state for the same backend. +// prober's running state for the same backend. The two answer different +// questions and must be read together: probe_unsupported means "not being +// verified", not "well", so reachable is the field that says whether the backend +// is actually answering queries right now. type backendReport struct { - Reachable string `json:"reachable"` // "ok" | error text - State string `json:"state"` // healthy | unhealthy | probe_unsupported | unprobed | unmonitored + // Reachable is this request's own live query to the backend, run against + // every configured backend regardless of probe state: "ok" or the error text. + Reachable string `json:"reachable"` + // State is the background prober's verdict. probe_unsupported means the probe + // path is unusable on this backend, so its health is unknown — read Reachable + // to find out whether it is up. + State string `json:"state"` // healthy | unhealthy | probe_unsupported | unprobed | unmonitored Failures int `json:"consecutive_failures"` Successes int `json:"consecutive_successes"` LastProbe string `json:"last_probe,omitempty"`