fix(cleanup): backstop check namespaces; fence out-of-band installs#1773
Conversation
lockwobr
added
the
theme/ci-dx
|
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:
📝 WalkthroughWalkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Coverage Report ✅
Coverage BadgeCoverage unchanged by this PR. |
fc47888 to
b351f69
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tools/cleanup`:
- Around line 73-96: Add automated shell regression coverage for the cleanup
script’s exclusion handling, including repeatable comma-separated --exclude-ns
and --exclude-crd parsing. Exercise exclusions across Helm, CRD,
namespace/finalizer, and --dry-run paths, verifying excluded resources are
preserved and non-excluded resources are processed.
In `@validators/conformance/gang_scheduling_check.go`:
- Around line 441-446: Give the namespace deletion in the gang-scheduling
cleanup flow its own bounded timeout context instead of reusing the potentially
exhausted cleanupCtx. Create and cancel the dedicated context immediately around
the Namespaces().Delete call, while preserving the existing IgnoreNotFound
handling and deletion options.
🪄 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: 41fe1202-452d-4559-b26c-2b11b686d078
📒 Files selected for processing (4)
DEVELOPMENT.mdtools/cleanupvalidators/conformance/gang_scheduling_check.govalidators/conformance/gang_scheduling_check_test.go
b351f69 to
2fbcfee
Compare
|
Addressed both CodeRabbit findings in the latest push:
Also filed #1791 to capture a larger follow-up discussed on this PR: rewriting |
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 `@tools/cleanup`:
- Around line 73-83: Update split_csv_append to trim leading and trailing
whitespace from each comma-separated token before appending it, and reject
tokens that are empty after trimming. Ensure comma-only and whitespace-only
values produce no exclusions and do not permit cleanup to proceed with
ineffective filters.
In `@tools/cleanup_test.sh`:
- Around line 99-100: Extend tools/cleanup_test.sh beyond the dry-run assertion
around phase3-excluded-crd-echoed by adding a hermetic --yes test with stateful
kubectl stubs. Exercise both is_excluded_crd and the finalizer-rescue skip
branches, then assert excluded resources receive neither delete nor patch calls
while preserving the existing dry-run check.
🪄 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: 495016bc-19c3-421e-8851-f12737a19840
📒 Files selected for processing (6)
DEVELOPMENT.mdMakefiletools/cleanuptools/cleanup_test.shvalidators/conformance/gang_scheduling_check.govalidators/conformance/gang_scheduling_check_test.go
9e46d1e to
91460ab
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 `@Makefile`:
- Around line 219-229: Update the test-shell recipe to stay within checkmake’s
five-line maxbodylength limit by compacting the shell loop or extracting it into
a separate script/target. Preserve its behavior of discovering tools/*_test.sh
files, skipping missing matches, printing each test name, and executing each
with bash; keep the test target’s dependency on test-shell unchanged.
🪄 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: 1cc3c533-4161-48cf-ae3f-ddc6d76164ce
📒 Files selected for processing (6)
DEVELOPMENT.mdMakefiletools/cleanuptools/cleanup_test.shvalidators/conformance/gang_scheduling_check.govalidators/conformance/gang_scheduling_check_test.go
96535f9 to
ffa4fd9
Compare
tools/cleanup missed the gang-scheduling-test namespace and had no way to protect out-of-band / operator-owned installs, both hit during a live cluster reset. Namespace leak (two-sided): - The gang-scheduling conformance check now deletes its own gang-scheduling-test namespace once pods and the PodGroup are gone. The namespace delete uses its own bounded deadline rather than the shared cleanup context, so a pod stuck on a finalizer in the earlier waits cannot starve the teardown (the leak this fixes). - tools/cleanup lists gang-scheduling-test in a new AICR_CHECK_NAMESPACES backstop, merged into phase 4 for interrupted runs. Exclusions for out-of-band installs: - New repeatable, comma-separated --exclude-ns and --exclude-crd flags. --exclude-ns skips phase 1 Helm uninstall and phase 4 namespace deletion + finalizer rescue; --exclude-crd skips matching CRDs in phase 3, applied after pattern matching so the broad nvidia.com pattern cannot pull skyhook.nvidia.com CRDs back in. - Warn on an asymmetric invocation (only one of the pair), since fencing just the namespace still lets phase 3 cascade-delete the install's CRs. Testing: - tools/cleanup_test.sh: hermetic shell unit test (stubs kubectl/helm on PATH, drives --dry-run) covering CSV/repeat parsing, per-phase exclusion, the backstop, asymmetric-flag warnings, and fail-closed arg handling. Wired into 'make test' via a new test-shell target. - Unit test covering the gang-scheduling namespace teardown. - Documented the flags in DEVELOPMENT.md. Fixes #1672 Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
ffa4fd9 to
bea0fc6
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Overall assessment — ✅ Approve with comments
Multi-persona review (shell-correctness, data-loss-safety, go-correctness), each finding independently confirmed or refuted by a senior meta-reviewer against the resolved code at head bea0fc6.
Adds --exclude-ns/--exclude-crd fencing to tools/cleanup, a hermetic shell-test harness wired into make test, a check-namespace backstop, and fixes the gang-scheduling check to actually delete its namespace and report the true outcome (uses pkg/errors + a fresh deadline for teardown per repo conventions). The eval in split_csv_append is injection-safe ("$__tok" defers to a variable ref), empty-array expansions are guarded under set -u, and arg parsing fails closed. Both persona findings describe real behavior but are correctly nitpick-grade.
✅ Confirmed non-issues (examined and cleared)
evalsafety —eval "${__arr}+=(\"\$__tok\")"is injection-safe;$__tokis a deferred variable reference, not interpolated.set -u+ empty arrays —EXCLUDE_CRD[@]:+guard present; no unbound-variable abort on the no-exclusion path.- Fencing completeness — helm uninstall (Phase 1), CRD delete (Phase 3, applied post-pattern so
nvidia.comcan't pullskyhook.nvidia.comback), and namespace delete + finalizer rescue (Phase 4) are all gated byis_excluded_ns/is_excluded_crd. - Gang namespace delete — single bounded background-propagation call on a fresh
K8sCleanupTimeoutdeadline; correctpkg/errorswrap; new test exercises delete +IsNotFound.
Summary
| Tier | Count |
|---|---|
| 🔴 Blocker | 0 |
| 🟠 Major | 0 |
| 🟡 Minor | 0 |
| 🔵 Nitpick | 2 |
Recommendation: Approve with comments. Neither nitpick blocks merge.
| local __arr="$1" __val="$2" __tok | ||
| local __ifs="$IFS" | ||
| IFS=',' | ||
| for __tok in $__val; do |
There was a problem hiding this comment.
🔵 Nitpick — CSV split glob-expands tokens (no set -f)
for __tok in $__val with IFS=',' suppresses whitespace splitting but not pathname/glob expansion, and the script runs set -euo pipefail with no set -f. A token with glob metacharacters (--exclude-ns 'skyhook,*') expands against cwd, or unmatched stays literal * and silently protects nothing. Real but non-triggering in practice: Kubernetes namespace / CRD-group names never contain glob chars. The eval itself is injection-safe.
Blast radius: Exclusion parsing only; cannot trigger with valid k8s names — direction of error is over-exclusion (safe) or a no-op exclusion.
Fix: IFS=',' read -ra __toks <<<"$__val" (which does not glob) and iterate "${__toks[@]}", or wrap the loop in set -f/set +f.
| # namespace. Warn on an asymmetric invocation so a half-fenced install is not | ||
| # silently destroyed. | ||
| if (( ${#EXCLUDE_NS[@]} > 0 )) && (( ${#EXCLUDE_CRD[@]} == 0 )); then | ||
| log_warning "--exclude-ns given without --exclude-crd: Phase 3 will still delete the install's CRDs (and cascade its CRs). Add --exclude-crd for its API group(s)." |
There was a problem hiding this comment.
🔵 Nitpick — Asymmetric --exclude-ns/--exclude-crd warns but still destroys under --yes
cleanup -y --exclude-ns skyhook (no --exclude-crd) emits only log_warning and continues, so Phase 3 still deletes the skyhook CRDs (cascade-deleting its CRs); under -y there is no prompt, so the warning scrolls past in automation. This is intentional, documented behavior (the warning names exactly what will still be destroyed, and DEVELOPMENT.md states the flags are almost always needed as a pair) — hard-failing would break the legitimate additive-protection model. Flagged as a hardening option, not a defect.
Blast radius: Only when an operator supplies one of the paired flags in non-interactive mode.
Fix: Optionally fail closed on asymmetric invocation behind an explicit --allow-asymmetric-exclusion, or force a confirmation prompt (ignore -y) when exclusions are asymmetric.
2 participants
Summary
Stops
tools/cleanupfrom missing thegang-scheduling-testnamespace and from destroying out-of-band / operator-owned installs during a live cluster reset.Motivation / Context
Two gaps surfaced during a live cluster reset:
gang-scheduling-testnamespace that survived a "complete" cleanup: the Go check retained it intentionally, and cleanup's namespace list did not include it.tools/cleanuphad no way to protect a registry component that is deliberately installed and owned outside AICR (in our case nodewright/skyhook, installed out-of-band by the platform team, excluded from the AICR recipe). Stock cleanup destroyed such an install three ways: Helm uninstall in its namespace, CRD pattern match (the broadnvidia.compattern also matchesskyhook.nvidia.com), and namespace deletion. That is data loss for an install AICR does not own.Fixes: #1672
Related: N/A
Type of Change
Component(s) Affected
pkg/validator)docs/,examples/)tools/cleanup,validators/conformanceImplementation Notes
Namespace leak (two-sided fix):
validators/conformance/gang_scheduling_check.go:cleanupGangTestResourcesnow deletes thegang-scheduling-testnamespace after pods and the PodGroup are gone. It is a single bounded background-propagation call underdefaults.K8sCleanupTimeout, so cleanup stays bounded.tools/cleanup: newAICR_CHECK_NAMESPACES=(gang-scheduling-test)list, merged into phase 4 as a backstop for interrupted runs.Exclusions for out-of-band installs:
--exclude-ns <ns>and--exclude-crd <match>flags.--exclude-nsskips phase 1 (Helm uninstall) and phase 4 (namespace deletion + finalizer rescue).--exclude-crdskips matching CRDs in phase 3, applied after pattern matching so the broadnvidia.compattern cannot pullskyhook.nvidia.comCRDs back in.--dry-runfor a pre-flight preview.The prior workaround now reduces to:
Testing
Manual
--dry-runverification: exclusions echoed and applied in phases 1/3/4; excluded namespace skipped while thegang-scheduling-testbackstop is still deleted; asymmetric-flag warnings fire; missing/flag-shaped values are rejected; the no-flag path runs cleanly underset -u(macOS bash 3.2).Not run locally: the full
make qualify(e2e + Grype scan need cluster/network access unavailable in this environment). The only Go surface touched isvalidators/conformance, covered above with-raceand lint;tools/cleanupis a bash script covered by shellcheck and dry-run exercises. A new unit test (gang_scheduling_check_test.go) covers the namespace teardown, raising coverage on the previously-untested cleanup path.Risk Assessment
Rollout notes: Backwards compatible. Default behavior is unchanged except that phase 4 now also deletes
gang-scheduling-test(a check-created namespace) and the gang check tears down its own namespace. New flags are opt-in.Checklist
make testwith-race) — rango test -race ./validators/conformance/; fullmake testnot run (see Testing)make lint) — rangolangci-linton the affected package +shellcheck; fullmake lintnot run (see Testing)git commit -S)