Merge pull request #9 from elgnay/merge-conditions

Update manifest condition merge logic

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))
+				}
 			}
 		})
 	}

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,

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",
+	}
+}

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{