From 5895a7e8f94940bb3f2b2ac85e7b4af6a4e59f32 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 18 Dec 2025 09:19:45 +0000 Subject: [PATCH] fix: default to foreground cascade when --wait is used in uninstall When `helm uninstall --wait` is called, the default deletion propagation is now "foreground" instead of "background". This ensures dependent resources (like Pods, ReplicaSets created by controllers) are fully cleaned up before the command returns. Previously, with background deletion, parent resources were deleted immediately while garbage collection handled dependents asynchronously. This caused subsequent namespace deletions to hang for 2+ minutes waiting for those dependent resources to be cleaned up. With foreground deletion, Kubernetes waits for all dependents to be deleted before removing the parent resource, ensuring `--wait` truly waits for all cleanup to complete. Cascade behavior by command: | Command | Cascade | |----------------------------------------------|------------| | `helm uninstall release` | background | | `helm uninstall release --wait` | foreground | | `helm uninstall release --wait=hookOnly` | background | | `helm uninstall release --wait --cascade=background` | background | Users can still explicitly set `--cascade=background` to restore the previous behavior if needed. Fixes #31651 Signed-off-by: Evans Mungai --- pkg/cmd/uninstall.go | 14 ++- pkg/cmd/uninstall_test.go | 120 ++++++++++++++++++++++++- pkg/kube/fake/capturing_kube_client.go | 39 ++++++++ 3 files changed, 168 insertions(+), 5 deletions(-) create mode 100644 pkg/kube/fake/capturing_kube_client.go diff --git a/pkg/cmd/uninstall.go b/pkg/cmd/uninstall.go index 4cc14ae1e..8e38fbed8 100644 --- a/pkg/cmd/uninstall.go +++ b/pkg/cmd/uninstall.go @@ -25,6 +25,7 @@ import ( "helm.sh/helm/v4/pkg/action" "helm.sh/helm/v4/pkg/cmd/require" + "helm.sh/helm/v4/pkg/kube" ) const uninstallDesc = ` @@ -50,11 +51,20 @@ func newUninstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { ValidArgsFunction: func(_ *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return compListReleases(toComplete, args, cfg) }, - RunE: func(_ *cobra.Command, args []string) error { + RunE: func(cmd *cobra.Command, args []string) error { validationErr := validateCascadeFlag(client) if validationErr != nil { return validationErr } + + // When --wait is used with a non-hookOnly strategy, default to foreground + // deletion to ensure dependent resources are cleaned up before returning. + // This prevents the issue where helm uninstall --wait returns before + // all resources (including controller-created dependents) are fully deleted. + if !cmd.Flags().Changed("cascade") && client.WaitStrategy != kube.HookOnlyStrategy { + client.DeletionPropagation = "foreground" + } + for i := range args { res, err := client.Run(args[i]) @@ -76,7 +86,7 @@ func newUninstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.BoolVar(&client.DisableHooks, "no-hooks", false, "prevent hooks from running during uninstallation") f.BoolVar(&client.IgnoreNotFound, "ignore-not-found", false, `Treat "release not found" as a successful uninstall`) f.BoolVar(&client.KeepHistory, "keep-history", false, "remove all associated resources and mark the release as deleted, but retain the release history") - f.StringVar(&client.DeletionPropagation, "cascade", "background", "Must be \"background\", \"orphan\", or \"foreground\". Selects the deletion cascading strategy for the dependents. Defaults to background.") + f.StringVar(&client.DeletionPropagation, "cascade", "background", "Must be \"background\", \"orphan\", or \"foreground\". Selects the deletion cascading strategy for the dependents. Defaults to background, or foreground if --wait is used.") f.DurationVar(&client.Timeout, "timeout", 300*time.Second, "time to wait for any individual Kubernetes operation (like Jobs for hooks)") f.StringVar(&client.Description, "description", "", "add a custom description") AddWaitFlag(cmd, &client.WaitStrategy) diff --git a/pkg/cmd/uninstall_test.go b/pkg/cmd/uninstall_test.go index ce436e68c..d4096a0c5 100644 --- a/pkg/cmd/uninstall_test.go +++ b/pkg/cmd/uninstall_test.go @@ -17,10 +17,19 @@ limitations under the License. package cmd import ( + "bytes" + "io" "testing" - "helm.sh/helm/v4/pkg/release/common" + "github.com/stretchr/testify/assert" + + "helm.sh/helm/v4/pkg/action" + "helm.sh/helm/v4/pkg/chart/common" + kubefake "helm.sh/helm/v4/pkg/kube/fake" + releasecommon "helm.sh/helm/v4/pkg/release/common" release "helm.sh/helm/v4/pkg/release/v1" + "helm.sh/helm/v4/pkg/storage" + "helm.sh/helm/v4/pkg/storage/driver" ) func TestUninstall(t *testing.T) { @@ -63,8 +72,8 @@ func TestUninstall(t *testing.T) { cmd: "uninstall aeneas --keep-history", golden: "output/uninstall-keep-history-earlier-deployed.txt", rels: []*release.Release{ - release.Mock(&release.MockReleaseOptions{Name: "aeneas", Version: 1, Status: common.StatusDeployed}), - release.Mock(&release.MockReleaseOptions{Name: "aeneas", Version: 2, Status: common.StatusFailed}), + release.Mock(&release.MockReleaseOptions{Name: "aeneas", Version: 1, Status: releasecommon.StatusDeployed}), + release.Mock(&release.MockReleaseOptions{Name: "aeneas", Version: 2, Status: releasecommon.StatusFailed}), }, }, { @@ -83,6 +92,111 @@ func TestUninstall(t *testing.T) { runTestCmd(t, tests) } +// TestUninstallCascadeDefaultWithWait tests that when --wait is used, +// the default cascade behavior changes to foreground to ensure +// dependent resources are cleaned up before returning. +func TestUninstallCascadeDefaultWithWait(t *testing.T) { + tests := []struct { + name string + cmd string + expectedDeletionPropagation string + }{ + { + name: "without --wait uses background cascade", + cmd: "uninstall test-release", + expectedDeletionPropagation: "Background", + }, + { + name: "--wait without --cascade uses foreground cascade", + cmd: "uninstall test-release --wait", + expectedDeletionPropagation: "Foreground", + }, + { + name: "--wait=watcher without --cascade uses foreground cascade", + cmd: "uninstall test-release --wait=watcher", + expectedDeletionPropagation: "Foreground", + }, + { + name: "--wait=legacy without --cascade uses foreground cascade", + cmd: "uninstall test-release --wait=legacy", + expectedDeletionPropagation: "Foreground", + }, + { + name: "--wait=hookOnly uses background cascade (default)", + cmd: "uninstall test-release --wait=hookOnly", + expectedDeletionPropagation: "Background", + }, + { + name: "--wait with explicit --cascade=background respects user setting", + cmd: "uninstall test-release --wait --cascade=background", + expectedDeletionPropagation: "Background", + }, + { + name: "--wait with explicit --cascade=orphan respects user setting", + cmd: "uninstall test-release --wait --cascade=orphan", + expectedDeletionPropagation: "Orphan", + }, + { + name: "--wait with explicit --cascade=foreground respects user setting", + cmd: "uninstall test-release --wait --cascade=foreground", + expectedDeletionPropagation: "Foreground", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a capturing KubeClient to verify the deletion propagation + capturedDeletionPropagation := "" + store := storage.Init(driver.NewMemory()) + rel := release.Mock(&release.MockReleaseOptions{Name: "test-release"}) + if err := store.Create(rel); err != nil { + t.Fatal(err) + } + + capturingClient := &kubefake.CapturingKubeClient{ + FailingKubeClient: kubefake.FailingKubeClient{ + PrintingKubeClient: kubefake.PrintingKubeClient{Out: io.Discard}, + BuildDummy: true, + }, + CapturedDeletionPropagation: &capturedDeletionPropagation, + } + + actionConfig := &action.Configuration{ + Releases: store, + KubeClient: capturingClient, + Capabilities: common.DefaultCapabilities, + } + + buf := new(bytes.Buffer) + args := splitCmd(tt.cmd) + + root, err := newRootCmdWithConfig(actionConfig, buf, args, SetupLogging) + if err != nil { + t.Fatal(err) + } + + root.SetArgs(args) + root.SetOut(buf) + root.SetErr(buf) + + _ = root.Execute() + + // Verify the captured deletion propagation + assert.Equal(t, tt.expectedDeletionPropagation, capturedDeletionPropagation, + "Expected DeletionPropagation to be %q but got %q", tt.expectedDeletionPropagation, capturedDeletionPropagation) + }) + } +} + +func splitCmd(cmd string) []string { + // Simple split by space - doesn't handle quoted strings + result := []string{} + for _, part := range bytes.Fields([]byte(cmd)) { + result = append(result, string(part)) + } + return result +} + func TestUninstallCompletion(t *testing.T) { checkReleaseCompletion(t, "uninstall", true) } diff --git a/pkg/kube/fake/capturing_kube_client.go b/pkg/kube/fake/capturing_kube_client.go new file mode 100644 index 000000000..947020ff2 --- /dev/null +++ b/pkg/kube/fake/capturing_kube_client.go @@ -0,0 +1,39 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fake + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "helm.sh/helm/v4/pkg/kube" +) + +// CapturingKubeClient is a fake KubeClient that captures internal flags such as the +// deletion propagation policy. It extends FailingKubeClient to provide dummy resources. +type CapturingKubeClient struct { + FailingKubeClient + // CapturedDeletionPropagation stores the DeletionPropagation value passed to Delete + CapturedDeletionPropagation *string +} + +// Delete captures the deletion propagation policy and returns success +func (c *CapturingKubeClient) Delete(resources kube.ResourceList, policy metav1.DeletionPropagation) (*kube.Result, []error) { + if c.CapturedDeletionPropagation != nil { + *c.CapturedDeletionPropagation = string(policy) + } + return &kube.Result{Deleted: resources}, nil +}