feat(operator): enable worker canary health checks - #11083
Conversation
|
👋 Hi xianlubird! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
Walkthrough
Health Check Env Var Default
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/operator/internal/controller/upgrade_test.go`:
- Line 167: The pre-upgrade fixture data in upgrade_test.go is now modeling the
post-change state, so the hash comparison in the rollout compatibility tests can
no longer detect a default flip. Update the persisted child object fixtures used
by the specHash(oldChild) and specHash(newChild) assertions to keep
DYN_HEALTH_CHECK_ENABLED at the old persisted value, or if the rollout is
intended, split the affected cases in the relevant test helpers so they
explicitly assert a rollout instead of no-change behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 378786f7-a274-48a0-9915-366ec256a624
📒 Files selected for processing (4)
deploy/operator/internal/controller/dynamocomponentdeployment_controller_test.godeploy/operator/internal/controller/upgrade_test.godeploy/operator/internal/dynamo/component_worker.godeploy/operator/internal/dynamo/graph_test.go
edd7f74 to
422a8d4
Compare
|
/ok to test 422a8d4 |
|
@rmccorm4 The base worker default now stays conservative, and each backend opts in only for modes that are known to provide a non-nil canary health_check_payload. Specifically, vLLM/TRT-LLM decode workers remain disabled because they do not currently provide a canary payload, and SGLang tokenizer mode remains disabled because its unified backend explicitly returns None for health_check_payload. Regular vLLM, TRT-LLM, and SGLang worker modes still default to canary health checks. This keeps the Kubernetes default aligned with the docs for supported worker modes while avoiding permanent NotReady endpoints for modes that cannot satisfy the generate canary yet. |
|
🔄 Datadog auto-retried 1 job - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 8a61cd2 | Docs | Datadog PR Page | Give us feedback! |
|
/ok to test f53dc29 |
TRT-LLM decode worker does provide a canary payload: , as does vllm. Since unified backend is not the default, we are not concerned about the SGLang tokenizer mode.Adding conditional logic in the operator based on flags/env-vars is fragile and best avoided. |
Signed-off-by: xianlubird <xianlubird@gmail.com>
3823ffb to
8a61cd2
Compare
|
/ok to test 8a61cd2 |
|
The vLLM deploy CI failure is unrelated to this PR's code: |
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test 52da0a0 |
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test 0f0b07c |
|
/ok to test 66b0fce |
Overview
Enable canary worker health checks by default for Kubernetes operator-managed workers. This brings the operator back in line with the published health check documentation, which states that Kubernetes deployments default
DYN_HEALTH_CHECK_ENABLEDtotrue: https://docs.nvidia.com/dynamo/user-guides/observability-local/health-checks#worker-liveness-and-health-checkDetails
DYN_HEALTH_CHECK_ENABLEDvalue totrue.falsevalue and explicitly assert that upgrading across this default flip changes the child spec hash, while still preserving legacy worker identity labels.The worker default was disabled in #5038 as a temporary mitigation for DIS-1185 races involving discovery propagation, NATS subscription readiness, and health check requests. The underlying issues have since been addressed:
With those fixes in place, Kubernetes workers can now use the documented default and require the canary path to validate readiness.
Where should the reviewer start?
Start with
deploy/operator/internal/dynamo/component_worker.go, then review the test expectation updates indeploy/operator/internal/dynamo/graph_test.go,deploy/operator/internal/controller/dynamocomponentdeployment_controller_test.go, anddeploy/operator/internal/controller/upgrade_test.go.Related Issues
Summary
DYN_HEALTH_CHECK_ENABLED=trueby default for operator-managed worker components in Kubernetes.Validation
go test ./internal/dynamo ./internal/controllerfromdeploy/operatorgit diff --check