From c360449bca908c36f8fc2704fca369d0de091ec2 Mon Sep 17 00:00:00 2001 From: Jian Qiu Date: Tue, 16 Mar 2021 13:31:42 +0800 Subject: [PATCH] Ignore version when finding untracked resource Signed-off-by: Jian Qiu --- .../appliedmanifestwork_controller.go | 11 +- .../appliedmanifestwork_controller_test.go | 14 ++ test/e2e/work_agent_test.go | 164 ++++++++++--- test/integration/work_test.go | 216 +++++++++++++++++- 4 files changed, 364 insertions(+), 41 deletions(-) diff --git a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go index edf85b73a..2e613f261 100644 --- a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go +++ b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go @@ -201,19 +201,26 @@ func (m *AppliedManifestWorkController) syncManifestWork( } // findUntrackedResources returns applied resources which are no longer tracked by manifestwork +// API version should be ignored when checking if a resource is no longer tracked by a manifestwork. +// This is because we treat resources of same GroupResource but different version equivalent. +// It also compares UID of the appliedResources to identify the untracked appliedResources because +// 1. The UID should keep the same for resources with different versions. +// 2. The UID in the newAppliedResources is always the latest updated one. The only possibility that UID +// in appliedResources differs from what in newAppliedResources is that this resource is recreated. +// Its UID in appliedResources is invalid hence recording it as untracked applied resource and delete it is safe. func findUntrackedResources(appliedResources, newAppliedResources []workapiv1.AppliedManifestResourceMeta) []workapiv1.AppliedManifestResourceMeta { var untracked []workapiv1.AppliedManifestResourceMeta resourceIndex := map[workapiv1.AppliedManifestResourceMeta]struct{}{} for _, resource := range newAppliedResources { key := resource.DeepCopy() - key.UID = "" + key.UID, key.Version = "", "" resourceIndex[*key] = struct{}{} } for _, resource := range appliedResources { key := resource.DeepCopy() - key.UID = "" + key.UID, key.Version = "", "" if _, ok := resourceIndex[*key]; !ok { untracked = append(untracked, resource) } diff --git a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go index 146a93301..43c525ad0 100644 --- a/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go +++ b/pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go @@ -275,6 +275,20 @@ func TestFindUntrackedResources(t *testing.T) { {Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"}, }, }, + { + name: "changing version of original resources does not make it untracked", + appliedResources: []workapiv1.AppliedManifestResourceMeta{ + {Group: "g1", Version: "v1", Resource: "r1", Namespace: "ns1", Name: "n1"}, + {Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"}, + }, + newAppliedResources: []workapiv1.AppliedManifestResourceMeta{ + {Group: "g1", Version: "v2", Resource: "r1", Namespace: "ns1", Name: "n1"}, + {Group: "g4", Version: "v4", Resource: "r4", Namespace: "ns4", Name: "n4"}, + }, + expectedUntrackedResources: []workapiv1.AppliedManifestResourceMeta{ + {Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"}, + }, + }, } for _, c := range cases { diff --git a/test/e2e/work_agent_test.go b/test/e2e/work_agent_test.go index eb00d2416..44518de4a 100644 --- a/test/e2e/work_agent_test.go +++ b/test/e2e/work_agent_test.go @@ -18,6 +18,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" ) @@ -25,8 +26,8 @@ const ( eventuallyTimeout = 60 // seconds eventuallyInterval = 1 // seconds - guestbookCrdJson = `{ - "apiVersion": "apiextensions.k8s.io/v1beta1", + guestBookCRDJson = `{ + "apiVersion": "apiextensions.k8s.io/v1", "kind": "CustomResourceDefinition", "metadata": { "name": "guestbooks.my.domain" @@ -42,39 +43,70 @@ const ( "plural": "guestbooks", "singular": "guestbook" }, - "preserveUnknownFields": true, + "preserveUnknownFields": false, "scope": "Namespaced", - "validation": { - "openAPIV3Schema": { - "properties": { - "apiVersion": { - "type": "string" - }, - "kind": { - "type": "string" - }, - "metadata": { - "type": "object" - }, - "spec": { - "properties": { - "foo": { - "type": "string" - } - }, - "type": "object" - }, - "status": { - "type": "object" - } - }, - "type": "object" - } - }, - "version": "v1", "versions": [ { "name": "v1", + "schema": { + "openAPIV3Schema": { + "properties": { + "apiVersion": { + "type": "string" + }, + "kind": { + "type": "string" + }, + "metadata": { + "type": "object" + }, + "spec": { + "properties": { + "foo": { + "type": "string" + } + }, + "type": "object" + }, + "status": { + "type": "object" + } + }, + "type": "object" + } + }, + "served": true, + "storage": false + }, + { + "name": "v2", + "schema": { + "openAPIV3Schema": { + "properties": { + "apiVersion": { + "type": "string" + }, + "kind": { + "type": "string" + }, + "metadata": { + "type": "object" + }, + "spec": { + "properties": { + "foo": { + "type": "string" + } + }, + "type": "object" + }, + "status": { + "type": "object" + } + }, + "type": "object" + } + }, "served": true, "storage": true } @@ -82,7 +114,7 @@ const ( } }` - guestbookCrJson = `{ + guestbookCRJson = `{ "apiVersion": "my.domain/v1", "kind": "Guestbook", "metadata": { @@ -93,6 +125,18 @@ const ( "foo": "bar" } }` + + upgradedGuestBookCRJson = `{ + "apiVersion": "my.domain/v2", + "kind": "Guestbook", + "metadata": { + "name": "guestbook1", + "namespace": "default" + }, + "spec": { + "foo": "foo" + } + }` ) var _ = ginkgo.Describe("Work agent", func() { @@ -366,13 +410,13 @@ var _ = ginkgo.Describe("Work agent", func() { }) ginkgo.It("should create crd/cr with aggregated cluster role successfully", func() { - crd, err := newCrd() + crd, err := newCrd(guestBookCRDJson) gomega.Expect(err).ToNot(gomega.HaveOccurred()) clusterRoleName := fmt.Sprintf("cr-%s", nameSuffix) clusterRole := newAggregatedClusterRole(clusterRoleName, "my.domain", "guestbooks") - cr, err := newCr(crNamespace, "cr1") + cr, err := newCr(guestbookCRJson, crNamespace, "cr1") gomega.Expect(err).ToNot(gomega.HaveOccurred()) objects := []runtime.Object{crd, clusterRole, cr} @@ -400,6 +444,50 @@ var _ = ginkgo.Describe("Work agent", func() { return meta.IsStatusConditionPresentAndEqual(work.Status.Conditions, workapiv1.WorkApplied, metav1.ConditionTrue) && meta.IsStatusConditionPresentAndEqual(work.Status.Conditions, workapiv1.WorkAvailable, metav1.ConditionTrue) }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + // Upgrade crd/cr and check if cr resource is recreated. + // Get UID of cr resource at first. + guestbook, err := spokeDynamicClient.Resource(schema.GroupVersionResource{ + Resource: "guestbooks", + Version: "v1", + Group: "my.domain", + }).Namespace(crNamespace).Get(context.Background(), "cr1", metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + currentUID := guestbook.GetUID() + + // Upgrade crd/cr in the work + cr, err = newCr(upgradedGuestBookCRJson, crNamespace, "cr1") + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + objects = []runtime.Object{crd, clusterRole, cr} + work = newManifestWork(clusterName, workName, objects...) + + // Update work + existingWork, err := hubWorkClient.WorkV1().ManifestWorks(clusterName).Get(context.Background(), work.Name, metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + work.ResourceVersion = existingWork.ResourceVersion + _, err = hubWorkClient.WorkV1().ManifestWorks(clusterName).Update(context.Background(), work, metav1.UpdateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // check if v2 cr is applied + gomega.Eventually(func() error { + guestbook, err := spokeDynamicClient.Resource(schema.GroupVersionResource{ + Resource: "guestbooks", + Version: "v2", + Group: "my.domain", + }).Namespace(crNamespace).Get(context.Background(), "cr1", metav1.GetOptions{}) + if err != nil { + return err + } + // Check if the spec in the cr is updated + if !reflect.DeepEqual(guestbook.Object["spec"], cr.Object["spec"]) { + return fmt.Errorf("Expect CR spec is updated, expected: %v, actual %v", cr.Object["spec"], guestbook.Object["spec"]) + } + // Ensure that the CR is updated rather than recreated + if currentUID != guestbook.GetUID() { + return fmt.Errorf("Expect UID to be the same, expected: %q, actual %q", currentUID, guestbook.GetUID()) + } + return nil + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) }) }) }) @@ -472,13 +560,13 @@ func haveManifestCondition(conditions []workapiv1.ManifestCondition, expectedTyp return true } -func newCrd() (crd *unstructured.Unstructured, err error) { - crd, err = loadResourceFromJSON(guestbookCrdJson) +func newCrd(content string) (crd *unstructured.Unstructured, err error) { + crd, err = loadResourceFromJSON(content) return crd, err } -func newCr(namespace, name string) (cr *unstructured.Unstructured, err error) { - cr, err = loadResourceFromJSON(guestbookCrJson) +func newCr(content, namespace, name string) (cr *unstructured.Unstructured, err error) { + cr, err = loadResourceFromJSON(content) if err != nil { return nil, err } diff --git a/test/integration/work_test.go b/test/integration/work_test.go index 3b06cb784..64ff2e8ca 100644 --- a/test/integration/work_test.go +++ b/test/integration/work_test.go @@ -541,7 +541,7 @@ var _ = ginkgo.Describe("ManifestWork", func() { ginkgo.It("should delete applied manifest work if it is orphan", func() { appliedManifestWork := &workapiv1.AppliedManifestWork{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("%s-fakeworrk", hubHash), + Name: fmt.Sprintf("%s-fakework", hubHash), }, Spec: workapiv1.AppliedManifestWorkSpec{ HubHash: hubHash, @@ -554,4 +554,218 @@ var _ = ginkgo.Describe("ManifestWork", func() { util.AssertAppliedManifestWorkDeleted(appliedManifestWork.Name, spokeWorkClient, eventuallyTimeout, eventuallyInterval) }) }) + + ginkgo.Context("Resource sharing and adoption between manifestworks", func() { + var anotherWork *workapiv1.ManifestWork + var anotherAppliedManifestWorkName string + ginkgo.BeforeEach(func() { + manifests = []workapiv1.Manifest{ + util.ToManifest(util.NewConfigmap(o.SpokeClusterName, "cm1", map[string]string{"a": "b"}, []string{})), + util.ToManifest(util.NewConfigmap(o.SpokeClusterName, "cm2", map[string]string{"c": "d"}, []string{})), + } + }) + + ginkgo.JustBeforeEach(func() { + // Create another manifestworks with one shared resource. + // TODO We might not want the sharing in this cases, since the content of a resource in two manifestworks + // can be different. + anotherWork = util.NewManifestWork(o.SpokeClusterName, "sharing-resource-work", []workapiv1.Manifest{manifests[0]}) + anotherWork, err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Create(context.Background(), anotherWork, metav1.CreateOptions{}) + anotherAppliedManifestWorkName = fmt.Sprintf("%s-%s", hubHash, anotherWork.Name) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + }) + + ginkgo.It("shared resource between the manifestwork should be recreated when one manifestwork is deleted", func() { + // Ensure two manifestworks are all applied + util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkApplied), metav1.ConditionTrue, + []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval) + util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkAvailable), metav1.ConditionTrue, + []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval) + util.AssertWorkCondition(anotherWork.Namespace, anotherWork.Name, hubWorkClient, string(workapiv1.WorkApplied), metav1.ConditionTrue, + []metav1.ConditionStatus{metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval) + util.AssertWorkCondition(anotherWork.Namespace, anotherWork.Name, hubWorkClient, string(workapiv1.WorkAvailable), metav1.ConditionTrue, + []metav1.ConditionStatus{metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval) + + // ensure configmap exists and get its uid + util.AssertExistenceOfConfigMaps(manifests, spokeKubeClient, eventuallyTimeout, eventuallyInterval) + curentConfigMap, err := spokeKubeClient.CoreV1().ConfigMaps(o.SpokeClusterName).Get(context.Background(), "cm1", metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + currentUID := curentConfigMap.UID + + // Ensure that uid recorded in the appliedmanifestwork and anotherappliedmanifestwork is correct. + gomega.Eventually(func() error { + appliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), appliedManifestWorkName, metav1.GetOptions{}) + if err != nil { + return err + } + + for _, appliedResource := range appliedManifestWork.Status.AppliedResources { + if appliedResource.Name == "cm1" && appliedResource.UID == string(currentUID) { + return nil + } + } + + return fmt.Errorf("Resource name or uid in appliedmanifestwork does not match") + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + + gomega.Eventually(func() error { + anotherappliedmanifestwork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), anotherAppliedManifestWorkName, metav1.GetOptions{}) + if err != nil { + return err + } + + for _, appliedResource := range anotherappliedmanifestwork.Status.AppliedResources { + if appliedResource.Name == "cm1" && appliedResource.UID == string(currentUID) { + return nil + } + } + + return fmt.Errorf("Resource name or uid in appliedmanifestwork does not match") + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + + // Delete one manifestwork + err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Delete(context.Background(), work.Name, metav1.DeleteOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Ensure the configmap is recreated and tracked by anotherappliedmanifestwork. + gomega.Eventually(func() error { + recreatedConfigMap, err := spokeKubeClient.CoreV1().ConfigMaps(o.SpokeClusterName).Get(context.Background(), "cm1", metav1.GetOptions{}) + if err != nil { + return err + } + + if currentUID == recreatedConfigMap.UID { + return fmt.Errorf("UID should not be equal") + } + + anotherappliedmanifestwork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), anotherAppliedManifestWorkName, metav1.GetOptions{}) + if err != nil { + return err + } + + for _, appliedResource := range anotherappliedmanifestwork.Status.AppliedResources { + if appliedResource.Name != "cm1" { + return fmt.Errorf("Resource Name should be cm1") + } + + if appliedResource.UID == string(currentUID) { + return fmt.Errorf("UID should not be equal") + } + } + + return nil + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + + // Ensure the appliedmanifestwork of deleted manifestwork is removed so it won't try to delete shared resource + gomega.Eventually(func() bool { + _, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), appliedManifestWorkName, metav1.GetOptions{}) + if errors.IsNotFound(err) { + return true + } + return false + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + }) + + ginkgo.It("shared resource between the manifestwork should be recreated when the shared resource is removed from one manifestwork", func() { + // Ensure two manifestworks are all applied + util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkApplied), metav1.ConditionTrue, + []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval) + util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkAvailable), metav1.ConditionTrue, + []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval) + util.AssertWorkCondition(anotherWork.Namespace, anotherWork.Name, hubWorkClient, string(workapiv1.WorkApplied), metav1.ConditionTrue, + []metav1.ConditionStatus{metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval) + util.AssertWorkCondition(anotherWork.Namespace, anotherWork.Name, hubWorkClient, string(workapiv1.WorkAvailable), metav1.ConditionTrue, + []metav1.ConditionStatus{metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval) + + // ensure configmap exists and get its uid + util.AssertExistenceOfConfigMaps(manifests, spokeKubeClient, eventuallyTimeout, eventuallyInterval) + curentConfigMap, err := spokeKubeClient.CoreV1().ConfigMaps(o.SpokeClusterName).Get(context.Background(), "cm1", metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + currentUID := curentConfigMap.UID + + // Ensure that uid recorded in the appliedmanifestwork and anotherappliedmanifestwork is correct. + gomega.Eventually(func() error { + appliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), appliedManifestWorkName, metav1.GetOptions{}) + if err != nil { + return err + } + + for _, appliedResource := range appliedManifestWork.Status.AppliedResources { + if appliedResource.Name == "cm1" && appliedResource.UID == string(currentUID) { + return nil + } + } + + return fmt.Errorf("Resource name or uid in appliedmanifestwork does not match") + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + + gomega.Eventually(func() error { + anotherAppliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), anotherAppliedManifestWorkName, metav1.GetOptions{}) + if err != nil { + return err + } + + for _, appliedResource := range anotherAppliedManifestWork.Status.AppliedResources { + if appliedResource.Name == "cm1" && appliedResource.UID == string(currentUID) { + return nil + } + } + + return fmt.Errorf("Resource name or uid in appliedmanifestwork does not match") + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + + // Update one manifestwork to remove the shared resource + work, err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Get(context.Background(), work.Name, metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + work.Spec.Workload.Manifests = []workapiv1.Manifest{manifests[1]} + _, err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Update(context.Background(), work, metav1.UpdateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Ensure the configmap is recreated and tracked by anotherappliedmanifestwork + gomega.Eventually(func() error { + recreatedConfigMap, err := spokeKubeClient.CoreV1().ConfigMaps(o.SpokeClusterName).Get(context.Background(), "cm1", metav1.GetOptions{}) + if err != nil { + return err + } + + if currentUID == recreatedConfigMap.UID { + return fmt.Errorf("UID should not be equal") + } + + anotherAppliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), anotherAppliedManifestWorkName, metav1.GetOptions{}) + if err != nil { + return err + } + + for _, appliedResource := range anotherAppliedManifestWork.Status.AppliedResources { + if appliedResource.Name != "cm1" { + return fmt.Errorf("Resource Name should be cm1") + } + + if appliedResource.UID == string(currentUID) { + return fmt.Errorf("UID should not be equal") + } + } + + return nil + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + + // ensure the resource is not tracked by the appliedmanifestwork. + gomega.Eventually(func() bool { + appliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), appliedManifestWorkName, metav1.GetOptions{}) + if err != nil { + return false + } + + for _, appliedResource := range appliedManifestWork.Status.AppliedResources { + if appliedResource.Name == "cm1" { + return false + } + } + + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + }) + }) })