feat: harden deletion with timeout, namespace cleanup

Author: vrdc-sap

config/rbac/role.yaml

@@ -24,6 +24,7 @@ rules:
   - namespaces
   verbs:
   - create
+  - delete
   - get
   - list
   - watch

internal/controller/gpu_controller.go

@@ -16,6 +16,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -47,6 +48,7 @@ import (
 
 const (
 	requeueWarn          = 30 * time.Second
+	deleteTimeout        = 3 * time.Minute
 	finalizer            = "gpu.kyma-project.io/gpu-operator"
 	gpuOperatorNamespace = "gpu-operator"
 	driverAppLabel       = "nvidia-driver-daemonset"
@@ -92,7 +94,7 @@ type GpuReconciler struct {
 // +kubebuilder:rbac:groups=gpu.kyma-project.io,resources=gpus/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=gpu.kyma-project.io,resources=gpus/finalizers,verbs=update
 // +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch
-// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch;create
+// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch;create;delete
 // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups="",resources=serviceaccounts;configmaps,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=apps,resources=deployments;daemonsets,verbs=get;list;watch;create;update;patch;delete
@@ -318,13 +320,43 @@ func (r *GpuReconciler) reconcileDelete(ctx context.Context, gpu *gpuv1beta1.Gpu
 	}
 
 	// Uninstall is idempotent - returns nil if the release is already gone.
-	if err := r.Installer.Uninstall(ctx); err != nil {
+	if err := r.Installer.Uninstall(ctx, deleteTimeout); err != nil {
+		if errors.Is(err, context.DeadlineExceeded) {
+			logger.Error(err, "helm uninstall timed out, forcing finalizer removal; manual cleanup of gpu-operator namespace may be required")
+			if nsErr := r.deleteNamespace(ctx, gpuOperatorNamespace); nsErr != nil {
+				logger.Error(nsErr, "failed to delete namespace after helm timeout")
+			}
+			return r.removeFinalizer(ctx, gpu.Name)
+		}
 		return ctrl.Result{}, fmt.Errorf("helm uninstall: %w", err)
 	}
 
-	// Re-read after the status apply above bumped ResourceVersion.
+	// Helm never deletes namespaces; clean it up explicitly with foreground propagation
+	// so all child resources are gone before the namespace itself disappears.
+	if err := r.deleteNamespace(ctx, gpuOperatorNamespace); err != nil {
+		return ctrl.Result{}, err
+	}
+
+	return r.removeFinalizer(ctx, gpu.Name)
+}
+
+func (r *GpuReconciler) deleteNamespace(ctx context.Context, name string) error {
+	ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: name}}
+	if err := r.Delete(ctx, ns, client.PropagationPolicy(metav1.DeletePropagationForeground)); err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		return fmt.Errorf("deleting namespace %s: %w", name, err)
+	}
+	return nil
+}
+
+func (r *GpuReconciler) removeFinalizer(ctx context.Context, name string) (ctrl.Result, error) {
 	live := &gpuv1beta1.Gpu{}
-	if err := r.Get(ctx, types.NamespacedName{Name: gpu.Name}, live); err != nil {
+	if err := r.Get(ctx, types.NamespacedName{Name: name}, live); err != nil {
+		if apierrors.IsNotFound(err) {
+			return ctrl.Result{}, nil
+		}
 		return ctrl.Result{}, fmt.Errorf("re-fetching Gpu CR for finalizer removal: %w", err)
 	}
 	controllerutil.RemoveFinalizer(live, finalizer)

internal/controller/gpu_controller_test.go

@@ -17,6 +17,8 @@ package controller
 import (
 	"context"
 	"errors"
+	"fmt"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -47,7 +49,7 @@ func (f *fakeInstaller) InstallOrUpgrade(_ context.Context, _ []byte, _ map[stri
 	return f.installErr
 }
 
-func (f *fakeInstaller) Uninstall(_ context.Context) error {
+func (f *fakeInstaller) Uninstall(_ context.Context, _ time.Duration) error {
 	f.uninstallCalled = true
 	return f.uninstallErr
 }
@@ -464,22 +466,22 @@ var _ = Describe("GpuReconciler", func() {
 	})
 
 	Describe("deletion", func() {
-		It("calls Helm uninstall and removes the finalizer", func() {
+		BeforeEach(func() {
 			newGpu(gpuName)
 			_, _ = reconciler.Reconcile(ctx, req) // adds finalizer
 			newGpuNode("gpu-node-del", "g4dn.xlarge", "Garden Linux 1312.3")
 			DeferCleanup(deleteNode, "gpu-node-del")
 			_, err := reconciler.Reconcile(ctx, req)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(installer.installCalls).To(Equal(1))
+		})
 
-			By("deleting the CR")
+		It("calls Helm uninstall and removes the finalizer on success", func() {
 			gpu := &gpuv1beta1.Gpu{}
 			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: gpuName}, gpu)).To(Succeed())
 			Expect(k8sClient.Delete(ctx, gpu)).To(Succeed())
 
-			By("reconciling the deletion")
-			_, err = reconciler.Reconcile(ctx, req)
+			_, err := reconciler.Reconcile(ctx, req)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(installer.uninstallCalled).To(BeTrue())
 
@@ -489,6 +491,88 @@ var _ = Describe("GpuReconciler", func() {
 				Expect(gpu.Finalizers).NotTo(ContainElement(finalizer))
 			}
 		})
+
+		It("returns an error and keeps the finalizer on non-timeout Helm failure", func() {
+			installer.uninstallErr = errors.New("simulated uninstall failure")
+
+			gpu := &gpuv1beta1.Gpu{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: gpuName}, gpu)).To(Succeed())
+			Expect(k8sClient.Delete(ctx, gpu)).To(Succeed())
+
+			_, err := reconciler.Reconcile(ctx, req)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("helm uninstall"))
+
+			// Finalizer must still be present so the CR is not lost.
+			gpu = &gpuv1beta1.Gpu{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: gpuName}, gpu)).To(Succeed())
+			Expect(gpu.Finalizers).To(ContainElement(finalizer))
+		})
+
+		It("force-removes the finalizer when Helm uninstall times out", func() {
+			installer.uninstallErr = fmt.Errorf("uninstalling gpu-operator: %w", context.DeadlineExceeded)
+
+			ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: gpuOperatorNamespace}}
+			err := k8sClient.Create(ctx, ns)
+			if err != nil {
+				Expect(err.Error()).To(ContainSubstring("already exists"))
+			}
+
+			gpu := &gpuv1beta1.Gpu{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: gpuName}, gpu)).To(Succeed())
+			Expect(k8sClient.Delete(ctx, gpu)).To(Succeed())
+
+			_, err = reconciler.Reconcile(ctx, req)
+			Expect(err).NotTo(HaveOccurred(), "timeout must force-remove finalizer, not block the CR forever")
+
+			// Namespace cleanup must be attempted even on timeout.
+			liveNs := &corev1.Namespace{}
+			err = k8sClient.Get(ctx, types.NamespacedName{Name: gpuOperatorNamespace}, liveNs)
+			if err == nil {
+				Expect(liveNs.DeletionTimestamp).NotTo(BeNil(), "namespace should be terminating even after timeout")
+			}
+
+			gpu = &gpuv1beta1.Gpu{}
+			err = k8sClient.Get(ctx, types.NamespacedName{Name: gpuName}, gpu)
+			if err == nil {
+				Expect(gpu.Finalizers).NotTo(ContainElement(finalizer))
+			}
+		})
+
+		It("deletes the gpu-operator namespace after successful Helm uninstall", func() {
+			ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: gpuOperatorNamespace}}
+			err := k8sClient.Create(ctx, ns)
+			if err != nil {
+				// Namespace may already exist (e.g. terminating from a prior test) - that's fine,
+				// we just need it to be present so deleteNamespace can act on it.
+				Expect(err.Error()).To(ContainSubstring("already exists"))
+			}
+
+			gpu := &gpuv1beta1.Gpu{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: gpuName}, gpu)).To(Succeed())
+			Expect(k8sClient.Delete(ctx, gpu)).To(Succeed())
+
+			_, err = reconciler.Reconcile(ctx, req)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(installer.uninstallCalled).To(BeTrue(), "Uninstall must be called before namespace cleanup")
+
+			liveNs := &corev1.Namespace{}
+			err = k8sClient.Get(ctx, types.NamespacedName{Name: gpuOperatorNamespace}, liveNs)
+			// Either deleted (NotFound) or marked for deletion (DeletionTimestamp set).
+			if err == nil {
+				Expect(liveNs.DeletionTimestamp).NotTo(BeNil(), "namespace should be terminating")
+			}
+		})
+
+		It("ignores NotFound when deleting the gpu-operator namespace", func() {
+			// Namespace does not exist - deleteNamespace must not return an error.
+			gpu := &gpuv1beta1.Gpu{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: gpuName}, gpu)).To(Succeed())
+			Expect(k8sClient.Delete(ctx, gpu)).To(Succeed())
+
+			_, err := reconciler.Reconcile(ctx, req)
+			Expect(err).NotTo(HaveOccurred())
+		})
 	})
 })
 

internal/helm/installer.go

@@ -18,6 +18,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"time"
 
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart"
@@ -71,8 +72,11 @@ func (c *Client) InstallOrUpgrade(ctx context.Context, chartData []byte, values
 	return upgrade(ctx, cfg, chrt, values)
 }
 
-// Uninstall removes the NVIDIA GPU Operator Helm release.
-func (c *Client) Uninstall(_ context.Context) error {
+// Uninstall removes the NVIDIA GPU Operator Helm release. It waits for pods to
+// terminate up to timeout so a rapid reinstall does not race with the old driver unloading.
+// ctx cancellation is not honored: Helm's action.Uninstall.Run does not accept a context,
+// so the call blocks until the driver pods are gone or u.Timeout expires.
+func (c *Client) Uninstall(_ context.Context, timeout time.Duration) error {
 	cfg, err := c.actionConfig()
 	if err != nil {
 		return err
@@ -87,7 +91,8 @@ func (c *Client) Uninstall(_ context.Context) error {
 	}
 
 	u := action.NewUninstall(cfg)
-	u.Wait = false
+	u.Wait = true
+	u.Timeout = timeout
 	if _, err := u.Run(releaseName); err != nil {
 		return fmt.Errorf("uninstalling gpu-operator: %w", err)
 	}

internal/helm/interface.go

@@ -14,7 +14,10 @@ limitations under the License.
 
 package helm
 
-import "context"
+import (
+	"context"
+	"time"
+)
 
 // Installer is the contract the controller uses to drive Helm operations.
 // The concrete *Client talks to a real cluster; tests inject a fake.
@@ -23,6 +26,6 @@ type Installer interface {
 	InstallOrUpgrade(ctx context.Context, chartData []byte, values map[string]any) error
 
 	// Uninstall removes the GPU Operator Helm release. It is idempotent: returns nil
-	// if no release exists.
-	Uninstall(ctx context.Context) error
+	// if no release exists. timeout bounds how long the uninstall waits for pods to terminate.
+	Uninstall(ctx context.Context, timeout time.Duration) error
 }