Skip to content

fix(deployment): increase time before LAPIS readiness probe fails to 1min from 30s - #7298

Open
corneliusroemer-agent wants to merge 2 commits into
mainfrom
fix/lapis-readiness-probe-tolerate-silo-blip
Open

corneliusroemer-agent wants to merge 2 commits into
mainfrom
fix/lapis-readiness-probe-tolerate-silo-blip

Conversation

@corneliusroemer-agent

@corneliusroemer-agent corneliusroemer-agent commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

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:

  • require more evidence of issues before declaring unready (increase readiness probe failure threshold)
  • increase readiness probe frequency from once every 10s to 5s to recover more quickly

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 503 that had nothing to do with the test: the group page asks every organism's LAPIS for its sequence count, and the pod for dummy-organism-with-files had just been dropped from its Service endpoints, so Traefik answered with an empty backend list. SILO was never down — it answered its own /info in 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, crossing failureThreshold: 3, and the pod stayed out of rotation for about ten seconds.

The probe fails because /sample/info makes 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: the timeoutSeconds: 5 on 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 periodSeconds multiplied by failureThreshold: 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, and failureThreshold only governs the demotion, so wait_for_pods_to_be_ready.py still 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 pods and 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 returns 500 rather than taking LAPIS's existing "SILO not reachable" path to a 503 with a message (SiloClient.kt:246-250HttpTimeoutException is not a ConnectException), both of which live upstream in LAPIS. I also left the CPU requests for silo and lapis alone, though they are the only knob here that would address the cause rather than the symptom.

🚀 Preview: Add preview label to enable

…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 claude Bot added the deployment Code changes targetting the deployment infrastructure label Sep 10, 2026
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

This PR may be related to: #6096 (Integration tests are flaky), #5118 (Unflake integration tests epic) — the fix addresses a specific flaky-test root cause (LAPIS pod eviction from its readiness probe tripping under brief SILO latency).

Comment thread kubernetes/loculus/templates/lapis-deployment.yaml Outdated
@corneliusroemer
corneliusroemer marked this pull request as ready for review September 10, 2026 10:20
Copilot AI lite review requested due to automatic review settings September 10, 2026 10:20
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @corneliusroemer's task in 2m 38s —— View job


Review

  • Read PR diff
  • Read lapis-deployment.yaml (probes, resources, rollout)
  • Cross-check other charts and the CI readiness gate
  • Post review

The change itself is sound and the reasoning in the description holds up against the repo:

  • No effect on initial readiness. .github/scripts/wait_for_pods_to_be_ready.py:45 polls containerStatuses[].ready, and Kubernetes starts a container not-ready and promotes it on the first success — failureThreshold only governs demotion. The deployment gate still waits for genuine readiness.
  • The cost of a longer tolerance is near zero here. replicas.lapis is 1 (values.yaml:2956), so evicting the only pod from its Service gains nothing except turning LAPIS's own 500 into a Traefik 503. The downside only appears at >1 replica, where a genuinely sick pod stays in rotation for up to 60s instead of 30s — fine for the stated goal.
  • Keeping failureThreshold finite rather than enormous is the right call; a misconfigured SILO URL should still surface in kubectl get pods.

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 # 60s annotation.

Nothing to flag on security, testing or docs — this is a value-only edit inside an existing probe block. I could not run helm lint (command not permitted in this job), but the change adds no template syntax.
· branch fix/lapis-readiness-probe-tolerate-silo-blip

@corneliusroemer corneliusroemer changed the title fix(deployment): let the LAPIS readiness probe ride out a brief SILO timeout fix(deployment): increase time before LAPIS readiness probe fails to 1min from 30s Sep 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. If /actuator/health also consults SILO (the 100 ms budget in SiloClient.kt was 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 fresh startupProbe cycle instead of a ~10s eviction.
  2. Even if /actuator/health is purely local, the PR's own diagnosis is that the stall is LAPIS-side under contention (resources.lapis.requests.cpu is 100m, 30m on the preview server, no CPU limit). A JVM starved enough to blow a 100 ms budget can also blow the liveness probe's timeoutSeconds: 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 →

Comment on lines +59 to +60
periodSeconds: 5
failureThreshold: 12

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@corneliusroemer-agent

corneliusroemer-agent commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

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 /actuator/health. Readiness is the last SILO-dependent probe left and the only one nobody has revisited — and #3839's sentence describes this incident one severity level down, since the dependency was made cheaper to fail rather than removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment Code changes targetting the deployment infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants