From 312d215187f0c1375e4f0ccb512fc2db51bb42c4 Mon Sep 17 00:00:00 2001 From: Tariq Ibrahim Date: Fri, 24 Jul 2026 16:16:35 -0700 Subject: [PATCH] add finaliser to GPUCluster CR using PATCH Signed-off-by: Tariq Ibrahim Co-authored-by: Rajath Agasthya --- controllers/gpucluster_controller.go | 9 +++-- internal/utils/utils.go | 15 ++++++++ internal/utils/utils_test.go | 54 ++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 5 deletions(-) diff --git a/controllers/gpucluster_controller.go b/controllers/gpucluster_controller.go index 8918171b7e..df277fe9e5 100644 --- a/controllers/gpucluster_controller.go +++ b/controllers/gpucluster_controller.go @@ -45,6 +45,7 @@ import ( "github.com/NVIDIA/gpu-operator/internal/conditions" "github.com/NVIDIA/gpu-operator/internal/consts" "github.com/NVIDIA/gpu-operator/internal/state" + "github.com/NVIDIA/gpu-operator/internal/utils" ) // gpuClusterFinalizer holds the GPUCluster until reconcileDelete has ordered teardown. @@ -97,11 +98,9 @@ func (r *GPUClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) if !instance.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, instance) } - if !controllerutil.ContainsFinalizer(instance, gpuClusterFinalizer) { - controllerutil.AddFinalizer(instance, gpuClusterFinalizer) - if err := r.Update(ctx, instance); err != nil { - return ctrl.Result{}, fmt.Errorf("error adding finalizer: %w", err) - } + + if err := utils.EnsureFinalizer(ctx, r.Client, instance, gpuClusterFinalizer); err != nil { + return ctrl.Result{}, fmt.Errorf("error adding finalizer to GPUCluster %s: %w", req.NamespacedName, err) } // GPUCluster (DRA stack) may coexist with a ClusterPolicy (device-plugin diff --git a/internal/utils/utils.go b/internal/utils/utils.go index add101e46d..27692f2711 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -17,6 +17,7 @@ package utils import ( + "context" "fmt" "hash" "hash/fnv" @@ -29,6 +30,8 @@ import ( "github.com/davecgh/go-spew/spew" "k8s.io/apimachinery/pkg/util/rand" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) // GetFilesWithSuffix returns all files under a given base directory that have a specific suffix @@ -163,3 +166,15 @@ func WriteFileAtomically(path, content string) error { } return nil } + +// EnsureFinalizer adds a finalizer to an object that has not been marked for deletion. +// It is idempotent and returns an error only if an attempt to add the finalizer has failed. +func EnsureFinalizer(ctx context.Context, c client.Client, o client.Object, finalizer string) error { + if !o.GetDeletionTimestamp().IsZero() || controllerutil.ContainsFinalizer(o, finalizer) { + return nil + } + + original := o.DeepCopyObject().(client.Object) + controllerutil.AddFinalizer(o, finalizer) + return c.Patch(ctx, o, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})) +} diff --git a/internal/utils/utils_test.go b/internal/utils/utils_test.go index 47e961e6b5..0cc5acc5d5 100644 --- a/internal/utils/utils_test.go +++ b/internal/utils/utils_test.go @@ -17,10 +17,16 @@ package utils import ( + "context" "reflect" "testing" "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) func TestGetObjectHash(t *testing.T) { @@ -190,3 +196,51 @@ func TestGetStringHash(t *testing.T) { assert.Equal(t, tc.expected, actual) } } + +func TestEnsureFinalizer(t *testing.T) { + const testFinalizer = "test.io/finalizer" + + s := scheme.Scheme + + newObj := func(name string, finalizers ...string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "default", + ResourceVersion: "1", + Finalizers: finalizers, + }, + } + } + + t.Run("adds finalizer when not present", func(t *testing.T) { + obj := newObj("obj") + c := fake.NewClientBuilder().WithScheme(s).WithObjects(obj).Build() + + err := EnsureFinalizer(context.Background(), c, obj, testFinalizer) + assert.NoError(t, err) + assert.True(t, controllerutil.ContainsFinalizer(obj, testFinalizer)) + }) + + t.Run("no-op when finalizer already present", func(t *testing.T) { + obj := newObj("obj", testFinalizer) + // object not registered in client — Patch would fail if reached + c := fake.NewClientBuilder().WithScheme(s).Build() + + err := EnsureFinalizer(context.Background(), c, obj, testFinalizer) + assert.NoError(t, err) + assert.True(t, controllerutil.ContainsFinalizer(obj, testFinalizer)) + }) + + t.Run("no-op when object is being deleted", func(t *testing.T) { + now := metav1.Now() + obj := newObj("obj") + obj.DeletionTimestamp = &now + // object not registered in client — Patch would fail if reached + c := fake.NewClientBuilder().WithScheme(s).Build() + + err := EnsureFinalizer(context.Background(), c, obj, testFinalizer) + assert.NoError(t, err) + assert.False(t, controllerutil.ContainsFinalizer(obj, testFinalizer)) + }) +}