separate add finalizer controller

pkg/spoke/controllers/finalizercontroller/add_finalizer_controller.go

@@ -0,0 +1,85 @@
+package finalizercontroller
+
+import (
+	"context"
+
+	workapiv1 "github.com/open-cluster-management/api/work/v1"
+
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/klog"
+
+	workv1client "github.com/open-cluster-management/api/client/work/clientset/versioned/typed/work/v1"
+	workinformer "github.com/open-cluster-management/api/client/work/informers/externalversions/work/v1"
+	worklister "github.com/open-cluster-management/api/client/work/listers/work/v1"
+	"github.com/openshift/library-go/pkg/controller/factory"
+	"github.com/openshift/library-go/pkg/operator/events"
+)
+
+// AddFinalizerController is to add the cluster.open-cluster-management.io/manifest-work-cleanup finalizer to manifestworks.
+type AddFinalizerController struct {
+	manifestWorkClient workv1client.ManifestWorkInterface
+	manifestWorkLister worklister.ManifestWorkNamespaceLister
+}
+
+const (
+	manifestWorkFinalizer = "cluster.open-cluster-management.io/manifest-work-cleanup"
+)
+
+// NewAddFinalizerController returns a ManifestWorkController
+func NewAddFinalizerController(
+	recorder events.Recorder,
+	manifestWorkClient workv1client.ManifestWorkInterface,
+	manifestWorkInformer workinformer.ManifestWorkInformer,
+	manifestWorkLister worklister.ManifestWorkNamespaceLister,
+) factory.Controller {
+
+	controller := &AddFinalizerController{
+		manifestWorkClient: manifestWorkClient,
+		manifestWorkLister: manifestWorkLister,
+	}
+
+	return factory.New().
+		WithInformersQueueKeyFunc(func(obj runtime.Object) string {
+			accessor, _ := meta.Accessor(obj)
+			return accessor.GetName()
+		}, manifestWorkInformer.Informer()).
+		WithSync(controller.sync).ToController("ManifestWorkAddFinalizerController", recorder)
+}
+
+func (m *AddFinalizerController) sync(ctx context.Context, controllerContext factory.SyncContext) error {
+	manifestWorkName := controllerContext.QueueKey()
+	klog.V(4).Infof("Reconciling ManifestWork %q", manifestWorkName)
+
+	manifestWork, err := m.manifestWorkLister.Get(manifestWorkName)
+	if errors.IsNotFound(err) {
+		// work  not found, could have been deleted, do nothing.
+		return nil
+	}
+	if err != nil {
+		return err
+	}
+	return m.syncManifestWork(ctx, manifestWork)
+}
+
+func (m *AddFinalizerController) syncManifestWork(ctx context.Context, originalManifestWork *workapiv1.ManifestWork) error {
+	manifestWork := originalManifestWork.DeepCopy()
+
+	// don't add finalizers to instances that are deleted
+	if !manifestWork.DeletionTimestamp.IsZero() {
+		return nil
+	}
+
+	// don't add finalizer to instances that already have it
+	for i := range manifestWork.Finalizers {
+		if manifestWork.Finalizers[i] == manifestWorkFinalizer {
+			return nil
+		}
+	}
+	// if this conflicts, we'll simply try again later
+	manifestWork.Finalizers = append(manifestWork.Finalizers, manifestWorkFinalizer)
+	_, err := m.manifestWorkClient.Update(ctx, manifestWork, metav1.UpdateOptions{})
+	return err
+}

pkg/spoke/controllers/finalizercontroller/add_finalizer_controller_test.go

@@ -0,0 +1,90 @@
+package finalizercontroller
+
+import (
+	"context"
+	"reflect"
+	"testing"
+
+	"github.com/davecgh/go-spew/spew"
+	fakeworkclient "github.com/open-cluster-management/api/client/work/clientset/versioned/fake"
+	workapiv1 "github.com/open-cluster-management/api/work/v1"
+	"github.com/open-cluster-management/work/pkg/spoke/spoketesting"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	clienttesting "k8s.io/client-go/testing"
+)
+
+func TestAddFinalizer(t *testing.T) {
+	cases := []struct {
+		name               string
+		existingFinalizers []string
+		terminated         bool
+
+		validateActions func(t *testing.T, actions []clienttesting.Action)
+	}{
+		{
+			name: "add when empty",
+			validateActions: func(t *testing.T, actions []clienttesting.Action) {
+				if len(actions) != 1 {
+					t.Fatal(spew.Sdump(actions))
+				}
+				work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.ManifestWork)
+				if !reflect.DeepEqual(work.Finalizers, []string{manifestWorkFinalizer}) {
+					t.Fatal(spew.Sdump(actions))
+				}
+			},
+		},
+		{
+			name:               "add when missing",
+			existingFinalizers: []string{"other"},
+			validateActions: func(t *testing.T, actions []clienttesting.Action) {
+				if len(actions) != 1 {
+					t.Fatal(spew.Sdump(actions))
+				}
+				work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.ManifestWork)
+				if !reflect.DeepEqual(work.Finalizers, []string{"other", manifestWorkFinalizer}) {
+					t.Fatal(spew.Sdump(actions))
+				}
+			},
+		},
+		{
+			name:       "skip when deleted",
+			terminated: true,
+			validateActions: func(t *testing.T, actions []clienttesting.Action) {
+				if len(actions) > 0 {
+					t.Fatal(spew.Sdump(actions))
+				}
+			},
+		},
+		{
+			name:               "skip when present",
+			existingFinalizers: []string{manifestWorkFinalizer},
+			validateActions: func(t *testing.T, actions []clienttesting.Action) {
+				if len(actions) > 0 {
+					t.Fatal(spew.Sdump(actions))
+				}
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			testingWork, _ := spoketesting.NewManifestWork(0)
+			testingWork.Finalizers = c.existingFinalizers
+			if c.terminated {
+				now := metav1.Now()
+				testingWork.DeletionTimestamp = &now
+			}
+
+			fakeClient := fakeworkclient.NewSimpleClientset(testingWork)
+			controller := AddFinalizerController{
+				manifestWorkClient: fakeClient.WorkV1().ManifestWorks(testingWork.Namespace),
+			}
+
+			err := controller.syncManifestWork(context.TODO(), testingWork)
+			if err != nil {
+				t.Fatal(err)
+			}
+			c.validateActions(t, fakeClient.Actions())
+		})
+	}
+}

pkg/spoke/controllers/manifestwork_controller.go

@@ -103,22 +103,6 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac
 	}
 	manifestWork = manifestWork.DeepCopy()
 
-	// Update finalizer at first
-	if manifestWork.DeletionTimestamp.IsZero() {
-		hasFinalizer := false
-		for i := range manifestWork.Finalizers {
-			if manifestWork.Finalizers[i] == manifestWorkFinalizer {
-				hasFinalizer = true
-				break
-			}
-		}
-		if !hasFinalizer {
-			manifestWork.Finalizers = append(manifestWork.Finalizers, manifestWorkFinalizer)
-			_, err := m.manifestWorkClient.Update(ctx, manifestWork, metav1.UpdateOptions{})
-			return err
-		}
-	}
-
 	// Work is deleting, we remove its related resources on spoke cluster
 	// TODO: once we make this work initially, the finalizer would live in a different loop.
 	// It will have different backoff considerations.

pkg/spoke/spokeagent.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"time"
 
+	"github.com/open-cluster-management/work/pkg/spoke/controllers/finalizercontroller"
+
 	"github.com/openshift/library-go/pkg/controller/controllercmd"
 	"github.com/spf13/cobra"
 	apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
@@ -88,8 +90,15 @@ func (o *WorkloadAgentOptions) RunWorkloadAgent(ctx context.Context, controllerC
 		workInformerFactory.Work().V1().ManifestWorks().Lister().ManifestWorks(o.SpokeClusterName),
 		restMapper,
 	)
+	addFinalizerController := finalizercontroller.NewAddFinalizerController(
+		controllerContext.EventRecorder,
+		hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName),
+		workInformerFactory.Work().V1().ManifestWorks(),
+		workInformerFactory.Work().V1().ManifestWorks().Lister().ManifestWorks(o.SpokeClusterName),
+	)
 
 	go workInformerFactory.Start(ctx.Done())
+	go addFinalizerController.Run(ctx, 1)
 	go manifestWorkController.Run(ctx, 1)
 	<-ctx.Done()
 	return nil