From 7ff87da266cb129161bb62ee379cf9be98fc1834 Mon Sep 17 00:00:00 2001 From: Yang Le Date: Mon, 18 May 2020 16:29:56 +0800 Subject: [PATCH] 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,