fix(helm): give gateway and backend probes an explicit timeoutSeconds - #35497
Conversation
|
@greptileai please review; this adds an explicit probe timeoutSeconds to the chart plus a mutation-checked helm-unittest suite |
|
|
Greptile SummaryThis PR gives gateway and backend health probes explicit ten-second timeouts and increases the liveness failure threshold to tolerate temporary event-loop saturation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| helm/litellm/values.yaml | Adds the intended gateway and backend probe defaults without changing UI probes or preventing component-specific overrides. |
| helm/litellm/tests/probe_tests.yaml | Adds focused regression tests that verify the rendered defaults and preserve operator override behavior. |
Reviews (4): Last reviewed commit: "fix(helm): give gateway and backend prob..." | Re-trigger Greptile
882037b to
7f15804
Compare
|
@greptileai re-review please; the body was corrected to drop an unproven causal claim and the commit was amended (now 7f15804), the diff itself is unchanged |
7f15804 to
5fafea9
Compare
|
@greptileai please re-review at 5fafea9. The ui component was dropped from the diff (it is nginx serving a static export, so the single-event-loop reasoning does not apply to it); gateway and backend are unchanged from the previous review. |
The gateway and backend probes omitted timeoutSeconds, so kubelet applied its 1s default. Both containers run a single uvicorn worker (the gateway defaults NUM_WORKERS to 1; the backend passes no --workers at all), so each pod is one asyncio event loop and its per-request latency under closed-loop saturation rises by queueing (~57-62ms serial vs ~6s at 100 concurrent users against one replica). Both /health/readiness and /health/liveliness then time out on the stage cluster while the pod is serving traffic correctly, which exposes the deployment to losing a healthy pod from its load balancer during a burst and to restarting a merely busy one. Readiness now gets timeoutSeconds 10, equal to periodSeconds and above the measured saturated latency, and keeps failureThreshold 3. kubelet drives each probe from a time.Ticker of periodSeconds rather than sleeping between attempts, and coalesces ticks that arrive mid-probe, so the interval between probe starts is max(periodSeconds, probeDuration) and not their sum. Keeping timeoutSeconds <= periodSeconds is what holds that interval at 10s, so three consecutive failures still evict a genuinely wedged pod in ~30s. Liveness gets the same timeout plus failureThreshold 6: /health/liveliness is an in-memory flag check, so a timeout there only ever means event-loop starvation, which a restart makes worse, and it now needs ~90s of sustained unresponsiveness to fire. The ui container keeps the default. It is nginx serving a Next.js static export, so / is a file off disk with no application runtime that could queue behind saturated work, and nothing measured suggests it needs more than 1s.
5fafea9 to
bca4bd4
Compare
|
@greptileai please re-review at bca4bd4. Only the commit message and PR body changed since the last review; the diff is identical to 5fafea9. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
TLDR
Problem this solves:
timeoutSeconds, so kubelet used 1sHow it solves it:
timeoutSeconds: 10on both probes, gateway and backendfailureThreshold: 6so a busy pod is not restartedRelevant issues
Linear ticket
Resolves LIT-5054
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Field evidence (observed on the stage EKS cluster by a maintainer, before this change)
The deployed stage gateway spec carries the implicit default:
timeoutSeconds: 1,periodSeconds: 10,failureThreshold: 3. Under closed-loop load Kubernetes logged probe timeouts repeatedly and reproducibly, across several pods and several runs, for both endpoints:and the same for
/health/liveliness. Per-request latency on that replica measured ~57-62ms serial and ~6s at 100 concurrent users, which is what pushes both probes past a 1s budgetThe consequence is stated here as risk, not as an observed outage. A 100-user, 180s run against a settled single replica came back inside the 1% failure ceiling even while readiness probes were still timing out during it, so no specific request-error storm is attributed to the probe timeout. What stands on its own is that probes time out against a pod that is serving traffic correctly, and what that exposes the deployment to: readiness failures pull a healthy pod out of its load balancer during a burst, and liveness failures restart a merely busy pod, each removing capacity at the moment it is needed
Rendered probes, before (base commit
de43328f63)$ helm template rel ./helm/litellm -f helm/litellm/tests/values/required.yamlRendered probes, after (commit
bca4bd4b36)$ helm template rel ./helm/litellm -f helm/litellm/tests/values/required.yamlThe full rendered diff is exactly two hunks, one for the gateway Deployment and one for the backend Deployment, both of the shape above. The ui Deployment renders byte-identically to before
helm-unittest (commit
bca4bd4b36)Mutation check, the other direction. Reverting
helm/litellm/values.yamlto its pre-change state (restored afterwards from a copy taken before any edit, never withgit checkout) and re-running the new suite fails 4 of its 5 tests and every assertion that is about the defaults:The fifth test sets its own probe overrides through
set:, so it is deliberately insensitive to the defaults and passes in both directions; it pins that operators can still override the values rather than pinning the values themselveshelm lint (commit
bca4bd4b36)Type
🐛 Bug Fix
Changes
helm/litellm/values.yamllefttimeoutSecondsunset on the gateway and backend probes, which means Kubernetes applies its default of 1 second. Both containers run a single uvicorn worker, so a pod is a single asyncio event loop and its per-request latency under closed-loop saturation is dominated by queueing rather than by work. Once queueing exceeds a second, both probes time out even though the pod is serving traffic correctly, which is a bug in the probe configuration regardless of what the request path is doing at the time: the probe is answering the question "is this pod broken" with a budget that a working pod cannot meet. WithfailureThreshold: 3andperiodSeconds: 10a readiness probe in that state takes the pod out of its Service after roughly 30 seconds, and the liveness probe restarts it, so the exposure is losing healthy capacity during exactly the bursts that need itBoth endpoints were checked before picking numbers.
/health/livelinessreturns a constant after reading an in-process shutdown flag, so it does no I/O at all; the only way it can be slow is event-loop starvation./health/readinessalso calls_db_health_readiness_check, which does a real Postgres round trip (cached for 15s on success), so it is strictly the more expensive of the two and additionally exposed to database latencyReadiness gets
timeoutSeconds: 10, andfailureThresholdstays at the default 3. The measured saturated latency was ~6s, so a budget at or below 5s still trips during exactly the burst we are trying to tolerate; the probe's own latency distribution was never measured separately, only the fact that it exceeded 1s, so picking a number under the one measurement we do have would be precision we have not earned10s equals
periodSeconds, which is the ceiling, and that boundary is what keeps the eviction arithmetic intact. kubelet's prober worker drivesdoProbefrom atime.TickerofperiodSeconds(pkg/kubelet/prober/worker.go) rather than sleeping between attempts, and a Go ticker coalesces ticks that arrive while the receiver is busy, so the interval between probe starts ismax(periodSeconds, probeDuration)and not their sum. With the timeout at the period, probes run effectively back to back and three consecutive failures still take about 30 seconds. Raising the timeout above the period is what would break this: attimeoutSeconds: 15withperiodSeconds: 10the probe duration becomes the cadence and eviction stretches to ~45s, which is the real reason for thetimeoutSeconds <= periodSecondsruleA pod that is genuinely down refuses the connection immediately rather than consuming the timeout, so the longer budget does not slow real failure detection either
Liveness gets the same
timeoutSeconds: 10plusfailureThreshold: 6. Restarting a merely busy pod is strictly worse than leaving it busy: it throws away in-flight requests, pays a cold start, and hands the load to the remaining replicas. Because the liveness endpoint does no I/O, ten seconds without an answer already means the loop is wedged, and requiring six consecutive such failures (period 15s) means roughly 90 seconds of sustained unresponsiveness before a restart. Readiness therefore reacts in ~30s and liveness in ~90s, which is the ordering we wantThe backend gets the same treatment on its own evidence rather than for symmetry with the gateway.
backend/Dockerfileends inENTRYPOINT ["uvicorn", "backend.main:app"]with no--workersflag, so uvicorn's default of one worker applies and the backend is unconditionally single-loop; unlike the gateway it does not even expose anumWorkersknob to raise. It serves the same/health/readinesswith the same Postgres round trip, so the identical omission has the identical consequenceThe
uicomponent is deliberately left on the Kubernetes default and is not touched by this PR.ui/Dockerfilebuilds a Next.js static export and serves it fromnginx:1.27-alpine, so/is a file read off disk with no application runtime that could queue behind saturated work. The single-event-loop reasoning above does not transfer to it, there is no measurement suggesting it needs more than a second to serve a static file, and widening its budget tenfold without a reason of its own would be scope creephelm/litellm-helm/values.yaml, the older chart, was checked and deliberately left alone: it already setstimeoutSecondsexplicitly on its liveness, readiness, and startup probes, so the omission this PR fixes does not exist there. Its value is 5s, which is below the latency measured here, but retuning a published chart's defaults is a separate decision with a much wider blast radius than fixing a missing key, and it is not needed to resolve this ticketTests live in
helm/litellm/tests/probe_tests.yamland pin the full rendered probe block for the gateway and the backend, assert that neither single-event-loop component is left without an explicit timeout, assert the liveness/readiness threshold asymmetry, and assert that operators can still override any of it per componentA companion e2e PR, #35494, makes the throughput load test's failure messages surface this class of failure by name instead of reporting an undifferentiated throughput miss. That test only goes fully green in stage once this chart change ships and stage redeploys
Final Attestation