Repository navigation
fix(recipes): merge shared DGD environment variables by name - #15064
Conversation
Signed-off-by: yunzhoul-nv <232973175+yunzhoul-nv@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe generator now marks ChangesDynamoGraphDeployment environment merging
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to No actionable merge risk remains from the reviewed change. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation 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. (2 skipped: 2 unsupported.)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
…onent Depends on ai-dynamo#15064. The component previously used JSON 6902 patches to work around spec.env being an atomic list: a strategic merge replaced the whole environment rather than adding to it, dropping MODEL_NAME and HF_HOME. ai-dynamo#15064 gives spec.env a merge key of name in the generated Kustomize OpenAPI document, so the workaround is no longer needed. Switch to a strategic merge. It is more readable, and more importantly it addresses components by NAME rather than by list index -- the JSON patch hardcoded /spec/components/1 and /2, so inserting or reordering a component in the base would silently have moved the RDMA request onto the wrong role. The rendered manifest is unchanged: it still matches the manifest validated on 8x B200, compared with env and components keyed by name. Until ai-dynamo#15064 merges, `render --check` reports this manifest as stale, because the render pipeline regenerates the OpenAPI document from the generator in the tree. That is a loud failure, not a silent one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Cheng Wang <chengwa@nvidia.com>
Signed-off-by: yunzhoul-nv <232973175+yunzhoul-nv@users.noreply.github.com>
github-actions
Bot
added
the
documentation
ynpandey-nv
left a comment
ynpandey-nv
left a comment
There was a problem hiding this comment.
Performance codeowners review.
This is the correct Kustomize-only fix: beta DGD spec.env is an atomic array in the CRD, so SMP was replacing the list. Merge-by-name matches container env and is what recipe overlays need for HF_HOME / UCX_TLS / NCCL_* without dropping MODEL_NAME.
Please add a schema-level unit test on nvidia.com.v1beta1.DynamoGraphDeployment spec.env merge keys (same style as the existing container-env test). The new kustomize build test is useful but skips when kustomize is missing.
Nits: retitle away from fix(docs), fill the PR template, and optionally note that SMP field-merges EnvVar so value + valueFrom can coexist (same as container env; not a reason to drop merge-by-name).
Not blocking on the review-agent valueFrom request.
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Review of fb12e94
Approving. The generated artifact is current and reproducible, the new tests fail on the pre-fix code, and the merge rule does what the docs now say. Two non-blocking items are inline, plus one reply on the review-agent thread.
What I ran, and what it returned
Renderer sigs.k8s.io/kustomize/kustomize/v5@v5.8.1, the same version Recipe Check installs. Merge base 0bc706b, head fb12e94.
Generated artifact:
python3 scripts/generate_kustomize_openapi.py --checkreturns 0 at head.- A full regeneration leaves the working tree clean. Both checked-in copies are byte-identical (
md5 1af5eb50152a0b24bb48e32e4cc7348a), and--checkcovers both paths.
Merge rule, base schema against head schema:
| case | merge base 0bc706b |
head fb12e94 |
|---|---|---|
add UCX_TLS |
the other two entries are dropped | all three kept |
update HF_HOME |
MODEL_NAME is dropped |
MODEL_NAME kept |
| existing list is empty | patch entry only | patch entry only |
no env key in the base |
patch entry only | patch entry only |
patch is env: [] |
list cleared | base entries kept |
$patch: delete on one name |
literal $patch key left in the output |
the named entry is removed |
Test strength: removing the 15-line v1beta1 block from crd_definitions() makes both new parameter cases fail (2 failed, 2 passed). The unmutated tree gives 4 passed, and the file was restored to md5 8f93584f38fec515ab821ce69770fee1 before and after.
Links and paths added by the diff: ../../CONTRIBUTING.md#contribute-a-variant resolves to recipes/CONTRIBUTING.md line 180, ### Contribute a Variant.
On the request for a schema-level unit test: the Kustomize test does not skip in CI
@ynpandey-nv asked for a schema-level test because the new one skips when kustomize is missing. That skip does not happen on this PR. The Recipe Check job installs sigs.k8s.io/kustomize/kustomize/v5@v5.8.1 with go install and exports KUSTOMIZE_BIN, and the job log for fb12e94 reads:
tests/test_generate_kustomize_openapi.py .... [ 10%]
======================== 323 passed in 63.80s (0:01:03) ========================
Four tests, no skips. The ask still helps a developer who has no kustomize on the local machine, but CI coverage is real today.
Push shape and base drift
Two commits, both by the PR author, both with a single parent, no merge commits and no force push.
- The issue timeline holds two
committedevents and nohead_ref_force_pushedevent. compare/0bc706b...fb12e94givesahead_by 2,behind_by 2,total_commits 2, and the same seven files as the pull request file list.- Local ancestry on a clone that reports
is-shallow-repository falsegives the same two commits. git patch-id --stableover the non-merge commits:eeff336is00e58c8d...,fb12e94isbeb8b94a....
Base drift: I merged cedab72, the current tip of main, into fb12e94. The merge is clean and touches 76 files, none of them under recipes/, scripts/generate_kustomize_openapi.py, tests/test_generate_kustomize_openapi.py, or deploy/operator/config/crd/. On the merged tree, --check still returns 0 and the four tests still pass, so no finding here depends on the drift.
What I dropped after measuring it
- Entry order changes. The merge puts patch entries first, so
A,B,Cplus a patch ofC,DrendersC,D,A,B. No recipe YAML file uses$(VAR)inside an env value, so nothing reads that order. - Drift between the two
dynamo-openapi.jsoncopies. They are byte-identical and--checkcovers both. - The comment update that landed in only one
kustomization.yaml. Therecipes/kustomize/components/dynamo-openapi/copy has six lines and never carried that comment, so nothing went out of step. - A gap for
v1alpha1. That version has nospec.envat all, so the hard-coded version tuple covers the only place the rule can apply. - Losing the ability to clear the list with
env: [].$patch: deleteworks, and it works better at head than at the merge base.
Where verification stopped
I did not apply either rendered manifest to a live API server, so "Kubernetes rejects value together with valueFrom" is read from the core EnvVar rule and not measured here. I did not run the operator test suite. No red checks to triage: every check on fb12e94 is green or skipped, and Recipe Check, the job that runs the new tests, passed.
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The code still indexes source_schema["properties"]["spec"]["properties"]["env"] directly, so a v1beta1 DynamoGraphDeployment CRD schema without spec.env would still raise an uncaught KeyError instead of the script's handled error path.
- Original discussion: The documentation still omits that, after a JSON 6902 append creates duplicate shared-env names, a later strategic merge patch for that name can silently leave the appended value unchanged.
- Original discussion: The generated schema still makes beta DGD spec.env a name-keyed strategic-merge list, so same-name patches continue to field-merge EnvVar entries; a value-to-valueFrom patch can retain the old value instead of replacing the EnvVar entry.
- Original discussion: The new CONTRIBUTING.md text still warns only that JSON 6902 appends can create duplicate env names; it does not warn that, after this schema change, a later strategic merge patch by the same name can be dropped when duplicates already exist.
- Original discussion: The new direct indexing of
spec.properties.envstill raises an uncaught KeyError when that field is absent, bypassing the generator's normal handled-error path. - Original discussion: The new documentation still omits that duplicate shared-env names can cause a later strategic merge patch for that name to be silently ignored.
- Original discussion: The new merge-by-name rule still field-merges same-name EnvVar entries. A patch changing an entry from
valuetovalueFromleaves both fields in the rendered DGD, which Kubernetes rejects. - Original discussion: The same-name
spec.envstrategic merge still combinesvalueandvalueFrom, which produces an invalid Kubernetes EnvVar instead of replacing the entry. - Original discussion: Verified at head: after JSON 6902 appends a duplicate shared environment name, a later strategic merge patch for that name can be silently dropped by Kustomize. The changed docs acknowledge duplicates but omit this runtime configuration consequence.
- Original discussion: Verified at head: name-keyed strategic merging of spec.env retains fields from an existing EnvVar. A value-to-valueFrom (or reverse) patch therefore renders both mutually exclusive fields, causing Kubernetes to reject the resulting workload.
- Original discussion: Verified at head: the new direct indexing of the beta DGD spec.env schema raises an uncaught KeyError when that field is absent, bypassing main()'s existing handled input-error path.
- Original discussion: Verified unchanged: after a JSON 6902 append creates duplicate
spec.envnames, a later strategic merge patch can be silently ignored by Kustomize. The new documentation warns about duplicates but not this consequence. - Original discussion: Verified unchanged: keyed strategic merge field-merges an existing EnvVar. A same-name patch changing
valuetovalueFromretainsvalue, rendering an invalid EnvVar with both fields. - Original discussion: Verified unchanged: the new direct
source_schema["properties"]["spec"]["properties"]["env"]lookup raises an uncaught KeyError if the beta DGD schema lacksspec.env, bypassing the generator's handled error path. - Original discussion:
crd_definitions()still indexes the beta DGDspec.envschema directly, so a CRD missing that field raises an uncaughtKeyError.
yunzhoul-nv
changed the title
Signed-off-by: yunzhoul-nv <232973175+yunzhoul-nv@users.noreply.github.com>
|
/ok to test 914fc62 |
…onent The component previously used JSON 6902 patches to work around spec.env being an atomic list: a strategic merge replaced the whole environment rather than adding to it, dropping MODEL_NAME and HF_HOME. ai-dynamo#15064 gives spec.env a merge key of name in the generated Kustomize OpenAPI document, so the workaround is no longer needed. Switch to a strategic merge. It is more readable, and more importantly it addresses components by NAME rather than by list index -- the JSON patch hardcoded /spec/components/1 and /2, so inserting or reordering a component in the base would silently have moved the RDMA request onto the wrong role. The rendered manifest is unchanged: it still matches the manifest validated on 8x B200, compared with env and components keyed by name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Cheng Wang <chengwa@nvidia.com>
…onent The component previously used JSON 6902 patches to work around spec.env being an atomic list: a strategic merge replaced the whole environment rather than adding to it, dropping MODEL_NAME and HF_HOME. ai-dynamo#15064 gives spec.env a merge key of name in the generated Kustomize OpenAPI document, so the workaround is no longer needed. Switch to a strategic merge. It is more readable, and more importantly it addresses components by NAME rather than by list index -- the JSON patch hardcoded /spec/components/1 and /2, so inserting or reordering a component in the base would silently have moved the RDMA request onto the wrong role. The rendered manifest is unchanged: it still matches the manifest validated on 8x B200, compared with env and components keyed by name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Cheng Wang <chengwa@nvidia.com>
Overview:
Details:
Where should the reviewer start?
Related Issues
🔗 This PR is linked to an issue:
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit
New Features
Bug Fixes
Tests