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..9a8be990 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"); + // 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. `beforeAll` gives that window 5 seconds, and the poll below + // gives up before it closes. const deadline = Date.now() + 3000; - let observedUnhealthy = false; + 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"); + } }); });