fix(recipes): installer health check must assert a label Helm 4 keeps - #2444
fix(recipes): installer health check must assert a label Helm 4 keeps#2444atif1996 wants to merge 3 commits into
Conversation
atif1996
added
the
theme/validation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe GCP driver installer DaemonSet now includes the Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change corrects the installer health check’s label assertion and reduces its timeout so expected resources complete within the Job deadline; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
Coverage ReportCoverage data unavailable for this run. This can happen if the qualification workflow was cancelled or failed before generating coverage artifacts. |
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Review — multi-persona + adversarial meta-review
Method: three parallel persona reviewers (Correctness/Domain · Timeout-safety/Operability · Test-coverage/Consistency), then a senior meta-reviewer that independently re-derived every finding from the resolved code. Anchored to head 8d515e05.
Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
Correct and safe to merge — approve with comments. The core fix was independently reproduced on all three legs:
- Subset match works. The assert does a single-Get by name, then kyverno-json subset-matches the whole spec including
metadata.labels. Assertingpart-of: aicrpasses on the deployed object even though it also carries the SSA-stampedmanaged-by: Helm; the oldmanaged-by: aicrassert could never match a Helm-installed bundle. - The stall mechanism is real. A label mismatch on an existing DaemonSet returns a shape-mismatch (
ErrCodeInternal), which disables the 30s absent-grace — so the bad assert burned its full budget rather than fast-failing (the #2186 shape). - Migration defense preserved. Google's hand-applied upstream installer carries neither
part-of: aicrnormanaged-by: aicr, so a leftover unmanaged DaemonSet still cannot false-pass.part-ofsits on mutablemetadata.labels, not the immutablespec.selector→ no upgrade conflict.
No blockers, no majors. One worthwhile hardening ask and two prose/coverage nits (inline).
Confirmed non-issues (examined, not flagged)
managed-by: aicris now a permanently-dead label (Helm rewrites it toHelmat install). Harmless, documented inline, matches repo convention; no consumer selects this DaemonSet bymanaged-by; the PR body already flags a repo-wide follow-up sweep. No action for this PR.- No ownership/prune/undeploy path selects driver-installer by
managed-byorpart-of(searchedrecipes/ pkg/ validators/ cmd/). - Status asserts (
desiredNumberScheduled > 0,numberReady == desiredNumberScheduled) unchanged and sound; BOM unaffected (label, not image-bearing); registry wiring intact.
Tier table
🔴 Blocker 0 | 🟠 Major 0 | 🟡 Minor 1 | 🔵 Nitpick 2 Recommendation: Approve with comments
| # Helm 4 server-side apply rewrites managed-by to "Helm", | ||
| # so that key can never match on a deployed bundle | ||
| # (verified live on GKE, 2026-08-28). | ||
| app.kubernetes.io/part-of: aicr |
There was a problem hiding this comment.
🟡 Minor — The part-of label ↔ assert-key contract has no regression test
This fix works only because the manifest emits app.kubernetes.io/part-of: aicr (nvidia-driver-installer.yaml:71) and this assert keys on exactly that. But the only render test, recipes/gcp_driver_installer_render_test.go, pins tolerations + the render gate and never inspects labels. If a future edit drops or renames the manifest label — or reverts this assert to managed-by — the check silently returns to the never-match bug this PR fixes, and every existing test stays green. Because it is SSA-runtime behavior, only live GKE validation caught it originally.
Blast radius: Silent regression to a never-true assert → the #2186 "pod not found" failure shape on GKE bundle-installer validation, invisible to CI.
Fix: Add a render/registry test asserting the rendered nvidia-driver-installer DaemonSet carries app.kubernetes.io/part-of: aicr (the exact key asserted here). Note: pkg/bundler/validations/nvsentinel_driver_label_test.go is not a precedent — it guards a config-value gate (labeler.assumeDriverInstalled), not a rendered-manifest label; this would be net-new hardening.
There was a problem hiding this comment.
Added in 21f2dca — TestGCPDriverInstallerLabelContract pins both sides: the rendered DaemonSet must carry part-of: aicr, the health check must assert exactly that key, and asserting managed-by at all fails the test with a message explaining the SSA rewrite. Folded your budget-pin suggestion into it per the third thread.
| metadata: | ||
| name: gcp-driver-installer-health-check | ||
| spec: | ||
| # 5m: below the expected-resources Job's 8m activeDeadline — a budget |
There was a problem hiding this comment.
🔵 Nitpick — Timeout comment describes the pre-cap world (effective change is 6m→5m, not 10m→5m)
The comment (and PR body) frame this as "10m exceeds the Job's 8m activeDeadline → deadline-kill (#2186 shape)." But the in-process executor already caps any authored budget at the 6m caller budget: effectiveTimeout = min(ChainsawAssertTimeout=6m, authored) (pkg/chainsaw/inprocess.go:139-152, already on main). So the old 10m was already clamped to 6m — below the 8m deadline — and the effective change here is 6m→5m. The change is still correct and safe (5m < 6m < 8m, matches sibling checks), and the label fix is what actually removes the stall; only the causal chain in the comment is stale.
Blast radius: None functional — rationale-comment accuracy only. A future reader may over-trust "10m exceeds 8m" as the live mechanism.
Fix: Soften to: 5m fails a genuinely-not-ready DaemonSet ~1m sooner (freeing a worker slot) and stays consistent with sibling checks — #2186 being the historical motivation, not current post-cap behavior.
There was a problem hiding this comment.
Right — verified the clamp at pkg/chainsaw/inprocess.go (effectiveTimeout = min(caller 6m, authored)), so the old 10m never applied as written. The comment (21f2dca) now states the accurate chain: the never-true assert burned the full clamped budget and the Job's aggregate run crossed its 8m activeDeadline; the label fix is what removed the stall, and 5m keeps the worst case inside both bounds with a real assert diff. PR body Motivation holds the observed behavior but I've kept the mechanism correction in the check comment where a future reader needs it.
| # fresh node mid driver-install) completes in ~2m. | ||
| timeouts: | ||
| assert: 10m | ||
| assert: 5m |
There was a problem hiding this comment.
🔵 Nitpick — No budget-pin test, asymmetric with the sibling #2186 fix
pkg/chainsaw/nvsentinel_budget_test.go pins nvsentinel's assert budget ≤ 2m so an edit can't silently reintroduce the stall. This PR fixes the same #2186 shape for gcp-driver-installer but adds no equivalent pin.
Blast radius: Low — the 6m in-process cap means a revert to 10m couldn't reproduce a single-check deadline-kill anyway, so a pin would guard little.
Fix: If you add the label-contract test from the Minor finding above, that is strictly higher-value coverage — fold this into it rather than adding a separate budget pin.
There was a problem hiding this comment.
Folded into the label-contract test per your suggestion rather than a separate budget pin — the 6m clamp means a budget revert alone can't reproduce the deadline kill, so the contract test is the coverage that matters.
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Re-Review — all prior findings addressed ✅
Method: delta re-review. I reviewed at 8d515e05 (3 inline findings); commit 21f2dca9 "test: pin the health-check label contract; correct the budget rationale" landed since. I dispositioned each prior finding against the resolved code at head 21f2dca9 and swept the delta for net-new issues.
Prior-feedback status
| # | Prior finding | Disposition | Evidence |
|---|---|---|---|
| 1 | 🟡 Minor — part-of label ↔ assert-key contract has no regression test | ✔️ Addressed | TestGCPDriverInstallerLabelContract pins both sides — rendered DaemonSet must carry part-of: aicr, the check must assert exactly that key, and asserting managed-by at all fails the test with an SSA-rewrite explanation. go test ./recipes/ passes. |
| 2 | 🔵 Nitpick — timeout comment describes the pre-cap world | ✔️ Addressed | Comment rewritten to the accurate chain. Verified independently: ChainsawAssertTimeout = 6*time.Minute and inprocess.go effectiveTimeout only shortens the caller budget, so the old authored 10m never applied. |
| 3 | 🔵 Nitpick — no budget-pin test, asymmetric with sibling #2186 fix | ✔️ Addressed (alt) | Folded into the label-contract test rather than a standalone budget pin. Sound: with the label key pinned and the 6m clamp, a budget revert alone can't reproduce the #2186 deadline kill here — unlike nvsentinel, whose DaemonSets stall forever at 0-desired, which is why its check pins a 2m ceiling. The asymmetry is legitimate. |
Delta sweep
No net-new findings. The new TestGCPDriverInstallerLabelContract is well-constructed: single-doc render, correct steps→try→assert→resource→metadata→labels walk, unmarshalErr avoids shadowing, and it fails closed both ways (missing part-of and present managed-by).
Notes (no action needed)
- Sub-nitpick residual on #3: the literal
assert: 5mvalue isn't itself pinned, but per the (correct) rationale it's no longer load-bearing for the failure mode. - I couldn't run
golangci-lintlocally (pinned binary built with go1.26 vs config targeting go1.27 — environment skew, not a code defect); the added Go is idiomatic andgo testbuilds clean. Worth a green CI lint job before merge.
🔴 0 | 🟠 0 | 🟡 0 | 🔵 0 — all 3 prior findings ✔️ Addressed; 0 net-new
Approving — every prior finding is resolved against the current code, the fix is backed by a real regression test pinning the exact contract live GKE validation surfaced, and the delta introduces nothing new.
… check Live validation on GKE caught it: Helm 4 server-side apply rewrites app.kubernetes.io/managed-by to "Helm" at install, so the managed-by: aicr assert could never match a deployed bundle. The manifest now also carries app.kubernetes.io/part-of: aicr — which SSA leaves alone, verified on-cluster — and the health check asserts that instead. The migration-defense intent is unchanged: a hand-applied installer DaemonSet carries neither label. Signed-off-by: Atif Mahmood <atif1996@users.noreply.github.com>
…dline A 10m assert budget exceeds the expected-resources Job's 8m activeDeadline, converting any never-true assert into a deadline kill with no surviving logs (the #2186 failure shape, reproduced live while diagnosing the managed-by assert). 5m fails fast with the real assert diff; the healthy path passes in seconds and a fresh node mid driver-install completes in ~2m. Signed-off-by: Atif Mahmood <atif1996@users.noreply.github.com>
The label-contract test asserts both sides: the rendered DaemonSet carries part-of: aicr and the health check keys on exactly that label, never managed-by (which Helm 4 SSA rewrites, silently regressing to the never-match stall). The budget comment now reflects the in-process 6m clamp: the old 10m never applied as written — the never-true assert burned the clamped budget and the Job aggregate crossed its 8m deadline. Signed-off-by: Atif Mahmood <atif1996@users.noreply.github.com>
|
Rebased onto current main ( |
21f2dca to
dc2981f
Compare
2 participants
Summary
Two fixes to the
gcp-driver-installerhealth check found during the live GKE H100 validation of #2360 (merged before these could be appended): the migration-defense label assert keys onapp.kubernetes.io/part-of: aicrinstead ofmanaged-by: aicr, and the check's assert budget drops from 10m to 5m, below the expected-resources Job's 8m activeDeadline.Motivation / Context
Live validation on a GKE COS H100 cluster (
gtprjpor-dgxc-k8s-gcp-ams-dev1, europe-west4) caught a compound failure the unit and render tests could not see:app.kubernetes.io/managed-bytoHelmat install (the generatedinstall.shdetects Helm 4 via--force-conflicts). The health check'smanaged-by: aicrassert — added for the migration defense in the feat(recipes)!: GKE bundle-installer replaces driver-installer #2360 review — can therefore never match a deployed bundle. Verified on-cluster: the deployed DaemonSet carriesmanaged-by: Helm; after this fix it also carriespart-of: aicr, which SSA leaves alone. The migration-defense intent is unchanged — a hand-applied installer DaemonSet carries neither label.pkg/chainsaw/inprocess.go), pushing the expected-resources Job's aggregate run past its 8m activeDeadline — the exact validate: DeadlineExceeded validator reports 'pod not found' status=other, hiding real assert failures #2186 failure shape the nvsentinel check documents: the Job is deadline-killed, the pod is deleted, and the report shows "no pod remains for it, so its logs are unavailable" instead of the assert diff. Confirmed live: 13/14 component health checks passed in 3s and the pod stalled on this one until the deadline. With both fixes,expected-resourcespasses in seconds.Fixes: N/A
Related: #2360, #1716, #2186
Type of Change
Component(s) Affected
recipes/checks/gcp-driver-installer/,recipes/components/gcp-driver-installer/)Implementation Notes
The manifest keeps
managed-by: aicr(repo convention; several other manifests set it) and addspart-of: aicrwith a comment explaining the Helm 4 rewrite; only the assert moves topart-of. Note the same silent rewrite affects every other manifest that setsmanaged-by: aicr(e.g.nvidia-peermem-reloader) — harmless there since nothing asserts on it, but worth a follow-up sweep.Testing
Live-cluster validation (aicr built from this branch)
Environment: fresh DGXC-blueprint GKE cluster
gtprjpor-dgxc-k8s-gcp-ams-dev1(europe-west4, GKE 1.34.10), 1×a3-megagpu-8g(8× H100) on COS 125 (19216.532.62), GPU pool provisioned withgke-no-default-nvidia-gpu-device-plugin=trueand no GKE-managed driver, node taintednvidia.com/gpu=present:NoSchedule, no hand-applied installer present. Flow:aicr snapshot→recipe --profile gpuStack=bundle-installer→bundle→ deploy all 15 components →aicr validate.driver-loaded: falsegpuDriverState: absentpersisted, no driver-mismatch warning580.173.02against the COS curated list, installs and verifies the drivernvidia-gpu-device-plugin-*at 0 desired); GPU Operator's plugin sole advertiser,nvidia.com/gpu: 8; CUDA validator CompletedassumeDriverInstalled=falsepath)expected-resources— all 14 component health checks, incl. this check assertingpart-of: aicragainst the deployed DaemonSetEnvironmental residuals on this 1-node, infra-only cluster (both fail with clean deterministic messages, unrelated to this fix):
nccl-all-reduce-bwneeds ≥2 GPU nodes;gke-gpu-nic-networksneeds the multi-networkNetworkobjects the DGXC runtime creates. Same report posted on #2360 for the feature-PR record.Risk Assessment
Rollout notes: N/A — check-content only; no version pins moved (BOM unaffected).
Checklist
make testwith-race)make lint)git commit -S)