From a886f77392f5d5346b5f21bb12335b9223a65dcf Mon Sep 17 00:00:00 2001 From: Vanja Radic Date: Fri, 29 May 2026 09:13:15 +0200 Subject: [PATCH 1/3] feat: harden deletion with timeout, namespace cleanup, and force-remove on stuck uninstall --- config/rbac/role.yaml | 1 + internal/controller/gpu_controller.go | 40 +++++++++-- internal/controller/gpu_controller_test.go | 80 ++++++++++++++++++++-- internal/helm/installer.go | 9 ++- internal/helm/interface.go | 9 ++- 5 files changed, 124 insertions(+), 15 deletions(-) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 34050c6..373a2f6 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -24,6 +24,7 @@ rules: - namespaces verbs: - create + - delete - get - list - watch diff --git a/internal/controller/gpu_controller.go b/internal/controller/gpu_controller.go index c385e66..2027054 100644 --- a/internal/controller/gpu_controller.go +++ b/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,39 @@ 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 isTimeoutError(err) { + // Helm timed out waiting for pods to terminate. Force-remove the finalizer + // so the CR can be deleted. The user must clean up the gpu-operator namespace manually. + logger.Error(err, "helm uninstall timed out, forcing finalizer removal; manual cleanup of gpu-operator namespace may be required") + 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 { return ctrl.Result{}, fmt.Errorf("re-fetching Gpu CR for finalizer removal: %w", err) } controllerutil.RemoveFinalizer(live, finalizer) @@ -334,6 +362,10 @@ func (r *GpuReconciler) reconcileDelete(ctx context.Context, gpu *gpuv1beta1.Gpu return ctrl.Result{}, nil } +func isTimeoutError(err error) bool { + return errors.Is(err, context.DeadlineExceeded) +} + // statusUpdate carries the partial status the reconciler wants to write in this cycle. // Only the conditions listed here are applied; absent ones are left untouched on the // live object. operatorVersion and driver are applied only when non-zero / non-nil. diff --git a/internal/controller/gpu_controller_test.go b/internal/controller/gpu_controller_test.go index f65e48f..e2e7738 100644 --- a/internal/controller/gpu_controller_test.go +++ b/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,7 +466,7 @@ 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") @@ -472,14 +474,14 @@ var _ = Describe("GpuReconciler", func() { _, 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,74 @@ 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) + + 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") + + 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()) + + 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()) + }) }) }) diff --git a/internal/helm/installer.go b/internal/helm/installer.go index f183812..bb1638f 100644 --- a/internal/helm/installer.go +++ b/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,9 @@ 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. +func (c *Client) Uninstall(_ context.Context, timeout time.Duration) error { cfg, err := c.actionConfig() if err != nil { return err @@ -87,7 +89,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) } diff --git a/internal/helm/interface.go b/internal/helm/interface.go index cb013bf..f0b898b 100644 --- a/internal/helm/interface.go +++ b/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 } From a23153895430f1d4838470fa5c3f2649d0ba6aba Mon Sep 17 00:00:00 2001 From: Vanja Radic Date: Fri, 29 May 2026 11:11:13 +0200 Subject: [PATCH 2/3] address deletion edge cases from code review --- internal/controller/gpu_controller.go | 8 ++++++-- internal/controller/gpu_controller_test.go | 16 +++++++++++++++- internal/helm/installer.go | 2 ++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/internal/controller/gpu_controller.go b/internal/controller/gpu_controller.go index 2027054..eca9d75 100644 --- a/internal/controller/gpu_controller.go +++ b/internal/controller/gpu_controller.go @@ -322,9 +322,10 @@ 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, deleteTimeout); err != nil { if isTimeoutError(err) { - // Helm timed out waiting for pods to terminate. Force-remove the finalizer - // so the CR can be deleted. The user must clean up the gpu-operator namespace manually. 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) @@ -353,6 +354,9 @@ func (r *GpuReconciler) deleteNamespace(ctx context.Context, name string) error func (r *GpuReconciler) removeFinalizer(ctx context.Context, name string) (ctrl.Result, error) { live := &gpuv1beta1.Gpu{} 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) diff --git a/internal/controller/gpu_controller_test.go b/internal/controller/gpu_controller_test.go index e2e7738..b126827 100644 --- a/internal/controller/gpu_controller_test.go +++ b/internal/controller/gpu_controller_test.go @@ -512,13 +512,26 @@ var _ = Describe("GpuReconciler", func() { 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) + _, 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 { @@ -541,6 +554,7 @@ var _ = Describe("GpuReconciler", func() { _, 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) diff --git a/internal/helm/installer.go b/internal/helm/installer.go index bb1638f..4af6b8d 100644 --- a/internal/helm/installer.go +++ b/internal/helm/installer.go @@ -74,6 +74,8 @@ func (c *Client) InstallOrUpgrade(ctx context.Context, chartData []byte, values // 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. +// Note: ctx is accepted for interface consistency but is not forwarded to Helm - action.Uninstall +// does not accept a context. Cancellation is bounded solely by the timeout parameter. func (c *Client) Uninstall(_ context.Context, timeout time.Duration) error { cfg, err := c.actionConfig() if err != nil { From fdef21e38d99603f6b9dd069111fd5b8423c9c5f Mon Sep 17 00:00:00 2001 From: Vanja Radic Date: Fri, 29 May 2026 15:08:49 +0200 Subject: [PATCH 3/3] inline isTimeoutError and clarify Uninstall ctx limitation --- internal/controller/gpu_controller.go | 6 +----- internal/helm/installer.go | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/internal/controller/gpu_controller.go b/internal/controller/gpu_controller.go index eca9d75..c09b2d9 100644 --- a/internal/controller/gpu_controller.go +++ b/internal/controller/gpu_controller.go @@ -321,7 +321,7 @@ 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, deleteTimeout); err != nil { - if isTimeoutError(err) { + 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") @@ -366,10 +366,6 @@ func (r *GpuReconciler) removeFinalizer(ctx context.Context, name string) (ctrl. return ctrl.Result{}, nil } -func isTimeoutError(err error) bool { - return errors.Is(err, context.DeadlineExceeded) -} - // statusUpdate carries the partial status the reconciler wants to write in this cycle. // Only the conditions listed here are applied; absent ones are left untouched on the // live object. operatorVersion and driver are applied only when non-zero / non-nil. diff --git a/internal/helm/installer.go b/internal/helm/installer.go index 4af6b8d..b1ce30d 100644 --- a/internal/helm/installer.go +++ b/internal/helm/installer.go @@ -74,8 +74,8 @@ func (c *Client) InstallOrUpgrade(ctx context.Context, chartData []byte, values // 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. -// Note: ctx is accepted for interface consistency but is not forwarded to Helm - action.Uninstall -// does not accept a context. Cancellation is bounded solely by the timeout parameter. +// 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 {