Skip to content

Commit b6bde3b

Browse files
yuanchen8911yuanchen8911
committed
fix(bundler): harden undeploy.sh resilience and CRD discovery
Combines three related hardening changes to the generated undeploy.sh (previously split across NVIDIA#599, NVIDIA#601, NVIDIA#600). They share a file and a theme -- make undeploy recover from partial or transient failure -- so bundling them avoids three sequential PR rebases over the same template. 1. Transient-failure resilience (was NVIDIA#599). Three `kubectl | jq | while` pipelines in the post-uninstall cleanup path ran under `set -euo pipefail` with no fallback. A momentary control-plane 502 or auth refresh killed the whole script after Helm uninstalls succeeded, leaving orphan CRDs, webhooks, and `Active` namespaces. Wrap each pipeline's trailing `done` in `|| echo "Warning: ..." >&2` so the failure is visible but the script continues. Matches the `|| true` pattern already used in the adjacent ORPHANED_CRD_GROUPS loop. 2. Pre-flight CRD discovery via Helm annotation (was NVIDIA#601). check_release_for_stuck_crds sourced its CRD list only from `helm get manifest`, which returns CRDs under the chart's templates/ section but omits CRDs installed from the chart's crds/ directory (the Helm-recommended layout used by most operator charts). CRs backed by crds/-installed CRDs (e.g., NIMService) slipped past the finalizer pre-flight. Union both sources (manifest + annotated) and dedupe. Uses `awk 'NF'` instead of `grep -v '^$'` to drop empty lines without the grep-exits-1-on-no-match behavior, which would abort the script under pipefail when a release has no CRDs. 3. Post-flight Helm-annotation CRD check (was NVIDIA#600). Helm-managed CRDs that survive the per-release deletion loop went unnoticed until the next deploy.sh run rejected them. Add a verification pass at the end of undeploy.sh that re-enumerates CRDs filtered by meta.helm.sh/release-name + release-namespace annotations and surfaces any leftovers as a post-flight warning. CRDs already being deleted (non-null .metadata.deletionTimestamp) are excluded: the earlier `kubectl delete crd ... --wait=false` returns immediately, so a slow finalizer can leave the CRD listed though cleanup is normal. Only truly stuck (not-yet-terminating) CRDs are reported. Unit test TestUndeployScript_TransientFailureWarnsAndContinues generates a bundle, stubs kubectl to exit non-zero, sources each of the three helpers, and asserts the helper returns 0 and emits the expected Warning: on stderr. Fixes: N/A Related: NVIDIA#477 (introduced the per-component CRD cleanup pipeline), NVIDIA#599, NVIDIA#600, NVIDIA#601 (superseded by this PR). Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
1 parent b82af6f commit b6bde3b

2 files changed

Lines changed: 220 additions & 17 deletions

File tree

‎pkg/bundler/deployer/helm/helm_test.go‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,10 @@
1515
package helm
1616

1717
import (
18+
"bytes"
1819
"context"
1920
"os"
21+
"os/exec"
2022
"path/filepath"
2123
"strings"
2224
"testing"
@@ -2274,3 +2276,125 @@ func TestGenerate_DoesNotMutateComponentValues(t *testing.T) {
22742276
t.Error("original driver.version was mutated (removed) — deep copy is missing")
22752277
}
22762278
}
2279+
2280+
// TestUndeployScript_TransientFailureWarnsAndContinues asserts that the three
2281+
// post-uninstall cleanup pipelines tolerate a transient kubectl failure instead
2282+
// of letting set -euo pipefail kill the script.
2283+
//
2284+
// Sites covered (matching the warn-on-failure pattern added in this PR):
2285+
// - delete_release_cluster_resources (per-release per-kind cleanup helper)
2286+
// - force_clear_namespace_finalizers (last-resort namespace unstick helper)
2287+
// - per-component orphan-CRD cleanup loop in the script body
2288+
//
2289+
// Setup: stub `kubectl` to always exit non-zero (simulating a 502/timeout/auth
2290+
// hiccup). For each site, source the relevant section of the generated script,
2291+
// invoke it, and assert (a) the wrapper exits 0 — proving set -e was not
2292+
// triggered — and (b) the descriptive `Warning:` is on stderr — proving the
2293+
// failure was visible to the operator.
2294+
func TestUndeployScript_TransientFailureWarnsAndContinues(t *testing.T) {
2295+
if _, err := exec.LookPath("bash"); err != nil {
2296+
t.Skip("bash not available; skipping shell-behavior test")
2297+
}
2298+
if _, err := exec.LookPath("awk"); err != nil {
2299+
t.Skip("awk not available; skipping shell-behavior test")
2300+
}
2301+
if _, err := exec.LookPath("sed"); err != nil {
2302+
t.Skip("sed not available; skipping shell-behavior test")
2303+
}
2304+
2305+
ctx := context.Background()
2306+
outputDir := t.TempDir()
2307+
2308+
g := &Generator{
2309+
RecipeResult: createTestRecipeResult(),
2310+
ComponentValues: map[string]map[string]any{
2311+
"cert-manager": {},
2312+
"gpu-operator": {},
2313+
},
2314+
Version: "v1.0.0",
2315+
}
2316+
if _, err := g.Generate(ctx, outputDir); err != nil {
2317+
t.Fatalf("Generate failed: %v", err)
2318+
}
2319+
undeployPath := filepath.Join(outputDir, "undeploy.sh")
2320+
2321+
// Stub kubectl: `api-resources` succeeds with a minimal kind list (so the
2322+
// helpers reach the inner pipeline we want to exercise); every other
2323+
// invocation fails to simulate a transient API hiccup. Placed at the
2324+
// front of PATH so it shadows the real kubectl. jq is left alone — the
2325+
// pipelines pipe-fail at the kubectl stage either way.
2326+
stubDir := t.TempDir()
2327+
stubKubectl := filepath.Join(stubDir, "kubectl")
2328+
stubScript := "#!/bin/sh\n" +
2329+
"if [ \"$1\" = \"api-resources\" ]; then\n" +
2330+
" echo configmaps\n" +
2331+
" exit 0\n" +
2332+
"fi\n" +
2333+
"echo 'simulated transient API failure' >&2\n" +
2334+
"exit 1\n"
2335+
if err := os.WriteFile(stubKubectl, []byte(stubScript), 0o755); err != nil {
2336+
t.Fatalf("write kubectl stub: %v", err)
2337+
}
2338+
2339+
tests := []struct {
2340+
name string
2341+
bashSnippet string
2342+
wantStderr string
2343+
}{
2344+
{
2345+
// L97-L103 in template: the helper's outer pipeline must end in `done || echo "Warning: ..." >&2`.
2346+
// sed+eval (not `source <(awk ...)` process substitution) for portability
2347+
// across bash environments where <(...) is flaky.
2348+
name: "delete_release_cluster_resources",
2349+
bashSnippet: `
2350+
snippet=$(sed -n '/^delete_release_cluster_resources()/,/^}/p' "$UNDEPLOY")
2351+
eval "$snippet"
2352+
HELM_TIMEOUT=10
2353+
delete_release_cluster_resources "gpu-operator" "gpu-operator"
2354+
`,
2355+
wantStderr: "Warning: customresourcedefinitions cleanup pipeline for release gpu-operator/gpu-operator failed",
2356+
},
2357+
{
2358+
// L150-L154 in template: same pattern in the namespace finalizer-unstick helper.
2359+
name: "force_clear_namespace_finalizers",
2360+
bashSnippet: `
2361+
snippet=$(sed -n '/^force_clear_namespace_finalizers()/,/^}/p' "$UNDEPLOY")
2362+
eval "$snippet"
2363+
force_clear_namespace_finalizers "gpu-operator"
2364+
`,
2365+
wantStderr: "Warning: finalizer-clear pipeline for",
2366+
},
2367+
{
2368+
// L296-L302 in template: the per-Helm-component orphan-CRD loop in the script body.
2369+
// Extract from the section header through (but not including) the next section.
2370+
name: "orphan_crd_inline_loop",
2371+
bashSnippet: `
2372+
snippet=$(sed -n '/^# Clean up orphaned CRDs that were owned by this bundle/,/^# Clean up CRDs created by operators at runtime/p' "$UNDEPLOY" | sed '$d')
2373+
eval "$snippet"
2374+
`,
2375+
wantStderr: "Warning: orphan-CRD cleanup for",
2376+
},
2377+
}
2378+
2379+
for _, tt := range tests {
2380+
t.Run(tt.name, func(t *testing.T) {
2381+
cmd := exec.CommandContext(ctx, "bash", "-c", "set -euo pipefail\n"+tt.bashSnippet)
2382+
cmd.Env = append(os.Environ(),
2383+
"PATH="+stubDir+":"+os.Getenv("PATH"),
2384+
"UNDEPLOY="+undeployPath,
2385+
)
2386+
var stdout, stderr bytes.Buffer
2387+
cmd.Stdout = &stdout
2388+
cmd.Stderr = &stderr
2389+
err := cmd.Run()
2390+
if err != nil {
2391+
t.Fatalf("regression: cleanup pipeline killed the script with set -e instead of warning.\nerr: %v\nstdout: %s\nstderr: %s",
2392+
err, stdout.String(), stderr.String())
2393+
}
2394+
if !strings.Contains(stderr.String(), tt.wantStderr) {
2395+
t.Errorf("expected %q in stderr (proves operators get a visible signal on transient failure), got:\nstderr: %s",
2396+
tt.wantStderr, stderr.String())
2397+
}
2398+
})
2399+
}
2400+
}

‎pkg/bundler/deployer/helm/templates/undeploy.sh.tmpl‎

Lines changed: 96 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ delete_release_cluster_resources() {
100100
| while read -r name; do
101101
echo "Deleting ${kind}/${name}..."
102102
kubectl delete "${kind}" "${name}" --ignore-not-found --timeout="${HELM_TIMEOUT}s" || true
103-
done
103+
done || echo "Warning: ${kind} cleanup pipeline for release ${release}/${ns} failed (kubectl get / jq error); leftovers will surface in post-flight" >&2
104104
done
105105
}
106106

@@ -151,10 +151,26 @@ force_clear_namespace_finalizers() {
151151
| jq -r '.items[] | select(.metadata.finalizers // [] | length > 0) | .kind + "/" + .metadata.name' 2>/dev/null \
152152
| while read -r resource; do
153153
kubectl patch "${resource}" -n "${ns}" --type merge -p '{"metadata":{"finalizers":null}}' 2>/dev/null || true
154-
done
154+
done || echo "Warning: finalizer-clear pipeline for ${kind} in ${ns} failed (kubectl get / jq error); namespace may stay Terminating" >&2
155155
done
156156
}
157157

158+
# Emit the API groups owned by a release (space-separated; empty if release
159+
# has no known groups). Single source of truth used by both pre-flight CRD
160+
# discovery and post-flight orphan-group cleanup -- keeps the two paths in
161+
# sync when a new operator is added.
162+
release_groups() {
163+
case "$1" in
164+
kai-scheduler) echo "kai.scheduler scheduling.run.ai" ;;
165+
gpu-operator) echo "nvidia.com nfd.k8s-sigs.io" ;;
166+
dynamo-platform) echo "grove.io scheduler.grove.io" ;;
167+
kube-prometheus-stack) echo "monitoring.coreos.com" ;;
168+
k8s-nim-operator) echo "apps.nvidia.com" ;;
169+
kubeflow-trainer) echo "trainer.kubeflow.org jobset.x-k8s.io" ;;
170+
*) echo "" ;;
171+
esac
172+
}
173+
158174
# Check a single CRD for custom resource instances with active finalizers.
159175
# Appends details and remediation commands to the preflight temp files.
160176
# Args: $1 = CRD name, $2 = component name (for display)
@@ -188,16 +204,53 @@ check_crd_for_stuck_resources() {
188204
}
189205

190206
# Check a Helm release for CRDs whose custom resources have active finalizers.
191-
# Extracts CRD names from the release manifest and checks each one.
192-
# Silently skips releases that are not installed (helm get manifest fails).
193-
# Args: $1 = release name, $2 = namespace
207+
# Discovers CRDs from three complementary sources:
208+
# 1. The chart's templates/ section, via `helm get manifest`. Catches CRDs
209+
# installed via templated manifests (the common pattern for modern
210+
# operator charts).
211+
# 2. CRDs annotated with meta.helm.sh/release-name == this release. Catches
212+
# CRDs that carry Helm release metadata, including CRDs retained across
213+
# chart upgrades (helm.sh/resource-policy: keep) that may have been
214+
# removed from the current chart version but still exist in the cluster.
215+
# 3. CRDs whose API group matches an operator group owned by this release
216+
# (passed as $3 when the caller knows the mapping). This is the only
217+
# source that catches CRDs installed from the chart's crds/ directory
218+
# -- Helm's crds/ install path does NOT stamp meta.helm.sh/release-*
219+
# annotations, and crds/ CRDs never appear in `helm get manifest`, so
220+
# sources 1 and 2 miss them.
221+
# Silently skips releases that are not installed.
222+
# Args: $1 = release name, $2 = namespace, $3 = optional space-separated API
223+
# groups owned by this release (e.g., "nvidia.com nfd.k8s-sigs.io")
194224
check_release_for_stuck_crds() {
195225
local release="$1"
196226
local ns="$2"
197-
local manifest
227+
local groups="${3:-}"
228+
local manifest manifest_crds annotated_crds group_crds
198229
manifest=$(helm get manifest "${release}" -n "${ns}" 2>/dev/null) || return 0
199-
echo "${manifest}" \
200-
| awk '/^kind:/{kind=$2} /^ name:/ && kind=="CustomResourceDefinition"{print $2; kind=""}' \
230+
manifest_crds=$(echo "${manifest}" \
231+
| awk '/^kind:/{kind=$2} /^ name:/ && kind=="CustomResourceDefinition"{print $2; kind=""}')
232+
annotated_crds=$(kubectl get crd -o json 2>/dev/null \
233+
| jq -r --arg rel "${release}" --arg ns "${ns}" \
234+
'.items[] | select(.metadata.annotations["meta.helm.sh/release-name"]==$rel and .metadata.annotations["meta.helm.sh/release-namespace"]==$ns) | .metadata.name' 2>/dev/null || true)
235+
group_crds=""
236+
if [[ -n "${groups}" ]]; then
237+
for group in ${groups}; do
238+
local matched
239+
matched=$(kubectl get crd -o name 2>/dev/null \
240+
| grep "\.${group}$" \
241+
| sed 's|^customresourcedefinition\.apiextensions\.k8s\.io/||' || true)
242+
if [[ -n "${matched}" ]]; then
243+
group_crds+="${matched}"$'\n'
244+
fi
245+
done
246+
fi
247+
# awk 'NF' drops empty lines without the exit-1-on-no-match behavior of
248+
# `grep -v '^$'`, which would abort under `set -euo pipefail` when a release
249+
# has no CRDs (e.g., chart ships none, installed with --skip-crds, already
250+
# cleaned up).
251+
printf '%s\n%s\n%s\n' "${manifest_crds}" "${annotated_crds}" "${group_crds}" \
252+
| awk 'NF' \
253+
| sort -u \
201254
| while read -r crd_name; do
202255
check_crd_for_stuck_resources "${crd_name}" "${release}"
203256
done
@@ -219,7 +272,7 @@ else
219272

220273
{{ range .ComponentsReversed -}}
221274
{{ if .HasChart -}}
222-
check_release_for_stuck_crds "{{ .Name }}" "{{ .Namespace }}"
275+
check_release_for_stuck_crds "{{ .Name }}" "{{ .Namespace }}" "$(release_groups "{{ .Name }}")"
223276
{{ end -}}
224277
{{ end }}
225278
if [[ -s "${PREFLIGHT_DETAILS}" ]]; then
@@ -298,20 +351,22 @@ kubectl get crd -o json 2>/dev/null \
298351
'.items[] | select(.metadata.annotations["meta.helm.sh/release-name"]==$rel and .metadata.annotations["meta.helm.sh/release-namespace"]==$ns) | .metadata.name' 2>/dev/null \
299352
| while read -r name; do
300353
echo "Deleting CRD ${name} (owned by {{ .Name }}/{{ .Namespace }})..."
301-
kubectl delete crd "${name}" --ignore-not-found --wait=false
302-
done
354+
# Per-CRD `|| echo Warning:` keeps the loop making best-effort progress
355+
# across the remaining CRDs while surfacing each delete failure
356+
# (RBAC, timeout) with the specific CRD name -- post-flight's
357+
# Helm-annotation re-check catches any that still linger.
358+
kubectl delete crd "${name}" --ignore-not-found --wait=false \
359+
|| echo "Warning: failed to delete CRD ${name} (owned by {{ .Name }}/{{ .Namespace }}); leftovers will surface in post-flight" >&2
360+
done || echo "Warning: orphan-CRD cleanup for {{ .Name }}/{{ .Namespace }} failed (kubectl get / jq error); leftovers will surface in post-flight" >&2
303361
{{- end }}{{ end }}
304362

305363
# Clean up CRDs created by operators at runtime (not Helm-managed).
306364
# Only includes groups whose parent operator was part of this bundle.
365+
# Groups come from release_groups() so pre-flight and post-flight share one
366+
# mapping.
307367
ORPHANED_CRD_GROUPS=""
308368
{{- range .ComponentsReversed }}
309-
{{- if eq .Name "kai-scheduler" }} ORPHANED_CRD_GROUPS="${ORPHANED_CRD_GROUPS} kai.scheduler scheduling.run.ai"{{ end }}
310-
{{- if eq .Name "gpu-operator" }} ORPHANED_CRD_GROUPS="${ORPHANED_CRD_GROUPS} nvidia.com nfd.k8s-sigs.io"{{ end }}
311-
{{- if eq .Name "dynamo-platform" }} ORPHANED_CRD_GROUPS="${ORPHANED_CRD_GROUPS} grove.io scheduler.grove.io"{{ end }}
312-
{{- if eq .Name "kube-prometheus-stack" }} ORPHANED_CRD_GROUPS="${ORPHANED_CRD_GROUPS} monitoring.coreos.com"{{ end }}
313-
{{- if eq .Name "k8s-nim-operator" }} ORPHANED_CRD_GROUPS="${ORPHANED_CRD_GROUPS} apps.nvidia.com"{{ end }}
314-
{{- if eq .Name "kubeflow-trainer" }} ORPHANED_CRD_GROUPS="${ORPHANED_CRD_GROUPS} trainer.kubeflow.org jobset.x-k8s.io"{{ end }}
369+
ORPHANED_CRD_GROUPS="${ORPHANED_CRD_GROUPS} $(release_groups "{{ .Name }}")"
315370
{{- end }}
316371
for group in ${ORPHANED_CRD_GROUPS}; do
317372
crds=$(kubectl get crd -o name 2>/dev/null | grep "\.${group}$" || true)
@@ -434,6 +489,30 @@ if [[ -n "${orphaned_crds}" ]]; then
434489
postflight_issues=true
435490
fi
436491

492+
# Check for Helm-annotated CRDs from uninstalled releases.
493+
# Mirrors the per-component CRD deletion loop above: if cleanup succeeded,
494+
# every annotated CRD is gone and this check stays silent. Catches cases
495+
# where the deletion loop logged a transient kubectl/jq warning and moved on.
496+
# CRDs already being deleted (non-null .metadata.deletionTimestamp) are excluded --
497+
# the earlier `kubectl delete crd ... --wait=false` returns immediately, so a slow
498+
# finalizer can leave the CRD still listed here even though it is being cleaned up
499+
# normally. Only truly stuck (not-yet-terminating) CRDs should be surfaced.
500+
helm_orphaned_crds=""
501+
{{- range .ComponentsReversed }}{{ if .HasChart }}
502+
remaining_helm_crds=$(kubectl get crd -o json 2>/dev/null \
503+
| jq -r --arg rel "{{ .Name }}" --arg ns "{{ .Namespace }}" \
504+
'.items[] | select(.metadata.annotations["meta.helm.sh/release-name"]==$rel and .metadata.annotations["meta.helm.sh/release-namespace"]==$ns and .metadata.deletionTimestamp==null) | .metadata.name' 2>/dev/null || true)
505+
if [[ -n "${remaining_helm_crds}" ]]; then
506+
helm_orphaned_crds="${helm_orphaned_crds} ${remaining_helm_crds}"
507+
fi
508+
{{- end }}{{ end }}
509+
if [[ -n "${helm_orphaned_crds}" ]]; then
510+
echo "WARNING: Helm-annotated CRDs from uninstalled releases still present:${helm_orphaned_crds}"
511+
echo " Cleanup did not remove all CRDs owned by this bundle's releases."
512+
echo " Delete with: kubectl delete crd <name>"
513+
postflight_issues=true
514+
fi
515+
437516
if [[ "${postflight_issues}" == "true" ]]; then
438517
echo ""
439518
echo "Post-flight: some stale resources remain. Run deploy.sh pre-flight checks to verify before redeploying."

0 commit comments

Comments
 (0)