From b269b91a24d2e4a86273da59865dbf3af655b30d Mon Sep 17 00:00:00 2001 From: Yang Le Date: Wed, 8 Jul 2020 10:44:57 +0800 Subject: [PATCH] Check if finalizer exists before applying manifests --- pkg/spoke/controllers/constants.go | 7 +++++++ .../add_finalizer_controller.go | 9 +++------ .../add_finalizer_controller_test.go | 7 ++++--- .../finalizercontroller/finalize_controller.go | 5 +++-- .../finalize_controller_test.go | 9 +++++---- .../manifestwork_controller.go | 18 ++++++++++++++---- .../manifestwork_controller_test.go | 5 +++-- 7 files changed, 39 insertions(+), 21 deletions(-) create mode 100644 pkg/spoke/controllers/constants.go diff --git a/pkg/spoke/controllers/constants.go b/pkg/spoke/controllers/constants.go new file mode 100644 index 000000000..e9985c2e2 --- /dev/null +++ b/pkg/spoke/controllers/constants.go @@ -0,0 +1,7 @@ +package controllers + +const ( + // ManifestWorkFinalizer is the name of the finalizer added to manifestworks. It is used to ensure + // all maintained resources of a manifestwork are deleted before the manifestwork itself is deleted + ManifestWorkFinalizer = "cluster.open-cluster-management.io/manifest-work-cleanup" +) diff --git a/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller.go b/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller.go index b4a9c6647..e8e8b131c 100644 --- a/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller.go +++ b/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller.go @@ -14,6 +14,7 @@ import ( 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/open-cluster-management/work/pkg/spoke/controllers" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" ) @@ -24,10 +25,6 @@ type AddFinalizerController struct { manifestWorkLister worklister.ManifestWorkNamespaceLister } -const ( - manifestWorkFinalizer = "cluster.open-cluster-management.io/manifest-work-cleanup" -) - // NewAddFinalizerController returns a ManifestWorkController func NewAddFinalizerController( recorder events.Recorder, @@ -74,12 +71,12 @@ func (m *AddFinalizerController) syncManifestWork(ctx context.Context, originalM // don't add finalizer to instances that already have it for i := range manifestWork.Finalizers { - if manifestWork.Finalizers[i] == manifestWorkFinalizer { + if manifestWork.Finalizers[i] == controllers.ManifestWorkFinalizer { return nil } } // if this conflicts, we'll simply try again later - manifestWork.Finalizers = append(manifestWork.Finalizers, manifestWorkFinalizer) + manifestWork.Finalizers = append(manifestWork.Finalizers, controllers.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 index 88574a65b..82bc9c610 100644 --- a/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller_test.go +++ b/pkg/spoke/controllers/finalizercontroller/add_finalizer_controller_test.go @@ -8,6 +8,7 @@ import ( "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/controllers" "github.com/open-cluster-management/work/pkg/spoke/spoketesting" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clienttesting "k8s.io/client-go/testing" @@ -28,7 +29,7 @@ func TestAddFinalizer(t *testing.T) { t.Fatal(spew.Sdump(actions)) } work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.ManifestWork) - if !reflect.DeepEqual(work.Finalizers, []string{manifestWorkFinalizer}) { + if !reflect.DeepEqual(work.Finalizers, []string{controllers.ManifestWorkFinalizer}) { t.Fatal(spew.Sdump(actions)) } }, @@ -41,7 +42,7 @@ func TestAddFinalizer(t *testing.T) { t.Fatal(spew.Sdump(actions)) } work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.ManifestWork) - if !reflect.DeepEqual(work.Finalizers, []string{"other", manifestWorkFinalizer}) { + if !reflect.DeepEqual(work.Finalizers, []string{"other", controllers.ManifestWorkFinalizer}) { t.Fatal(spew.Sdump(actions)) } }, @@ -57,7 +58,7 @@ func TestAddFinalizer(t *testing.T) { }, { name: "skip when present", - existingFinalizers: []string{manifestWorkFinalizer}, + existingFinalizers: []string{controllers.ManifestWorkFinalizer}, validateActions: func(t *testing.T, actions []clienttesting.Action) { if len(actions) > 0 { t.Fatal(spew.Sdump(actions)) diff --git a/pkg/spoke/controllers/finalizercontroller/finalize_controller.go b/pkg/spoke/controllers/finalizercontroller/finalize_controller.go index 1485ef28d..94bc87490 100644 --- a/pkg/spoke/controllers/finalizercontroller/finalize_controller.go +++ b/pkg/spoke/controllers/finalizercontroller/finalize_controller.go @@ -10,6 +10,7 @@ import ( worklister "github.com/open-cluster-management/api/client/work/listers/work/v1" workapiv1 "github.com/open-cluster-management/api/work/v1" "github.com/open-cluster-management/work/pkg/helper" + "github.com/open-cluster-management/work/pkg/spoke/controllers" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" "k8s.io/apimachinery/pkg/api/errors" @@ -82,7 +83,7 @@ func (m *FinalizeController) syncManifestWork(ctx context.Context, controllerCon // don't do work if the finalizer is not present found := false for i := range manifestWork.Finalizers { - if manifestWork.Finalizers[i] == manifestWorkFinalizer { + if manifestWork.Finalizers[i] == controllers.ManifestWorkFinalizer { found = true break } @@ -122,7 +123,7 @@ func (m *FinalizeController) syncManifestWork(ctx context.Context, controllerCon // reset the rate limiter for the manifest work m.rateLimiter.Forget(manifestWork.Name) - removeFinalizer(manifestWork, manifestWorkFinalizer) + removeFinalizer(manifestWork, controllers.ManifestWorkFinalizer) _, err = m.manifestWorkClient.Update(ctx, manifestWork, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("Failed to remove finalizer from ManifestWork %s/%s: %w", manifestWork.Namespace, manifestWork.Name, err) diff --git a/pkg/spoke/controllers/finalizercontroller/finalize_controller_test.go b/pkg/spoke/controllers/finalizercontroller/finalize_controller_test.go index 733212f2e..8c7d6028e 100644 --- a/pkg/spoke/controllers/finalizercontroller/finalize_controller_test.go +++ b/pkg/spoke/controllers/finalizercontroller/finalize_controller_test.go @@ -9,6 +9,7 @@ import ( "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/controllers" "github.com/open-cluster-management/work/pkg/spoke/spoketesting" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -31,7 +32,7 @@ func TestFinalize(t *testing.T) { }{ { name: "skip when not delete", - existingFinalizers: []string{manifestWorkFinalizer}, + existingFinalizers: []string{controllers.ManifestWorkFinalizer}, validateManifestWorkActions: noAction, validateDynamicActions: noAction, }, @@ -45,7 +46,7 @@ func TestFinalize(t *testing.T) { { name: "delete resources and remove finalizer", terminated: true, - existingFinalizers: []string{"a", manifestWorkFinalizer, "b"}, + existingFinalizers: []string{"a", controllers.ManifestWorkFinalizer, "b"}, resourcesToRemove: []workapiv1.AppliedManifestResourceMeta{ {Group: "g1", Version: "v1", Resource: "r1", Namespace: "", Name: "n1"}, {Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"}, @@ -96,7 +97,7 @@ func TestFinalize(t *testing.T) { { name: "requeue work when deleting resources are still visiable", terminated: true, - existingFinalizers: []string{manifestWorkFinalizer}, + existingFinalizers: []string{controllers.ManifestWorkFinalizer}, existingResources: []runtime.Object{ spoketesting.NewUnstructuredSecret("ns1", "n1", true, "ns1-n1"), spoketesting.NewUnstructuredSecret("ns2", "n2", true, "ns2-n2"), @@ -127,7 +128,7 @@ func TestFinalize(t *testing.T) { { name: "ignore re-created resource and remove finalizer", terminated: true, - existingFinalizers: []string{manifestWorkFinalizer}, + existingFinalizers: []string{controllers.ManifestWorkFinalizer}, existingResources: []runtime.Object{ spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"), }, diff --git a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go index 884e6fb3d..2cd82047b 100644 --- a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go +++ b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go @@ -30,6 +30,7 @@ import ( "github.com/openshift/library-go/pkg/operator/resource/resourceapply" "github.com/open-cluster-management/work/pkg/helper" + "github.com/open-cluster-management/work/pkg/spoke/controllers" "github.com/open-cluster-management/work/pkg/spoke/resource" ) @@ -45,10 +46,6 @@ type ManifestWorkController struct { restMapper *resource.Mapper } -const ( - manifestWorkFinalizer = "cluster.open-cluster-management.io/manifest-work-cleanup" -) - // NewManifestWorkController returns a ManifestWorkController func NewManifestWorkController( ctx context.Context, @@ -100,6 +97,19 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac return nil } + // don't do work if the finalizer is not present + // it ensures all maintained resources will be cleaned once manifestwork is deleted + found := false + for i := range manifestWork.Finalizers { + if manifestWork.Finalizers[i] == controllers.ManifestWorkFinalizer { + found = true + break + } + } + if !found { + return nil + } + errs := []error{} // Apply resources on spoke cluster. resourceResults := m.applyManifest(manifestWork.Spec.Workload.Manifests, controllerContext.Recorder()) diff --git a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go index 156fa9f7e..9a90b6042 100644 --- a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go +++ b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go @@ -8,6 +8,7 @@ import ( fakeworkclient "github.com/open-cluster-management/api/client/work/clientset/versioned/fake" workinformers "github.com/open-cluster-management/api/client/work/informers/externalversions" workapiv1 "github.com/open-cluster-management/api/work/v1" + "github.com/open-cluster-management/work/pkg/spoke/controllers" "github.com/open-cluster-management/work/pkg/spoke/resource" "github.com/open-cluster-management/work/pkg/spoke/spoketesting" corev1 "k8s.io/api/core/v1" @@ -283,7 +284,7 @@ func TestSync(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { work, workKey := spoketesting.NewManifestWork(0, c.workManifest...) - work.Finalizers = []string{manifestWorkFinalizer} + work.Finalizers = []string{controllers.ManifestWorkFinalizer} controller := newController(work, spoketesting.NewFakeRestMapper()). withKubeObject(c.spokeObject...). withUnstructuredObject(c.spokeDynamicObject...) @@ -309,7 +310,7 @@ func TestFailedToApplyResource(t *testing.T) { withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionFalse}) work, workKey := spoketesting.NewManifestWork(0, tc.workManifest...) - work.Finalizers = []string{manifestWorkFinalizer} + work.Finalizers = []string{controllers.ManifestWorkFinalizer} controller := newController(work, spoketesting.NewFakeRestMapper()).withKubeObject(tc.spokeObject...).withUnstructuredObject() // Add a reactor on fake client to throw error when creating secret on namespace ns2