fix(deployment): increase time before LAPIS readiness probe fails to 1min from 30s - #7298
corneliusroemer-agent wants to merge 2 commits into
Conversation
…timeout /sample/info calls SILO live with a hard-coded 100ms budget, so a moment of load on the node makes the probe fail. Three failures in a row evicted the pod from its Service, and with one replica the ingress then had no backend and answered 503 while SILO was perfectly healthy. Eviction latency is periodSeconds x failureThreshold: 30s before, which loses to the ~40s failure burst measured in run 33877810215. This makes it 60s, and the shorter period halves how long recovery takes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lk4J5vNvAC1X6KZ1JsWhar
|
Claude finished @corneliusroemer's task in 2m 38s —— View job Review
The change itself is sound and the reasoning in the description holds up against the repo:
Two things left inline: the liveness probe was not adjusted alongside readiness and is now the stricter of the two (the more important point), and a note on the period-vs-threshold trade-off plus the missing Nothing to flag on security, testing or docs — this is a value-only edit inside an existing probe block. I could not run |
There was a problem hiding this comment.
🔵 Needs a closer look
The shorter interval doubles SILO request load; use a 10-second interval with threshold 6 or provide evidence supporting the faster polling.
Pull request overview
This pull request tunes LAPIS readiness probing to tolerate transient load-related failures and reduce spurious 503 responses.
Changes:
- Checks readiness every 5 seconds.
- Requires 12 consecutive failures before marking LAPIS unready.
File summaries
| File | Summary |
|---|---|
kubernetes/loculus/templates/lapis-deployment.yaml |
Adjusts LAPIS readiness probe timing and failure threshold. |
Review details
Suppressed comments (1)
kubernetes/loculus/templates/lapis-deployment.yaml:60
- This halves the probe interval for
/sample/info, so every LAPIS pod now makes twice as many live SILO calls on the same path that is timing out under load. That added traffic can increase probe failures and prolong flapping; the same approximately 60-second failure tolerance can be achieved without doubling this load by keeping a 10-second period and using a threshold of 6. Please either use that trade-off or provide evidence that the faster re-addition of endpoints justifies the extra request rate.
periodSeconds: 5
failureThreshold: 12
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| periodSeconds: 5 | ||
| failureThreshold: 12 | ||
| timeoutSeconds: 5 | ||
| livenessProbe: |
There was a problem hiding this comment.
The liveness probe below is left at 10s × 3 = 30s, so it is now stricter than the readiness probe it is meant to outlive. Two ways that bites:
- If
/actuator/healthalso consults SILO (the 100 ms budget inSiloClient.ktwas reportedly introduced precisely so the health endpoint wouldn't block), then the same bursts that trip readiness can trip liveness — and liveness crossing its threshold restarts the container after 30s, well before readiness reaches 60s. The mitigation would be replaced by a worse failure mode: a restart plus a freshstartupProbecycle instead of a ~10s eviction. - Even if
/actuator/healthis purely local, the PR's own diagnosis is that the stall is LAPIS-side under contention (resources.lapis.requests.cpuis100m,30mon the preview server, no CPU limit). A JVM starved enough to blow a 100 ms budget can also blow the liveness probe'stimeoutSeconds: 5.
Raising failureThreshold to 6 on the liveness probe (60s, same tolerance) would make it impossible for liveness to fire first.
I couldn't verify from this job whether LAPIS's actuator health contributes a SILO indicator (no network access here) — if you can confirm it doesn't, point 2 alone is still a reason to widen it. Fix this →
| periodSeconds: 5 | ||
| failureThreshold: 12 |
There was a problem hiding this comment.
Halving the period doubles how often each pod runs a real /sample/info → SILO round trip: ~0.8 → ~1.6 probes/s across eight organisms in CI. That is small in absolute terms, but it is extra work on exactly the code path whose latency is the problem, on pods that request 100m CPU. The 10s / failureThreshold: 6 variant mentioned in the description buys the identical 60s tolerance without adding load; the only thing it gives up is halved recovery time after an eviction. Worth being explicit about which of the two you're optimising for, since the added load pushes slightly in the direction of the failure being mitigated.
Also a nit: the neighbouring failureThreshold: 36 # 3 minutes to start carries its arithmetic inline. With the long comment dropped, 5 × 12 has no in-file record of meaning 60s — a trailing # 60s would keep the two probes readable side by side.
|
Some history on how we ended up with our current LAPIS readiness/liveness probes: #3833 gave LAPIS startup, liveness and readiness probes all pointing at it; #3839 then moved liveness off it the same day ("before this when SILO saturated all LAPIS pods would be restarted"), and #4261 removed the startup probe ("to not kill it on startup when SILO is down", explicitly helping #2701, the CPU-runaway spiral). Both times the fix was to repoint at |
Chromium e2e sometimes fails with 503 (from traefik) because LAPIS fails readiness probe when it times out (timeout just 100ms) asking silo whether it's up. Silo actually replies just fine, delay seems to be on LAPIS side. 100ms is definitely too short for this but we can't make that change here - needs to be upstream LAPIS. 100ms can be breached under load.
Quick mitigations we can make ourselves here:
I'll make some PRs on LAPIS side as well but these changes here wouldn't harm.
Claude desc
One chromium integration test failed in run 33877810215 on a single
503that had nothing to do with the test: the group page asks every organism's LAPIS for its sequence count, and the pod fordummy-organism-with-fileshad just been dropped from its Service endpoints, so Traefik answered with an empty backend list. SILO was never down — it answered its own/infoin under a millisecond throughout, never restarted, and its importer was idle. What had happened is that LAPIS's readiness probe failed four times in a row, crossingfailureThreshold: 3, and the pod stayed out of rotation for about ten seconds.The probe fails because
/sample/infomakes a live call to SILO on every request, and that call is given a hard-coded budget of 100 milliseconds (SiloClient.kt:179, added in LAPIS #1593 so that the health endpoint would not block). Since SILO itself was answering instantly, that 100 ms was spent somewhere on the LAPIS side under load, which on a CI runner hosting seven organisms' worth of services is easy to believe. Worth knowing before anyone tries to tune this from the chart: thetimeoutSeconds: 5on the probe is inert, because the application gives up long before the probe does.Counting every probe in that job's archived logs, all eight organisms together, gives 30 failures out of 683 probes, and the failures arrive in bursts rather than independently — the one burst that actually evicted a pod lasted 20 seconds of observed failure and at most 40 seconds. What matters is how long a bad window LAPIS can ride out, which is
periodSecondsmultiplied byfailureThreshold: 30 seconds today, so a 40-second window wins. This change makes that 60 seconds. Going to a shorter period with a proportionally higher threshold, rather than just raising the threshold, keeps the same tolerance while halving how long recovery takes after an eviction and logging the first failure sooner. It cannot delay a pod becoming ready in the first place: Kubernetes starts a container not-ready and promotes it on the first successful probe, andfailureThresholdonly governs the demotion, sowait_for_pods_to_be_ready.pystill waits for genuine readiness.Two judgement calls to push back on if you disagree. The numbers come from one burst in one job on one runner, so the 60 seconds is a reasoned guess with roughly 1.5x margin rather than a measured requirement — 10 seconds with a threshold of 6 gets the same tolerance if you would rather not double the probe rate, at the cost of slower recovery. And the threshold is deliberately left finite rather than made enormous: a LAPIS that genuinely cannot reach SILO, say from a wrong URL, should still go not-ready so that it shows up in
kubectl get podsand fails the deployment's readiness wait. Deliberately not addressed here: the 100 ms budget itself, and the fact that a timeout is currently classified as an unexpected error and returns500rather than taking LAPIS's existing "SILO not reachable" path to a503with a message (SiloClient.kt:246-250 —HttpTimeoutExceptionis not aConnectException), both of which live upstream in LAPIS. I also left the CPU requests forsiloandlapisalone, though they are the only knob here that would address the cause rather than the symptom.🚀 Preview: Add
previewlabel to enable