From 9734749023a44dd926dfe0e5fbfcb9d074b7a436 Mon Sep 17 00:00:00 2001 From: David Eads Date: Thu, 21 May 2020 11:01:31 -0400 Subject: [PATCH] separate add finalizer controller --- .../add_finalizer_controller.go | 85 ++++++++++++++++++ .../add_finalizer_controller_test.go | 90 +++++++++++++++++++ .../controllers/manifestwork_controller.go | 16 ---- pkg/spoke/spokeagent.go | 9 ++ 4 files changed, 184 insertions(+), 16 deletions(-) create mode 100644 pkg/spoke/controllers/finalizercontroller/add_finalizer_controller.go create mode 100644 pkg/spoke/controllers/finalizercontroller/add_finalizer_controller_test.go diff --git a/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller.go b/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller.go new file mode 100644 index 000000000..b4a9c6647 --- /dev/null +++ b/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 +} diff --git a/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller_test.go b/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller_test.go new file mode 100644 index 000000000..88574a65b --- /dev/null +++ b/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()) + }) + } +} diff --git a/pkg/spoke/controllers/manifestwork_controller.go b/pkg/spoke/controllers/manifestwork_controller.go index f2ded839d..d73711125 100644 --- a/pkg/spoke/controllers/manifestwork_controller.go +++ b/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. diff --git a/pkg/spoke/spokeagent.go b/pkg/spoke/spokeagent.go index f4638a5b3..c9ada02c7 100644 --- a/pkg/spoke/spokeagent.go +++ b/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