Skip to content

fix(recipe): align AKS training K8s floors with DRA-GA >= 1.34 rationale #1908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mohityadav8 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(recipe): align AKS training K8s floors with DRA-GA >= 1.34 rationale #1908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79e74aa
fix(recipe): align AKS training K8s floors with DRA-GA >= 1.34 rationale
mohityadav8 Jul 24, 2026
5481b86
Merge branch 'main' into fix/issue-1772-aks-training-k8s-floor
njhensley Jul 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/recipes/aks-training.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ criteria:
platform: any
constraints:
- name: K8s.server.version
value: '>= 1.32.4'
value: '>= 1.34'
- name: OS.release.ID
value: ubuntu
- name: OS.release.VERSION_ID
Expand Down
7 changes: 7 additions & 0 deletions pkg/recipe/metadata_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2700,6 +2700,13 @@ func TestH100AKSUbuntuTrainingSlurmFloorNotClobbered(t *testing.T) {
},

@yuanchen8911 yuanchen8911 Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

wantK8sFloor: ">= 1.34",
},
{name: "h100 aks training preserves >= 1.34 floor", criteria: &Criteria{Service: CriteriaServiceAKS, Accelerator: CriteriaAcceleratorH100, Intent: CriteriaIntentTraining}, wantK8sFloor: ">= 1.34"},

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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: "h100 aks ubuntu training preserves >= 1.34 floor", criteria: &Criteria{Service: CriteriaServiceAKS, Accelerator: CriteriaAcceleratorH100, Intent: CriteriaIntentTraining, OS: CriteriaOSUbuntu}, wantK8sFloor: ">= 1.34"},
{name: "h100 aks ubuntu kubeflow training preserves >= 1.34 floor", criteria: &Criteria{Service: CriteriaServiceAKS, Accelerator: CriteriaAcceleratorH100, Intent: CriteriaIntentTraining, OS: CriteriaOSUbuntu, Platform: CriteriaPlatformKubeflow}, wantK8sFloor: ">= 1.34"},
{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"},

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

}

for _, tt := range tests {
Expand Down
2 changes: 1 addition & 1 deletion recipes/overlays/a100-aks-training.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ spec:
# Constraint names use fully qualified measurement paths: {type}.{subtype}.{key}

@yuanchen8911 yuanchen8911 Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

constraints:
- name: K8s.server.version
value: ">= 1.30"
value: ">= 1.34"

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.


componentRefs:
# GPU Operator dependency ordering (inherits valuesFile from aks-training;
Expand Down
2 changes: 1 addition & 1 deletion recipes/overlays/a100-aks-ubuntu-training-kubeflow.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,6 @@ spec:
# A100 + AKS specific constraints (not covered by mixin)
constraints:
- name: K8s.server.version
value: ">= 1.30"
value: ">= 1.34"

componentRefs: []
2 changes: 1 addition & 1 deletion recipes/overlays/a100-aks-ubuntu-training.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ spec:
# A100 + AKS specific constraints (not covered by mixin)
constraints:
- name: K8s.server.version
value: ">= 1.30"
value: ">= 1.34"

componentRefs: []

Expand Down
2 changes: 1 addition & 1 deletion recipes/overlays/aks-training.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ spec:
# Constraint names use fully qualified measurement paths: {type}.{subtype}.{key}
constraints:
- name: K8s.server.version
value: ">= 1.30"
value: ">= 1.34"

componentRefs:
# Training workloads use the training-optimized GPU Operator values
Expand Down
2 changes: 1 addition & 1 deletion recipes/overlays/h100-aks-training.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ spec:
# Constraint names use fully qualified measurement paths: {type}.{subtype}.{key}
constraints:
- name: K8s.server.version
value: ">= 1.32.4"
value: ">= 1.34"

@yuanchen8911 yuanchen8911 Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at pkg/client/v1/aicr_test.go:929, :936 and :970 each hit t.Fatalf on ResolveRecipeFromSnapshot.
  • TestBundleComponents_DriverOwnershipPreflight — newAKSRecipe (aicr_test.go:1901-1921) resolves the same criteria and fatals at :1919. Both subtests feed gpuHardwareSnapshot, which pins const version = "v1.33.0" at aicr_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.

@yuanchen8911 yuanchen8911 Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


componentRefs:
# GPU Operator dependency ordering (inherits valuesFile from aks-training;
Expand Down
2 changes: 1 addition & 1 deletion recipes/overlays/h100-aks-ubuntu-training-kubeflow.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,6 @@ spec:
# H100 + AKS specific constraints (not covered by mixin)
constraints:
- name: K8s.server.version
value: ">= 1.32.4"
value: ">= 1.34"

@yuanchen8911 yuanchen8911 Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


componentRefs: []
2 changes: 1 addition & 1 deletion recipes/overlays/h100-aks-ubuntu-training.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ spec:
# H100 + AKS specific constraints (not covered by mixin)
constraints:
- name: K8s.server.version
value: ">= 1.32.4"
value: ">= 1.34"

componentRefs: []

Expand Down
Loading