From 484dafb5edfe23261959e654a5900b0318703551 Mon Sep 17 00:00:00 2001 From: Yang Le Date: Mon, 11 May 2020 18:49:10 +0800 Subject: [PATCH 1/2] Update manifest condition merge logic --- pkg/helper/helper_test.go | 66 +++++++++---- pkg/helper/helpers.go | 92 ++++++++++++++----- .../controllers/manifestwork_controller.go | 72 ++++++++++----- .../manifestwork_controller_test.go | 17 +++- 4 files changed, 184 insertions(+), 63 deletions(-) diff --git a/pkg/helper/helper_test.go b/pkg/helper/helper_test.go index 615981691..93405b5c0 100644 --- a/pkg/helper/helper_test.go +++ b/pkg/helper/helper_test.go @@ -2,7 +2,6 @@ package helper import ( "context" - "fmt" "testing" "time" @@ -136,17 +135,21 @@ func TestUpdateStatusCondition(t *testing.T) { } // TestSetManifestCondition tests SetManifestCondition function -func TestSetManifestCondition(t *testing.T) { +func TestMergeManifestConditions(t *testing.T) { + transitionTime := metav1.Now() + cases := []struct { name string startingConditions []workapiv1.ManifestCondition - newCondition workapiv1.ManifestCondition + newConditions []workapiv1.ManifestCondition expectedConditions []workapiv1.ManifestCondition }{ { name: "add to empty", startingConditions: []workapiv1.ManifestCondition{}, - newCondition: newManifestCondition(0, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), + newConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), + }, expectedConditions: []workapiv1.ManifestCondition{ newManifestCondition(0, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), }, @@ -156,33 +159,64 @@ func TestSetManifestCondition(t *testing.T) { startingConditions: []workapiv1.ManifestCondition{ newManifestCondition(0, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), }, - newCondition: newManifestCondition(1, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), + newConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), + newManifestCondition(0, "resource2", newCondition("two", "True", "my-reason", "my-message", nil)), + }, expectedConditions: []workapiv1.ManifestCondition{ newManifestCondition(0, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), - newManifestCondition(1, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), + newManifestCondition(0, "resource2", newCondition("two", "True", "my-reason", "my-message", nil)), }, }, { name: "update existing", startingConditions: []workapiv1.ManifestCondition{ - newManifestCondition(2, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), - newManifestCondition(1, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), + newManifestCondition(0, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), + }, + newConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource1", newCondition("two", "True", "my-reason", "my-message", nil)), }, - newCondition: newManifestCondition(1, "resource2", newCondition("two", "True", "my-reason", "my-message", nil)), expectedConditions: []workapiv1.ManifestCondition{ - newManifestCondition(2, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), - newManifestCondition(1, "resource2", newCondition("two", "True", "my-reason", "my-message", nil)), + newManifestCondition(0, "resource1", newCondition("two", "True", "my-reason", "my-message", nil)), + }, + }, + { + name: "remove useless", + startingConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource1", newCondition("one", "True", "my-reason", "my-message", nil)), + newManifestCondition(1, "resource2", newCondition("two", "True", "my-reason", "my-message", &transitionTime)), + }, + newConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource2", newCondition("two", "True", "my-reason", "my-message", nil)), + }, + expectedConditions: []workapiv1.ManifestCondition{ + newManifestCondition(0, "resource2", newCondition("two", "True", "my-reason", "my-message", &transitionTime)), }, }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - actual := c.startingConditions - SetManifestCondition(&actual, c.newCondition) - fmt.Printf("found con %v\n", actual) - if !equality.Semantic.DeepEqual(actual, c.expectedConditions) { - t.Errorf(diff.ObjectDiff(actual, c.expectedConditions)) + merged := MergeManifestConditions(c.startingConditions, c.newConditions) + + if len(merged) != len(c.expectedConditions) { + t.Errorf("expected condition size %d but got: %d", len(c.expectedConditions), len(merged)) + } + + for i, expectedCondition := range c.expectedConditions { + actualCondition := merged[i] + if len(actualCondition.Conditions) != len(expectedCondition.Conditions) { + t.Errorf("expected condition size %d but got: %d", len(expectedCondition.Conditions), len(actualCondition.Conditions)) + } + for j, expect := range expectedCondition.Conditions { + if expect.LastTransitionTime == (metav1.Time{}) { + actualCondition.Conditions[j].LastTransitionTime = metav1.Time{} + } + } + + if !equality.Semantic.DeepEqual(actualCondition, expectedCondition) { + t.Errorf(diff.ObjectDiff(actualCondition, expectedCondition)) + } } }) } diff --git a/pkg/helper/helpers.go b/pkg/helper/helpers.go index 3ffb04f85..79770c767 100644 --- a/pkg/helper/helpers.go +++ b/pkg/helper/helpers.go @@ -2,6 +2,7 @@ package helper import ( "context" + "fmt" "time" workv1client "github.com/open-cluster-management/api/client/work/clientset/versioned/typed/work/v1" @@ -11,34 +12,83 @@ import ( "k8s.io/client-go/util/retry" ) -// FindManifestConditionByIndex finds manifest condition by index in manifests -func FindManifestConditionByIndex(index int32, conds []workapiv1.ManifestCondition) *workapiv1.ManifestCondition { - // Finds the cond conds that ordinal is the same as index - if conds == nil { - return nil - } - for i, cond := range conds { - if index == cond.ResourceMeta.Ordinal { - return &conds[i] +// MergeManifestConditions return a new ManifestCondition array which merges the existing manifest +// conditions and the new manifest conditions. Rules to match ManifestCondition between two arrays: +// 1. match the manifest condition with the whole ManifestResourceMeta; +// 2. if not matched, try to match with properties in ManifestResourceMeta other than ordinal +// 3. otherwise, try to match with ordinal only in ManifestResourceMeta +func MergeManifestConditions(conditions, newConditions []workapiv1.ManifestCondition) []workapiv1.ManifestCondition { + merged := []workapiv1.ManifestCondition{} + + // build search indices + metaIndex := map[workapiv1.ManifestResourceMeta]workapiv1.ManifestCondition{} + metaWithoutOridinalIndex := map[string]workapiv1.ManifestCondition{} + ordinalIndex := map[int32]workapiv1.ManifestCondition{} + + duplicateKeys := []string{} + for _, condition := range conditions { + metaIndex[condition.ResourceMeta] = condition + if key := manifestResourceMetaKey(condition.ResourceMeta); key != "" { + if _, exists := metaWithoutOridinalIndex[key]; exists { + duplicateKeys = append(duplicateKeys, key) + } else { + metaWithoutOridinalIndex[key] = condition + } } + ordinalIndex[condition.ResourceMeta.Ordinal] = condition } - return nil + // remove key from index if it is not unique + for _, key := range duplicateKeys { + delete(metaWithoutOridinalIndex, key) + } + + // try to match and merge manifest conditions + for _, newCondition := range newConditions { + // match with ResourceMeta + condition, ok := metaIndex[newCondition.ResourceMeta] + + // match with properties in ResourceMeta other than ordinal if not found yet + if !ok { + condition, ok = metaWithoutOridinalIndex[manifestResourceMetaKey(newCondition.ResourceMeta)] + } + + // match with ordinal in ResourceMeta if not found yet + if !ok { + condition, ok = ordinalIndex[newCondition.ResourceMeta.Ordinal] + } + + // if there is existing condition, merge it with new condition + if ok { + merged = append(merged, mergeManifestCondition(condition, newCondition)) + continue + } + + // otherwise use the new condition + for i := range newCondition.Conditions { + newCondition.Conditions[i].LastTransitionTime = metav1.NewTime(time.Now()) + } + + merged = append(merged, newCondition) + } + + return merged } -func SetManifestCondition(conds *[]workapiv1.ManifestCondition, cond workapiv1.ManifestCondition) { - if conds == nil { - conds = &[]workapiv1.ManifestCondition{} +func manifestResourceMetaKey(meta workapiv1.ManifestResourceMeta) string { + key := fmt.Sprintf("%s:%s:%s:%s:%s:%s", meta.Group, meta.Version, meta.Kind, meta.Resource, meta.Namespace, meta.Name) + if len(key) == 5 { + return "" } - existingCond := FindManifestConditionByIndex(cond.ResourceMeta.Ordinal, *conds) - if existingCond == nil { - *conds = append(*conds, cond) - return - } + return key +} - existingCond.ResourceMeta = cond.ResourceMeta - existingCond.Conditions = cond.Conditions +func mergeManifestCondition(condition, newCondition workapiv1.ManifestCondition) workapiv1.ManifestCondition { + return workapiv1.ManifestCondition{ + ResourceMeta: newCondition.ResourceMeta, + Conditions: MergeStatusConditions(condition.Conditions, newCondition.Conditions), + } } // MergeStatusConditions returns a new status condition array with merged status conditions. It is based on newConditions, @@ -69,7 +119,7 @@ func MergeStatusConditions(conditions []workapiv1.StatusCondition, newConditions // It assumes the two conditions have the same condition type. func mergeStatusCondition(condition, newCondition workapiv1.StatusCondition) workapiv1.StatusCondition { merged := workapiv1.StatusCondition{ - Type: condition.Type, + Type: newCondition.Type, Status: newCondition.Status, Reason: newCondition.Reason, Message: newCondition.Message, diff --git a/pkg/spoke/controllers/manifestwork_controller.go b/pkg/spoke/controllers/manifestwork_controller.go index 41969a98b..944213167 100644 --- a/pkg/spoke/controllers/manifestwork_controller.go +++ b/pkg/spoke/controllers/manifestwork_controller.go @@ -130,36 +130,42 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac errs := []error{} // Apply resources on spoke cluster. resourceResults := m.applyManifest(manifestWork.Spec.Workload.Manifests, controllerContext.Recorder()) + newManifestConditions := []workapiv1.ManifestCondition{} for index, result := range resourceResults { - manifestCondition := helper.FindManifestConditionByIndex(int32(index), manifestWork.Status.ResourceStatus.Manifests) - if manifestCondition == nil { - manifestCondition = &workapiv1.ManifestCondition{ - ResourceMeta: workapiv1.ManifestResourceMeta{ - Ordinal: int32(index), - }, - Conditions: []workapiv1.StatusCondition{}, + if result.Error != nil { + errs = append(errs, result.Error) + } + + manifestCondition := workapiv1.ManifestCondition{ + ResourceMeta: workapiv1.ManifestResourceMeta{ + Ordinal: int32(index), + }, + Conditions: []workapiv1.StatusCondition{}, + } + + if result.Result != nil { + gvk := result.Result.GetObjectKind().GroupVersionKind() + manifestCondition.ResourceMeta.Group = gvk.Group + manifestCondition.ResourceMeta.Version = gvk.Version + manifestCondition.ResourceMeta.Kind = gvk.Kind + + if accessor, err := meta.Accessor(result.Result); err != nil { + errs = append(errs, fmt.Errorf("cannot access metadata of %v: %w", result.Result, err)) + } else { + manifestCondition.ResourceMeta.Namespace = accessor.GetNamespace() + manifestCondition.ResourceMeta.Name = accessor.GetName() } } - if result.Error != nil { - errs = append(errs, result.Error) - helper.SetStatusCondition(&manifestCondition.Conditions, workapiv1.StatusCondition{ - Type: string(workapiv1.ManifestApplied), - Status: metav1.ConditionFalse, - Reason: "AppliedManifestFailed", - Message: "Failed to apply manifest", - }) - } else { - helper.SetStatusCondition(&manifestCondition.Conditions, workapiv1.StatusCondition{ - Type: string(workapiv1.ManifestApplied), - Status: metav1.ConditionTrue, - Reason: "AppliedManifestComplete", - Message: "Apply manifest complete", - }) - } - helper.SetManifestCondition(&manifestWork.Status.ResourceStatus.Manifests, *manifestCondition) + // Add applied status condition + manifestCondition.Conditions = append(manifestCondition.Conditions, buildAppliedStatusCondition(result.Error != nil)) + + newManifestConditions = append(newManifestConditions, manifestCondition) } + // merge the new manifest conditions with the existing manifest conditions + manifestWork.Status.ResourceStatus.Manifests = helper.MergeManifestConditions(manifestWork.Status.ResourceStatus.Manifests, newManifestConditions) + // TODO find resources to be deleted if it is not owned by manifestwork any longer // Update work status @@ -391,3 +397,21 @@ func allInCondition(conditionType string, manifests []workapiv1.ManifestConditio return exists, exists } + +func buildAppliedStatusCondition(failed bool) workapiv1.StatusCondition { + if failed { + return workapiv1.StatusCondition{ + Type: string(workapiv1.ManifestApplied), + Status: metav1.ConditionFalse, + Reason: "AppliedManifestFailed", + Message: "Failed to apply manifest", + } + } + + return workapiv1.StatusCondition{ + Type: string(workapiv1.ManifestApplied), + Status: metav1.ConditionTrue, + Reason: "AppliedManifestComplete", + Message: "Apply manifest complete", + } +} diff --git a/pkg/spoke/controllers/manifestwork_controller_test.go b/pkg/spoke/controllers/manifestwork_controller_test.go index b6fd86fa6..fbf0ab4e0 100644 --- a/pkg/spoke/controllers/manifestwork_controller_test.go +++ b/pkg/spoke/controllers/manifestwork_controller_test.go @@ -22,7 +22,6 @@ 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/helper" "github.com/open-cluster-management/work/pkg/spoke/resource" ) @@ -207,7 +206,7 @@ func assertCondition(t *testing.T, conditions []workapiv1.StatusCondition, expec func assertManifestCondition( t *testing.T, conds []workapiv1.ManifestCondition, index int32, expectedCondition string, expectedStatus metav1.ConditionStatus) { - cond := helper.FindManifestConditionByIndex(index, conds) + cond := findManifestConditionByIndex(index, conds) if cond == nil { t.Errorf("expected to find the condition with index %d", index) } @@ -347,6 +346,20 @@ func newManifestCondition(ordinal int32, resource string, conds ...workapiv1.Sta } } +func findManifestConditionByIndex(index int32, conds []workapiv1.ManifestCondition) *workapiv1.ManifestCondition { + // Finds the cond conds that ordinal is the same as index + if conds == nil { + return nil + } + for i, cond := range conds { + if index == cond.ResourceMeta.Ordinal { + return &conds[i] + } + } + + return nil +} + // TestSync test cases when running sync func TestSync(t *testing.T) { cases := []*testCase{ From 7ff87da266cb129161bb62ee379cf9be98fc1834 Mon Sep 17 00:00:00 2001 From: Yang Le Date: Mon, 18 May 2020 16:29:56 +0800 Subject: [PATCH 2/2] Refine manifest condition merge logic --- pkg/helper/helpers.go | 46 ++++++++----------- .../controllers/manifestwork_controller.go | 20 ++++++-- .../manifestwork_controller_test.go | 4 ++ 3 files changed, 39 insertions(+), 31 deletions(-) diff --git a/pkg/helper/helpers.go b/pkg/helper/helpers.go index 79770c767..3ae912e57 100644 --- a/pkg/helper/helpers.go +++ b/pkg/helper/helpers.go @@ -2,7 +2,6 @@ package helper import ( "context" - "fmt" "time" workv1client "github.com/open-cluster-management/api/client/work/clientset/versioned/typed/work/v1" @@ -15,32 +14,30 @@ import ( // MergeManifestConditions return a new ManifestCondition array which merges the existing manifest // conditions and the new manifest conditions. Rules to match ManifestCondition between two arrays: // 1. match the manifest condition with the whole ManifestResourceMeta; -// 2. if not matched, try to match with properties in ManifestResourceMeta other than ordinal -// 3. otherwise, try to match with ordinal only in ManifestResourceMeta +// 2. if not matched, try to match with properties other than ordinal in ManifestResourceMeta +// If no existing manifest condition is matched, the new manifest condition will be used. func MergeManifestConditions(conditions, newConditions []workapiv1.ManifestCondition) []workapiv1.ManifestCondition { merged := []workapiv1.ManifestCondition{} // build search indices metaIndex := map[workapiv1.ManifestResourceMeta]workapiv1.ManifestCondition{} - metaWithoutOridinalIndex := map[string]workapiv1.ManifestCondition{} - ordinalIndex := map[int32]workapiv1.ManifestCondition{} + metaWithoutOridinalIndex := map[workapiv1.ManifestResourceMeta]workapiv1.ManifestCondition{} - duplicateKeys := []string{} + duplicated := []workapiv1.ManifestResourceMeta{} for _, condition := range conditions { metaIndex[condition.ResourceMeta] = condition - if key := manifestResourceMetaKey(condition.ResourceMeta); key != "" { - if _, exists := metaWithoutOridinalIndex[key]; exists { - duplicateKeys = append(duplicateKeys, key) + if metaWithoutOridinal := resetOrdinal(condition.ResourceMeta); metaWithoutOridinal != (workapiv1.ManifestResourceMeta{}) { + if _, exists := metaWithoutOridinalIndex[metaWithoutOridinal]; exists { + duplicated = append(duplicated, metaWithoutOridinal) } else { - metaWithoutOridinalIndex[key] = condition + metaWithoutOridinalIndex[metaWithoutOridinal] = condition } } - ordinalIndex[condition.ResourceMeta.Ordinal] = condition } - // remove key from index if it is not unique - for _, key := range duplicateKeys { - delete(metaWithoutOridinalIndex, key) + // remove metaWithoutOridinal from index if it is not unique + for _, metaWithoutOridinal := range duplicated { + delete(metaWithoutOridinalIndex, metaWithoutOridinal) } // try to match and merge manifest conditions @@ -50,12 +47,7 @@ func MergeManifestConditions(conditions, newConditions []workapiv1.ManifestCondi // match with properties in ResourceMeta other than ordinal if not found yet if !ok { - condition, ok = metaWithoutOridinalIndex[manifestResourceMetaKey(newCondition.ResourceMeta)] - } - - // match with ordinal in ResourceMeta if not found yet - if !ok { - condition, ok = ordinalIndex[newCondition.ResourceMeta.Ordinal] + condition, ok = metaWithoutOridinalIndex[resetOrdinal(newCondition.ResourceMeta)] } // if there is existing condition, merge it with new condition @@ -75,13 +67,15 @@ func MergeManifestConditions(conditions, newConditions []workapiv1.ManifestCondi return merged } -func manifestResourceMetaKey(meta workapiv1.ManifestResourceMeta) string { - key := fmt.Sprintf("%s:%s:%s:%s:%s:%s", meta.Group, meta.Version, meta.Kind, meta.Resource, meta.Namespace, meta.Name) - if len(key) == 5 { - return "" +func resetOrdinal(meta workapiv1.ManifestResourceMeta) workapiv1.ManifestResourceMeta { + return workapiv1.ManifestResourceMeta{ + Group: meta.Group, + Version: meta.Version, + Kind: meta.Kind, + Resource: meta.Resource, + Name: meta.Name, + Namespace: meta.Namespace, } - - return key } func mergeManifestCondition(condition, newCondition workapiv1.ManifestCondition) workapiv1.ManifestCondition { diff --git a/pkg/spoke/controllers/manifestwork_controller.go b/pkg/spoke/controllers/manifestwork_controller.go index 944213167..ba90844be 100644 --- a/pkg/spoke/controllers/manifestwork_controller.go +++ b/pkg/spoke/controllers/manifestwork_controller.go @@ -143,22 +143,32 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac Conditions: []workapiv1.StatusCondition{}, } - if result.Result != nil { + if result.Result != nil && result.Result.GetObjectKind() != nil { gvk := result.Result.GetObjectKind().GroupVersionKind() + + // set gvk manifestCondition.ResourceMeta.Group = gvk.Group manifestCondition.ResourceMeta.Version = gvk.Version manifestCondition.ResourceMeta.Kind = gvk.Kind + // set namespace/name if accessor, err := meta.Accessor(result.Result); err != nil { errs = append(errs, fmt.Errorf("cannot access metadata of %v: %w", result.Result, err)) } else { manifestCondition.ResourceMeta.Namespace = accessor.GetNamespace() manifestCondition.ResourceMeta.Name = accessor.GetName() } + + // set resource + if mapping, err := m.restMapper.MappingForGVK(gvk); err != nil { + errs = append(errs, fmt.Errorf("unable to get rest mapping of gvk %v: %w", gvk, err)) + } else if mapping != nil { + manifestCondition.ResourceMeta.Resource = mapping.Resource.Resource + } } // Add applied status condition - manifestCondition.Conditions = append(manifestCondition.Conditions, buildAppliedStatusCondition(result.Error != nil)) + manifestCondition.Conditions = append(manifestCondition.Conditions, buildAppliedStatusCondition(result.Error)) newManifestConditions = append(newManifestConditions, manifestCondition) } @@ -398,13 +408,13 @@ func allInCondition(conditionType string, manifests []workapiv1.ManifestConditio return exists, exists } -func buildAppliedStatusCondition(failed bool) workapiv1.StatusCondition { - if failed { +func buildAppliedStatusCondition(err error) workapiv1.StatusCondition { + if err != nil { return workapiv1.StatusCondition{ Type: string(workapiv1.ManifestApplied), Status: metav1.ConditionFalse, Reason: "AppliedManifestFailed", - Message: "Failed to apply manifest", + Message: fmt.Sprintf("Failed to apply manifest: %v", err), } } diff --git a/pkg/spoke/controllers/manifestwork_controller_test.go b/pkg/spoke/controllers/manifestwork_controller_test.go index fbf0ab4e0..e787c6737 100644 --- a/pkg/spoke/controllers/manifestwork_controller_test.go +++ b/pkg/spoke/controllers/manifestwork_controller_test.go @@ -52,6 +52,10 @@ func (f fakeSyncContext) Recorder() events.Recorder { return f.reco func newSecret(name, namespace string, content string) *corev1.Secret { return &corev1.Secret{ + TypeMeta: metav1.TypeMeta{ + Kind: "Secret", + APIVersion: "v1", + }, ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace,