feat(evidence): mode-aware CNCF dra-support and secure-access collection#1694
Conversation
|
🌿 Preview your docs: https://nvidia-preview-issue-1629-mode-aware-evidence.docs.buildwithfern.com/aicr |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates CNCF conformance evidence collection to resolve a GPU allocation policy from recipe context, pass it into the collector, and propagate it into the evidence script. The script now detects the served Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
30413d9 to
119415a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/evidence/cncf/scripts/collect-evidence.sh (1)
835-904: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
kubectl applyhere is not routed throughcapture(unlikecollect_dra).The DRA-support path records the apply via
capture "Apply test manifest" ...(Line 611), but this isolation path applies bare (Line 903), so the generated manifest command isn't reflected in the evidence file. Consider wrapping it for parity/auditability of the evidence artifact.🤖 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/evidence/cncf/scripts/collect-evidence.sh` around lines 835 - 904, The isolation test path applies the manifest directly, so the `kubectl apply` command is not captured in the evidence output like the `collect_dra` flow. Update the `collect-evidence.sh` isolation test section that builds `test_manifest` and applies it so the apply goes through `capture` with a clear label, preserving parity and auditability in the evidence file.
🤖 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/cli/validate.go`:
- Around line 785-810: The recipe-client setup and teardown logic in
resolveCNCFAllocationPolicy is duplicated from the main validateCmd
recipe-loading flow, which risks the two paths drifting. Extract the shared
recipeClientFromCmd → PropagateOrWrap → defer Close → slog.Info → LoadRecipe
sequence into a small helper that both validateCmd and
resolveCNCFAllocationPolicy call, and keep the helper responsible for client
lifecycle and load error handling.
---
Outside diff comments:
In `@pkg/evidence/cncf/scripts/collect-evidence.sh`:
- Around line 835-904: The isolation test path applies the manifest directly, so
the `kubectl apply` command is not captured in the evidence output like the
`collect_dra` flow. Update the `collect-evidence.sh` isolation test section that
builds `test_manifest` and applies it so the apply goes through `capture` with a
clear label, preserving parity and auditability in the evidence file.
🪄 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: 58ab79e6-d63e-4353-8ffe-d25bc8690706
📒 Files selected for processing (8)
demos/cuj2-demo.mddocs/conformance/cncf/index.mdpkg/cli/validate.gopkg/cli/validate_test.gopkg/evidence/cncf/claim_shape_test.gopkg/evidence/cncf/collector.gopkg/evidence/cncf/collector_test.gopkg/evidence/cncf/scripts/collect-evidence.sh
119415a to
145d9d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/evidence/cncf/scripts/collect-evidence.sh (1)
881-929: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake the DRA isolation test assert exactly one visible GPU.
The container only prints GPU information. It exits successfully—and reports isolation PASS—even if a misconfigured runtime exposes multiple GPUs.
Proposed fix
- nvidia-smi -L + gpu_list=$(nvidia-smi -L) || exit 1 + echo "${gpu_list}" + gpu_count=$(printf '%s\n' "${gpu_list}" | grep -c '^GPU ') + if [ "${gpu_count}" -ne 1 ]; then + echo "FAIL: expected exactly one visible GPU, saw ${gpu_count}" + exit 1 + fi🤖 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/evidence/cncf/scripts/collect-evidence.sh` around lines 881 - 929, The DRA isolation test currently only logs GPU visibility from the gpu-test container and can pass even if more than one GPU is exposed. Update the test manifest and verification logic in collect-evidence.sh around the isolation-test / gpu-test flow to assert that exactly one GPU is visible, using the existing nvidia-smi output from the container. Fail the test if the visible GPU count is not 1, and include the count in the evidence so the final PASS verdict in the isolation test is only emitted when a single allocated GPU is confirmed.
♻️ Duplicate comments (1)
pkg/cli/validate.go (1)
792-810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate recipe-client lifecycle boilerplate (repeat of a prior finding).
resolveCNCFAllocationPolicyrepeats the samerecipeClientFromCmd→PropagateOrWrap→defer client.Close()→slog.Info→client.LoadRecipesequence already present in the mainvalidateCmdbody (lines 699-710). This duplication was flagged in a previous review round on this file, and the same pattern persists here.🤖 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/cli/validate.go` around lines 792 - 810, `resolveCNCFAllocationPolicy` repeats the same recipe-client setup and teardown flow already used in `validateCmd`, so factor that shared `recipeClientFromCmd`/error wrapping/`defer client.Close()`/recipe loading logic into a reusable helper. Keep the policy-specific part in `resolveCNCFAllocationPolicy` focused on resolving the GPU allocation policy after the recipe is loaded, and reuse the helper from both places to remove the duplicated boilerplate.
🤖 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/evidence/cncf/scripts/collect-evidence.sh`:
- Around line 589-612: The DRA GPU evidence flow can report a stale PASS because
`cleanup_ns` errors are ignored, `capture` hides the `kubectl apply` status, and
the later apply steps are not checked. Update the `collect-evidence.sh` test
path around `cleanup_ns`, `capture`, and the `kubectl apply` calls to fail fast
on cleanup/apply errors, verify the old `dra-test` namespace/pod is gone before
proceeding, and emit FAIL instead of continuing when any deployment step does
not succeed. Also apply the same guard pattern to the other affected apply paths
in the script.
- Around line 556-571: The DRA inventory path in collect-evidence.sh can still
return PASS when ResourceSlice counting fails because gpu_slices and cd_slices
are forced through with || true. Update the count_resourceslices_for_driver
calls and the subsequent inventory/reporting flow so failures are detected and
propagated in both device-plugin and DRA modes, and ensure
collect_dra_device_plugin_verdict or the DRA equivalent cannot treat an
inventory error as a success. Use the existing symbols
count_resourceslices_for_driver, collect_dra_device_plugin_verdict, and
ALLOC_MODE to locate the affected logic.
---
Outside diff comments:
In `@pkg/evidence/cncf/scripts/collect-evidence.sh`:
- Around line 881-929: The DRA isolation test currently only logs GPU visibility
from the gpu-test container and can pass even if more than one GPU is exposed.
Update the test manifest and verification logic in collect-evidence.sh around
the isolation-test / gpu-test flow to assert that exactly one GPU is visible,
using the existing nvidia-smi output from the container. Fail the test if the
visible GPU count is not 1, and include the count in the evidence so the final
PASS verdict in the isolation test is only emitted when a single allocated GPU
is confirmed.
---
Duplicate comments:
In `@pkg/cli/validate.go`:
- Around line 792-810: `resolveCNCFAllocationPolicy` repeats the same
recipe-client setup and teardown flow already used in `validateCmd`, so factor
that shared `recipeClientFromCmd`/error wrapping/`defer client.Close()`/recipe
loading logic into a reusable helper. Keep the policy-specific part in
`resolveCNCFAllocationPolicy` focused on resolving the GPU allocation policy
after the recipe is loaded, and reuse the helper from both places to remove the
duplicated boilerplate.
🪄 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: e5bee5be-0508-4bb7-ae3c-0b860541ddd1
📒 Files selected for processing (8)
demos/cuj2-demo.mddocs/conformance/cncf/index.mdpkg/cli/validate.gopkg/cli/validate_test.gopkg/evidence/cncf/claim_shape_test.gopkg/evidence/cncf/collector.gopkg/evidence/cncf/collector_test.gopkg/evidence/cncf/scripts/collect-evidence.sh
145d9d9 to
9db8b99
Compare
There was a problem hiding this comment.
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/evidence/cncf/scripts/collect-evidence.sh`:
- Around line 883-918: The DRA isolation check in collect-evidence.sh is only
confirming that nvidia-smi runs, so a pod with multiple visible GPUs can still
pass; update the probe logic in the ResourceClaim test flow after
emit_resourceclaim_yaml to verify exactly one GPU is visible before writing a
PASS result. Use the existing test manifest/namespace setup and the
container-side nvidia-smi output to count visible devices, and fail the test
unless the count is one.
🪄 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: e03a121e-fbc8-469c-b349-9fbc18501b9e
📒 Files selected for processing (8)
demos/cuj2-demo.mddocs/conformance/cncf/index.mdpkg/cli/validate.gopkg/cli/validate_test.gopkg/evidence/cncf/claim_shape_test.gopkg/evidence/cncf/collector.gopkg/evidence/cncf/collector_test.gopkg/evidence/cncf/scripts/collect-evidence.sh
9db8b99 to
3175208
Compare
3175208 to
3d723ae
Compare
a9c3844 to
0ab5436
Compare
mchmarny
left a comment
There was a problem hiding this comment.
Mode-aware evidence collection done right — the script mirrors the validator semantics faithfully: the policy vocabulary matches allocation_policy.go exactly (including the reserved dra-extended-resource failing closed and unspecified falling to detection), ambient AICR_GPU_ALLOCATION_POLICY is stripped before threading, and ensure_fresh_namespace closes a real false-PASS hole (stale Succeeded pod satisfying wait_for_pod). I verified capture's new status propagation is safe: the script doesn't run set -e and run_check grades from the evidence file, so diagnostics-only callers are unaffected. The claim-shape test exercising the shipped emitter via the hidden emit-claim subcommand (asserted against vendored types) is a nice pattern.
Three low nits inline (error message swallowed by a stdout redirect, a 60s vs POD_TIMEOUT inconsistency between the two secure-access modes, and missing securityContext hardening on the new isolation pod) — none blocking. Several CI lanes (tests/Test, E2E, GPU, Tier-1 UAT) were still running at review time; good to merge from my side once they're green.
Bring collect-evidence.sh to parity with the policy-aware Go validators (NVIDIA#1327): the dra-support and secure-access submission paths previously hardcoded full-GPU DRA (gpu.nvidia.com ResourceClaim at resource.k8s.io/v1), which fails structurally on stock clusters now that the device-plugin allocation policy is the production default (NVIDIA#1671). Policy threading: aicr validate --cncf-submission resolves the recipe's GPU allocation policy (ResolveGPUAllocationPolicy) and threads it to the script via WithAllocationPolicy -> AICR_GPU_ALLOCATION_POLICY. Recipe load or resolution errors fail closed before any evidence is collected. The script maps device-plugin-extended-resource and dra-resource-claim directly; unknown or reserved values (dra-extended-resource) fail closed, mirroring allocmode.Verify — no silent fallback to detection. Standalone runs (no recipe: unspecified/empty policy) fall back to bash capability detection mirroring allocmode.Detect as far as bash reasonably can: DRA usable requires the gpu.nvidia.com DeviceClass plus >=1 node-local gpu.nvidia.com ResourceSlice device on a Ready, schedulable node; device-plugin usable requires allocatable nvidia.com/gpu on such a node. Preference order matches secure_access_check.go (DRA when usable, else device plugin, else FAIL). Every query failure is fail-closed and recorded; the evidence notes that pool-generation/taint validation remains Go-side. dra-support now always records the served resource.k8s.io version and per-driver ResourceSlice inventory (compute-domain slices count as valid NVIDIA publication — the NVIDIA#1620 forward constraint). The behavioral full-GPU claim pod runs only under DRA mode, with a version-correct ResourceClaim (v1/v1beta2 exactly wrapper; v1beta1 bare deviceClassName) built from served-version discovery instead of hardcoded v1. Under device-plugin mode the behavioral subtest is recorded not applicable and the verdict comes from the API + slice evidence, matching dra_support_check.go semantics. secure-access gains the device-plugin isolation path (authorized container with nvidia.com/gpu: 1 must see exactly one GPU via nvidia-smi; the unauthorized sibling must have no /dev/nvidia* nodes and no working nvidia-smi), ported from the kubernetes-sigs/ai-conformance PR NVIDIA#75 shape; the DRA path remains for DRA-mode clusters, version-corrected. A hidden emit-claim subcommand exposes the claim emitter so a Go test exercises the embedded script's v1/v1beta2/v1beta1 shapes (asserted against the vendored API types) and the fail-closed unknown-version branch. Failure propagation (Result: FAIL -> nonzero exit) is intentionally out of scope; it requires first-class SKIP semantics and is tracked in NVIDIA#1692. Fixes NVIDIA#1629 Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
Address reviewer nits on the CNCF evidence collector: - Redirect the unsupported-version log_error in emit_resourceclaim_yaml to stderr so the fail-closed reason survives the stdout->manifest redirect. - Use POD_TIMEOUT for the DRA isolation wait so both isolation modes share the same budget on slow image pulls. - Add pod- and container-level securityContext (seccompProfile RuntimeDefault, allowPrivilegeEscalation: false) to the device-plugin isolation pod so it is admissible under PSA restricted, matching manifests/dra-gpu-test.yaml. Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
0ab5436 to
f502877
Compare
Summary
Makes CNCF submission evidence collection (
aicr validate --cncf-submission) mode-aware: thedra-supportandsecure-accesssections now follow the recipe-configured GPU allocation policy (threaded viaAICR_GPU_ALLOCATION_POLICY, fail-closed on unknown values) or, on standalone runs, a fail-closed capability detection mirroringallocmode.Detect— instead of hardcoding full-GPU DRA atresource.k8s.io/v1.Motivation / Context
The Go validators became policy/mode-aware in the #1327 train, but
collect-evidence.shstill hardcoded a full-GPUgpu.nvidia.comResourceClaim in both paths. After the device-plugin default flip (#1671), those sections fail structurally on every stock cluster while the CLI reports success. This PR brings the submission script to parity with the validator semantics (dra_support_check.go,secure_access_check.go).Fixes: #1629
Related: #1327, #1671, #1692 (failure propagation — deliberately out of scope here), kubernetes-sigs/ai-conformance#75 (device-plugin secure-access shape)
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)docs/,examples/)pkg/evidence/cncf)Implementation Notes
--cncf-submissionresolves the recipe's policy viaResolveGPUAllocationPolicyand passes it to the collector (WithAllocationPolicy→AICR_GPU_ALLOCATION_POLICY). Recipe load/resolution errors fail closed before any evidence is collected. No recipe → standalone detection.device-plugin-extended-resource/dra-resource-claimdirectly; unknown or reserved values (e.g.dra-extended-resource) FAIL rather than silently falling back — mirrorsallocmode.Verify.allocmode.Detectas far as bash reasonably can (DeviceClass + node-localgpu.nvidia.comslice devices on Ready, schedulable nodes; allocatablenvidia.com/gpufor device plugin), with the validator's preference order (DRA when usable, else device plugin, else FAIL). Every query failure is fail-closed and recorded; the evidence notes that pool-generation/taint validation remains Go-side.dra-support: always records the servedresource.k8s.ioversion and per-driver ResourceSlice inventory (compute-domain slices count as valid NVIDIA publication — the feat(validator): GPU allocation capability inspection and hardening #1620 forward constraint). Behavioral claim pod only under DRA mode, with a version-correct claim (v1/v1beta2exactlywrapper;v1beta1baredeviceClassName) from served-version discovery. Under device-plugin mode the behavioral subtest is recorded Not applicable, verdict from API + slice evidence — matchingdra_support_check.go.secure-access: new device-plugin isolation path (authorized container withnvidia.com/gpu: 1must see exactly one GPU vianvidia-smi; unauthorized sibling must have no/dev/nvidia*and no workingnvidia-smi), ported from the ai-conformance PR fix: open webhook container ports in NetworkPolicy workaround #75 shape; DRA path kept, version-corrected.emit-claimscript subcommand lets a Go test exercise the embedded emitter for all three API versions (asserted against vendored types) plus the fail-closed unknown-version branch.Result: FAIL→ nonzero exit) is intentionally out of scope — it needs first-class SKIP semantics first; tracked in evidence: propagate section FAIL as collection error; add SKIP for absent prerequisites #1692.Testing
make qualify # full gate — PASS (isolated golangci-lint cache)pkg/evidence/cncf77.9% → 86.3% (+8.4%);pkg/cli69.0% → 69.1%.WithAllocationPolicyenv threading (subprocess);resolveCNCFAllocationPolicytable test.bash -nclean; shellcheck delta vs baseline: one style note.Live matrix (EKS GB200
yljtrxpmzu, stock post-flip cluster, chart0.4.1, 2026-07-09):gpuAllocationPolicy=device-plugin-extended-resourcelogged;dra-supportPASS (behavioral N/A note, compute-domain slice evidence);secure-accessPASS via the new device-plugin isolation test (authorized saw exactly 1 GPU, sibling saw none)device-pluginwith fail-closed provenance ("DeviceClass gpu.nvidia.com not found"); both sections PASSdra(validator ordering); behavioral claim pod PASS via version-discoveredresource.k8s.io/v1claim; DRA isolation PASS| Full policy-threaded collection at final head
3175208b(2026-07-10) | binary from the final head, stock recipe, deployment values verified == main's stock hydrated tuple | 4/4 PASS (dra-supportN/A-path,secure-accessdevice-plugin isolation,gang-scheduling,accelerator-metrics) — exercises the fresh-namespace and status-checked-apply fixes live || 8-feature run, generated inference recipe (
eks/gb200/inference/dynamo/ubuntu, 2026-07-10) | recipe generated by the head binary; stock vllm-agg DGD deployed for the Dynamo-dependent sections | 8/8 PASS — the 4 core cells plusai-service-metrics(Dynamo PodMonitor discovery),inference-gateway,robust-operator,pod-autoscaling(HPA ongpu_utilization);cluster-autoscalingexcluded (scales the shared cluster's nodegroups). First pass surfaced two prerequisite-shaped issues, both resolved by deploying the DGD:robust-operatorneeds a live DynamoGraphDeployment, andai-service-metricsexceeded the per-section timeout while self-deploying one (its evidence file read PASS while the section timed out — exactly the FAIL/exit-status inconsistency #1692 will formalize) |Cluster restored to the stock post-flip state afterward (compute-domain-only slices, slinky workload 2/2 with IMEX claims re-allocated).
Risk Assessment
Rollout notes: No behavior change for
aicr validateoutside--cncf-submission. DRA-mode clusters keep the previous evidence behavior (version-corrected); device-plugin clusters go from structurally-failing evidence to correct mode-aware evidence. Section FAIL still exits 0 (pre-existing; #1692).Checklist
make testwith-race)make lint)git commit -S)