fix: Change FrontEnd Health thresholds #2336
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThe deployment YAML files for various backend Frontend services were updated to add a Changes
Sequence Diagram(s)sequenceDiagram
participant Kubelet
participant Frontend
Kubelet->>Frontend: startupProbe (HTTP GET /health)
Note right of Kubelet: Every 5s, timeout 5s, threshold 60
alt Startup success
Kubelet->>Frontend: livenessProbe (HTTP GET /health or exec)
Note right of Kubelet: Every 10s, timeout 5s
Kubelet->>Frontend: readinessProbe (HTTP GET /health or exec)
Note right of Kubelet: Every 15s, timeout 5s, threshold 3
else Startup fails
Kubelet--xFrontend: Restart container
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
components/backends/sglang/deploy/agg.yaml (1)
22-24: Liveness interval doubled – verify detection latency
periodSecondswas increased from 5 → 10 s. This slows down failure detection and recovery. If the pod is expected to crash‐loop quickly after the startup phase (e.g. OOM on first request), you may prefer to keep the original 5 s to minimise MTTR.components/backends/sglang/deploy/disagg.yaml (1)
22-24: Confirm longer liveness period is acceptableAs with the aggregate variant, doubling the liveness interval increases the time to notice failures. Please confirm this aligns with SLOs.
components/backends/sglang/deploy/agg_router.yaml (1)
22-24: Re-evaluate liveness interval change (5 → 10 s)See earlier comments – ensure the increased interval still meets router availability requirements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
components/backends/sglang/deploy/agg.yaml(2 hunks)components/backends/sglang/deploy/agg_router.yaml(2 hunks)components/backends/sglang/deploy/disagg.yaml(2 hunks)components/backends/trtllm/deploy/agg.yaml(2 hunks)components/backends/trtllm/deploy/agg_router.yaml(1 hunks)components/backends/trtllm/deploy/disagg.yaml(1 hunks)components/backends/trtllm/deploy/disagg_router.yaml(1 hunks)components/backends/vllm/deploy/agg.yaml(2 hunks)components/backends/vllm/deploy/agg_router.yaml(2 hunks)components/backends/vllm/deploy/disagg.yaml(2 hunks)components/backends/vllm/deploy/disagg_planner.yaml(2 hunks)components/backends/vllm/deploy/disagg_router.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: nnshah1
PR: ai-dynamo/dynamo#2124
File: components/backends/vllm/deploy/disagg.yaml:54-60
Timestamp: 2025-07-25T22:34:11.384Z
Learning: In vLLM worker deployments, startup probes (with longer periods and higher failure thresholds like periodSeconds: 10, failureThreshold: 60) are used to handle the slow model loading startup phase, while liveness probes are intentionally kept aggressive (periodSeconds: 5, failureThreshold: 1) for quick failure detection once the worker is operational. This pattern separates startup concerns from operational health monitoring in GPU-heavy workloads.
Learnt from: biswapanda
PR: ai-dynamo/dynamo#1890
File: examples/vllm/deploy/agg.yaml:63-70
Timestamp: 2025-07-14T23:01:16.218Z
Learning: In vLLM worker deployments, grep-based log checks for "VllmWorker.*has been initialized" are appropriate for readiness probes to verify worker startup, but should not be used for liveness probes which need to detect ongoing worker health.
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:1178-1180
Timestamp: 2025-07-18T16:05:05.534Z
Learning: The stopSignal field under lifecycle in DynamoComponentDeployment CRDs is autogenerated due to Kubernetes library upgrades (k8s.io/api and k8s.io/apimachinery from v0.32.3 to v0.33.1), not a manual design decision by the user.
Learnt from: KrishnanPrash
PR: ai-dynamo/dynamo#2217
File: components/backends/trtllm/engine_configs/deepseek_r1/wide_ep/wide_ep_prefill.yaml:18-0
Timestamp: 2025-07-31T11:26:48.422Z
Learning: TRTLLM LLM-API expects all caps for backend field names in configuration files. When migrating TRTLLM configurations, backend values like "WideEP" should be changed to "WIDEEP" to comply with the API requirements.
📚 Learning: in vllm worker deployments, startup probes (with longer periods and higher failure thresholds like p...
Learnt from: nnshah1
PR: ai-dynamo/dynamo#2124
File: components/backends/vllm/deploy/disagg.yaml:54-60
Timestamp: 2025-07-25T22:34:11.384Z
Learning: In vLLM worker deployments, startup probes (with longer periods and higher failure thresholds like periodSeconds: 10, failureThreshold: 60) are used to handle the slow model loading startup phase, while liveness probes are intentionally kept aggressive (periodSeconds: 5, failureThreshold: 1) for quick failure detection once the worker is operational. This pattern separates startup concerns from operational health monitoring in GPU-heavy workloads.
Applied to files:
components/backends/vllm/deploy/disagg_planner.yamlcomponents/backends/vllm/deploy/agg.yamlcomponents/backends/sglang/deploy/agg_router.yamlcomponents/backends/vllm/deploy/agg_router.yamlcomponents/backends/trtllm/deploy/agg_router.yamlcomponents/backends/sglang/deploy/agg.yamlcomponents/backends/vllm/deploy/disagg.yamlcomponents/backends/trtllm/deploy/disagg_router.yamlcomponents/backends/trtllm/deploy/agg.yamlcomponents/backends/trtllm/deploy/disagg.yamlcomponents/backends/vllm/deploy/disagg_router.yamlcomponents/backends/sglang/deploy/disagg.yaml
📚 Learning: in vllm worker deployments, grep-based log checks for "vllmworker.*has been initialized" are appropr...
Learnt from: biswapanda
PR: ai-dynamo/dynamo#1890
File: examples/vllm/deploy/agg.yaml:63-70
Timestamp: 2025-07-14T23:01:16.218Z
Learning: In vLLM worker deployments, grep-based log checks for "VllmWorker.*has been initialized" are appropriate for readiness probes to verify worker startup, but should not be used for liveness probes which need to detect ongoing worker health.
Applied to files:
components/backends/vllm/deploy/disagg_planner.yamlcomponents/backends/vllm/deploy/agg.yamlcomponents/backends/sglang/deploy/agg_router.yamlcomponents/backends/vllm/deploy/agg_router.yamlcomponents/backends/trtllm/deploy/agg_router.yamlcomponents/backends/sglang/deploy/agg.yamlcomponents/backends/vllm/deploy/disagg.yamlcomponents/backends/trtllm/deploy/disagg_router.yamlcomponents/backends/trtllm/deploy/agg.yamlcomponents/backends/trtllm/deploy/disagg.yamlcomponents/backends/vllm/deploy/disagg_router.yamlcomponents/backends/sglang/deploy/disagg.yaml
📚 Learning: in components/backends/sglang/deploy/agg_router.yaml, the clear_namespace command is intentionally d...
Learnt from: biswapanda
PR: ai-dynamo/dynamo#2137
File: components/backends/sglang/deploy/agg_router.yaml:0-0
Timestamp: 2025-07-28T17:00:07.968Z
Learning: In components/backends/sglang/deploy/agg_router.yaml, the clear_namespace command is intentionally designed to block the router from starting if it fails (using &&). This is a deliberate design decision where namespace clearing is a critical prerequisite and the router should not start with an uncleared namespace.
Applied to files:
components/backends/sglang/deploy/agg_router.yamlcomponents/backends/vllm/deploy/agg_router.yamlcomponents/backends/trtllm/deploy/agg_router.yamlcomponents/backends/trtllm/deploy/disagg_router.yamlcomponents/backends/vllm/deploy/disagg_router.yaml
📚 Learning: the stopsignal field under lifecycle in dynamocomponentdeployment crds is autogenerated due to kuber...
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:1178-1180
Timestamp: 2025-07-18T16:05:05.534Z
Learning: The stopSignal field under lifecycle in DynamoComponentDeployment CRDs is autogenerated due to Kubernetes library upgrades (k8s.io/api and k8s.io/apimachinery from v0.32.3 to v0.33.1), not a manual design decision by the user.
Applied to files:
components/backends/sglang/deploy/agg.yamlcomponents/backends/trtllm/deploy/agg.yamlcomponents/backends/sglang/deploy/disagg.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: Build and Test - vllm
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (9)
components/backends/vllm/deploy/disagg_planner.yaml (2)
32-32: Liveness period change is saneIncreasing to 10 s aligns with worker patterns and reduces noise without hurting detection time.
41-43: Readiness probe tuning looks goodTighter period/timeout/failure thresholds are appropriate once a startup probe is in place.
components/backends/vllm/deploy/agg.yaml (2)
22-22: Liveness-probe period bump LGTM
31-33: Readiness-probe timing LGTMcomponents/backends/vllm/deploy/disagg.yaml (1)
35-37: Readiness tweaks approvedcomponents/backends/vllm/deploy/agg_router.yaml (2)
22-24: Liveness period update looks fine
31-33: Readiness probe params OKcomponents/backends/vllm/deploy/disagg_router.yaml (2)
25-27: Liveness probe change approved
34-36: Readiness probe tuning approved
Contributor
Author
|
dup |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview:
Move probes from the backward-compatible to new locations and add the StartupProbe.
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit