feat(recipe): auto-detect pre-installed GPU driver from snapshot#1697
feat(recipe): auto-detect pre-installed GPU driver from snapshot#1697xdu31 wants to merge 1 commit into
Conversation
xdu31
added
theme/validation
|
🌿 Preview your docs: https://nvidia-preview-feat-gpu-driver-auto-detect.docs.buildwithfern.com/aicr |
xdu31
force-pushed
the
feat/gpu-driver-auto-detect
branch
from
b36e7d2 to
6c420cd
Compare
📝 WalkthroughWalkthroughSnapshot-based recipe resolution now detects preinstalled GPU drivers from GPU hardware measurements and injects Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/integrator/recipe-development.md`:
- Line 279: Update the “Snapshot-driven override — gpu-operator.driver.enabled”
documentation to state that the injected override takes precedence over base and
provider values files but not CLI --set flags. Clarify that CLI --set values are
applied after Overrides and can supersede snapshot-based GPU driver
auto-detection.
In `@pkg/client/v1/aicr_test.go`:
- Around line 875-1013: Consolidate the four repeated auto-detect tests into a
single table-driven TestResolveRecipeFromSnapshot_GPUDriverAutoDetect test with
cases for AKS preinstalled, AKS absent, EKS preinstalled, and missing GPU
measurement. Parameterize service, snapshot, expected override presence, and
expected enabled value, then run each case with t.Run while retaining the shared
client, criteria construction, resolution, and assertions.
In `@pkg/client/v1/gpu_driver_state.go`:
- Around line 142-146: Replace the “byte-for-byte idempotent” claim in the
comment near the driver injection logic and in component-catalog.md with
“semantically idempotent,” clarifying that rendered Helm values remain unchanged
while serialized resolved recipes may gain driver.enabled overrides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7c9163cf-4224-400a-986f-08a65e03292a
📒 Files selected for processing (7)
docs/integrator/aks-gpu-setup.mddocs/integrator/recipe-development.mddocs/user/component-catalog.mdpkg/client/v1/aicr.gopkg/client/v1/aicr_test.gopkg/client/v1/gpu_driver_state.gopkg/client/v1/gpu_driver_state_test.go
| # Result: driver.version="580.13.01", driver.repository="nvcr.io/nvidia" (preserved) | ||
| ``` | ||
|
|
||
| **Snapshot-driven override — `gpu-operator.driver.enabled`.** When a recipe is resolved from a snapshot (via `aicr recipe --snapshot` or `ResolveRecipeFromSnapshot`), AICR reads the sampled GPU node's `driver-loaded` measurement and injects `gpu-operator.overrides.driver.enabled=false` when the NVIDIA kernel module is already loaded on the node — landing at the top of the merge chain so it wins over any provider values file. Policy is only-false (never forces `true`), so recipes resolved without a snapshot fall back to today's static defaults. See [Component Catalog › GPU Operator Driver Auto-Detect](../user/component-catalog.md#gpu-operator-driver-auto-detect). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that CLI --set still has higher precedence.
The override is above base/provider values files, but not at the top of the documented chain: CLI --set flags follow Overrides and can still supersede auto-detection.
Proposed wording
- ...landing at the top of the merge chain so it wins over any provider values file.
+ ...as an Overrides entry, so it wins over base and provider values files.
+ Explicit CLI --set flags retain higher precedence.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Snapshot-driven override — `gpu-operator.driver.enabled`.** When a recipe is resolved from a snapshot (via `aicr recipe --snapshot` or `ResolveRecipeFromSnapshot`), AICR reads the sampled GPU node's `driver-loaded` measurement and injects `gpu-operator.overrides.driver.enabled=false` when the NVIDIA kernel module is already loaded on the node — landing at the top of the merge chain so it wins over any provider values file. Policy is only-false (never forces `true`), so recipes resolved without a snapshot fall back to today's static defaults. See [Component Catalog › GPU Operator Driver Auto-Detect](../user/component-catalog.md#gpu-operator-driver-auto-detect). | |
| **Snapshot-driven override — `gpu-operator.driver.enabled`.** When a recipe is resolved from a snapshot (via `aicr recipe --snapshot` or `ResolveRecipeFromSnapshot`), AICR reads the sampled GPU node's `driver-loaded` measurement and injects `gpu-operator.overrides.driver.enabled=false` when the NVIDIA kernel module is already loaded on the node — as an Overrides entry, so it wins over base and provider values files. Explicit CLI --set flags retain higher precedence. Policy is only-false (never forces `true`), so recipes resolved without a snapshot fall back to today's static defaults. See [Component Catalog › GPU Operator Driver Auto-Detect](../user/component-catalog.md#gpu-operator-driver-auto-detect). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/integrator/recipe-development.md` at line 279, Update the
“Snapshot-driven override — gpu-operator.driver.enabled” documentation to state
that the injected override takes precedence over base and provider values files
but not CLI --set flags. Clarify that CLI --set values are applied after
Overrides and can supersede snapshot-based GPU driver auto-detection.
| // TestResolveRecipeFromSnapshot_GPUDriverAutoDetect_AKSPreinstalled asserts | ||
| // the AKS win path: when the sampled GPU node reports driver-loaded=true, | ||
| // the resolved h100-aks-training recipe carries the injected | ||
| // gpu-operator.overrides.driver.enabled=false so the Operator does not | ||
| // install a second driver on top of the one Azure's --gpu-driver Install | ||
| // nodepool has already provisioned. | ||
| func TestResolveRecipeFromSnapshot_GPUDriverAutoDetect_AKSPreinstalled(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| c, err := aicr.NewClient(aicr.WithRecipeSource(aicr.EmbeddedSource())) | ||
| if err != nil { | ||
| t.Fatalf("NewClient: %v", err) | ||
| } | ||
| defer c.Close() | ||
|
|
||
| crit, err := recipe.BuildCriteriaWithRegistry(nil, | ||
| recipe.WithServiceRegistry("aks"), | ||
| recipe.WithAcceleratorRegistry("h100"), | ||
| recipe.WithIntentRegistry("training"), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("BuildCriteria: %v", err) | ||
| } | ||
|
|
||
| rec, err := c.ResolveRecipeFromSnapshot(t.Context(), aicr.WrapCriteria(crit), gpuHardwareSnapshot(true)) | ||
| if err != nil { | ||
| t.Fatalf("ResolveRecipeFromSnapshot: %v", err) | ||
| } | ||
|
|
||
| enabled, present := gpuOperatorDriverEnabled(t, rec) | ||
| if !present { | ||
| t.Fatal("driver.enabled override missing — expected the auto-detect injection") | ||
| } | ||
| if enabled { | ||
| t.Fatalf("driver.enabled = true, want false (Operator must NOT install driver when platform preinstalled it)") | ||
| } | ||
| } | ||
|
|
||
| // TestResolveRecipeFromSnapshot_GPUDriverAutoDetect_AKSAbsent asserts the | ||
| // symmetric no-op: when the sampled node reports driver-loaded=false, no | ||
| // driver override is injected, so the AKS values file's default (chart | ||
| // default: true — the Operator DOES install the driver) stands. | ||
| func TestResolveRecipeFromSnapshot_GPUDriverAutoDetect_AKSAbsent(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| c, err := aicr.NewClient(aicr.WithRecipeSource(aicr.EmbeddedSource())) | ||
| if err != nil { | ||
| t.Fatalf("NewClient: %v", err) | ||
| } | ||
| defer c.Close() | ||
|
|
||
| crit, err := recipe.BuildCriteriaWithRegistry(nil, | ||
| recipe.WithServiceRegistry("aks"), | ||
| recipe.WithAcceleratorRegistry("h100"), | ||
| recipe.WithIntentRegistry("training"), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("BuildCriteria: %v", err) | ||
| } | ||
|
|
||
| rec, err := c.ResolveRecipeFromSnapshot(t.Context(), aicr.WrapCriteria(crit), gpuHardwareSnapshot(false)) | ||
| if err != nil { | ||
| t.Fatalf("ResolveRecipeFromSnapshot: %v", err) | ||
| } | ||
|
|
||
| if _, present := gpuOperatorDriverEnabled(t, rec); present { | ||
| t.Fatal("driver.enabled override present, want no injection under only-false policy") | ||
| } | ||
| } | ||
|
|
||
| // TestResolveRecipeFromSnapshot_GPUDriverAutoDetect_EKSPreinstalled proves | ||
| // the injection is provider-agnostic (no service gate) — an EKS resolve | ||
| // against a preinstalled-driver snapshot also receives driver.enabled=false. | ||
| // This is the fix path for EKS GPU-optimized AMIs that ship an NVIDIA | ||
| // driver, where the base values.yaml default driver.enabled=true would | ||
| // otherwise cause a double-install. | ||
| func TestResolveRecipeFromSnapshot_GPUDriverAutoDetect_EKSPreinstalled(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| c, err := aicr.NewClient(aicr.WithRecipeSource(aicr.EmbeddedSource())) | ||
| if err != nil { | ||
| t.Fatalf("NewClient: %v", err) | ||
| } | ||
| defer c.Close() | ||
|
|
||
| crit, err := recipe.BuildCriteriaWithRegistry(nil, | ||
| recipe.WithServiceRegistry("eks"), | ||
| recipe.WithAcceleratorRegistry("h100"), | ||
| recipe.WithIntentRegistry("training"), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("BuildCriteria: %v", err) | ||
| } | ||
|
|
||
| rec, err := c.ResolveRecipeFromSnapshot(t.Context(), aicr.WrapCriteria(crit), gpuHardwareSnapshot(true)) | ||
| if err != nil { | ||
| t.Fatalf("ResolveRecipeFromSnapshot: %v", err) | ||
| } | ||
|
|
||
| enabled, present := gpuOperatorDriverEnabled(t, rec) | ||
| if !present { | ||
| t.Fatal("driver.enabled override missing — expected the auto-detect injection on EKS too") | ||
| } | ||
| if enabled { | ||
| t.Fatalf("driver.enabled = true, want false") | ||
| } | ||
| } | ||
|
|
||
| // TestResolveRecipeFromSnapshot_GPUDriverAutoDetect_NoGPUMeasurement asserts | ||
| // that a snapshot with only a K8s measurement (no GPU subtype at all — the | ||
| // signal is Unknown) leaves the recipe untouched. This guards the | ||
| // no-cluster / criteria-only path where callers pass a partial snapshot. | ||
| func TestResolveRecipeFromSnapshot_GPUDriverAutoDetect_NoGPUMeasurement(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| c, err := aicr.NewClient(aicr.WithRecipeSource(aicr.EmbeddedSource())) | ||
| if err != nil { | ||
| t.Fatalf("NewClient: %v", err) | ||
| } | ||
| defer c.Close() | ||
|
|
||
| crit, err := recipe.BuildCriteriaWithRegistry(nil, | ||
| recipe.WithServiceRegistry("aks"), | ||
| recipe.WithAcceleratorRegistry("h100"), | ||
| recipe.WithIntentRegistry("training"), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("BuildCriteria: %v", err) | ||
| } | ||
|
|
||
| rec, err := c.ResolveRecipeFromSnapshot(t.Context(), aicr.WrapCriteria(crit), k8sVersionSnapshot()) | ||
| if err != nil { | ||
| t.Fatalf("ResolveRecipeFromSnapshot: %v", err) | ||
| } | ||
|
|
||
| if _, present := gpuOperatorDriverEnabled(t, rec); present { | ||
| t.Fatal("driver.enabled override present without a GPU measurement — signal must be Unknown") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the auto-detect scenarios table-driven.
AKS preinstalled/absent, EKS preinstalled, and missing-GPU cases repeat the same flow with varying service, snapshot, and expected override. Consolidate them into one table-driven test.
As per coding guidelines, “Use table-driven tests for cases with multiple inputs or outcomes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/client/v1/aicr_test.go` around lines 875 - 1013, Consolidate the four
repeated auto-detect tests into a single table-driven
TestResolveRecipeFromSnapshot_GPUDriverAutoDetect test with cases for AKS
preinstalled, AKS absent, EKS preinstalled, and missing GPU measurement.
Parameterize service, snapshot, expected override presence, and expected enabled
value, then run each case with t.Run while retaining the shared client, criteria
construction, resolution, and assertions.
Source: Coding guidelines
| // Policy is only-false: the function never forces driver.enabled=true. | ||
| // That keeps the change safe across every provider — GKE-COS and OKE | ||
| // values files already carry driver.enabled=false, so the injection is | ||
| // byte-for-byte idempotent for them; AKS with --gpu-driver Install and | ||
| // EKS on preinstalled-driver AMIs get the fix; every other state |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use “semantically idempotent,” not “byte-for-byte idempotent.”
A ValuesFile-only driver.enabled=false still causes this function to add ComponentRef.Overrides["driver"]["enabled"], changing serialized resolved recipes. The rendered Helm value is idempotent, not the bytes. Update the matching claim in docs/user/component-catalog.md Line 71 too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/client/v1/gpu_driver_state.go` around lines 142 - 146, Replace the
“byte-for-byte idempotent” claim in the comment near the driver injection logic
and in component-catalog.md with “semantically idempotent,” clarifying that
rendered Helm values remain unchanged while serialized resolved recipes may gain
driver.enabled overrides.
20f205a to
678c090
Compare
Signed-off-by: Jayson Du <jaydu@nvidia.com>
xdu31
force-pushed
the
feat/gpu-driver-auto-detect
branch
from
678c090 to
3374081
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/client/v1/aicr.go`:
- Line 683: Cache the result of toInternalSnapshot(snap) once in the surrounding
logic, then reuse that variable both inside the evaluator closure and in the
applyGPUDriverAutoOverride call. Update the references near evaluator and
applyGPUDriverAutoOverride to eliminate the duplicate conversion.
In `@pkg/client/v1/gpu_driver_state.go`:
- Around line 153-176: Clone the existing Overrides map before modifying it in
applyGPUDriverAutoOverride, and use the clone for all subsequent driver updates.
Preserve nil initialization behavior, but ensure the original ComponentRef
override map from the merge path is never mutated or shared after setting
driver.enabled to false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 28091b86-e195-4b37-a1b4-3094ea8d2a24
📒 Files selected for processing (7)
docs/integrator/aks-gpu-setup.mddocs/integrator/recipe-development.mddocs/user/component-catalog.mdpkg/client/v1/aicr.gopkg/client/v1/aicr_test.gopkg/client/v1/gpu_driver_state.gopkg/client/v1/gpu_driver_state_test.go
| // gpu-operator.driver.enabled=false so the Operator does not install | ||
| // a second driver on top. Only-false policy — safe across every | ||
| // provider (see gpu_driver_state.go for the full rationale). | ||
| applyGPUDriverAutoOverride(internal, computeGPUDriverState(toInternalSnapshot(snap))) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Minor: toInternalSnapshot(snap) is computed twice.
It's already converted inside the evaluator closure (line 663) and now again here. If toInternalSnapshot does non-trivial work (deep copy of measurements), caching the result once and reusing it avoids repeated conversion.
♻️ Proposed refactor
+ internalSnap := toInternalSnapshot(snap)
evaluator := func(constraint recipe.Constraint) recipe.ConstraintEvalResult {
- v := constraints.Evaluate(constraint, toInternalSnapshot(snap))
+ v := constraints.Evaluate(constraint, internalSnap)
return recipe.ConstraintEvalResult{
Passed: v.Passed,
Actual: v.Actual,
Error: v.Error,
}
}
...
- applyGPUDriverAutoOverride(internal, computeGPUDriverState(toInternalSnapshot(snap)))
+ applyGPUDriverAutoOverride(internal, computeGPUDriverState(internalSnap))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| applyGPUDriverAutoOverride(internal, computeGPUDriverState(toInternalSnapshot(snap))) | |
| internalSnap := toInternalSnapshot(snap) | |
| evaluator := func(constraint recipe.Constraint) recipe.ConstraintEvalResult { | |
| v := constraints.Evaluate(constraint, internalSnap) | |
| return recipe.ConstraintEvalResult{ | |
| Passed: v.Passed, | |
| Actual: v.Actual, | |
| Error: v.Error, | |
| } | |
| } | |
| ... | |
| applyGPUDriverAutoOverride(internal, computeGPUDriverState(internalSnap)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/client/v1/aicr.go` at line 683, Cache the result of
toInternalSnapshot(snap) once in the surrounding logic, then reuse that variable
both inside the evaluator closure and in the applyGPUDriverAutoOverride call.
Update the references near evaluator and applyGPUDriverAutoOverride to eliminate
the duplicate conversion.
| func applyGPUDriverAutoOverride(r *recipe.RecipeResult, state gpuDriverState) { | ||
| if r == nil || state != gpuDriverPreinstalled { | ||
| return | ||
| } | ||
| for i := range r.ComponentRefs { | ||
| if r.ComponentRefs[i].Name != gpuOperatorComponentName { | ||
| continue | ||
| } | ||
| ref := &r.ComponentRefs[i] | ||
| if ref.Overrides == nil { | ||
| ref.Overrides = map[string]any{} | ||
| } | ||
| driverAny, _ := ref.Overrides["driver"].(map[string]any) | ||
| if driverAny == nil { | ||
| driverAny = map[string]any{} | ||
| ref.Overrides["driver"] = driverAny | ||
| } | ||
| driverAny["enabled"] = false | ||
| slog.Info("auto-disabled gpu-operator driver install: pre-installed driver detected in snapshot", | ||
| "component", gpuOperatorComponentName, | ||
| "reason", "driver-loaded=true") | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ComponentRef.Overrides maps are deep-copied during recipe
# merge/build, or shared/aliased from a cached registry/template.
ast-grep outline pkg/recipe --items all --type struct,function --match 'Overrides|ComponentRef|merge|clone|Copy'
rg -nP -C4 '\.Overrides\s*=' pkg/recipe
rg -nP -C4 'func.*[Mm]erge.*ComponentRef' pkg/recipeRepository: NVIDIA/aicr
Length of output: 5543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant recipe/result code paths first.
ast-grep outline pkg/recipe/metadata.go --view expanded
ast-grep outline pkg/recipe/metadata_store.go --view expanded
ast-grep outline pkg/client/v1/gpu_driver_state.go --view expanded
ast-grep outline pkg/recipe/builder.go --view expanded 2>/dev/null || true
ast-grep outline pkg/recipe/*.go --view expanded | sed -n '1,260p'
# Read the cloning/merge code around the deep-copy logic and the result builder.
sed -n '940,1020p' pkg/recipe/metadata.go
sed -n '1090,1160p' pkg/recipe/metadata.go
sed -n '1,260p' pkg/client/v1/gpu_driver_state.go
rg -n "BuildFromCriteriaWithEvaluator|RecipeResult|DeepCopy|cloneComponentRef|applyGPUDriverAutoOverride" pkg/recipe pkg/client/v1 -A4 -B4
# Probe whether any code path intentionally reuses mutable overrides from shared state.
python3 - <<'PY'
from pathlib import Path
text = Path("pkg/recipe/metadata.go").read_text()
for needle in ["serializer.DeepCopyAnyMap(ref.Overrides)", "out := ref", "mergeComponentRef(base, overlay ComponentRef) ComponentRef"]:
print(f"{needle}: {needle in text}")
PYRepository: NVIDIA/aicr
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the merge/hydration path that feeds final RecipeResult.ComponentRefs.
sed -n '920,1160p' pkg/recipe/metadata.go
sed -n '620,980p' pkg/recipe/metadata_store.go
# Look for any direct assignment of ComponentRefs / Overrides that bypasses cloneComponentRef.
rg -n "ComponentRefs =|Overrides =|cloneComponentRef\(|DeepCopyAnyMap\(|mergeComponentRef\(" pkg/recipe -A2 -B2
# Check tests that specifically defend against aliasing of overrides or result copies.
rg -n "alias|DeepCopy|Overrides.*copy|NoSliceAliasing|mutable" pkg/recipe/*_test.go -A3 -B3Repository: NVIDIA/aicr
Length of output: 42405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1240,1355p' pkg/recipe/metadata_store.go
sed -n '410,470p' pkg/recipe/components.go
sed -n '930,1015p' pkg/recipe/metadata_store.go
rg -n "Overrides\\[|driver\\.enabled|DeepCopyAnyMap|cloneComponentRef|ApplyRegistryDefaults" pkg/recipe -A3 -B3Repository: NVIDIA/aicr
Length of output: 35259
Clone the Overrides map before mutating it. RecipeResult still carries ComponentRef map fields through from the merge path, so this write can leak driver.enabled=false into shared recipe state when the gpu-operator ref already has overrides.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/client/v1/gpu_driver_state.go` around lines 153 - 176, Clone the existing
Overrides map before modifying it in applyGPUDriverAutoOverride, and use the
clone for all subsequent driver updates. Preserve nil initialization behavior,
but ensure the original ComponentRef override map from the merge path is never
mutated or shared after setting driver.enabled to false.
Source: Coding guidelines
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
🔬 Multi-Persona Review
Method: 4 independent persona reviewers (Correctness · Domain/Architecture · Test-coverage · Docs) → an adversarial senior meta-reviewer that re-derived every finding from the resolved code at 33740818. Refuted claims are listed under Confirmed non-issues so you can see they were examined.
Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
The mechanism is clean, well-tested, and correctly wired — the internal RecipeResult mutation reaches both Resolved() and rendered Helm output, nil-safety is thorough, and the only-false policy is genuinely safe against the static provider values files it enumerates. The four-state reducer and the Unknown-not-Absent fail-closed default are nicely done.
Two Major findings converge on one root cause: injecting a lone driver.enabled=false is under-specified. The signal can't distinguish a platform-preinstalled driver from one the GPU Operator itself installed (so a re-snapshot can tear down a working driver — see inline at gpu_driver_state.go:124), and a single key doesn't reproduce the holistic preinstalled-driver contract the maintainers deliberately encoded in the provider profiles (inline at gpu_driver_state.go:170). Both have a real, already-collected in-snapshot signal the reducer ignores (the GPU-Operator ClusterPolicy, and the full profile values).
Recommendation: address the two 🟠 before merge. No blocker — the feature is opt-in behind the snapshot path — but the two Major items undercut the core premise on the very clusters this targets.
Additional finding (off-diff, folded here)
🟡 docs/user/cli-reference.md:323 — the recipe --snapshot bullet omits the new behavior. It enumerates snapshot-detected effects (accelerator/service/OS/node-count) but not the new gpu-operator driver override, and doesn't cross-link the catalog section this PR adds. Per CLAUDE.md (CLI behavior change → cli-reference.md), add one sentence + a link to component-catalog.md#gpu-operator-driver-auto-detect.
✅ Confirmed non-issues (examined, refuted)
- Nil-safety — every dereference in both functions is guarded (nil snap/measurement/subtype, missing keys, nil Overrides, absent gpu-operator ref). No panic path.
- Mutation reaches consumers —
facadeResultFromInternalstores the same*recipe.RecipeResultpointer; the override reachesResolved()andBundleComponents(confirmed by the end-to-end test). --setstill force-enables — merge is base→ValuesFile→Overrides, then CLI--seton top, so--set gpuoperator:driver.enabled=truebeats the auto-injected false. Works as intended.driver-loadedbool assertion — bool round-trips as bool under bothyaml.v3andencoding/json→ToReading; the float64 nitpick is integer-only.- Doc anchor links valid — both
#alternative-use-the-aks-driver-only-profileand#gpu-operator-driver-auto-detectresolve to real headings; lychee will pass. ResolveRecipeFromCriteriaskips the injection — consistent (only-false needs a snapshot signal).- PR body "4 integration tests" — accurate; it just omits mentioning the 5th (
TestBundleComponents_..._RendersInHelmValues).
Summary
| Tier | Count |
|---|---|
| 🔴 Blocker | 0 |
| 🟠 Major | 2 |
| 🟡 Minor | 4 |
| 🔵 Nitpick | 4 |
Generated via the multi-persona + adversarial-meta-review workflow. Findings were independently reproduced against the code before posting.
| driverAny = map[string]any{} | ||
| ref.Overrides["driver"] = driverAny | ||
| } | ||
| driverAny["enabled"] = false |
There was a problem hiding this comment.
🟠 Major — A lone driver.enabled=false violates the documented driver/toolkit lockstep
The auto-detect injects only driver.enabled=false. But recipes/components/gpu-operator/values-aks.yaml:20-24 states the AKS driver-only profile requires driver.enabled=false, toolkit.enabled=false, AND operator.runtimeClass=nvidia-container-runtime set together — never just one of them. On the exact target cases (AKS --gpu-driver Install, EKS preinstalled-driver AMIs inheriting base values.yaml), base leaves toolkit.enabled=true (values.yaml:50) and gdrcopy.enabled=true (values.yaml:144) with no hostPaths.driverInstallDir. The idempotent GKE-COS/OKE cases only work because their values files already carry the full holistic set (driver=false + gdrcopy=false + driverInstallDir) — the fix cases don't.
Blast radius: The very clusters this feature targets (AKS/EKS with a platform driver) get a half-configured Operator: driver off, but toolkit/gdrcopy still on and expecting an operator-managed driver root that no longer exists.
Fix: Inject the coordinated set the profile requires (toolkit.enabled=false + operator.runtimeClass where applicable), or gate auto-detect to overlays that already carry a full preinstalled profile and treat bare AKS/EKS as out-of-scope until one exists.
| } | ||
| } | ||
|
|
||
| loaded := hw.Get(measurement.KeyGPUDriverLoaded) |
There was a problem hiding this comment.
🟠 Major — driver-loaded=true has no provenance — re-snapshot can tear down the driver AICR installed
DriverLoaded (pkg/collector/gpu/nfd.go) is set purely on nvidia kernel-module presence — installer-agnostic. The GPU Operator's driver container loads that module into the node-global kernel, so a cluster where a prior AICR deploy set driver.enabled=true reports driver-loaded=true on re-snapshot → re-resolve flips true→false → next helm upgrade removes the operator-managed driver DaemonSet → new/rebooted GPU nodes come up driverless. The 'monotonically safe, cannot regress a working overlay' claim holds against static values files but not against this self-referential signal, and nothing enforces 'snapshots must be pre-deploy' (re-snapshot on drift-check / GitOps reconcile is normal). Only a slog.Info marks the flip.
Blast radius: Any workflow that resolves a recipe from a snapshot taken after the GPU Operator has run — silent driver teardown on redeploy.
Fix: The guard is already in the snapshot: pkg/collector/k8s/policy.go collects the GPU-Operator ClusterPolicy. Only auto-disable when driver-loaded=true AND no gpu-operator ClusterPolicy is present. At minimum, document the pre-deploy-snapshot requirement and warn when both a loaded driver and a ClusterPolicy are observed.
| const gpuHardwareSubtypeName = "hardware" | ||
|
|
||
| // gpuDriverState reports what the snapshot tells us about the NVIDIA | ||
| // kernel driver on the sampled GPU node. |
There was a problem hiding this comment.
🟡 Minor — Single-node sample makes mixed pools fail TOWARD a missing driver (new, silent, worse mode)
One node is sampled. Before this PR a mixed cluster resolved driver.enabled=true → a driver on every pool. Now one preinstalled-node sample injects false globally → non-preinstalled pools get no driver = dead GPUs.
Blast radius: Mixed-pool clusters — a strictly worse and undetectable failure than the double-install being prevented. Parking mixed pools behind #464 is fine; parking them into a worse silent mode is the concern.
Fix: The topology collector already sees all node labels — at least warn when GPU pools appear non-uniform before injecting, and note the fail-direction in the #464 deferral + catalog doc.
| // gpu-operator.driver.enabled=false so the Operator does not install | ||
| // a second driver on top. Only-false policy — safe across every | ||
| // provider (see gpu_driver_state.go for the full rationale). | ||
| applyGPUDriverAutoOverride(internal, computeGPUDriverState(toInternalSnapshot(snap))) |
There was a problem hiding this comment.
🟡 Minor — Snapshot-driven recipe shaping lives in the pkg/client/v1 facade, not pkg/recipe
Mutating ComponentRef.Overrides from measured state is recipe business logic; CLAUDE.md designates the facade as a no-business-logic adapter. Latent today (server has no snapshot-resolve path; CLI routes through the facade), but the moment a snapshot endpoint calls pkg/recipe.BuildFromCriteriaWithEvaluator directly, the override silently vanishes with no compile-time or test signal. The evaluator already threads the snapshot into pkg/recipe, which is the natural home.
Blast radius: Latent divergence for any future non-facade snapshot-resolve caller. Trade-off, not a defect.
Fix: Move the post-process into pkg/recipe as an explicit measurement-driven step, or document at the pkg/recipe seam that snapshot-driven overrides are applied by the facade.
| if loaded == nil { | ||
| return gpuDriverUnknown | ||
| } | ||
| b, ok := loaded.Any().(bool) |
There was a problem hiding this comment.
🟡 Minor — Fail-closed non-bool branch untested; reducer never exercised through real deserialization
computeGPUDriverState sits at 90% (package 75.5%, passes). Untested: the fail-closed guard (driver-loaded present but non-bool → Unknown) the design comment leans on, and the dead int64 count branch. Every fixture builds Readings via SubtypeBuilder (Go-native int/bool), so the ToReading→float64 path is never exercised — which is what masks the float64 gap below.
Blast radius: The untested branch is the security-relevant fail-closed path; untested = unverified.
Fix: Add a table row for non-bool driver-loaded → Unknown, and one reducer test that round-trips a fixture through json.Unmarshal / sigs.k8s.io/yaml before reducing.
| return gpuDriverNotObserved | ||
| } | ||
| } | ||
| if count := hw.Get(measurement.KeyGPUCount); count != nil { |
There was a problem hiding this comment.
🔵 Nitpick — gpu-count type switch omits float64 (documented anti-pattern)
The count switch handles int/int64 only; a JSON / sigs.k8s.io/yaml-decoded snapshot delivers gpu-count as float64. Zero impact today (only caller loads via yaml.v3 → int; and gpu-present=false, a bool, short-circuits the no-GPU case first, so the count check never changes an outcome). Still the CLAUDE.md 'handles int/int64 but not float64' anti-pattern.
Blast radius: Latent — trips only if a future server/external caller feeds a JSON-decoded snapshot.
Fix: Add a case float64 branch (reject non-integral via float64(int64(v)) != v), or reuse hw.GetInt64 which already normalizes int/int64/float64.
| if r.ComponentRefs[i].Name != gpuOperatorComponentName { | ||
| continue | ||
| } | ||
| ref := &r.ComponentRefs[i] |
There was a problem hiding this comment.
🔵 Nitpick — In-place Overrides mutation aliases the sync.Once-cached MetadataStore
initBaseMergedSpec shallow-copies ComponentRefs, so each ref's Overrides map pointer aliases the cached base. Safe only because the base gpu-operator ref carries no Overrides (valuesFile only) and overlays deep-merge into fresh maps. If anyone ever adds an Overrides block to the base ref, this mutation poisons every future resolve (including snapshot-less ones).
Blast radius: None today; latent cross-resolve state poisoning if the base ref ever gains Overrides.
Fix: Defensive — assign a fresh driver submap / deep-copy (serializer.DeepCopyAnyMap) before writing.
| ref.Overrides["driver"] = driverAny | ||
| } | ||
| driverAny["enabled"] = false | ||
| slog.Info("auto-disabled gpu-operator driver install: pre-installed driver detected in snapshot", |
There was a problem hiding this comment.
🔵 Nitpick — Observability is inject-only
Only the Preinstalled→inject path logs. Unknown (incl. stale snapshot missing the reading), NotObserved, Absent, and Preinstalled-but-no-gpu-operator-ref are all silent.
Blast radius: An operator debugging 'why didn't it disable' (or an unexpected double-install) gets no trace of the computed state.
Fix: Add a slog.Debug of the resolved gpuDriverState (and reason) on the no-op paths.
| // Policy is only-false: the function never forces driver.enabled=true. | ||
| // That keeps the change safe across every provider — GKE-COS and OKE | ||
| // values files already carry driver.enabled=false, so the injection is | ||
| // byte-for-byte idempotent for them; AKS with --gpu-driver Install and |
There was a problem hiding this comment.
🔵 Nitpick — "byte-for-byte idempotent" is imprecise in the code comment
For GKE-COS/OKE the rendered Helm value is unchanged, but the resolved-recipe artifact gains an overrides.driver.enabled block a criteria-only resolve omits — so aicr recipe --snapshot -o recipe.yaml diffs are not byte-identical. The catalog doc scopes this correctly ('the rendered value is unchanged'); only this comment drops the qualifier.
Blast radius: Could mislead an integrator diffing emitted recipes.
Fix: Scope the phrasing to rendered Helm output, e.g. 'the rendered driver.enabled value is unchanged (the resolved recipe now records the override explicitly).'
2 participants
Summary
When resolving a recipe from a snapshot, read the sampled GPU node's
driver-loadedmeasurement and injectcomponents.gpu-operator.overrides.driver.enabled=falsewhen the NVIDIA kernel module is already loaded. Prevents the GPU Operator from installing a second driver on top of one the platform has already provisioned.Motivation / Context
Today
driver.enabledis set statically per provider overlay:values.yaml=true(base, inherited by EKS and AKS)values-gke-cos*.yaml=false(GKE-COS preinstalls drivers)values-oke*.yaml=false(OKE preinstalls drivers on GPU shapes)values-aks*.yaml— unset, falls back to helm chart defaulttrueBroken case: an AKS nodepool created with
--gpu-driver Install(Azure installs the driver) receives AICR's defaultdriver.enabled=true, and the GPU Operator tries to install a second driver on top → conflict, cluster left in a bad state. The same failure mode exists on EKS GPU-optimized AMIs that ship an NVIDIA driver.Fixes: #475
Related: #464 (heterogeneous per-node visibility for mixed pools)
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe) — behavior only; the mutation lives inpkg/client/v1pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)pkg/client/v1(facade — the injection layer)Implementation Notes
pkg/client/v1) and post-process the internalrecipe.RecipeResultbefore it is projected to the facade shape. Zero coupling ofpkg/recipetopkg/snapshotter. The injection lands inComponentRef.Overridesat the top of the merge chain (base values.yaml → ValuesFile → Overrides), so it wins over every provider values file at bundle time.pkg/client/v1/gpu_driver_state.go).computeGPUDriverState(snap)maps the snapshot to one of four states —Unknown/NotObserved/Preinstalled/Absent. A missingdriver-loadedkey is deliberatelyUnknown(notAbsent), so a stale snapshot from an older CLI cannot flip a hardened overlay.applyGPUDriverAutoOverridenever forcesdriver.enabled=true. Safe under no-cluster mode (Unknown→ no-op). Idempotent for GKE-COS/OKE (values files already carryfalse). Fixes AKS-with---gpu-driver Installand EKS-with-preinstalled-driver AMIs. AKS-with---gpu-driver noneand stock EKS AMIs are unaffected (state=Absent → no-op).driver.enabled--gpu-driver Installtrue)--gpu-driver nonetrue)pkg/collector/gpu/gpu.go) runs on onenvidia.com/gpu.present=truenode the snapshotter Job schedules onto, so the reducer trusts a single-node sample. On homogeneous clusters — the common case — that's representative. Mixed-pool clusters (some nodes with a preinstalled driver, some without) need per-node collector fanout and stay parked behind RFC: Dynamic GPU Orchestration Layer — From Day-Zero Recipes to Day-N Runtime Intelligence #464; documented in both the component catalog and the AKS integrator guide.slog.Infoat the injection point when the override actually lands. Other states are silent to avoid noise on every criteria-only resolve.Testing
make qualify: passes. Total coverage 78.2% (threshold 75%).golangci-lint run -c .golangci.yaml ./pkg/client/v1/...: 0 issues (mandatory Go-change gate).pkg/client/v1via thegit worktreebaseline method fromAGENTS.md: comparable to baseline; every new unexported func has coverage (computeGPUDriverState— 9-case table;applyGPUDriverAutoOverride— 8-case table + sibling-preservation test).TestComputeGPUDriverState(9 subtests) — reducer table covers nil snapshot, empty, K8s-only, GPU-no-subtypes, missing driver-loaded, gpu-present=false, gpu-count=0, driver-loaded=true, driver-loaded=false.TestApplyGPUDriverAutoOverride(8 subtests) +TestApplyGPUDriverAutoOverride_PreservesSiblingsUnderDriver— injection policy: nil result, non-Preinstalled states never inject, sibling underdriver(e.g.driver.version) preserved, existingdriver.enabled=truegets flipped, no gpu-operator ref is a no-op.TestResolveRecipeFromSnapshot_GPUDriverAutoDetect_*(4 integration tests) — end-to-end throughResolveRecipeFromSnapshoton the embedded recipes: AKS+Preinstalled → override present; AKS+Absent → no override; EKS+Preinstalled → override present (proves provider-agnostic wire-up); AKS+K8s-only snapshot → no override.recipes/registry.yamlor version pins, somake bom-docsis not required.Manual verification (not yet run). The change is exercised end-to-end by the integration tests using the embedded recipe catalog. A live e2e on the
aicr-test1AKS testbed with a real--gpu-driver Installnodepool is worth confirming before backport if any is planned; the assertion is thataicr recipe --snapshot snap.yaml --service aks --accelerator h100 --intent training --os ubuntuemitscomponents.gpu-operator.overrides.driver.enabled=false.Risk Assessment
Rollout notes:
ResolveRecipeFromSnapshotcode path. Recipes resolved without a snapshot (criteria-only, no-cluster mode) see zero behavior change.pkg/client/v1and the doc paragraphs. No data migration, no persisted state, no on-disk artifacts.driver.versionin the recipe).Checklist
make testwith-race)make lint)git commit -S) — GPG signing info