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..c09b2d9 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,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) diff --git a/internal/controller/gpu_controller_test.go b/internal/controller/gpu_controller_test.go index f65e48f..b126827 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,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()) + }) }) }) diff --git a/internal/helm/installer.go b/internal/helm/installer.go index f183812..b1ce30d 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,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) } 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 }