From 1d0803b91018767d7f48d316e251bfa4e1204d5a Mon Sep 17 00:00:00 2001 From: David Eads Date: Thu, 21 May 2020 15:50:38 -0400 Subject: [PATCH] separate finalizer controller --- .../finalize_controller.go | 148 ++++++++++++++++++ .../finalize_controller_test.go | 134 ++++++++++++++++ .../controllers/manifestwork_controller.go | 57 ------- .../manifestwork_controller_test.go | 33 ---- pkg/spoke/spokeagent.go | 8 + 5 files changed, 290 insertions(+), 90 deletions(-) create mode 100644 pkg/spoke/controllers/finalizercontroller/finalize_controller.go create mode 100644 pkg/spoke/controllers/finalizercontroller/finalize_controller_test.go diff --git a/pkg/spoke/controllers/finalizercontroller/finalize_controller.go b/pkg/spoke/controllers/finalizercontroller/finalize_controller.go new file mode 100644 index 000000000..4be46f996 --- /dev/null +++ b/pkg/spoke/controllers/finalizercontroller/finalize_controller.go @@ -0,0 +1,148 @@ +package finalizercontroller + +import ( + "context" + "fmt" + + 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" + workapiv1 "github.com/open-cluster-management/api/work/v1" + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + "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/apimachinery/pkg/runtime/schema" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/client-go/dynamic" + "k8s.io/klog" +) + +// FinalizeController handles cleanup of manifestwork resources before deletion is allowed. +type FinalizeController struct { + manifestWorkClient workv1client.ManifestWorkInterface + manifestWorkLister worklister.ManifestWorkNamespaceLister + spokeDynamicClient dynamic.Interface +} + +func NewFinalizeController( + recorder events.Recorder, + spokeDynamicClient dynamic.Interface, + manifestWorkClient workv1client.ManifestWorkInterface, + manifestWorkInformer workinformer.ManifestWorkInformer, + manifestWorkLister worklister.ManifestWorkNamespaceLister, +) factory.Controller { + + controller := &FinalizeController{ + manifestWorkClient: manifestWorkClient, + manifestWorkLister: manifestWorkLister, + spokeDynamicClient: spokeDynamicClient, + } + + return factory.New(). + WithInformersQueueKeyFunc(func(obj runtime.Object) string { + accessor, _ := meta.Accessor(obj) + return accessor.GetName() + }, manifestWorkInformer.Informer()). + WithSync(controller.sync).ToController("ManifestWorkFinalizer", recorder) +} + +func (m *FinalizeController) 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) +} + +// syncManifestWork ensures that when a manifestwork has been deleted, everything it created is also deleted before removing +// the finalizer +func (m *FinalizeController) syncManifestWork(ctx context.Context, originalManifestWork *workapiv1.ManifestWork) error { + manifestWork := originalManifestWork.DeepCopy() + + // no work to do until we're deleted + if manifestWork.DeletionTimestamp.IsZero() { + return nil + } + + // don't do work if the finalizer is not present + found := false + for i := range manifestWork.Finalizers { + if manifestWork.Finalizers[i] == manifestWorkFinalizer { + found = true + break + } + } + if !found { + return nil + } + + // Work is deleting, we remove its related resources on spoke cluster + if errs := m.cleanupResourceOfWork(manifestWork); len(errs) != 0 { + return utilerrors.NewAggregate(errs) + } + + // We consider the case of deletion of created resources, with finalization still to come, as sufficient to remove the finalizer + // We do this because resources cannot be un-deleted. This means that deletion is inevitable. + // Also, since we don't track UIDs, we have no reliable way of know when "this" particular resource has been removed as + // compared with a case where this controller deletes it and another controller (or manifestwork) creates it. + + removeFinalizer(manifestWork, manifestWorkFinalizer) + _, err := m.manifestWorkClient.Update(ctx, manifestWork, metav1.UpdateOptions{}) + return err +} + +func (m *FinalizeController) cleanupResourceOfWork(work *workapiv1.ManifestWork) []error { + klog.V(4).Infof("cleaning up %q", work.Name) + + errs := []error{} + + // TODO this can later be based on a list of all resources created by this manifest work + // not just the resources currently managed. This overlaps with the need to remove resources we have created + // separate from the application of current resources. + for _, resourceStatus := range work.Status.ResourceStatus.Manifests { + gvr := schema.GroupVersionResource{Group: resourceStatus.ResourceMeta.Group, Version: resourceStatus.ResourceMeta.Version, Resource: resourceStatus.ResourceMeta.Resource} + if len(gvr.Resource) == 0 || len(gvr.Version) == 0 || len(resourceStatus.ResourceMeta.Name) == 0 { + // without a resource or version, the request cannot be constructed, so we must not have created this either + continue + } + + err := m.spokeDynamicClient. + Resource(gvr). + Namespace(resourceStatus.ResourceMeta.Namespace). + Delete(context.TODO(), resourceStatus.ResourceMeta.Name, metav1.DeleteOptions{}) + switch { + case errors.IsNotFound(err): + // no-oop + case err != nil: + errs = append(errs, fmt.Errorf( + "Failed to delete resource %v with key %s/%s: %w", + gvr, resourceStatus.ResourceMeta.Namespace, resourceStatus.ResourceMeta.Name, err)) + continue + } + klog.Infof("Successfully delete resource %v with key %s/%s", gvr, resourceStatus.ResourceMeta.Namespace, resourceStatus.ResourceMeta.Name) + } + + return errs +} + +// removeFinalizer removes a finalizer from the list. It mutates its input. +func removeFinalizer(manifestWork *workapiv1.ManifestWork, finalizerName string) { + newFinalizers := []string{} + for i := range manifestWork.Finalizers { + if manifestWork.Finalizers[i] == finalizerName { + continue + } + newFinalizers = append(newFinalizers, manifestWork.Finalizers[i]) + } + manifestWork.Finalizers = newFinalizers +} diff --git a/pkg/spoke/controllers/finalizercontroller/finalize_controller_test.go b/pkg/spoke/controllers/finalizercontroller/finalize_controller_test.go new file mode 100644 index 000000000..1d30435fd --- /dev/null +++ b/pkg/spoke/controllers/finalizercontroller/finalize_controller_test.go @@ -0,0 +1,134 @@ +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" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + fakedynamic "k8s.io/client-go/dynamic/fake" + clienttesting "k8s.io/client-go/testing" +) + +func TestFinalize(t *testing.T) { + cases := []struct { + name string + existingFinalizers []string + resourcesToRemove []workapiv1.ManifestResourceMeta + terminated bool + + validateManifestWorkActions func(t *testing.T, actions []clienttesting.Action) + validateDynamicActions func(t *testing.T, actions []clienttesting.Action) + }{ + { + name: "skip when not delete", + existingFinalizers: []string{manifestWorkFinalizer}, + validateManifestWorkActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) > 0 { + t.Fatal(spew.Sdump(actions)) + } + }, + validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) > 0 { + t.Fatal(spew.Sdump(actions)) + } + }, + }, + { + name: "skip when finalizer gone", + terminated: true, + existingFinalizers: []string{"other-finalizer"}, + validateManifestWorkActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) > 0 { + t.Fatal(spew.Sdump(actions)) + } + }, + validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) > 0 { + t.Fatal(spew.Sdump(actions)) + } + }, + }, + { + name: "delete resources", + terminated: true, + existingFinalizers: []string{"a", manifestWorkFinalizer, "b"}, + resourcesToRemove: []workapiv1.ManifestResourceMeta{ + {Group: "g1", Version: "v1", Resource: "r1", Namespace: "", Name: "n1"}, + {Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"}, + {Group: "g3", Version: "v3", Resource: "r3", Namespace: "ns3", Name: "n3"}, + {Group: "g4", Version: "v4", Resource: "r4", Namespace: "", Name: "n4"}, + }, + validateManifestWorkActions: 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{"a", "b"}) { + t.Fatal(spew.Sdump(actions)) + } + }, + validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) != 4 { + t.Fatal(spew.Sdump(actions)) + } + + action := actions[0].(clienttesting.DeleteAction) + resource, namespace, name := action.GetResource(), action.GetNamespace(), action.GetName() + if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "g1", Version: "v1", Resource: "r1"}) || namespace != "" || name != "n1" { + t.Fatal(spew.Sdump(actions)) + } + action = actions[1].(clienttesting.DeleteAction) + resource, namespace, name = action.GetResource(), action.GetNamespace(), action.GetName() + if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "g2", Version: "v2", Resource: "r2"}) || namespace != "ns2" || name != "n2" { + t.Fatal(spew.Sdump(actions)) + } + action = actions[2].(clienttesting.DeleteAction) + resource, namespace, name = action.GetResource(), action.GetNamespace(), action.GetName() + if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "g3", Version: "v3", Resource: "r3"}) || namespace != "ns3" || name != "n3" { + t.Fatal(spew.Sdump(actions)) + } + action = actions[3].(clienttesting.DeleteAction) + resource, namespace, name = action.GetResource(), action.GetNamespace(), action.GetName() + if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "g4", Version: "v4", Resource: "r4"}) || namespace != "" || name != "n4" { + 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 + } + for _, curr := range c.resourcesToRemove { + testingWork.Status.ResourceStatus.Manifests = append(testingWork.Status.ResourceStatus.Manifests, workapiv1.ManifestCondition{ResourceMeta: curr}) + } + + fakeDynamicClient := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme()) + fakeClient := fakeworkclient.NewSimpleClientset(testingWork) + controller := FinalizeController{ + manifestWorkClient: fakeClient.WorkV1().ManifestWorks(testingWork.Namespace), + spokeDynamicClient: fakeDynamicClient, + } + + err := controller.syncManifestWork(context.TODO(), testingWork) + if err != nil { + t.Fatal(err) + } + c.validateManifestWorkActions(t, fakeClient.Actions()) + c.validateDynamicActions(t, fakeDynamicClient.Actions()) + }) + } + +} diff --git a/pkg/spoke/controllers/manifestwork_controller.go b/pkg/spoke/controllers/manifestwork_controller.go index d73711125..9482c095c 100644 --- a/pkg/spoke/controllers/manifestwork_controller.go +++ b/pkg/spoke/controllers/manifestwork_controller.go @@ -103,16 +103,6 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac } manifestWork = manifestWork.DeepCopy() - // 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. - if !manifestWork.DeletionTimestamp.IsZero() { - if errs := m.cleanupResourceOfWork(manifestWork); len(errs) != 0 { - return utilerrors.NewAggregate(errs) - } - return m.removeWorkFinalizer(ctx, manifestWork) - } - errs := []error{} // Apply resources on spoke cluster. resourceResults := m.applyManifest(manifestWork.Spec.Workload.Manifests, controllerContext.Recorder()) @@ -155,53 +145,6 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac return err } -func (m *ManifestWorkController) cleanupResourceOfWork(work *workapiv1.ManifestWork) []error { - errs := []error{} - for _, manifest := range work.Spec.Workload.Manifests { - gvr, object, err := m.decodeUnstructured(manifest.Raw) - if err != nil { - klog.Errorf("Failed to decode object: %v", err) - errs = append(errs, err) - continue - } - - err = m.spokeDynamicClient. - Resource(gvr). - Namespace(object.GetNamespace()). - Delete(context.TODO(), object.GetName(), metav1.DeleteOptions{}) - switch { - case errors.IsNotFound(err): - // no-oop - case err != nil: - errs = append(errs, fmt.Errorf( - "Failed to delete resource %v with key %s/%s: %w", - gvr, object.GetNamespace(), object.GetName(), err)) - continue - } - klog.V(4).Infof("Successfully delete resource %v with key %s/%s", gvr, object.GetNamespace(), object.GetName()) - } - - return errs -} - -func (m *ManifestWorkController) removeWorkFinalizer(ctx context.Context, manifestWork *workapiv1.ManifestWork) error { - copiedFinalizers := []string{} - for i := range manifestWork.Finalizers { - if manifestWork.Finalizers[i] == manifestWorkFinalizer { - continue - } - copiedFinalizers = append(copiedFinalizers, manifestWork.Finalizers[i]) - } - - if len(manifestWork.Finalizers) != len(copiedFinalizers) { - manifestWork.Finalizers = copiedFinalizers - _, err := m.manifestWorkClient.Update(ctx, manifestWork, metav1.UpdateOptions{}) - return err - } - - return nil -} - func (m *ManifestWorkController) applyManifest(manifests []workapiv1.Manifest, recorder events.Recorder) []resourceapply.ApplyResult { clientHolder := resourceapply.NewClientHolder(). WithAPIExtensionsClient(m.spokeAPIExtensionClient). diff --git a/pkg/spoke/controllers/manifestwork_controller_test.go b/pkg/spoke/controllers/manifestwork_controller_test.go index d7d9981e0..0921db7d3 100644 --- a/pkg/spoke/controllers/manifestwork_controller_test.go +++ b/pkg/spoke/controllers/manifestwork_controller_test.go @@ -298,39 +298,6 @@ func TestSync(t *testing.T) { } } -// TestDeleteWork tests the action when work is deleted -func TestDeleteWork(t *testing.T) { - tc := newTestCase("delete multiple resources"). - withWorkManifest(spoketesting.NewUnstructured("v1", "Secret", "ns1", "test"), spoketesting.NewUnstructured("v1", "Secret", "ns2", "test")). - withSpokeObject(spoketesting.NewSecret("test", "ns1", "value2")). - withExpectedWorkAction("update"). - withExpectedDynamicAction("delete", "delete") - - work, workKey := spoketesting.NewManifestWork(0, tc.workManifest...) - work.Finalizers = []string{manifestWorkFinalizer} - now := metav1.Now() - work.ObjectMeta.SetDeletionTimestamp(&now) - controller := newController(work, spoketesting.NewFakeRestMapper()).withKubeObject(tc.spokeObject...).withUnstructuredObject() - syncContext := spoketesting.NewFakeSyncContext(t, workKey) - err := controller.controller.sync(nil, syncContext) - if err != nil { - t.Errorf("Should be success with no err: %v", err) - } - - tc.validate(t, controller.dynamicClient, controller.workClient, controller.kubeClient) - - // Verify that finalizer is removed - workActions := controller.workClient.Actions() - actual, ok := workActions[len(workActions)-1].(clienttesting.UpdateActionImpl) - if !ok { - t.Errorf("Expected to get update action") - } - actualWork := actual.Object.(*workapiv1.ManifestWork) - if len(actualWork.Finalizers) != 0 { - t.Errorf("Expected 0 finailizer but got %#v", actualWork.Finalizers) - } -} - // Test applying resource failed func TestFailedToApplyResource(t *testing.T) { tc := newTestCase("multiple create&update resource"). diff --git a/pkg/spoke/spokeagent.go b/pkg/spoke/spokeagent.go index c9ada02c7..8b1202755 100644 --- a/pkg/spoke/spokeagent.go +++ b/pkg/spoke/spokeagent.go @@ -96,9 +96,17 @@ func (o *WorkloadAgentOptions) RunWorkloadAgent(ctx context.Context, controllerC workInformerFactory.Work().V1().ManifestWorks(), workInformerFactory.Work().V1().ManifestWorks().Lister().ManifestWorks(o.SpokeClusterName), ) + finalizeController := finalizercontroller.NewFinalizeController( + controllerContext.EventRecorder, + spokeDynamicClient, + 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 finalizeController.Run(ctx, 1) go manifestWorkController.Run(ctx, 1) <-ctx.Done() return nil