fix(gateway): make readiness health checks dependency-aware#1328
fix(gateway): make readiness health checks dependency-aware#1328alangou wants to merge 2 commits into
Conversation
alangou
requested review from
a team,
derekwaynecarr,
maxamillion and
mrunalp
as code owners
TaylorMutch
left a comment
TaylorMutch
left a comment
There was a problem hiding this comment.
A couple of focused comments on the readiness changes.
alangou
force-pushed
the
alangou/os-156-update-gateway-health-check-to-account-for-database
branch
5 times, most recently
from
2cb34c9 to
05b6998
Compare
|
@TaylorMutch I removed the hardcoded value for the database timeout some documentation and deployments resources has been updated to reflect that change (helm chart, doc) |
|
Label |
alangou
force-pushed
the
alangou/os-156-update-gateway-health-check-to-account-for-database
branch
from
05b6998 to
2b11909
Compare
|
@TaylorMutch I have rebased the branch PR |
TaylorMutch
left a comment
TaylorMutch
left a comment
There was a problem hiding this comment.
A few changes I think are needed on this PR, but it's getting closer.
| openshell gateway info | ||
| ``` | ||
|
|
||
| Gateways expose `/healthz` as a liveness signal and `/readyz` as a dependency-aware readiness signal. `/readyz` checks database connectivity and reports unhealthy when the probe exceeds the configured timeout. Tune that timeout with `--readiness-db-timeout-secs` or `OPENSHELL_READINESS_DB_TIMEOUT_SECS` when you need a slower or stricter readiness window. |
There was a problem hiding this comment.
This is probably only relevant on the Kubernetes docs which care about these things; should this move into the Kubernetes docs?
| --config "${GATEWAY_CONFIG}" | ||
| --bind-address 0.0.0.0 | ||
| --port "${HOST_PORT}" | ||
| --health-port "${HEALTH_PORT}" |
There was a problem hiding this comment.
We don't currently depend on the health port in any configuration other than Kubernetes, so I would suggest we not add it into the docker e2e path and only exercise it on the kubernetes e2e tests (which would enable the health port by default).
| # Database readiness timeout in seconds used by /readyz and /health. | ||
| # Keep this lower than probes.readiness.timeoutSeconds so the gateway can | ||
| # return a readiness decision before Kubernetes times out the probe. | ||
| readinessDbTimeoutSecs: 1 |
There was a problem hiding this comment.
This seems like a possible footgun; the readiness check should be pretty instantaneous right? And if we have to add in a configuration option to the server to determine this, that seems strange to me.
Perhaps the health check for DB health should run in the background and the health check handler only checks if we detect the state to be unhealthy? That would remove this race condition. WDYT?
There was a problem hiding this comment.
Yes that works for me. The database healthcheck would run in background (with the configurable timeout) and update some sort of store in the gateway. When /readyz is queried the store is read. This would indeed be almost instant
alangou
force-pushed
the
alangou/os-156-update-gateway-health-check-to-account-for-database
branch
3 times, most recently
from
d008505 to
ab4deb4
Compare
Emit Prometheus readiness metrics for database probes (healthy gauge and outcome-labeled latency histogram) with coverage in health HTTP tests. Restrict Store::close behind test support cfg to prevent accidental runtime pool shutdown under live traffic. Signed-off-by: Adrien Langou <alangou@nvidia.com>
Signed-off-by: Adrien Langou <alangou@nvidia.com>
alangou
force-pushed
the
alangou/os-156-update-gateway-health-check-to-account-for-database
branch
from
ab4deb4 to
a167000
Compare
2 participants
Summary
This PR makes gateway readiness signals dependency-aware instead of always healthy, while keeping liveness intentionally lightweight.
It adds a configurable database readiness timeout, wires it end-to-end (CLI/env -> server config -> health router), and aligns Helm defaults so application timeout stays below Kubernetes probe timeout.
It also extends readiness coverage with Docker e2e and updates the Rust test task so
openshell-servertest-only coverage runs withtest-support.Related Issue
Changes
openshell-server:/healthzremains liveness-only (200when process is responsive)/readyzand/healthperform DB connectivity checks and return503when unavailablechecks.database.status, latency, error)--readiness-db-timeout-secsOPENSHELL_READINESS_DB_TIMEOUT_SECS> 0)ping/close) for both SQLite and Postgres stores.openshell_server_readiness_database_healthygauge (1healthy,0unhealthy)openshell_server_readiness_database_probe_duration_secondshistogram labeled byoutcome(success,db_error,timeout)/metricsexposureStore::closeand backendclosemethods are behind#[cfg(any(test, feature = "test-support"))]health_routerso they provide a realStore.crates/openshell-server/tests/health_endpoint_integration.rse2e/rust/tests/readyz_health.rse2e/rust/Cargo.toml/readyzprobingserver.readinessDbTimeoutSecspassed to gateway argsprobes.readiness.timeoutSecondsdefault set to 2stest-supportcoverage by default in Rust test lane:tasks/test.tomlruns workspace tests excludingopenshell-server, then runsopenshell-serverwith--features test-supportWhy
/healthzstill returns 200 when DB is down/healthzis kept as a pure liveness probe by design. If liveness depended on DB, transient DB outages could trigger unnecessary pod restarts and CrashLoop behavior without fixing the dependency outage. Readiness (/readyz) is the dependency-aware signal used to remove unhealthy pods from traffic.Why
closeis test-onlycloseis used to simulate dependency outages in tests. Exposing it in runtime code would make it possible to tear down an active pool under live traffic. Until a dedicated graceful-shutdown flow exists, keeping it behind test support prevents accidental production use.Testing
mise run pre-commitpassesValidation run:
mise run e2emise run ciChecklist