fix(recipes): address VR200/RKE2 review follow-ups from #2520 - #2589
Merged
Conversation
Three items from the NVIDIA#2520 review, each bounded to what it corrects. The vendored TLSRoute CRD header still credited agentgateway v2.2.1 as the reason it exists. Those were the last v2.2.1 strings in the tree after NVIDIA#2520 dropped the pins, and the risk is not cosmetic: a maintainer trusting that provenance could revert the registry toward the v2.2.x line, which still carries the unbounded CEL rules a Kubernetes 1.37 apiserver rejects. Reworded version-agnostic; the Gateway API v1.2.1 reference on line 1 is the CRD's own upstream source and stays. TestNVLSRuntimeYAMLReferencesIMEXClaim listed its templates explicitly and so never covered vr200/rke2 when that runtime landed. A mistyped claim name would pass every unit test and surface only as an opaque pod-admission failure on live hardware. Derive the set by glob instead, with a zero-match guard so the test cannot pass while checking nothing. Both VR200 leaves now record that node targeting differs between the rebooting CRs: --accelerated-node-selector reaches the tuning CR, while dranet and rdma-netns-exclusive hardcode nvidia.com/gpu.present. Identical on NFD-labeled reference clusters; only diverges when an operator narrows the GPU node set. Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
yuanchen8911
added
the
theme/recipes
Contributor
Recipe evidence checkProtected recipesRecipes with committed evidence (
Other affected recipes without evidence yet: 1These recipes are affected by this PR but carry no committed evidence pointer, so there is
How to refresh evidenceRun on a cluster matching the recipe's aicr snapshot -o snapshot.yaml
# Profiled families (AKS/GKE gpuStack): hydrate the recipe with the
# pointer's recorded 'profile:' selection first — validating the raw
# overlay resolves only the declaration default, and 'aicr validate'
# has no --profile flag. AKS additionally needs the pool projection
# (GKE uses the plain snapshot above):
# az aks nodepool list -g <rg> --cluster-name <cluster> -o json > pools.json
# aicr snapshot --aks-gpu-pools pools.json -o snapshot.yaml
# aicr recipe -s snapshot.yaml --intent <intent> [--platform <platform>] \
# --profile <name>=<value> -o recipe.yaml
# State the target leaf's intent/platform explicitly (the snapshot
# fingerprint supplies service/accelerator/OS but intent and platform
# default to 'any') and pass -r recipe.yaml below instead of the raw
# overlay.
aicr validate \
-r recipes/overlays/<slug>.yaml \
-s snapshot.yaml \
--emit-attestation ./out \
--push ghcr.io/<your-fork>/aicr-evidence
# Copy to the per-source path printed in the emit 'copyTo' hint:
# recipes/evidence/<slug>/<source>/<bundle-digest>.yamlThis gate is warning-only and never blocks merge. See ADR-007 for the trust model. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change updates the inference bundle golden digest, revises TLSRoute CRD comments, and documents node-targeting behavior in inference and training overlays. The NVLS validation test now discovers and validates all matching runtime templates instead of two fixed paths. Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This updates deployment documentation and golden metadata while expanding NVLS template validation coverage; no current merge-blocking production or runtime risk is identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yuanchen8911
enabled auto-merge (squash)
September 4, 2026 20:48
njhensley
approved these changes
Sep 4, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Follow-up to #2520. Addresses the three bounded review findings from that PR's approval; the fourth is deliberately left to an issue (see below).
Motivation / Context
#2520 merged with a multi-persona review reporting 0 blockers and 0 majors, plus two 🟡 Minors and a 🔵 Nitpick explicitly called "safe-to-defer polish." This PR clears them while they are still fresh, rather than leaving them to rot in a merged PR's thread.
Follow-up to: #2520
Related: #2572, #2587
Type of Change
Component(s) Affected
pkg/recipe)pkg/validator,validators/performance)Implementation Notes
Stale v2.2.1 provenance (
tlsroute-crd.yaml). The header still credited "agentgateway v2.2.1" as the reason the CRD is vendored. Those were the lastv2.2.1strings in the tree after #2520 dropped the pins, and the risk is not cosmetic — a maintainer trusting that provenance could revert the registry toward the v2.2.x line, which still declares 39 unboundedrule: matches(self, ...)CEL rules that a Kubernetes 1.37 apiserver rejects. That is precisely the breakage #2587 fixed and #2520's pin-drop preserved. The line-1 "Gateway API v1.2.1 experimental channel" reference is the CRD's own upstream source and is left alone.Drift guard did not cover vr200/rke2 (
validators/performance/nccl_test.go).TestNVLSRuntimeYAMLReferencesIMEXClaimlistedgb200/eksandgb200/okeexplicitly, so thevr200/rke2runtime added in #2520 was never checked. That guard exists because a name mismatch between the template andncclIMEXClaimTemplateNameproduces an opaque "claim not found" pod-admission failure rather than a test failure — so a mistyped name would pass every unit test and surface only on live VR hardware.Rather than appending one path, the set is now derived by glob, so the next accelerator or service is covered the moment its template lands. A zero-match guard fails the test if the glob matches nothing, so it cannot pass while checking nothing. The guard now runs three subtests where it ran two.
Node targeting differs between the rebooting CRs (both VR200 leaves).
nodewright-customizationsdeclaresnodeScheduling.accelerated, so--accelerated-node-selectorflows into the tuning Skyhook CR;dranetandrdma-netns-exclusivedeclare no scheduling block and hardcodenodeSelector: nvidia.com/gpu.present, so those flags bypass them. On the NFD-labeled reference clusters all three selectors agree and nothing changes — it only diverges when an operator narrows the GPU node set, at which point the tuning reboot retargets to the subset while the RDMA reboot and DraNet still fan out to everygpu.presentnode. Documented in the caveat that already discusses the reboots, rather than templating the selectors, which would be a behavior change beyond this PR's scope.Deliberately not addressed here
The review's remaining nitpick — that tuning evidence is
status: complete-only, so the three documented-inertnvidia-tunedsettings ride as no-ops while evidence records "tuning applied" — is not in this PR. It touchesrecipes/checks/nodewright-customizations/health-check.yaml, which neither #2520 nor this PR modifies, and the fix is an AICR-side effective-state assertion rather than a comment. The reviewer framed it as a tracking-issue item gating GA, and it belongs with the other Preview-to-GA conditions in #2572 rather than expanding this diff.The two-Skyhook concurrent-reboot hazard is likewise out of scope and remains tracked in #2572.
Testing
go test ./validators/performance/ ./pkg/recipe/... ./pkg/bundler/ golangci-lint run -c .golangci.yaml ./validators/performance/... make update-goldens yamllint recipes/overlays/All pass;
golangci-lintreports 0 issues.TestNVLSRuntimeYAMLReferencesIMEXClaimnow coversgb200/eks,gb200/okeandvr200/rke2— three subtests where it previously ran two.The render golden moved for
vr200-rke2-ubuntu-inference-dynamoonly, and the cause is the CRD header rather than the overlays:tlsroute-crd.yamlis copied verbatim into the bundle throughrke2-inference.yaml'smanifestFiles, so its header comment is part of the rendered bytes. The overlay comment edits do not move any golden — the YAML loader parses those away. That also accounts for the asymmetry:vr200-rke2-ubuntu-inference-dynamois the only leaf in the chain that pullstlsroute-crd.yaml, so a one-line delta is both correct and complete.TestCatalogParityGolden, the BOM freshness tests, and the registry inventory are unchanged, which is expected — resolution records which values file a component points at rather than its content, and the BOM renders chart templates rather than vendored manifests.Risk Assessment
Two comment-only edits and one test-coverage widening. No recipe behavior, pin, or generated artifact semantics change.
Rollout notes: None.
Checklist
git commit -S)