From 5895a7e8f94940bb3f2b2ac85e7b4af6a4e59f32 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 18 Dec 2025 09:19:45 +0000 Subject: [PATCH 1/3] 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 +} From d5080d71ef742eef0722416d257b9472deb1dc7b Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 18 Dec 2025 18:57:51 +0000 Subject: [PATCH 2/3] Handle deletion propagation defaults in uninstall action Signed-off-by: Evans Mungai --- pkg/action/uninstall.go | 12 +++++-- pkg/action/uninstall_test.go | 69 ++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index efbc72fef..212cccc43 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -276,12 +276,12 @@ func (u *Uninstall) deleteRelease(rel *release.Release) (kube.ResourceList, stri return nil, "", []error{fmt.Errorf("unable to build kubernetes objects for delete: %w", err)} } if len(resources) > 0 { - _, errs = u.cfg.KubeClient.Delete(resources, parseCascadingFlag(u.DeletionPropagation)) + _, errs = u.cfg.KubeClient.Delete(resources, u.parseCascadingFlag(u.DeletionPropagation)) } return resources, kept.String(), errs } -func parseCascadingFlag(cascadingFlag string) v1.DeletionPropagation { +func (u *Uninstall) parseCascadingFlag(cascadingFlag string) v1.DeletionPropagation { switch cascadingFlag { case "orphan": return v1.DeletePropagationOrphan @@ -290,6 +290,14 @@ func parseCascadingFlag(cascadingFlag string) v1.DeletionPropagation { case "background": return v1.DeletePropagationBackground default: + // When --wait is used with a non-hookOnly strategy (i.e. watcher or legacy), 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 u.WaitStrategy != kube.HookOnlyStrategy { + slog.Debug("uninstall: defaulting to foreground deletion due to wait strategy", "wait strategy", u.WaitStrategy, "cascading flag", cascadingFlag) + return v1.DeletePropagationForeground + } slog.Debug("uninstall: given cascade value, defaulting to delete propagation background", "value", cascadingFlag) return v1.DeletePropagationBackground } diff --git a/pkg/action/uninstall_test.go b/pkg/action/uninstall_test.go index fba1e391f..e2e895dbe 100644 --- a/pkg/action/uninstall_test.go +++ b/pkg/action/uninstall_test.go @@ -169,3 +169,72 @@ func TestUninstallRun_UnreachableKubeClient(t *testing.T) { assert.Nil(t, result) assert.ErrorContains(t, err, "connection refused") } + +func TestParseCascadingFlag(t *testing.T) { + tests := []struct { + name string + cascadingFlag string + waitStrategy kube.WaitStrategy + expectedResult string + }{ + { + name: "explicit orphan", + cascadingFlag: "orphan", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Orphan", + }, + { + name: "explicit foreground", + cascadingFlag: "foreground", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Foreground", + }, + { + name: "explicit background", + cascadingFlag: "background", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Background", + }, + { + name: "default with hookOnly strategy returns background", + cascadingFlag: "", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Background", + }, + { + name: "default with watcher strategy returns foreground", + cascadingFlag: "", + waitStrategy: kube.StatusWatcherStrategy, + expectedResult: "Foreground", + }, + { + name: "default with legacy strategy returns foreground", + cascadingFlag: "", + waitStrategy: kube.LegacyStrategy, + expectedResult: "Foreground", + }, + { + name: "unknown value with hookOnly strategy returns background", + cascadingFlag: "unknown", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Background", + }, + { + name: "unknown value with watcher strategy returns foreground", + cascadingFlag: "unknown", + waitStrategy: kube.StatusWatcherStrategy, + expectedResult: "Foreground", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + unAction := uninstallAction(t) + unAction.WaitStrategy = tt.waitStrategy + + result := unAction.parseCascadingFlag(tt.cascadingFlag) + + assert.Equal(t, tt.expectedResult, string(result)) + }) + } +} From 5f449cbaed6128184d2783f1d634f2afedb80588 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 18 Dec 2025 19:03:26 +0000 Subject: [PATCH 3/3] Explict check of wait strategy in uninstall action Signed-off-by: Evans Mungai --- pkg/action/uninstall.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index 212cccc43..0af47045d 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -290,11 +290,11 @@ func (u *Uninstall) parseCascadingFlag(cascadingFlag string) v1.DeletionPropagat case "background": return v1.DeletePropagationBackground default: - // When --wait is used with a non-hookOnly strategy (i.e. watcher or legacy), default to foreground + // When --wait is used with watcher or legacy strategy, default to foreground // deletion to ensure dependent resources are cleaned up before returning. - // This prevents the issue where helm uninstall --wait returns before + // This prevents the issue where uninstallation with wait strategy returns before // all resources (including controller-created dependents) are fully deleted. - if u.WaitStrategy != kube.HookOnlyStrategy { + if u.WaitStrategy == kube.StatusWatcherStrategy || u.WaitStrategy == kube.LegacyStrategy { slog.Debug("uninstall: defaulting to foreground deletion due to wait strategy", "wait strategy", u.WaitStrategy, "cascading flag", cascadingFlag) return v1.DeletePropagationForeground }