fix(operator): support Dynamo 1.5 SGLang embedding canaries

[xianlubird]

Summary

• Add an Operator compatibility shim for Dynamo 1.5.0 SGLang embedding workers so active health checks use an embedding-shaped payload.
• Preserve explicit DYN_HEALTH_CHECK_PAYLOAD overrides and leave chat workers and newer runtimes unchanged.
• This incompatibility was discovered by the N-2 compatibility test in PR test(deploy): add N-2 component compatibility matrix #14460 with a 1.6 frontend and a 1.5.0 embedding worker.

Where should the reviewer start?

Start in deploy/operator/internal/dynamo/backend_sglang.go, where the compatibility payload is gated by the resolved runtime version and --embedding-worker mode. The adjacent table test covers the affected path and each scope boundary.

Related Issues

• Related to DEP: N-2 component compatibility testing on shared deployment infrastructure #14685
• Related to DEP: Operator–Runtime Version Compatibility Contract #9020
• Discovered by test(deploy): add N-2 component compatibility matrix #14460

Validation

• go test ./internal/dynamo -run "^TestSGLangBackend_"

Summary by CodeRabbit

• Bug Fixes
  • Improved SGLang 1.5 embedding worker health checks by applying the compatible default health-check payload.
  • Preserved custom health-check payloads when provided.
  • Left chat workers and newer SGLang runtime versions unchanged.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[dynamo-ops]

/ok to test 8a59e88

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e9438a20-d495-4fce-9fd0-ca0df65076f7

📥 Commits

Reviewing files that changed from the base of the PR and between 7d4c346 and 8a59e88.

📒 Files selected for processing (2)

• deploy/operator/internal/dynamo/backend_sglang.go
• deploy/operator/internal/dynamo/backend_sglang_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

Walkthrough

SGLang container configuration now injects a health-check payload for version 1.5.0 embedding workers when no payload exists. Tests cover default injection, user overrides, chat workers, and newer runtime versions.

Changes

SGLang health-check compatibility

Layer / File(s)	Summary
Conditional payload injection deploy/operator/internal/dynamo/backend_sglang.go	UpdateContainer resolves the runtime and adds the SGLang 1.5.0 embedding health-check payload when the command includes --embedding-worker and no payload is set.
Payload behavior coverage deploy/operator/internal/dynamo/backend_sglang_test.go	Table-driven tests verify default injection, user-provided payload precedence, and no injection for chat workers or newer runtimes.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 8a59e

This change provides the required embedding health-check payload for Dynamo 1.5.0 SGLang embedding workers while preserving overrides and leaving other workers unchanged. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main change: support for Dynamo 1.5 SGLang embedding canaries in the operator.
Description check	✅ Passed	The description explains the compatibility shim, scope boundaries, reviewer starting point, validation command, and related issues. It does not use the template headings exactly and does not select th…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

Warning

Some tools did not complete. Review the errors below.

🔧 golangci-lint (2.13.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

_{Comment @coderabbitai help to get the list of available commands.}

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

[dynamo-ops]

/ok to test 450811c

[dynamo-review-agent]

Questions for the author:

• deploy/operator/internal/dynamo/backend_sglang.go: DYN_SGL_EMBEDDING_WORKER can be supplied through Kubernetes valueFrom (for example, a ConfigMap or Secret), but the new resolver deliberately treats that as disabled and does not inject the compatibility payload. Please provide evidence that this configuration path is unsupported for 1.5 embedding workers, or a way to preserve the shim when the runtime resolves it to true.

[dynamo-ops]

/ok to test 585b027

[dynamo-review-agent]

Previously reported defects still present:

• Original discussion: Still present: sglangEmbeddingWorkerEnabled only treats literal DYN_SGL_EMBEDDING_WORKER values as enabling embedding mode, so a supported valueFrom source that resolves true at runtime still receives no compatibility health-check payload.
• Original discussion: Still present: sglangEmbeddingWorkerEnabled treats DYN_SGL_EMBEDDING_WORKER supplied through valueFrom or envFrom as disabled. A 1.5.0 embedding worker enabled by a ConfigMap or Secret therefore misses the compatibility payload and retains the failing chat-shaped canary.
• Original discussion: The previously reported valueFrom/envFrom embedding-worker configuration defect remains present: sglangEmbeddingWorkerEnabled treats a valueFrom-backed DYN_SGL_EMBEDDING_WORKER as disabled, and EnvFrom is not represented in container.Env. A 1.5.0 worker that resolves this supported setting to true still misses the compatibility payload and retains the failing canary.
• Original discussion: Verified still present: DYN_SGL_EMBEDDING_WORKER can be supplied via a supported Kubernetes valueFrom or envFrom source, but the resolver treats both as disabled. A 1.5.0 embedding worker resolving this setting to true therefore receives no compatibility payload and its active canary continues to fail.

[dynamo-ops]

/ok to test f9ff95e

[dynamo-ops]

/ok to test 55940de

[tmonty12]

A few concerns here.

For a 1.5.0 SGLang embedding worker with DYN_HEALTH_CHECK_ENABLED=true and no payload defined, get_health_status hits the health_check_enabled && !endpoint_health.is_empty() branch (system_health.rs:242) and uses transport registration as the readiness signal. The endpoint goes Ready and serves traffic normally. This is intended behavior, not a bug.

The fix adds ~100 lines of flag parsing to the operator to handle something that isn't broken — walking container.Env, checking for valueFrom, scanning container.Command/container.Args in precedence order, materializing envFrom from the API server. This is fragile: it silently misses any user who sets --embedding-worker through a wrapper script or an indirection the static parse can't see, and the Unknown error mode already acknowledges it can't cover all cases.

If users want active canary probing on a 1.5.0 embedding worker, the right path is a one-line addition to the upgrade guide: set DYN_HEALTH_CHECK_PAYLOAD: '{"model":"<model-name>","input":"Test"}' in the DCD env directly. That's explicit, always correct, and doesn't add operator complexity.

[dynamo-review-agent]

Previously reported defects still present:

• Original discussion: Still present: Grove materializes component envFrom before generatePodSpecForComponent applies graph-level spec.env. The temporary envFrom value wins MergeEnvs, then Restore removes it and also loses a conflicting graph-level explicit DYN_HEALTH_CHECK_PAYLOAD; the lower-precedence envFrom value takes effect instead.
• Original discussion: Still present: Grove materializes component envFrom before generatePodSpecForComponent merges graph-level spec.env. A materialized component DYN_HEALTH_CHECK_PAYLOAD wins MergeEnvs, then Restore removes that name entirely, dropping a conflicting graph-level explicit payload and allowing the lower-precedence envFrom value to take effect.
• Original discussion: Still present: Grove materializes component envFrom before generatePodSpecForComponent merges graph-level spec.env. The materialized entry wins that merge, and Restore then removes the matching name from the final pod, dropping a conflicting graph-level explicit DYN_HEALTH_CHECK_PAYLOAD override.
• Original discussion: Still present: the compatibility payload decision is persisted from a point-in-time ConfigMap/Secret lookup, while neither DCD nor DGD reconciliation watches referenced envFrom objects. Later adding or removing DYN_HEALTH_CHECK_PAYLOAD can therefore leave newly created pods with a stale explicit override or without the required default.
• Original discussion: Still present: the rendered explicit compatibility payload is based on a point-in-time envFrom read, but neither the DCD controller nor the Grove path watches referenced ConfigMaps or Secrets. Later source changes can leave the explicit default shadowing a newly supplied payload, or leave pods without a default after a sourced payload is removed.
• Original discussion: Still present: the rendered explicit compatibility payload is based on the envFrom source contents at reconciliation time, but neither the DGD nor DCD controller watches referenced ConfigMaps or Secrets. Adding or removing DYN_HEALTH_CHECK_PAYLOAD later leaves the existing pod template with a stale explicit override or without a needed default for subsequently created pods.

[xianlubird]

@tmonty12
Thanks for the review. I agree the current operator-side flag and envFrom handling is too complex, and I’d like to simplify it.

One important detail: the 1.5.0 embedding worker passes a generation-shaped SglangHealthCheckPayload to serve_endpoint by default.

It therefore registers a canary target; it does not take the payload-less transport-readiness fallback.
The embedding handler expects model and input, so that default canary fails.

An explicit DYN_HEALTH_CHECK_PAYLOAD is a valid workaround, but existing production users may not know they need to add it before upgrading.

If they keep their default DGD and upgrade to a version that enables active checks, embedding workers could remain unready and disrupt traffic.

The mixed-version compatibility documentation does not state this configuration requirement.

I think the default upgrade path needs a fix. WDYT the direction ?

[dynamo-review-agent]

Previously reported defects still present:

• Original discussion: Still present: Grove materializes component envFrom before generatePodSpecForComponent merges graph-level spec.env; a materialized component payload wins that merge, then Restore removes the name entirely, dropping a conflicting graph-level explicit payload.
• Original discussion: Still present: Grove materializes component envFrom before generatePodSpecForComponent merges graph-level spec.env. A materialized component DYN_HEALTH_CHECK_PAYLOAD wins MergeEnvs, then Restore removes that name from the final pod, dropping a conflicting graph-level explicit payload and allowing the lower-precedence envFrom value to take effect.
• Original discussion: Still present: Grove materializes component envFrom before graph-level spec.env is merged. The materialized component value wins MergeEnvs, then Restore removes that name, dropping a conflicting graph-level explicit DYN_HEALTH_CHECK_PAYLOAD override and allowing the lower-precedence envFrom value to take effect.
• Original discussion: Still present: compatibility injection is based on a point-in-time envFrom ConfigMap/Secret read, but the DCD and DGD reconciliation paths do not watch those referenced objects. Later adding or removing DYN_HEALTH_CHECK_PAYLOAD can leave newly created pods using a stale explicit payload or lacking the needed compatibility default.
• Original discussion: Still present: compatibility payload injection is based on a point-in-time ConfigMap/Secret lookup, but neither the DCD nor DGD reconciliation path watches referenced envFrom objects. Later source changes can leave rendered pod templates with a stale explicit override or without the required default.
• Original discussion: Still present: in the Grove path, component envFrom is materialized before graph-level spec.env is merged by generatePodSpecForComponent. A materialized component DYN_HEALTH_CHECK_PAYLOAD can win the merge and then be removed by Restore, dropping a conflicting graph-level explicit payload and allowing the lower-precedence envFrom source to take effect.
• Original discussion: Still present: the Grove path materializes component envFrom before generatePodSpecForComponent applies graph-level spec.env. The materialized value wins MergeEnvs, then Restore removes that name, dropping a conflicting graph-level explicit DYN_HEALTH_CHECK_PAYLOAD and leaving the lower-precedence envFrom value effective.
• Original discussion: Still present: the compatibility payload decision is persisted from a point-in-time ConfigMap or Secret lookup, but neither the DCD nor DGD controller watches referenced envFrom objects. Later adding or removing DYN_HEALTH_CHECK_PAYLOAD can leave newly created pods with a stale explicit override or without the required default.
• Original discussion: Still present: the compatibility payload decision is persisted from a point-in-time ConfigMap/Secret lookup, while neither rendering path watches referenced envFrom objects. Later source changes can leave new pods with a stale explicit override or without the required default.
• Original discussion: Still present: the compatibility payload decision is persisted from a point-in-time envFrom ConfigMap/Secret lookup, while the rendered workload is not tied to later changes in those referenced objects. Adding or removing DYN_HEALTH_CHECK_PAYLOAD after reconciliation can leave new pods with a stale explicit override or without the needed default.
• Original discussion: Still present: the override rows in TestSGLangBackend_Dynamo15EmbeddingHealthCheckPayload only inspect findEnvVar, which returns the first DYN_HEALTH_CHECK_PAYLOAD entry. A regression that appends a generated payload after the user-provided literal or valueFrom override would still pass; the override cases need to assert there is exactly one payload entry and that it is the configured override.
• Original discussion: Still present: the override tests still inspect only the first matching payload with findEnvVar, so an appended generated payload after a user-provided entry would not be detected.

[tmonty12]

Thanks for the clarification — you're right, I looked at the v1.5.0 init_embedding.py and confirmed it uses SglangHealthCheckPayload (generation-shaped), so the canary does register and does fail. The fix is warranted.

That said, container_env.go and the renderer-level envFrom materialization still seem like overkill. The complexity comes entirely from handling the case where DYN_SGL_EMBEDDING_WORKER is set via a ConfigMap/Secret envFrom rather than as a literal env var or CLI flag. In practice, the vast majority of DGDs set --embedding-worker directly in args or as a literal env entry — both of which are readable at render time without any API calls.

A simpler approach: in UpdateContainer, only inspect container.Env (literal values only, skip valueFrom) and container.Command/container.Args for --embedding-worker. If the flag is set either way, inject DYN_HEALTH_CHECK_PAYLOAD. If it comes from envFrom or valueFrom — drop the Unknown error, just skip the shim silently and let the user set the payload manually. That's maybe 30 lines confined to backend_sglang.go, no new file, no API calls during render.

The envFrom case is an edge case that adds the most complexity and introduces API server lookups in the rendering path. Worth cutting it and documenting that users in that configuration need to set DYN_HEALTH_CHECK_PAYLOAD explicitly.

[dynamo-ops]

/ok to test bb869b2

[xianlubird]

@tmonty12 I simplified the shim to read only literal DYN_SGL_EMBEDDING_WORKER values and CLI flags. The ConfigMap/Secret lookups and render-time envFrom materialization are gone.

I kept one guard for envFrom: if it could supply DYN_HEALTH_CHECK_PAYLOAD, the operator skips injection so an explicit default cannot override the user's payload. Indirect embedding settings also skip the shim, and the compatibility docs now explain when to set the payload explicitly.

[github-actions]

🌿 Fern Docs Preview: https://nvidia-preview-01a0ccd3-c6af-73c8-9ff7-b3c990a096a6.docs.buildwithfern.com/dynamo/dev

[dynamo-ops]

/ok to test d0a446d

[dynamo-review-agent]

Previously reported defects still present:

• Original discussion: Still present: sglangEmbeddingWorkerEnabled ignores DYN_SGL_EMBEDDING_WORKER when it is supplied through valueFrom, so a 1.5.0 pod whose referenced value enables embedding mode still receives no compatibility health-check payload.
• Original discussion: Still present: sglangEnvFromMaySet treats any unprefixed envFrom source as possibly defining DYN_HEALTH_CHECK_PAYLOAD, so an unrelated ConfigMap or Secret suppresses the required 1.5.0 embedding-worker default payload.