From 34993ac17382dbd06f64fbc0160692c0cb4231a9 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Tue, 25 Aug 2026 04:59:40 +0000 Subject: [PATCH 1/2] fix(health): a draining gateway stays live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/livez` answered `503` once graceful shutdown began, and the shipped Helm chart wires `/livez` as the container's livenessProbe. Liveness decides whether to RESTART an instance, and a draining one is finishing the requests it already accepted — restarting it kills exactly those. Withdrawing traffic is `/readyz`'s job, and it already does it. This was the half of AISIX-Cloud#591 that never landed. That issue's option 1 was "add /readyz for traffic eligibility, keep /livez focused on process liveness"; #655 added `/readyz` and softened `/livez` from 500 to 503, but left the shutdown check on it. The comment left behind on that branch — "503 so Kubernetes stops routing" — is readiness reasoning on a liveness endpoint, and Kubernetes does not route on liveness. Kubernetes stops probing liveness once a pod enters graceful termination, so a rolling update never acted on the answer. Verified on kind (1.33.1) against the real 0.10.0 image with `periodSeconds: 1` / `failureThreshold: 1`: `/livez` returned 503 throughout, and the restart count stayed 0 for the whole drain. That makes this a latent defect rather than a live one — but the gateway also ships as a single container under docker or systemd, where a supervisor watching `/livez` does act on it, and any monitor treating `/livez` as "is the process healthy" saw a false alarm on every rolling update. `livez_response` no longer takes `LivezState`, and neither route handler takes its state extractor. Not receiving the drain state is a stronger guarantee that the answer cannot depend on it than a comment saying so. The admin OpenAPI drops `/livez`'s 503 response: liveness now has no failure to document, since an instance that cannot answer does not reply at all. Documentation is corrected in the paired api7/docs#2191 and api7/docs.apiseven.com#500. --- crates/aisix-admin/src/lib.rs | 17 ++-- crates/aisix-admin/src/openapi.rs | 18 ++-- crates/aisix-proxy/src/health.rs | 85 +++++++++---------- crates/aisix-proxy/src/lib.rs | 18 ++-- .../e2e/src/cases/health-minimal-e2e.test.ts | 60 +++++++++---- 5 files changed, 112 insertions(+), 86 deletions(-) diff --git a/crates/aisix-admin/src/lib.rs b/crates/aisix-admin/src/lib.rs index 6caad71a..9ec2508b 100644 --- a/crates/aisix-admin/src/lib.rs +++ b/crates/aisix-admin/src/lib.rs @@ -380,11 +380,12 @@ fn normalized_prometheus_path(path: &str) -> String { } } +/// Takes no [`AdminState`], for the same reason the proxy listener's +/// does not: see [`aisix_proxy::health::livez_response`]. async fn livez( - axum::extract::State(state): axum::extract::State, axum::extract::Query(params): axum::extract::Query>, ) -> Response { - aisix_proxy::health::livez_response(&state.livez_state, params.contains_key("verbose")) + aisix_proxy::health::livez_response(params.contains_key("verbose")) } async fn readyz( @@ -1087,7 +1088,7 @@ mod tests { } #[tokio::test] - async fn livez_returns_503_when_shutting_down() { + async fn livez_stays_200_when_shutting_down() { let state = build_state(); state.livez_state.mark_shutting_down(); let app = build_router(state); @@ -1096,10 +1097,12 @@ mod tests { .body(Body::empty()) .unwrap(); let resp = run(app, req).await; - assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); - let bytes = to_bytes(resp.into_body(), 1024 * 1024).await.unwrap(); - let text = std::str::from_utf8(&bytes).unwrap(); - assert!(text.contains("livez check failed")); + assert_eq!( + resp.status(), + StatusCode::OK, + "the admin listener answers the same liveness question as the \ + proxy one, and a draining process is not one to restart", + ); } #[tokio::test] diff --git a/crates/aisix-admin/src/openapi.rs b/crates/aisix-admin/src/openapi.rs index 45479bb4..5bf8d46d 100644 --- a/crates/aisix-admin/src/openapi.rs +++ b/crates/aisix-admin/src/openapi.rs @@ -67,22 +67,12 @@ const OPENAPI_JSON_BASE: &str = r##"{ } } } - }, - "503": { - "description": "Process is shutting down (graceful drain)", - "content": { - "text/plain": { - "schema": { - "type": "string" - } - } - } } }, "tags": [ "Health" ], - "description": "Process liveness: 200 while the process is alive, 503 once graceful shutdown has begun (an expected drain, not a crash). Use /readyz for traffic eligibility (readiness)." + "description": "Process liveness: should this instance be restarted? Answers 200 whenever it answers at all, including throughout a graceful drain \u2014 draining is deliberate work, and restarting an instance that is finishing the requests it accepted would kill exactly those. Use /readyz for traffic eligibility (readiness), which is what reports the drain." } }, "/readyz": { @@ -3472,8 +3462,12 @@ mod tests { "verbose" ); // Graceful shutdown is documented as 503 (drain), not 500 (#591). - assert!(parsed["paths"]["/livez"]["get"]["responses"]["503"].is_object()); + // Liveness has no failure response to document: a draining + // instance answers 200 like any other, and an instance that + // cannot answer does not reply at all. + assert!(parsed["paths"]["/livez"]["get"]["responses"]["503"].is_null()); assert!(parsed["paths"]["/livez"]["get"]["responses"]["500"].is_null()); + assert!(parsed["paths"]["/readyz"]["get"]["responses"]["503"].is_object()); // /readyz is documented with 200 + 503 (readiness). assert!(parsed["paths"]["/readyz"]["get"]["responses"]["200"].is_object()); assert!(parsed["paths"]["/readyz"]["get"]["responses"]["503"].is_object()); diff --git a/crates/aisix-proxy/src/health.rs b/crates/aisix-proxy/src/health.rs index 9d65c86c..5c199161 100644 --- a/crates/aisix-proxy/src/health.rs +++ b/crates/aisix-proxy/src/health.rs @@ -143,46 +143,40 @@ pub fn config_readiness_block(last_apply_age: Option) -> Option<&'stat } } -pub fn livez_response(livez: &LivezState, verbose: bool) -> Response { - let mut body = String::new(); - let mut failed = false; - - body.push_str("[+]ping ok\n"); - match livez.shutdown_check() { - Ok(()) => body.push_str("[+]shutdown ok\n"), - Err(_) => { - failed = true; - body.push_str("[-]shutdown failed: reason withheld\n"); - } - } - +/// `GET /livez` — process liveness: should this instance be RESTARTED? +/// +/// Answering at all is the check. The listener is bound and the runtime +/// is servicing requests, which is the whole of what a liveness probe +/// decides, so this never fails. +/// +/// A draining instance answers `200` like any other. Draining is +/// deliberate work, not a fault: an instance that has been told to shut +/// down is finishing the requests it already accepted, and restarting it +/// kills exactly those. That is what a failing liveness probe asks a +/// platform to do, which is why "stop sending traffic here" belongs on +/// [`readyz_response`] instead — the two questions are what separates +/// the endpoints, and answering both with the drain state collapses them +/// into one. +/// +/// The platform is not the only caller that matters: the gateway also +/// runs as a single container under docker or systemd, where a +/// supervisor watching this endpoint would restart a healthy draining +/// process mid-flight. +/// +/// [`LivezState`] is deliberately not a parameter. The drain state is the +/// one thing this answer must not depend on, and not taking it is a +/// stronger guarantee than a comment saying so. +pub fn livez_response(verbose: bool) -> Response { let headers = [ (CONTENT_TYPE, TEXT_PLAIN_UTF8.clone()), (X_CONTENT_TYPE_OPTIONS.clone(), NOSNIFF.clone()), ]; - if failed { - // Graceful shutdown is an expected drain, not an internal error — - // 503 so Kubernetes stops routing without treating it as a crash - // loop (#591). - return ( - StatusCode::SERVICE_UNAVAILABLE, - headers, - format!("{body}livez check failed"), - ) - .into_response(); - } - if !verbose { return (StatusCode::OK, headers, "ok").into_response(); } - ( - StatusCode::OK, - headers, - format!("{body}livez check passed\n"), - ) - .into_response() + (StatusCode::OK, headers, "[+]ping ok\nlivez check passed\n").into_response() } /// `GET /readyz` — readiness (traffic eligibility), distinct from `/livez` @@ -1188,8 +1182,7 @@ mod tests { #[tokio::test] async fn livez_default_success_is_plain_ok() { - let state = LivezState::new(); - let resp = livez_response(&state, false); + let resp = livez_response(false); assert_eq!(resp.status(), StatusCode::OK); let body = to_bytes(resp.into_body(), 1024).await.unwrap(); @@ -1198,28 +1191,34 @@ mod tests { #[tokio::test] async fn livez_verbose_success_lists_checks() { - let state = LivezState::new(); - let resp = livez_response(&state, true); + let resp = livez_response(true); assert_eq!(resp.status(), StatusCode::OK); let body = to_bytes(resp.into_body(), 1024).await.unwrap(); let text = std::str::from_utf8(&body).unwrap(); assert!(text.contains("[+]ping ok")); - assert!(text.contains("[+]shutdown ok")); assert!(text.contains("livez check passed")); } + /// A draining instance is healthy and must not be restarted: + /// restarting it kills the in-flight requests the drain exists to + /// finish. Liveness therefore stays `200` throughout, and it is + /// `/readyz` that withdraws the instance from traffic — the test + /// below pins the other half. #[tokio::test] - async fn livez_failure_returns_503_with_reason_withheld() { + async fn livez_stays_ok_while_draining() { let state = LivezState::new(); state.mark_shutting_down(); - let resp = livez_response(&state, false); - assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); - let body = to_bytes(resp.into_body(), 1024).await.unwrap(); - let text = std::str::from_utf8(&body).unwrap(); - assert!(text.contains("[-]shutdown failed: reason withheld")); - assert!(text.contains("livez check failed")); + let resp = livez_response(false); + assert_eq!(resp.status(), StatusCode::OK); + + let readyz = readyz_response(&state, None, false); + assert_eq!( + readyz.status(), + StatusCode::SERVICE_UNAVAILABLE, + "the drain has to be visible somewhere, and readiness is where", + ); } /// The count has to survive an unpaired decrement: it is reported on diff --git a/crates/aisix-proxy/src/lib.rs b/crates/aisix-proxy/src/lib.rs index 80388688..7dde0e61 100644 --- a/crates/aisix-proxy/src/lib.rs +++ b/crates/aisix-proxy/src/lib.rs @@ -1010,11 +1010,13 @@ fn warn_allowed(outcome: DrainOutcome) -> bool { true } +/// Takes no [`ProxyState`]: liveness answers whether the process should +/// be restarted, and nothing about the gateway's state changes that +/// answer. See [`crate::health::livez_response`]. async fn livez( - State(state): State, axum::extract::Query(params): axum::extract::Query>, ) -> Response { - crate::health::livez_response(&state.livez, params.contains_key("verbose")) + crate::health::livez_response(params.contains_key("verbose")) } async fn readyz( @@ -1800,7 +1802,7 @@ mod tests { } #[tokio::test] - async fn livez_returns_503_when_shutting_down() { + async fn livez_stays_200_when_shutting_down() { let hub = Arc::new(Hub::new()); let snap = seed_snapshot("my-gpt4", &["my-gpt4"], "http://unused"); let state = build_state(snap, hub); @@ -1814,10 +1816,12 @@ mod tests { .unwrap(); let resp = run(app, req).await; - assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); - let bytes = to_bytes(resp.into_body(), 1024).await.unwrap(); - let text = std::str::from_utf8(&bytes).unwrap(); - assert!(text.contains("livez check failed")); + assert_eq!( + resp.status(), + StatusCode::OK, + "a draining process is healthy; failing liveness asks the platform \ + to restart it, which kills the requests the drain is finishing", + ); } #[tokio::test] diff --git a/tests/e2e/src/cases/health-minimal-e2e.test.ts b/tests/e2e/src/cases/health-minimal-e2e.test.ts index 61d7d74d..e6f67a08 100644 --- a/tests/e2e/src/cases/health-minimal-e2e.test.ts +++ b/tests/e2e/src/cases/health-minimal-e2e.test.ts @@ -11,7 +11,13 @@ describe("livez e2e: public liveness route is /livez and /health is gone", () => if (!etcdReachable) return; // Held-back: this test drives the admin listener's health endpoint, // so it keeps admin bound (the suite default is now admin-off). - app = await spawnApp({ admin: true }); + // + // A drain window is needed too. The harness default is + // `min_drain_secs: 0`, which lets the process exit within + // milliseconds of SIGTERM — leaving no interval in which to observe + // what the health endpoints report WHILE draining, which is the + // whole of what the last test asserts. + app = await spawnApp({ admin: true, extra: { shutdown: { min_drain_secs: 5 } } }); }); afterAll(async () => { @@ -90,7 +96,21 @@ describe("livez e2e: public liveness route is /livez and /health is gone", () => expect(await adminReadyz.body.text()).toBe("ok"); }); - test("proxy /livez turns unhealthy after SIGTERM before exit", async (ctx) => { + // A drain withdraws traffic; it does not make the process a candidate + // for restarting. Those are the two different questions the two + // endpoints answer, and a drain has to move exactly one of them: + // `/readyz` reports it so the balancer stops routing here, `/livez` + // stays `200` because a failing liveness probe asks the platform to + // restart the instance — which would kill the very requests the drain + // is staying alive to finish. + // + // Kubernetes stops probing liveness once a pod enters graceful + // termination, so a rolling update does not act on the answer. That + // makes this a contract about the endpoint rather than about kubelet, + // and it is not academic: the gateway also runs as a single container + // under docker or systemd, where a supervisor watching `/livez` does + // act on it. + test("a drain moves /readyz to 503 and leaves /livez at 200", async (ctx) => { if (!etcdReachable || !app) { ctx.skip(); return; @@ -98,25 +118,31 @@ describe("livez e2e: public liveness route is /livez and /health is gone", () => app.signal("SIGTERM"); - const deadline = Date.now() + 3000; - let observedUnhealthy = false; + // Gate on readiness having withdrawn rather than on a sleep: it + // proves the drain has actually begun AND that the process is still + // serving, which is the window the liveness assertion below is + // about. The default `shutdown.min_drain_secs` is 30, so the window + // is wide. + const deadline = Date.now() + 5000; + let draining = false; while (Date.now() < deadline) { - try { - const res = await harnessRequest(`${app.proxyUrl}/livez`, { method: "GET" }); - if (res.statusCode !== 200) { - observedUnhealthy = true; - await res.body.dump(); - break; - } - await res.body.dump(); - } catch { - observedUnhealthy = true; + const res = await harnessRequest(`${app.proxyUrl}/readyz`, { method: "GET" }); + const status = res.statusCode; + await res.body.dump(); + if (status === 503) { + draining = true; break; } await new Promise((r) => setTimeout(r, 50)); } - - expect(observedUnhealthy).toBe(true); - app = undefined; + expect(draining, "/readyz never reported 503 after SIGTERM").toBe(true); + + for (const url of [`${app.proxyUrl}/livez`, `${app.adminUrl}/livez`]) { + const res = await harnessRequest(url, { method: "GET" }); + const status = res.statusCode; + const body = await res.body.text(); + expect(status, `${url} must stay live while draining`).toBe(200); + expect(body).toBe("ok"); + } }); }); From db18f3d996f21525aa0a729066813de67ae07215 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Tue, 25 Aug 2026 05:08:17 +0000 Subject: [PATCH 2/2] test(health): the drain window the poll waits on is the one beforeAll sets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The comment cited the gateway's 30s default, but this spec configures 5 — and the poll deadline outlasted that window, so a genuinely stuck drain would have been reported as a timeout on the wrong side of it. --- tests/e2e/src/cases/health-minimal-e2e.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/e2e/src/cases/health-minimal-e2e.test.ts b/tests/e2e/src/cases/health-minimal-e2e.test.ts index e6f67a08..9a8be990 100644 --- a/tests/e2e/src/cases/health-minimal-e2e.test.ts +++ b/tests/e2e/src/cases/health-minimal-e2e.test.ts @@ -121,9 +121,9 @@ describe("livez e2e: public liveness route is /livez and /health is gone", () => // Gate on readiness having withdrawn rather than on a sleep: it // proves the drain has actually begun AND that the process is still // serving, which is the window the liveness assertion below is - // about. The default `shutdown.min_drain_secs` is 30, so the window - // is wide. - const deadline = Date.now() + 5000; + // about. `beforeAll` gives that window 5 seconds, and the poll below + // gives up before it closes. + const deadline = Date.now() + 3000; let draining = false; while (Date.now() < deadline) { const res = await harnessRequest(`${app.proxyUrl}/readyz`, { method: "GET" });