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..3ae912e57 100644 --- a/pkg/helper/helpers.go +++ b/pkg/helper/helpers.go @@ -11,34 +11,78 @@ 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 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[workapiv1.ManifestResourceMeta]workapiv1.ManifestCondition{} + + duplicated := []workapiv1.ManifestResourceMeta{} + for _, condition := range conditions { + metaIndex[condition.ResourceMeta] = condition + if metaWithoutOridinal := resetOrdinal(condition.ResourceMeta); metaWithoutOridinal != (workapiv1.ManifestResourceMeta{}) { + if _, exists := metaWithoutOridinalIndex[metaWithoutOridinal]; exists { + duplicated = append(duplicated, metaWithoutOridinal) + } else { + metaWithoutOridinalIndex[metaWithoutOridinal] = condition + } } } - return nil + // remove metaWithoutOridinal from index if it is not unique + for _, metaWithoutOridinal := range duplicated { + delete(metaWithoutOridinalIndex, metaWithoutOridinal) + } + + // 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[resetOrdinal(newCondition.ResourceMeta)] + } + + // 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 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, } +} - existingCond := FindManifestConditionByIndex(cond.ResourceMeta.Ordinal, *conds) - if existingCond == nil { - *conds = append(*conds, cond) - return +func mergeManifestCondition(condition, newCondition workapiv1.ManifestCondition) workapiv1.ManifestCondition { + return workapiv1.ManifestCondition{ + ResourceMeta: newCondition.ResourceMeta, + Conditions: MergeStatusConditions(condition.Conditions, newCondition.Conditions), } - - existingCond.ResourceMeta = cond.ResourceMeta - existingCond.Conditions = cond.Conditions } // MergeStatusConditions returns a new status condition array with merged status conditions. It is based on newConditions, @@ -69,7 +113,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..ba90844be 100644 --- a/pkg/spoke/controllers/manifestwork_controller.go +++ b/pkg/spoke/controllers/manifestwork_controller.go @@ -130,36 +130,52 @@ 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 && 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 } } - 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)) + + 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 +407,21 @@ func allInCondition(conditionType string, manifests []workapiv1.ManifestConditio return exists, exists } + +func buildAppliedStatusCondition(err error) workapiv1.StatusCondition { + if err != nil { + return workapiv1.StatusCondition{ + Type: string(workapiv1.ManifestApplied), + Status: metav1.ConditionFalse, + Reason: "AppliedManifestFailed", + Message: fmt.Sprintf("Failed to apply manifest: %v", err), + } + } + + 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..e787c6737 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" ) @@ -53,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, @@ -207,7 +210,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 +350,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{