Skip to content

Commit 96535f9

Browse files
committed
fix(cleanup): backstop check namespaces; fence out-of-band installs
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>
1 parent da4787c commit 96535f9

6 files changed

Lines changed: 418 additions & 16 deletions

File tree

DEVELOPMENT.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -579,6 +579,26 @@ aws ec2 reboot-instances --instance-ids <ids>
579579
kubectl uncordon <gpu-node>
580580
```
581581

582+
### Excluding Out-of-Band Installs from Cleanup
583+
584+
On shared or bring-up clusters, a registry component may be deliberately
585+
installed and owned outside AICR (for example, nodewright/skyhook installed
586+
out-of-band by the platform team, with the AICR recipe intentionally excluding
587+
it). Stock `tools/cleanup` would otherwise destroy such an install three ways:
588+
the Helm uninstall in its namespace, the CRD pattern match, and the namespace
589+
deletion. Fence it out of all three phases:
590+
591+
```bash
592+
tools/cleanup --exclude-ns skyhook --exclude-crd skyhook.nvidia.com
593+
```
594+
595+
Both flags are repeatable and accept comma-separated values. `--exclude-ns`
596+
protects a namespace from Helm uninstall and namespace deletion; `--exclude-crd`
597+
protects CRDs whose name contains the given match from deletion, applied *after*
598+
pattern matching so a broad pattern (`nvidia.com`) cannot pull an excluded
599+
group's CRDs (`skyhook.nvidia.com`) back in. Run with `--dry-run` first to
600+
confirm what will and will not be removed.
601+
582602
### Debugging Tests
583603

584604
```bash

Makefile

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,8 +216,12 @@ license-check: ## Check license is approved
216216
--ignore=github.com/hashicorp/hcl \
217217
--ignore=$$STDLIB_IGNORE
218218

219+
.PHONY: test-shell
220+
test-shell: ## Runs shell unit tests (tools/*_test.sh; hermetic, no cluster)
221+
@set -e; for t in tools/*_test.sh; do [ -e "$$t" ] || continue; echo "Running $$t..."; bash "$$t"; done
222+
219223
.PHONY: test
220-
test: ## Runs unit tests with race detector and coverage (use -short to skip integration tests)
224+
test: test-shell ## Runs unit tests with race detector and coverage (use -short to skip integration tests)
221225
@set -e; \
222226
echo "Running tests with race detector..."; \
223227
KUBEBUILDER_ASSETS=$$(setup-envtest use -p path 2>/dev/null || echo "") \

tools/cleanup

Lines changed: 121 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,18 @@
1919
# 1. Helm releases in known AICR component namespaces
2020
# 2. `aicr validate` artifacts (namespace, SA, CRB, leftover Jobs/CMs)
2121
# 3. Custom Resource Definitions owned by AICR components
22-
# 4. AICR component namespaces (with stuck-finalizer rescue)
22+
# 4. AICR component + check-created namespaces (with stuck-finalizer rescue)
2323
#
2424
# Scope is intentionally narrow: only namespaces and CRDs known to be installed
25-
# by AICR registry components are touched. Cluster-scoped resources outside the
26-
# AICR component domains are left alone.
25+
# by AICR registry components (plus namespaces created by conformance checks)
26+
# are touched. Cluster-scoped resources outside the AICR component domains are
27+
# left alone.
28+
#
29+
# On shared / bring-up clusters a registry component may be deliberately owned
30+
# out-of-band (installed by the platform team, excluded from the AICR recipe).
31+
# Destroying it is data loss AICR does not own, so `--exclude-ns` and
32+
# `--exclude-crd` fence such installs out of the Helm, CRD, and namespace
33+
# phases (phase 2 only touches aicr-labelled validator artifacts). See #1672.
2734

2835
set -euo pipefail
2936

@@ -35,6 +42,10 @@ DRY_RUN=false
3542
ASSUME_YES=false
3643
KEEP_CRDS=false
3744
KEEP_NAMESPACES=false
45+
# Namespaces / CRD-name substrings the operator has fenced out of every phase
46+
# (out-of-band or platform-owned installs). Populated by --exclude-ns/--exclude-crd.
47+
EXCLUDE_NS=()
48+
EXCLUDE_CRD=()
3849

3950
usage() {
4051
cat <<EOF
@@ -43,20 +54,50 @@ Usage: tools/cleanup [options]
4354
Removes AICR-installed Kubernetes resources from the currently selected cluster.
4455
4556
Options:
46-
-n, --dry-run Print actions without executing
47-
-y, --yes Skip confirmation prompt
48-
--keep-crds Don't delete CRDs (useful when reinstalling same version)
49-
--keep-namespaces Don't delete component namespaces
50-
-h, --help Show this help
57+
-n, --dry-run Print actions without executing
58+
-y, --yes Skip confirmation prompt
59+
--keep-crds Don't delete CRDs (useful when reinstalling same version)
60+
--keep-namespaces Don't delete component namespaces
61+
--exclude-ns <ns> Protect a namespace from Helm uninstall and namespace
62+
deletion (repeatable; comma-separated values accepted)
63+
--exclude-crd <match> Protect CRDs whose name contains <match> from deletion
64+
(repeatable; comma-separated values accepted)
65+
-h, --help Show this help
66+
67+
Exclusions fence out-of-band / platform-owned installs that AICR does not own.
68+
Example: nodewright/skyhook installed out-of-band, excluded from the recipe:
69+
tools/cleanup --exclude-ns skyhook --exclude-crd skyhook.nvidia.com
5170
EOF
5271
}
5372

73+
# split_csv_append <array-name> <value>: append comma-separated tokens of <value>
74+
# to the named array, trimming surrounding whitespace and skipping empties so
75+
# `--exclude-ns 'skyhook, gpu-operator'` protects BOTH (not ' gpu-operator',
76+
# which would never match and silently leave gpu-operator unprotected).
77+
split_csv_append() {
78+
local __arr="$1" __val="$2" __tok
79+
local __ifs="$IFS"
80+
IFS=','
81+
for __tok in $__val; do
82+
__tok="${__tok#"${__tok%%[![:space:]]*}"}" # strip leading whitespace
83+
__tok="${__tok%"${__tok##*[![:space:]]}"}" # strip trailing whitespace
84+
[[ -n "$__tok" ]] && eval "${__arr}+=(\"\$__tok\")"
85+
done
86+
IFS="$__ifs"
87+
}
88+
5489
while [[ $# -gt 0 ]]; do
5590
case "$1" in
5691
-n|--dry-run) DRY_RUN=true; shift ;;
5792
-y|--yes) ASSUME_YES=true; shift ;;
5893
--keep-crds) KEEP_CRDS=true; shift ;;
5994
--keep-namespaces) KEEP_NAMESPACES=true; shift ;;
95+
--exclude-ns)
96+
[[ $# -ge 2 && "$2" != -* ]] || err "--exclude-ns requires a namespace argument"
97+
split_csv_append EXCLUDE_NS "$2"; shift 2 ;;
98+
--exclude-crd)
99+
[[ $# -ge 2 && "$2" != -* ]] || err "--exclude-crd requires a match argument"
100+
split_csv_append EXCLUDE_CRD "$2"; shift 2 ;;
60101
-h|--help) usage; exit 0 ;;
61102
*) err "unknown flag: $1" ;;
62103
esac
@@ -94,6 +135,13 @@ AICR_NAMESPACES=(
94135
topograph
95136
)
96137

138+
# Namespaces created at runtime by AICR conformance checks (not registry
139+
# components). These are torn down by the check itself, but an interrupted run
140+
# leaves them behind, so cleanup deletes them as a backstop. See #1672.
141+
AICR_CHECK_NAMESPACES=(
142+
gang-scheduling-test
143+
)
144+
97145
# CRD API-group suffixes owned by AICR components. Matched as substrings
98146
# against `kubectl get crd -o name`. New component groups should be added here.
99147
AICR_CRD_PATTERNS=(
@@ -122,6 +170,23 @@ msg "Target cluster context: ${ctx}"
122170
if $DRY_RUN; then
123171
msg "DRY-RUN mode — no changes will be applied."
124172
fi
173+
if (( ${#EXCLUDE_NS[@]} > 0 )); then
174+
msg "Excluding namespaces (Helm + namespace deletion): ${EXCLUDE_NS[*]}"
175+
fi
176+
if (( ${#EXCLUDE_CRD[@]} > 0 )); then
177+
msg "Excluding CRDs matching: ${EXCLUDE_CRD[*]}"
178+
fi
179+
# --exclude-ns and --exclude-crd are almost always needed as a pair: protecting
180+
# only the namespace still lets Phase 3 delete the install's cluster-scoped CRDs
181+
# (cascade-deleting its CRs), and protecting only the CRDs still deletes the
182+
# namespace. Warn on an asymmetric invocation so a half-fenced install is not
183+
# silently destroyed.
184+
if (( ${#EXCLUDE_NS[@]} > 0 )) && (( ${#EXCLUDE_CRD[@]} == 0 )); then
185+
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)."
186+
fi
187+
if (( ${#EXCLUDE_CRD[@]} > 0 )) && (( ${#EXCLUDE_NS[@]} == 0 )); then
188+
log_warning "--exclude-crd given without --exclude-ns: Phase 1/4 will still uninstall Helm releases in and delete the install's namespace. Add --exclude-ns."
189+
fi
125190

126191
# Detect whether gpu-operator manages the NVIDIA kernel driver on this
127192
# cluster (driver.enabled=true — e.g. EKS; GKE COS is host-managed and
@@ -219,11 +284,41 @@ is_aicr_namespace() {
219284
return 1
220285
}
221286

287+
# is_excluded_ns <namespace>: true when the operator fenced this namespace out
288+
# with --exclude-ns.
289+
is_excluded_ns() {
290+
local target="$1" ns
291+
(( ${#EXCLUDE_NS[@]} == 0 )) && return 1
292+
for ns in "${EXCLUDE_NS[@]}"; do
293+
[[ "$target" == "$ns" ]] && return 0
294+
done
295+
return 1
296+
}
297+
298+
# is_excluded_crd <crd-name>: true when the CRD name contains any --exclude-crd
299+
# match. Applied after pattern matching, so a broad pattern (e.g. nvidia.com)
300+
# cannot pull an excluded group's CRDs (e.g. skyhook.nvidia.com) back in.
301+
is_excluded_crd() {
302+
local target="$1" pat
303+
(( ${#EXCLUDE_CRD[@]} == 0 )) && return 1
304+
for pat in "${EXCLUDE_CRD[@]}"; do
305+
[[ "$target" == *"$pat"* ]] && return 0
306+
done
307+
return 1
308+
}
309+
310+
# All namespaces cleanup owns: registry components plus check-created backstops.
311+
ALL_NAMESPACES=("${AICR_NAMESPACES[@]}" "${AICR_CHECK_NAMESPACES[@]}")
312+
222313
# Phase 1: Uninstall Helm releases in AICR namespaces.
223314
msg "Phase 1: Uninstalling Helm releases in AICR namespaces..."
224315
if command -v helm >/dev/null 2>&1; then
225316
while IFS=$'\t' read -r release ns; do
226317
[[ -z "$release" || -z "$ns" ]] && continue
318+
if is_excluded_ns "$ns"; then
319+
msg " skip (excluded ns): helm release ${release} -n ${ns}"
320+
continue
321+
fi
227322
if is_aicr_namespace "$ns"; then
228323
msg " helm uninstall ${release} -n ${ns}"
229324
hm uninstall "$release" -n "$ns" --wait --timeout 2m
@@ -267,12 +362,22 @@ if ! $KEEP_CRDS; then
267362
for pat in "${AICR_CRD_PATTERNS[@]}"; do
268363
echo " match CRDs against pattern: ${pat}"
269364
done
365+
for pat in "${EXCLUDE_CRD[@]:+${EXCLUDE_CRD[@]}}"; do
366+
echo " excluding CRDs matching: ${pat}"
367+
done
270368
else
271369
all_crds="$(kubectl get crd -o name --request-timeout=30s 2>/dev/null || true)"
272370
for pat in "${AICR_CRD_PATTERNS[@]}"; do
273371
# shellcheck disable=SC2086
274372
matches=$(echo "$all_crds" | grep -F "$pat" || true)
275373
for crd in $matches; do
374+
# Fence out operator-owned CRDs. Checked here (post-match) so a
375+
# broad pattern (nvidia.com) cannot pull an excluded group's
376+
# CRDs (skyhook.nvidia.com) back in.
377+
if is_excluded_crd "$crd"; then
378+
msg " skip (excluded): $crd"
379+
continue
380+
fi
276381
# --request-timeout bounds the API call so a wedged apiserver
277382
# cannot park this step indefinitely. Don't swallow failures:
278383
# a timed-out delete leaves a stale CRD that breaks reinstall,
@@ -289,8 +394,12 @@ fi
289394

290395
# Phase 4: Namespaces.
291396
if ! $KEEP_NAMESPACES; then
292-
msg "Phase 4: Deleting AICR component namespaces..."
293-
for ns in "${AICR_NAMESPACES[@]}"; do
397+
msg "Phase 4: Deleting AICR component + check namespaces..."
398+
for ns in "${ALL_NAMESPACES[@]}"; do
399+
if is_excluded_ns "$ns"; then
400+
msg " skip (excluded ns): ${ns}"
401+
continue
402+
fi
294403
if $DRY_RUN; then
295404
echo " kubectl delete ns ${ns} --ignore-not-found --wait=false"
296405
else
@@ -305,7 +414,8 @@ if ! $KEEP_NAMESPACES; then
305414
# finish first, then strip finalizers from residual namespaced objects.
306415
if ! $DRY_RUN; then
307416
sleep 10
308-
for ns in "${AICR_NAMESPACES[@]}"; do
417+
for ns in "${ALL_NAMESPACES[@]}"; do
418+
is_excluded_ns "$ns" && continue
309419
kubectl get ns "$ns" >/dev/null 2>&1 || continue
310420
phase="$(kubectl get ns "$ns" -o jsonpath='{.status.phase}' 2>/dev/null || echo "")"
311421
[[ "$phase" != "Terminating" ]] && continue

0 commit comments

Comments
 (0)