harden(recipes): lockstep DRA driver root with operator install dir by mchmarny · Pull Request #1106 · NVIDIA/aicr

mchmarny · 2026-05-29T20:33:10Z

Summary

Add TestDriverRootLockstep (and a supporting TestStringAtPath) in pkg/recipe to enforce that, for every leaf overlay shipping both nvidia-dra-driver-gpu and gpu-operator, the resolved nvidia-dra-driver-gpu.nvidiaDriverRoot equals the resolved gpu-operator.hostPaths.driverInstallDir. This is the lockstep guard requested in #1087.

Motivation / Context

These two paths must point to the same on-host driver location, but they're independently configurable across overlays with no schema link between them. An overlay editor can change one without the other and CI won't notice. When they drift:

DRA fails CDI spec generation (Driver/library version mismatch or missing libnvidia-ml.so)
DRA-allocated pods stay in ContainerCreating
aicr validate deployment phase fails

Fixes: #1087
Related: #1086 (BCM overlay gaps — where this lockstep would have prevented an earlier debug iteration)

Type of Change

New feature (non-breaking change that adds functionality)

Component(s) Affected

Recipe engine / data (pkg/recipe)

Implementation Notes

Discovery: mirrors TestBothBuildPathsProduceIdenticalContent — walks loadMetadataStore for every overlay not referenced as spec.base (the leaves). Each leaf is resolved through the production BuildRecipeResult so the wildcard contributions and mixins are exercised.

Assertion tiers (per the validation-phase-floor test precedent):

Hard fail when both values are explicitly set and differ — the core silent-drift case the issue is guarding against. This is what will catch a future regression.
Warn-only (promoted to fail under AICR_DRIVER_ROOT_LOCKSTEP_STRICT=1) when exactly one side is explicitly set and the other falls through to the upstream chart default. The chart default isn't visible to this test, so the lockstep is unverifiable rather than definitively broken. Strict mode is for the eventual cleanup PR that makes every overlay's pair explicit.
Pass when both are unset (relying on chart defaults — out of scope of overlay correctness) or both set and match.

Why two-tier: running the test against current main reveals 25 leaf overlays where gpu-operator.hostPaths.driverInstallDir falls through to the chart default while nvidia-dra-driver-gpu.nvidiaDriverRoot is explicitly set via the dra-driver's base values.yaml. In practice these coincidentally lockstep (both at /run/nvidia/driver), but the lockstep is not enforced by the recipe. Cleaning that up is a separate follow-up; this PR ships the guard now and leaves the cleanup tractable via the strict-mode toggle.

Coverage:

Iterates all 32 discovered leaf overlays
7 of them have both values explicitly set today (gke-cos, eks-training, etc.) — those exercise the hard-fail path's positive case.
The remaining 25 trigger the soft warn path; strict mode flips them to errors.
TestStringAtPath covers the nested-map traversal helper.

Testing

make qualify

Results:

All Go tests pass with -race.
golangci-lint clean (0 issues on ./pkg/recipe/...).
Chainsaw e2e: 22 passed / 0 failed.
Vuln scan: clean.

Verified the test catches today's gaps in strict mode:

$ AICR_DRIVER_ROOT_LOCKSTEP_STRICT=1 go test -run TestDriverRootLockstep ./pkg/recipe/
FAIL: 25 overlays where one side is unset

…and passes in non-strict (default) mode, which is what make qualify runs.

Coverage delta (pkg/recipe): 91.9% (no change — the new file is mostly assertions exercising existing code paths).

Risk Assessment

Low — Test-only change. No production code touched. Easy to revert; easy to evolve via the strict-mode toggle.

Rollout notes: Default-mode CI runs see the test pass today. A follow-up PR that makes the unset-side explicit on each overlay can flip CI to AICR_DRIVER_ROOT_LOCKSTEP_STRICT=1 to promote the warnings to errors.

Checklist

Tests pass locally (make test with -race)
Linter passes (make lint)
I did not skip/disable tests to make CI green
I added/updated tests for new functionality
Changes follow existing patterns in the codebase
Commits are cryptographically signed (git commit -S)

Add TestDriverRootLockstep in pkg/recipe to assert that, for every leaf overlay shipping both nvidia-dra-driver-gpu and gpu-operator, the resolved nvidia-dra-driver-gpu.nvidiaDriverRoot equals the resolved gpu-operator.hostPaths.driverInstallDir. These paths are independently configurable across overlays today with no schema link, so an editor can change one without the other and CI doesn't notice — that drift breaks CDI spec generation, stalls DRA-allocated pods, and fails the validate deployment phase. The test walks every leaf overlay (same discovery pattern used by TestBothBuildPathsProduceIdenticalContent), resolves it through the production builder, and: - HARD FAIL when both values are explicitly set and differ (the silent-drift case the issue is guarding against). - WARN (or fail under AICR_DRIVER_ROOT_LOCKSTEP_STRICT=1) when exactly one side is explicitly set and the other falls through to the upstream chart default. The chart default is not visible to this test, so the lockstep is unverifiable rather than definitively broken; strict mode is for the eventual cleanup PR that makes every overlay's pair explicit. - PASS when both are unset or both are set and match. Also adds a small unit test for the stringAtPath helper used to dig the nested gpu-operator.hostPaths.driverInstallDir value out of the resolved Helm values tree. Closes #1087

coderabbitai · 2026-05-29T20:37:03Z

📝 Walkthrough

Walkthrough

This PR adds a new test file that introduces driver root lockstep validation across recipe overlays. TestDriverRootLockstep loads recipe metadata, discovers leaf overlays, resolves each overlay's component configuration, extracts nvidiaDriverRoot from nvidia-dra-driver-gpu and hostPaths.driverInstallDir from gpu-operator, and enforces they match. Hard failures occur when both values are explicitly set and differ, or when no leaf overlays exist. One-sided findings (one value set, other unverifiable) are logged by default but can be escalated to errors via the AICR_DRIVER_ROOT_LOCKSTEP_STRICT environment variable. A supporting stringAtPath helper safely retrieves nested string values from maps, returning empty string on missing keys or type mismatches.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

area/recipes, area/tests

Suggested reviewers

lockwobr
xdu31

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: adding a test guard to enforce lockstep between nvidiaDriverRoot and driverInstallDir.
Description check	✅ Passed	The description comprehensively explains the test's purpose, motivation, implementation details, assertion tiers, coverage, and testing approach, all relevant to the changeset.
Linked Issues check	✅ Passed	The PR fully implements the acceptance criteria from `#1087`: test enumerates leaf overlays, resolves via production BuildRecipeResult, asserts lockstep, runs in make qualify, and emits diagnostics.
Out of Scope Changes check	✅ Passed	All changes are in-scope: new test file, helper function, test data in YAML overlay files, and updated component base values—all supporting the lockstep guard objective.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch test/recipe-driver-root-lockstep

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 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/recipe/driver_root_lockstep_test.go`:
- Around line 61-62: Replace the unbounded context used for I/O in the test with
a timeout-bounded context: instead of using context.Background() for the ctx
passed into loadMetadataStore, create ctx via context.WithTimeout (e.g., a short
test-safe timeout like 5s or 10s), call defer cancel() immediately after, and
then pass that ctx into loadMetadataStore so the metadata/store and build
resolution cannot hang indefinitely in CI.

🪄 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: a1733925-9309-4b6e-8d76-1ff183f55230

📥 Commits

Reviewing files that changed from the base of the PR and between 76647b7 and ef6b001.

📒 Files selected for processing (1)

pkg/recipe/driver_root_lockstep_test.go

github-actions · 2026-05-29T20:43:26Z

Coverage Report ✅

Metric	Value
Coverage	77.2%
Threshold	75%
Status	Pass

Coverage Badge

![Coverage](https://img.shields.io/badge/coverage-77.2%25-green)

No Go source files changed in this PR.

yuanchen8911 · 2026-05-29T21:00:52Z

Thanks for tackling #1087 — the invariant is real and the direction is right. There are a few gaps.

Per review feedback on PR #1106: 1. Tighten TestDriverRootLockstep so the guard is real in default CI. The earlier two-tier design ("warn unless AICR_DRIVER_ROOT_LOCKSTEP_STRICT=1") was effectively dormant: make test runs without -v, so the t.Logf warnings were suppressed AND non-failing, leaving 25 overlays unverified. The test now hard-fails when either side is unset or when both are set and differ — both are equally risky because chart defaults differ across components (gpu-operator chart 26.3.1 defaults driverInstallDir to /run/nvidia/driver but DRA chart 25.12.0 defaults nvidiaDriverRoot to /). The AICR_DRIVER_ROOT_LOCKSTEP_STRICT toggle is retired. 2. Broaden discovery to every overlay with non-nil Spec.Criteria, not just leaves. Production resolution is per-query: filterToMaximalLeaves drops an overlay only when a matching descendant exists for that query. Intermediates like h100-gke-cos-training are resolvable directly when the query omits platform — the earlier leaf-only filter missed them. The test now covers 78 overlays (up from 32). 3. Make every overlay's gpu-operator.hostPaths.driverInstallDir explicit so the lockstep is verifiable: - components/gpu-operator/values.yaml: add base hostPaths.driverInstallDir: /run/nvidia/driver (matches the nvidia-dra-driver-gpu base value). - components/gpu-operator/values-oke.yaml + values-oke-training.yaml: set hostPaths.driverInstallDir: / (matches OKE's nvidia-dra-driver-gpu/values-oke.yaml). - overlays/kind.yaml: add gpu-operator override hostPaths.driverInstallDir: "/" (matches kind's existing nvidia-dra-driver-gpu.nvidiaDriverRoot: "/"). All 78 overlays now pass the lockstep guard. `make qualify` clean.

coderabbitai

Actionable comments posted: 1

🤖 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/recipe/driver_root_lockstep_test.go`:
- Around line 141-146: The test currently only guards against overlayCount == 0
but may still vacuously pass if no overlays were actually checked; change the
guard to assert checked > 0 (replace or add to the existing overlayCount check)
so the test calls t.Fatal when checked == 0, and update the summary t.Logf to
report the number of verified overlays using checked (not overlayCount); ensure
references to GetComponentRef("nvidia-dra-driver-gpu"), IsEnabled(), and the
t.Skipf paths remain the same so the failure triggers when all subtests were
skipped.

🪄 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: 5c1fda06-1996-4752-8088-64d7d8f34739

📥 Commits

Reviewing files that changed from the base of the PR and between ef6b001 and ae912bd.

📒 Files selected for processing (5)

pkg/recipe/driver_root_lockstep_test.go
recipes/components/gpu-operator/values-oke-training.yaml
recipes/components/gpu-operator/values-oke.yaml
recipes/components/gpu-operator/values.yaml
recipes/overlays/kind.yaml

yuanchen8911

/lgtm

Both review findings are fully addressed (verified against the branch, including a negative test confirming the guard hard-fails on one-sided drift in default make qualify mode, and that intermediates like eks/bcm are now covered — 78/78 overlays pass).

One follow-up: update the PR description — it still describes the old strict-mode / leaf-only / test-only version (the AICR_DRIVER_ROOT_LOCKSTEP_STRICT toggle is retired, discovery now covers all 78 overlays not just 32 leaves, and the PR now also modifies gpu-operator values files, so it's no longer test-only).

… and oke PR NVIDIA#1106 set gpu-operator.hostPaths.driverInstallDir to "/" on the kind and oke overlays so it would match nvidia-dra-driver-gpu.nvidiaDriverRoot ("/") and satisfy the new TestDriverRootLockstep guard (issue NVIDIA#1087). But driverInstallDir is the host path the operator-validator bind-mounts as the driver-validation container's rootfs target. runc rejects a mount whose destination is "/" ("mountpoint is on the top of rootfs"), so nvidia-operator-validator crash-loops, ClusterPolicy never goes Ready, and the gpu-operator Helm install retries until it times out. This hung every GPU-on-kind CI job (and main's nightly schedule) starting right after NVIDIA#1106 merged; oke shares the bug but is not in CI. Fix: - Remove the driverInstallDir: "/" override from kind.yaml, values-oke.yaml, and values-oke-training.yaml so they inherit the base default /run/nvidia/driver (the value that ran green before NVIDIA#1106). - Correct TestDriverRootLockstep: the nvidiaDriverRoot == driverInstallDir lockstep only holds when the operator manages the driver (driver.enabled true). With host-installed drivers (driver.enabled false, e.g. kind/oke) the two paths are independent — nvidiaDriverRoot may be "/" while driverInstallDir must be a real subdirectory. Add a hard invariant that driverInstallDir is never "/" for any overlay, which directly guards this regression class.

mchmarny requested a review from a team as a code owner May 29, 2026 20:33

mchmarny added the enhancement label May 29, 2026

mchmarny self-assigned this May 29, 2026

github-actions Bot added the size/L label May 29, 2026

coderabbitai Bot reviewed May 29, 2026

View reviewed changes

Comment thread pkg/recipe/driver_root_lockstep_test.go

mchmarny enabled auto-merge (squash) May 29, 2026 20:39

yuanchen8911 reviewed May 29, 2026

View reviewed changes

Comment thread pkg/recipe/driver_root_lockstep_test.go Outdated

yuanchen8911 reviewed May 29, 2026

View reviewed changes

Comment thread pkg/recipe/driver_root_lockstep_test.go Outdated

mchmarny requested a review from a team as a code owner May 29, 2026 21:19

github-actions Bot added the area/recipes label May 29, 2026

coderabbitai Bot reviewed May 29, 2026

View reviewed changes

Comment thread pkg/recipe/driver_root_lockstep_test.go

yuanchen8911 approved these changes May 29, 2026

View reviewed changes

mchmarny merged commit fc8ea41 into main May 29, 2026
125 of 127 checks passed

mchmarny deleted the test/recipe-driver-root-lockstep branch May 29, 2026 21:28

mchmarny changed the title ~~test(recipe): guard nvidiaDriverRoot / driverInstallDir lockstep~~ harden(recipes): lockstep DRA driver root with operator install dir May 29, 2026

yuanchen8911 mentioned this pull request May 30, 2026

fix(recipes): stop setting gpu-operator driverInstallDir to / on kind and oke #1119

Merged

12 tasks

coderabbitai Bot mentioned this pull request May 30, 2026

feat(recipe): context-aware DataProvider interface #1121

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

harden(recipes): lockstep DRA driver root with operator install dir#1106

harden(recipes): lockstep DRA driver root with operator install dir#1106
mchmarny merged 2 commits into
mainfrom
test/recipe-driver-root-lockstep

mchmarny commented May 29, 2026

Uh oh!

coderabbitai Bot commented May 29, 2026 •

edited

Loading

Walkthrough

Estimated code review effort

Suggested labels

Suggested reviewers

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented May 29, 2026 •

edited

Loading

Uh oh!

yuanchen8911 commented May 29, 2026

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

yuanchen8911 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

mchmarny commented May 29, 2026

Summary

Motivation / Context

Type of Change

Component(s) Affected

Implementation Notes

Testing

Risk Assessment

Checklist

Uh oh!

coderabbitai Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Suggested labels

Suggested reviewers

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report ✅

Uh oh!

yuanchen8911 commented May 29, 2026

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuanchen8911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented May 29, 2026 •

edited

Loading

github-actions Bot commented May 29, 2026 •

edited

Loading