fix(recipe): align AKS training K8s floors with DRA-GA >= 1.34 rationale - #1908
fix(recipe): align AKS training K8s floors with DRA-GA >= 1.34 rationale#1908mohityadav8 wants to merge 2 commits into
Conversation
Bumps the remaining AKS training leaves (aks-training, a100/h100 base, ubuntu, and kubeflow overlays) from >= 1.30 / >= 1.32.4 to >= 1.34, matching the DRA-GA rationale already stated in aks.yaml. The Slurm leaf was already correct (bumped in NVIDIA#1744). Extends TestH100AKSUbuntuTrainingSlurmFloorNotClobbered with sibling leaf cases so the whole AKS training family's resolved floor is pinned, not just the Slurm leaf. Syncs the resolved example recipe. Fixes: NVIDIA#1772
|
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 (9)
📝 WalkthroughWalkthroughAKS training recipe overlays for the general, A100, and H100 variants now require Kubernetes server version Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Recipe evidence checkProtected recipesRecipes with committed evidence (
Other affected recipes without evidence yet: 5These 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
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. |
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Multi-persona review — PR #1908
Reviewed with 4 parallel persona lenses (correctness/recipe-resolution, test-coverage & CI, domain & architecture, operability/release-impact), then every claim independently re-derived by an adversarial senior meta-reviewer against the resolved code at 5481b86. 1 claim refuted outright, 2 sub-claims refuted, 3 re-tiered, 2 new findings surfaced. Everything below was reproduced by running code, not read off a claim.
Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
The change is correct and does exactly what #1772 option 1 asks. The restate-the-floor-on-each-leaf approach — which looks like avoidable duplication — is architecturally right: evaluateOverlayConstraints (pkg/recipe/metadata_store.go:1175) iterates only a candidate leaf's own Spec.Constraints, because filterToMaximalLeaves has already pruned ancestors. Verified by deleting the constraint block from all 7 bumped leaves: ./pkg/recipe and ./pkg/client/v1 both went green, i.e. inheriting instead of restating silently disables snapshot-driven exclusion. Keep the restatements.
Two must-fix one-liners block merge. Both are in files this PR doesn't touch, so they're here rather than inline.
🔴 1 — pkg/client/v1 is broken by this PR; tests / Test and gate are already red
pkg/client/v1/aicr_test.go:1504, :1523, pkg/client/v1/gpu_driver_state_test.go:52, :253
GOFLAGS=-mod=vendor go test ./pkg/client/v1/... fails on this branch and passes on origin/main (verified against a clean baseline worktree). Five subtests:
--- FAIL: TestResolveRecipeFromSnapshot_GPUDriverAutoDetect/aks_+_{preinstalled,k8s-only,absent}
--- FAIL: TestBundleComponents_DriverOwnershipPreflight/{absent,preinstalled}_driver
[INVALID_REQUEST] service 'aks' for criteria(service=aks, accelerator=h100,
intent=training) requires os (valid: ubuntu)
Mechanism: those fixtures pin the snapshot to v1.33.0. With h100-aks-training now restating >= 1.34, the leaf is constraint-excluded during resolution, and verifyCriteriaCoverage then reports the uncovered dimension via completionClause (pkg/recipe/coverage.go:266) rather than the version failure. Matches CI run 30300682638.
Note there are no per-service fixtures — k8sVersionSnapshot() / gpuHardwareSnapshot() are shared helpers used by EKS, GKE and AKS. Bumping all four v1.33.0 literals to v1.34.0 turns the package green (1.34 also satisfies every EKS/GKE floor, all ≤ 1.32.4). The now-wrong explanatory comments at aicr_test.go:806,828,1500-1503 ("clears >= 1.32.4") need updating too.
🔴 2 — Nightly Azure UAT golden not synced → AKS training lane breaks on merge
tests/uat/azure/tests/cuj1-training/assert-recipe.yaml:30 — still value: '>= 1.32.4' for criteria service: aks / accelerator: h100 / intent: training / os: ubuntu / platform: kubeflow, i.e. the exact leaf this PR bumps.
Not inferred — I generated the recipe and ran the consumer command from chainsaw-test.yaml verbatim (chainsaw 0.2.14): exit 1, diff - value: '>= 1.32.4' / + value: '>= 1.34'. uat-azure.yaml is on: workflow_call only, so this is not a PR gate — but infra/uat/reservations.yaml enrolls azure-h100 with nightly-intents: [training, inference], so it breaks deterministically on the next nightly, aborting CUJ1 at step 2 before validate/bundle/evidence ever run. (The UAT cluster itself is provisioned at 1.35 per tests/uat/azure/cluster-config.yaml:31 — the constraint would pass at runtime; this is purely the stale literal.) Sibling cuj2-inference/assert-recipe.yaml:31 is already >= 1.34. One-line fix, please land it in this PR.
🟠 3 — The only new test proves nothing (see inline comment on metadata_store_test.go)
🟠 4 (new) — h100-gke-cos-training-slurm inherits its floor and is therefore ungated
recipes/overlays/h100-gke-cos-training-slurm.yaml:39-42 says the floor is "inherited … so we don't restate it here." Given the leaf-only exclusion mechanic above, that's a latent correctness bug, and it reproduces: resolving gke/h100/cos/training/slurm against a v1.30.0 snapshot succeeds and emits a recipe carrying K8s.server.version: >= 1.32 and nvidia-dra-driver-gpu — a self-contradictory artifact. The control case in the same run (gke/h100/cos/training, whose leaf does restate) is correctly excluded at v1.30.0.
Pre-existing and not caused by this PR — but this PR's mechanism is what surfaces it. Worth a follow-up: either restate the floor there (and audit for other inheriting leaves), or make evaluateOverlayConstraints walk the spec.base chain so inheritance and gating agree.
🟠 5 (new) — This PR turns a clean resolve into a 3-hop misleading error for its own target users
pkg/recipe/coverage.go:266. On main, aks/h100/training (no OS) + a 1.33 snapshot resolves. On this branch it fails with requires os (valid: ubuntu); following that advice yields requires platform (valid: kubeflow, slurm); following that finally yields is provided only by overlays excluded by failing constraints. At no point does it say "your cluster is 1.33 and this recipe needs 1.34" — which, per finding 8, is a hard exit-2 at validate time too.
completionClause already has an onlyExcluded branch; the defect is that intermediate hops report a missing dimension when the real cause is constraint-exclusion of the sole covering overlay. Out of scope to fix here — worth filing.
🟡 6 — Stale rationale comment (see inline comment on a100-aks-training.yaml)
🟡 7 — AKS inference leaves still clobber the same aks.yaml floor
aks-inference.yaml:36 (>= 1.30), h100-aks-inference.yaml:33 and h100-aks-ubuntu-inference.yaml:38 (>= 1.32.4) — all ship nvidia-dra-driver-gpu from base.yaml plus the dra-support conformance check from aks.yaml, against docs/integrator/aks-gpu-setup.md:5's unqualified "AICR requires Kubernetes 1.34 or later on AKS". h100-aks-ubuntu-inference-dynamo.yaml is already >= 1.34, so the inference family is internally split too.
Entirely pre-existing and #1772 is explicitly training-scoped, so not a blocker. But #1772's option 2 ("reconcile the aks.yaml rationale so it no longer claims a family-wide DRA-GA floor") is left unaddressed by either option — after this merges, AKS training is aligned to the docs and AKS inference silently is not. Please file the follow-up and reference it in the PR body.
🟡 8 — Breaking change not classified; PR template not used; missing theme/* label
The floor is a hard gate, verified end to end: checkReadiness is called at pkg/validator/validator.go:209, before the if v.NoCluster short-circuit at :219; failure returns ErrCodeInvalidRequest (:68), which exitCodeFromErrorCode (pkg/errors/exitcode.go:82) maps to exit 2. --fail-on-error=false does not suppress it — the repo's own help text at pkg/cli/validate.go:607 says it "scopes to phase checks". So an AKS 1.30–1.33 training operator goes from passing to exit-2 with no Jobs deployed and no CTRF report.
The PR body is three prose paragraphs with none of .github/PULL_REQUEST_TEMPLATE.md's sections and no [x] Breaking change. Suggest refilling the canonical template, ticking Breaking change, and adding a Risk Assessment rollout note pointing at the documented no-skip upgrade path (docs/integrator/aks-gpu-setup.md:23-40 — AKS forbids version skipping, so 1.30 users need 1.30→…→1.33→1.34). Also missing the required theme/recipes label.
Confirmed non-issues (examined and cleared)
Tier 1 / oke-ol (argocd-git)+KWOK Test Summaryfailures are not this PR.mainhas been red 07-25 → 07-27 with a different random cell each run (a100-eks-training (argocd-oci),bcm-inference,gke-cos-training,h100-aks-inference…). oke-ol is Oracle Linux, untouched by an AKS-only diff. Pre-existing KWOK flake; deserves its own infra issue.examples/recipes/aks-training.yamlsync is correct. Regenerated viaaicr recipe --service aks --accelerator h100 --intent training --os ubuntuand compared:criteria,metadata.appliedOverlays, and the bumped constraint all match live output. (Nothing gates this file — no test or workflow referencesexamples/recipes, andMakefile:11excludes./examples/*from yamllint — butexamples/recipes/README.mddocuments examples as regenerate-on-demand, so that is by design, not drift.)- No docs update is owed.
docs/integrator/aks-gpu-setup.md:5already states the 1.34 requirement anddocs/integrator/index.md:25advertises "Kubernetes 1.34+ (DRA GA)" — this PR brings the recipes into alignment with already-published docs. Every other1.32.4/1.30hit underdocs/is a generic or EKS-based syntax example. - #1772's "run
make bom-docs/ regenerate the recipe-health matrix" is a no-op here. Verified both outputs are floor-agnostic:tools/health/markdown.go:275gradesresolves/chart_pinned/constraints_wellformedand never reads a constraint value;docs/user/recipe-health.mdhas no K8s column;docs/user/container-images.mdis chart/image-only. - The A100 1.30 → 1.34 jump is technically justified.
base.yaml:88addsnvidia-dra-driver-gpuunconditionally,aks.yamloverrides it with no accelerator gating, anda100-aks-training.yaml:95explicitly declares thedra-supportcheck. There is no A100 exclusion path anywhere, so the DRA-GA rationale applies identically. - Test-case coverage is exactly 1:1. Each of the 7 bumped overlays has a matching new case; no bumped leaf lacks one, no case targets an unbumped leaf.
- Adding
remediation:text to the overlays would be inert.Constraint.Remediationis rendered only byaicr query(pkg/recipe/query.go:89);grep -rn Remediation pkg/validator/returns nothing, and norecipes/overlays/*.yamlsets it — including every sibling already at>= 1.34. Plumbing it intocheckReadinessis the real fix, separately. - Restate-vs-inherit — the "7 more places to drift" objection is real but is the lesser risk; see the overall assessment.
Tally: 🔴 2 · 🟠 3 · 🟡 3 · 🔵 1
Blocking work is two one-line edits (findings 1 and 2) plus the test godoc / discriminating-power fix. The underlying change is correct and well-motivated.
| }, | ||
| wantK8sFloor: ">= 1.34", | ||
| }, | ||
| {name: "h100 aks training preserves >= 1.34 floor", criteria: &Criteria{Service: CriteriaServiceAKS, Accelerator: CriteriaAcceleratorH100, Intent: CriteriaIntentTraining}, wantK8sFloor: ">= 1.34"}, |
There was a problem hiding this comment.
🟠 Major — The only new test proves nothing, and its godoc describes a state this PR deleted
The godoc above (:2672-2679) still says the leaf "resolves under last-writer-wins merge with lower-valued ancestors (aks-training >= 1.30, h100-aks-ubuntu-training >= 1.32.4)" — both values are deleted by this PR.
Worse, the guard no longer guards. I replaced the constraint block with constraints: [] in all seven bumped overlays and ran go test ./pkg/recipe/ -run TestH100AKSUbuntuTrainingSlurmFloorNotClobbered — it still passed (ok), and so did the whole package. With every overlay in the chain at 1.34, all 8 cases pass even if every leaf restatement is removed. The test has degraded from an anti-clobber guard into a snapshot of aks.yaml's ancestor value.
Blast radius: False sense of coverage. This is the PR's only added test, and as written it cannot detect the regression class it names.
Fix: Rewrite the godoc to describe the post-bump chain, and restore discriminating power by asserting the leaf overlays themselves declare >= 1.34 (e.g. read store.GetRecipeByName(...).Spec.Constraints) — that is the property that actually drives snapshot-time exclusion.
| {name: "a100 aks training preserves >= 1.34 floor", criteria: &Criteria{Service: CriteriaServiceAKS, Accelerator: CriteriaAcceleratorA100, Intent: CriteriaIntentTraining}, wantK8sFloor: ">= 1.34"}, | ||
| {name: "a100 aks ubuntu training preserves >= 1.34 floor", criteria: &Criteria{Service: CriteriaServiceAKS, Accelerator: CriteriaAcceleratorA100, Intent: CriteriaIntentTraining, OS: CriteriaOSUbuntu}, wantK8sFloor: ">= 1.34"}, | ||
| {name: "a100 aks ubuntu kubeflow training preserves >= 1.34 floor", criteria: &Criteria{Service: CriteriaServiceAKS, Accelerator: CriteriaAcceleratorA100, Intent: CriteriaIntentTraining, OS: CriteriaOSUbuntu, Platform: CriteriaPlatformKubeflow}, wantK8sFloor: ">= 1.34"}, | ||
| {name: "aks training (accelerator-generic) preserves >= 1.34 floor", criteria: &Criteria{Service: CriteriaServiceAKS, Intent: CriteriaIntentTraining}, wantK8sFloor: ">= 1.34"}, |
There was a problem hiding this comment.
🔵 Nitpick — New table cases break the file's multi-line struct style
The 7 added cases are single-line struct literals of 228–273 chars, while the pre-existing case immediately above (and every case in the sibling TestGB200OKEFloorNotClobbered) is multi-line. Confirmed lll is not in the enable: list in .golangci.yaml and golangci-lint run -c .golangci.yaml ./pkg/recipe/... reports 0 issues — so this is style only, not a lint failure.
Fix: Reformat to the multi-line form used by the neighbouring case.
| constraints: | ||
| - name: K8s.server.version | ||
| value: ">= 1.30" | ||
| value: ">= 1.34" |
There was a problem hiding this comment.
🟡 Minor — Comment now argues against the value directly beneath it
The surviving comment at :29-31 reads "A100 has no IMEX/NVLink ComputeDomain requirement, so the recipe keeps the AKS training baseline rather than the H100 1.32.4 floor." After this PR the H100 AKS floor is also 1.34, so the contrast no longer exists — and it cites a value this PR just deleted from every AKS training overlay. The actual rationale (DRA GA in K8s 1.34) appears nowhere in the file.
Same issue with the surviving # {A100,H100} + AKS specific constraints (not covered by mixin) headers on the -ubuntu / -kubeflow leaves: the value is no longer accelerator-specific — it is identical to the service parent's.
Blast radius: Doc-comment only, but this is the exact failure mode that produced #1772 — a floor whose stated reason drifted from its parent.
Fix: Replace with the family-standard # DRA requires Kubernetes 1.34+ (GA) used by aks.yaml, gb200-eks-training.yaml, h100-kind-training.yaml et al.
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
One issue: this floor change breaks a UAT fixture that pins the old value. Details inline.
| constraints: | ||
| - name: K8s.server.version | ||
| value: ">= 1.32.4" | ||
| value: ">= 1.34" |
There was a problem hiding this comment.
Raising this floor breaks the Azure UAT CUJ1 fixture, which still pins the old value.
tests/uat/azure/tests/cuj1-training/assert-recipe.yaml:30 asserts value: '>= 1.32.4' under criteria service: aks, accelerator: h100, intent: training, os: ubuntu, platform: kubeflow — exactly the recipe this overlay produces. tests/uat/azure/tests/cuj1-training/chainsaw-test.yaml generates it with aicr recipe --service aks --accelerator h100 --intent training --os ubuntu --platform kubeflow and then runs chainsaw assert --resource recipe.yaml --file ./assert-recipe.yaml, so the generated recipe now emits >= 1.34 and no longer matches.
The sibling AWS fixture at tests/uat/aws/tests/cuj1-training/assert-recipe.yaml:30 confirms these assertions track the leaf's exact constraint value (the EKS leaf is still 1.32.4 and its fixture still says so), and the already-1.34 inference leaves have matching assertions at tests/uat/azure/tests/cuj2-inference/assert-recipe.yaml:31.
Impact is the nightly Azure UAT training lane: it fails at the assert-recipe step on a real provisioned AKS H100 cluster and skips the downstream validate/bundle steps — a red batch and a wasted GPU provision for a purely stale expectation. Updating that fixture to >= 1.34 in this PR would keep it consistent with the other leaves.
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
A fresh review of this PR turned up more than the fixture mismatch already noted. Inline comments below; one item could not be anchored inline.
The AKS inference family is left behind, so the clobber this PR fixes for training is still live for inference. recipes/overlays/aks.yaml:34 declares >= 1.34 on the rationale that DRA is GA only in 1.34 and every AKS recipe pulls nvidia-dra-driver-gpu from base, and descendants override it downward under last-writer-wins. After this PR the training chain is uniformly 1.34, but recipes/overlays/aks-inference.yaml:36 is still >= 1.30, and h100-aks-inference.yaml:33 and h100-aks-ubuntu-inference.yaml:38 are still >= 1.32.4.
So aicr recipe --service aks --accelerator h100 --intent inference still advertises a floor below the DRA-GA requirement: a 1.32/1.33 AKS cluster passes recipe-time constraint evaluation and then deploys nvidia-dra-driver-gpu against a non-GA resource.k8s.io API. docs/integrator/aks-gpu-setup.md:5 states AICR requires Kubernetes 1.34 or later on AKS.
It also splits the family: the dynamo leaf h100-aks-ubuntu-inference-dynamo.yaml:35 is already 1.34, which is why the CUJ2 inference fixture needs no change, while its non-dynamo siblings are not. Whether that belongs in this PR is your call, but it is the same defect shape left half-fixed.
| constraints: | ||
| - name: K8s.server.version | ||
| value: ">= 1.32.4" | ||
| value: ">= 1.34" |
There was a problem hiding this comment.
This floor raise breaks two existing Go tests, because their snapshot fixture pins v1.33.0.
ResolveRecipeFromSnapshot builds a constraints.Evaluate-backed evaluator, and pkg/recipe/metadata_store.go:1075-1094 excludes every overlay whose own constraints fail. With this leaf and its whole ancestry now at >= 1.34 (aks-training.yaml:32 raised 1.30 to 1.34, aks.yaml:34 already 1.34), a v1.33.0 snapshot leaves no service=aks overlay that can resolve (aks, h100, training).
Two consumers fail as a result:
TestResolveRecipeFromSnapshot_GPUDriverAutoDetect— the three AKS subtests atpkg/client/v1/aicr_test.go:929,:936and:970each hitt.FatalfonResolveRecipeFromSnapshot.TestBundleComponents_DriverOwnershipPreflight—newAKSRecipe(aicr_test.go:1901-1921) resolves the same criteria and fatals at:1919. Both subtests feedgpuHardwareSnapshot, which pinsconst version = "v1.33.0"ataicr_test.go:152, so the driver-ownership bundle preflight is not exercised at all.
Both need the fixture bumped to a >= 1.34 server version in this PR. This was established by reading the exclusion and resolve path rather than by running the suite.
There was a problem hiding this comment.
Confirmed by CI rather than inference — the run on this head is already red for exactly these two tests.
From tests / Test (run 30300682638, job 90092662045):
--- FAIL: TestBundleComponents_DriverOwnershipPreflight (0.00s)
--- FAIL: .../preinstalled_driver_→_BundleComponents_passes (0.29s)
aicr_test.go:1950: ResolveRecipeFromSnapshot: [INVALID_REQUEST] service 'aks' for
criteria(service=aks, accelerator=h100, intent=training) requires os (valid: ubuntu)
--- FAIL: .../absent_driver_→_BundleComponents_blocked (0.33s)
aicr_test.go:1926: ...
--- FAIL: TestResolveRecipeFromSnapshot_GPUDriverAutoDetect (0.00s)
--- FAIL: .../aks_+_absent_snapshot_records_state_for_the_bundle-time_gate (0.25s)
--- FAIL: .../aks_+_k8s-only_snapshot_is_Unknown_→_no_override (0.24s)
--- FAIL: .../aks_+_preinstalled_snapshot_injects_(profile_gate_satisfied) (0.44s)
aicr_test.go:1009: ...same INVALID_REQUEST
The message surfaces as "requires os" rather than a version mismatch because once the os-less AKS training overlays are excluded by the failed >= 1.34 constraint, only the ubuntu leaves remain, so the resolver reports a missing os instead of a floor failure. Same root cause.
The fix looks like one line: gpuHardwareSnapshot pins const version = "v1.33.0" at pkg/client/v1/aicr_test.go:152. Raising that to a >= 1.34 version should clear all five subtests.
| @@ -32,7 +32,7 @@ spec: | |||
| # Constraint names use fully qualified measurement paths: {type}.{subtype}.{key} | |||
There was a problem hiding this comment.
The rationale comment immediately above this constraint is now false. Lines 30-31 read "A100 has no IMEX/NVLink ComputeDomain requirement, so the recipe keeps the AKS training baseline rather than the H100 1.32.4 floor" — but this leaf is now >= 1.34 and recipes/overlays/h100-aks-training.yaml:33 is also >= 1.34, so there is no longer a distinct H100 floor to contrast against.
The risk is specific: a future editor reading that rationale could re-lower the A100 floor to restore the baseline, silently regressing the DRA-GA gate this PR establishes.
| @@ -2700,6 +2700,13 @@ func TestH100AKSUbuntuTrainingSlurmFloorNotClobbered(t *testing.T) { | |||
| }, | |||
There was a problem hiding this comment.
The doc comment on TestH100AKSUbuntuTrainingSlurmFloorNotClobbered (lines 2672-2678) still describes the ancestors as lower-valued — "last-writer-wins merge with lower-valued ancestors (aks-training >= 1.30, h100-aks-ubuntu-training >= 1.32.4)". This PR sets recipes/overlays/aks-training.yaml:32 and recipes/overlays/h100-aks-ubuntu-training.yaml:38 to >= 1.34, and the seven cases added just below assert 1.34 for exactly those ancestors, so the clobber scenario the comment describes no longer exists.
That comment is the only in-repo record of why this guard exists. Leaving the pre-PR floors in it makes the cases beneath look tautological and invites someone removing them.
3 participants
Bumps the remaining AKS training leaves (aks-training, a100/h100 base, ubuntu, and kubeflow overlays) from >= 1.30 / >= 1.32.4 to >= 1.34, matching the DRA-GA rationale already stated in aks.yaml. The Slurm leaf was already correct (bumped in #1744).
Extends TestH100AKSUbuntuTrainingSlurmFloorNotClobbered with sibling leaf cases so the whole AKS training family's resolved floor is pinned, not just the Slurm leaf. Syncs the resolved example recipe.
Fixes: #1772