From 7cede9fd2f3d3f7b5c053bc87924d7ca95decf25 Mon Sep 17 00:00:00 2001 From: Yang Le Date: Wed, 26 Aug 2020 17:38:32 +0800 Subject: [PATCH] Delete resources applied by controller only --- pkg/helper/helper_test.go | 1 - pkg/helper/helpers.go | 38 +---- .../appliedmanifestwork_controller.go | 31 +++- .../appliedmanifestwork_controller_test.go | 141 ++++++++++-------- .../manifestwork_finalize_controller_test.go | 1 - test/e2e/work_agent_test.go | 12 +- test/integration/util/assertion.go | 14 +- 7 files changed, 139 insertions(+), 99 deletions(-) diff --git a/pkg/helper/helper_test.go b/pkg/helper/helper_test.go index 3464eb179..25e2d7d1b 100644 --- a/pkg/helper/helper_test.go +++ b/pkg/helper/helper_test.go @@ -348,7 +348,6 @@ func TestDeleteAppliedResourcess(t *testing.T) { {Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"}, }, expectedResourcesPendingFinalization: []workapiv1.AppliedManifestResourceMeta{ - {Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"}, {Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"}, }, }, diff --git a/pkg/helper/helpers.go b/pkg/helper/helpers.go index 580fb5fc5..7e479eec3 100644 --- a/pkg/helper/helpers.go +++ b/pkg/helper/helpers.go @@ -18,6 +18,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" "k8s.io/client-go/util/retry" "k8s.io/klog" @@ -264,6 +265,7 @@ func UpdateManifestWorkStatus( } // DeleteAppliedResources deletes all given applied resources and returns those pending for finalization +// If the uid recorded in resources is different from what we get by client, ignore the deletion. func DeleteAppliedResources(resources []workapiv1.AppliedManifestResourceMeta, dynamicClient dynamic.Interface) ([]workapiv1.AppliedManifestResourceMeta, []error) { var resourcesPendingFinalization []workapiv1.AppliedManifestResourceMeta var errs []error @@ -286,28 +288,18 @@ func DeleteAppliedResources(resources []workapiv1.AppliedManifestResourceMeta, d continue } - pendingFinalization := u.GetDeletionTimestamp() != nil && !u.GetDeletionTimestamp().IsZero() - waitingForFinalization := len(resource.UID) > 0 - if pendingFinalization { - if waitingForFinalization && resource.UID != string(u.GetUID()) { - // the instance is deleted, so do not add to the resourcesPendingDeletion list - continue - } - - // if we aren't waiting for finalization or the UID does match, then we want to ensure we add it to the pending list - newResource := copyAppliedResource(resource) - newResource.UID = string(u.GetUID()) - resourcesPendingFinalization = append(resourcesPendingFinalization, newResource) + if resource.UID != string(u.GetUID()) { + // the traced instance has been deleted, and forget this item. continue } - // forget this item if the UID does not match the resource UID we previously deleted - if waitingForFinalization && resource.UID != string(u.GetUID()) { + if u.GetDeletionTimestamp() != nil && !u.GetDeletionTimestamp().IsZero() { + resourcesPendingFinalization = append(resourcesPendingFinalization, resource) continue } // delete the resource which is not deleted yet - uid := u.GetUID() + uid := types.UID(resource.UID) err = dynamicClient. Resource(gvr). Namespace(resource.Namespace). @@ -330,27 +322,13 @@ func DeleteAppliedResources(resources []workapiv1.AppliedManifestResourceMeta, d continue } - // set UID of applied resource once deletion is successful - newResource := copyAppliedResource(resource) - newResource.UID = string(u.GetUID()) - resourcesPendingFinalization = append(resourcesPendingFinalization, newResource) + resourcesPendingFinalization = append(resourcesPendingFinalization, resource) klog.V(2).Infof("Delete resource %v with key %s/%s", gvr, resource.Namespace, resource.Name) } return resourcesPendingFinalization, errs } -func copyAppliedResource(resource workapiv1.AppliedManifestResourceMeta) workapiv1.AppliedManifestResourceMeta { - return workapiv1.AppliedManifestResourceMeta{ - Group: resource.Group, - Version: resource.Version, - Resource: resource.Resource, - Name: resource.Name, - Namespace: resource.Namespace, - UID: resource.UID, - } -} - // GuessObjectGroupVersionKind returns GVK for the passed runtime object. func GuessObjectGroupVersionKind(object runtime.Object) (*schema.GroupVersionKind, error) { gvk := resourcehelper.GuessObjectGroupVersionKind(object) diff --git a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go index f3cee8de3..378adf92d 100644 --- a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go +++ b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go @@ -94,6 +94,10 @@ func (m *AppliedManifestWorkController) sync(ctx context.Context, controllerCont if err != nil { return err } + // no work to do if we're deleted + if !appliedManifestWork.DeletionTimestamp.IsZero() { + return nil + } return m.syncManifestWork(ctx, controllerContext, manifestWork, appliedManifestWork) } @@ -111,20 +115,43 @@ func (m *AppliedManifestWorkController) syncManifestWork( // spec because manifests in spec are only resource templates, while resource status records the real resources // maintained by the manifest work. var appliedResources []workapiv1.AppliedManifestResourceMeta + var errs []error for _, resourceStatus := range manifestWork.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 { continue } + u, err := m.spokeDynamicClient. + Resource(gvr). + Namespace(resourceStatus.ResourceMeta.Namespace). + Get(context.TODO(), resourceStatus.ResourceMeta.Name, metav1.GetOptions{}) + if errors.IsNotFound(err) { + klog.V(2).Infof( + "Resource %v with key %s/%s does not exist", + gvr, resourceStatus.ResourceMeta.Namespace, resourceStatus.ResourceMeta.Name) + continue + } + + if err != nil { + errs = append(errs, fmt.Errorf( + "Failed to get resource %v with key %s/%s: %w", + gvr, resourceStatus.ResourceMeta.Namespace, resourceStatus.ResourceMeta.Name, err)) + continue + } + appliedResources = append(appliedResources, workapiv1.AppliedManifestResourceMeta{ Group: resourceStatus.ResourceMeta.Group, Version: resourceStatus.ResourceMeta.Version, Resource: resourceStatus.ResourceMeta.Resource, Namespace: resourceStatus.ResourceMeta.Namespace, Name: resourceStatus.ResourceMeta.Name, + UID: string(u.GetUID()), }) } + if len(errs) != 0 { + return utilerrors.NewAggregate(errs) + } // delete applied resources which are no longer maintained by manifest work noLongerMaintainedResources := findUntrackedResources(appliedManifestWork.Status.AppliedResources, appliedResources) @@ -178,7 +205,9 @@ func findUntrackedResources(appliedResources, newAppliedResources []workapiv1.Ap resourceIndex := map[workapiv1.AppliedManifestResourceMeta]struct{}{} for _, resource := range newAppliedResources { - resourceIndex[resource] = struct{}{} + key := resource.DeepCopy() + key.UID = "" + resourceIndex[*key] = struct{}{} } for _, resource := range appliedResources { diff --git a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go index 624217f12..146a93301 100644 --- a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go +++ b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go @@ -38,39 +38,46 @@ func TestSyncManifestWork(t *testing.T) { appliedResources []workapiv1.AppliedManifestResourceMeta manifests []workapiv1.ManifestCondition validateAppliedManifestWorkActions func(t *testing.T, actions []clienttesting.Action) - validateDynamicActions func(t *testing.T, actions []clienttesting.Action) + expectedDeleteActions []clienttesting.DeleteActionImpl expectedQueueLen int }{ { name: "skip when no applied resource changed", - appliedResources: []workapiv1.AppliedManifestResourceMeta{ - {Group: "g1", Version: "v1", Resource: "r1", Namespace: "ns1", Name: "n1"}, + existingResources: []runtime.Object{ + spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"), }, - manifests: []workapiv1.ManifestCondition{newManifest("g1", "v1", "r1", "ns1", "n1")}, + appliedResources: []workapiv1.AppliedManifestResourceMeta{ + {Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"}, + }, + manifests: []workapiv1.ManifestCondition{newManifest("", "v1", "secrets", "ns1", "n1")}, validateAppliedManifestWorkActions: 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)) - } - }, + expectedDeleteActions: []clienttesting.DeleteActionImpl{}, }, { name: "delete untracked resources", + existingResources: []runtime.Object{ + spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"), + spoketesting.NewUnstructuredSecret("ns2", "n2", false, "ns2-n2"), + spoketesting.NewUnstructuredSecret("ns3", "n3", false, "ns3-n3"), + spoketesting.NewUnstructuredSecret("ns4", "n4", false, "ns4-n4"), + spoketesting.NewUnstructuredSecret("ns5", "n5", false, "ns5-n5"), + spoketesting.NewUnstructuredSecret("ns6", "n6", false, "ns6-n6"), + }, appliedResources: []workapiv1.AppliedManifestResourceMeta{ - {Group: "g1", Version: "v1", Resource: "r1", Namespace: "ns1", 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: "ns4", Name: "n4"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns3", Name: "n3", UID: "ns3-n3"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns4", Name: "n4", UID: "ns4-n4"}, }, manifests: []workapiv1.ManifestCondition{ - newManifest("g1", "v1", "r1", "ns1", "n1"), - newManifest("g2", "v2", "r2", "ns2", "n2"), - newManifest("g5", "v5", "r5", "ns5", "n5"), - newManifest("g6", "v6", "r6", "ns6", "n6"), + newManifest("", "v1", "secrets", "ns1", "n1"), + newManifest("", "v1", "secrets", "ns2", "n2"), + newManifest("", "v1", "secrets", "ns5", "n5"), + newManifest("", "v1", "secrets", "ns6", "n6"), }, validateAppliedManifestWorkActions: func(t *testing.T, actions []clienttesting.Action) { if len(actions) != 1 { @@ -78,39 +85,31 @@ func TestSyncManifestWork(t *testing.T) { } work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.AppliedManifestWork) if !reflect.DeepEqual(work.Status.AppliedResources, []workapiv1.AppliedManifestResourceMeta{ - {Group: "g1", Version: "v1", Resource: "r1", Namespace: "ns1", Name: "n1"}, - {Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"}, - {Group: "g5", Version: "v5", Resource: "r5", Namespace: "ns5", Name: "n5"}, - {Group: "g6", Version: "v6", Resource: "r6", Namespace: "ns6", Name: "n6"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns3", Name: "n3", UID: "ns3-n3"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns4", Name: "n4", UID: "ns4-n4"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns5", Name: "n5", UID: "ns5-n5"}, + {Group: "", Version: "v1", Resource: "secrets", Namespace: "ns6", Name: "n6", UID: "ns6-n6"}, }) { t.Fatal(spew.Sdump(actions)) } }, - validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) { - if len(actions) != 2 { - t.Fatal(spew.Sdump(actions)) - } - - action := actions[0].(clienttesting.GetAction) - 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[1].(clienttesting.GetAction) - resource, namespace, name = action.GetResource(), action.GetNamespace(), action.GetName() - if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "g4", Version: "v4", Resource: "r4"}) || namespace != "ns4" || name != "n4" { - t.Fatal(spew.Sdump(actions)) - } + expectedDeleteActions: []clienttesting.DeleteActionImpl{ + clienttesting.NewDeleteAction(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, "ns3", "n3"), + clienttesting.NewDeleteAction(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, "ns4", "n4"), }, }, { name: "requeue work when applied resource for stale manifest is deleting", existingResources: []runtime.Object{ + spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"), + spoketesting.NewUnstructuredSecret("ns2", "n2", false, "ns2-n2"), spoketesting.NewUnstructuredSecret("ns3", "n3", true, "ns3-n3"), }, appliedResources: []workapiv1.AppliedManifestResourceMeta{ - {Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1"}, - {Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2"}, + {Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"}, + {Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"}, {Version: "v1", Resource: "secrets", Namespace: "ns3", Name: "n3", UID: "ns3-n3"}, }, manifests: []workapiv1.ManifestCondition{ @@ -122,23 +121,15 @@ func TestSyncManifestWork(t *testing.T) { t.Fatal(spew.Sdump(actions)) } }, - validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) { - if len(actions) != 1 { - t.Fatal(spew.Sdump(actions)) - } - - action := actions[0].(clienttesting.GetAction) - resource, namespace, name := action.GetResource(), action.GetNamespace(), action.GetName() - if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}) || namespace != "ns3" || name != "n3" { - t.Fatal(spew.Sdump(actions)) - } - }, - expectedQueueLen: 1, + expectedDeleteActions: []clienttesting.DeleteActionImpl{}, + expectedQueueLen: 1, }, { name: "ignore re-created resource", existingResources: []runtime.Object{ spoketesting.NewUnstructuredSecret("ns3", "n3", false, "ns3-n3-recreated"), + spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"), + spoketesting.NewUnstructuredSecret("ns5", "n5", false, "ns5-n5"), }, appliedResources: []workapiv1.AppliedManifestResourceMeta{ {Version: "v1", Resource: "secrets", Namespace: "ns3", Name: "n3", UID: "ns3-n3"}, @@ -154,28 +145,41 @@ func TestSyncManifestWork(t *testing.T) { } work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.AppliedManifestWork) if !reflect.DeepEqual(work.Status.AppliedResources, []workapiv1.AppliedManifestResourceMeta{ - {Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1"}, - {Version: "v1", Resource: "secrets", Namespace: "ns5", Name: "n5"}, + {Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"}, + {Version: "v1", Resource: "secrets", Namespace: "ns5", Name: "n5", UID: "ns5-n5"}, }) { t.Fatal(spew.Sdump(actions)) } }, - validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) { - if len(actions) != 2 { + expectedDeleteActions: []clienttesting.DeleteActionImpl{}, + }, + { + name: "update resource uid", + existingResources: []runtime.Object{ + spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"), + spoketesting.NewUnstructuredSecret("ns2", "n2", false, "ns2-n2-updated"), + }, + appliedResources: []workapiv1.AppliedManifestResourceMeta{ + {Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"}, + {Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"}, + }, + manifests: []workapiv1.ManifestCondition{ + newManifest("", "v1", "secrets", "ns1", "n1"), + newManifest("", "v1", "secrets", "ns2", "n2"), + }, + validateAppliedManifestWorkActions: func(t *testing.T, actions []clienttesting.Action) { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - - action := actions[0].(clienttesting.GetAction) - resource, namespace, name := action.GetResource(), action.GetNamespace(), action.GetName() - if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}) || namespace != "ns3" || name != "n3" { - t.Fatal(spew.Sdump(actions)) - } - action = actions[1].(clienttesting.GetAction) - resource, namespace, name = action.GetResource(), action.GetNamespace(), action.GetName() - if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}) || namespace != "ns4" || name != "n4" { + work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.AppliedManifestWork) + if !reflect.DeepEqual(work.Status.AppliedResources, []workapiv1.AppliedManifestResourceMeta{ + {Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"}, + {Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2-updated"}, + }) { t.Fatal(spew.Sdump(actions)) } }, + expectedDeleteActions: []clienttesting.DeleteActionImpl{}, }, } @@ -207,7 +211,16 @@ func TestSyncManifestWork(t *testing.T) { t.Fatal(err) } c.validateAppliedManifestWorkActions(t, fakeClient.Actions()) - c.validateDynamicActions(t, fakeDynamicClient.Actions()) + + deleteActions := []clienttesting.DeleteActionImpl{} + for _, action := range fakeDynamicClient.Actions() { + if action.GetVerb() == "delete" { + deleteActions = append(deleteActions, action.(clienttesting.DeleteActionImpl)) + } + } + if !reflect.DeepEqual(c.expectedDeleteActions, deleteActions) { + t.Fatal(spew.Sdump(deleteActions)) + } queueLen := controllerContext.Queue().Len() if queueLen != c.expectedQueueLen { diff --git a/pkg/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go b/pkg/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go index d2c595501..65cd5c5d1 100644 --- a/pkg/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go +++ b/pkg/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go @@ -186,7 +186,6 @@ func TestSyncManifestWorkController(t *testing.T) { }, }, validateAppliedManifestWorkActions: func(t *testing.T, actions []clienttesting.Action) { - fmt.Printf("Actions are %#v\n", actions) if len(actions) != 1 { t.Errorf("Expect 2 actions on appliedmanifestwork, but have %d", len(actions)) } diff --git a/test/e2e/work_agent_test.go b/test/e2e/work_agent_test.go index e09aabfbe..51232e29e 100644 --- a/test/e2e/work_agent_test.go +++ b/test/e2e/work_agent_test.go @@ -214,7 +214,17 @@ var _ = ginkgo.Describe("Work agent", func() { {Version: "v1", Resource: "configmaps", Namespace: ns2, Name: "cm3"}, {Version: "v1", Resource: "namespaces", Name: ns1}, } - gomega.Expect(reflect.DeepEqual(appliedManifestWork.Status.AppliedResources, expectedAppliedResources)).To(gomega.BeTrue()) + actualAppliedResources := []workapiv1.AppliedManifestResourceMeta{} + for _, appliedResource := range appliedManifestWork.Status.AppliedResources { + actualAppliedResources = append(actualAppliedResources, workapiv1.AppliedManifestResourceMeta{ + Group: appliedResource.Group, + Version: appliedResource.Version, + Resource: appliedResource.Resource, + Namespace: appliedResource.Namespace, + Name: appliedResource.Name, + }) + } + gomega.Expect(reflect.DeepEqual(actualAppliedResources, expectedAppliedResources)).To(gomega.BeTrue()) ginkgo.By("update manifestwork") cmData := map[string]string{"x": "y"} diff --git a/test/integration/util/assertion.go b/test/integration/util/assertion.go index cec258a13..0ed9c15d7 100644 --- a/test/integration/util/assertion.go +++ b/test/integration/util/assertion.go @@ -181,6 +181,18 @@ func AssertAppliedResources(hubHash, workName string, gvrs []schema.GroupVersion return false } - return reflect.DeepEqual(appliedManifestWork.Status.AppliedResources, appliedResources) + // remove uid from each AppliedManifestResourceMeta + var actualAppliedResources []workapiv1.AppliedManifestResourceMeta + for _, appliedResource := range appliedManifestWork.Status.AppliedResources { + actualAppliedResources = append(actualAppliedResources, workapiv1.AppliedManifestResourceMeta{ + Group: appliedResource.Group, + Version: appliedResource.Version, + Resource: appliedResource.Resource, + Namespace: appliedResource.Namespace, + Name: appliedResource.Name, + }) + } + + return reflect.DeepEqual(actualAppliedResources, appliedResources) }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) }