fix UT and revert some changes

pkg/controllers/workgenerator/controller.go

@@ -151,6 +151,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req controllerruntime.Reques
 		resourceBinding.RemoveCondition(string(i.ResourceBindingConditionType()))
 	}
 	resourceBinding.Status.FailedPlacements = nil
+	resourceBinding.Status.DriftedPlacements = nil
+	resourceBinding.Status.DiffedPlacements = nil
 	if overrideSucceeded {
 		overrideReason := condition.OverriddenSucceededReason
 		overrideMessage := "Successfully applied the override rules on the resources"
@@ -1182,21 +1184,6 @@ func (r *Reconciler) SetupWithManager(mgr controllerruntime.Manager) error {
 						}
 					}
 				}
-				// we need to compare the drift placement
-				oldDriftedPlacements := extractDriftedResourcePlacementsFromWork(oldWork)
-				newDriftedPlacements := extractDriftedResourcePlacementsFromWork(newWork)
-				if utils.IsDriftedResourcePlacementsEqual(oldDriftedPlacements, newDriftedPlacements) {
-					klog.V(2).InfoS("The drifted placement list didn't change, no need to reconcile", "oldWork", klog.KObj(oldWork), "newWork", klog.KObj(newWork))
-					return
-				}
-
-				// we need to compare the diffed placement
-				oldDiffedPlacements := extractDiffedResourcePlacementsFromWork(oldWork)
-				newDiffedPlacements := extractDiffedResourcePlacementsFromWork(newWork)
-				if utils.IsDiffedResourcePlacementsEqual(oldDiffedPlacements, newDiffedPlacements) {
-					klog.V(2).InfoS("The diffed placement list didn't change, no need to reconcile", "oldWork", klog.KObj(oldWork), "newWork", klog.KObj(newWork))
-					return
-				}
 				// We need to update the binding status in this case
 				klog.V(2).InfoS("Received a work update event that we need to handle", "work", klog.KObj(newWork), "parentBindingName", parentBindingName)
 				queue.Add(reconcile.Request{NamespacedName: types.NamespacedName{

pkg/controllers/workgenerator/controller_test.go

@@ -1238,7 +1238,7 @@ func TestSetBindingStatus(t *testing.T) {
 									FirstDriftedObservedTime:          metav1.NewTime(timeNow.Add(-time.Hour)),
 									ObservedDrifts: []fleetv1beta1.PatchDetail{
 										{
-											Path:          "/spec/ports/1/port",
+											Path:          "/spec/ports/0/port",
 											ValueInHub:    "80",
 											ValueInMember: "90",
 										},
@@ -1339,7 +1339,7 @@ func TestSetBindingStatus(t *testing.T) {
 					FirstDriftedObservedTime:        metav1.NewTime(timeNow.Add(-time.Hour)),
 					ObservedDrifts: []fleetv1beta1.PatchDetail{
 						{
-							Path:          "/spec/ports/1/port",
+							Path:          "/spec/ports/0/port",
 							ValueInHub:    "80",
 							ValueInMember: "90",
 						},
@@ -1442,7 +1442,7 @@ func TestSetBindingStatus(t *testing.T) {
 									FirstDiffedObservedTime:           metav1.NewTime(timeNow.Add(-time.Second)),
 									ObservedDiffs: []fleetv1beta1.PatchDetail{
 										{
-											Path:          "/spec/ports/1/port",
+											Path:          "/spec/ports/0/port",
 											ValueInHub:    "80",
 											ValueInMember: "90",
 										},
@@ -1472,12 +1472,12 @@ func TestSetBindingStatus(t *testing.T) {
 						Group:     "",
 						Version:   "v1",
 						Kind:      "Service",
-						Name:      "svc-name-1",
+						Name:      "svc-name",
 						Namespace: "svc-namespace",
 					},
 					ObservationTime:                 metav1.NewTime(timeNow),
 					TargetClusterObservedGeneration: 2,
-					FirstDiffedObservedTime:         metav1.NewTime(timeNow.Add(-time.Second)),
+					FirstDiffedObservedTime:         metav1.NewTime(timeNow.Add(-time.Hour)),
 					ObservedDiffs: []fleetv1beta1.PatchDetail{
 						{
 							Path:          "/spec/ports/1/port",
@@ -1694,9 +1694,9 @@ func TestSetBindingStatus(t *testing.T) {
 			binding := &fleetv1beta1.ClusterResourceBinding{}
 			setBindingStatus(tt.works, binding)
 			got := binding.Status.FailedPlacements
-			// setBindingStatus is using map to populate the failedResourcePlacement.
+			// setBindingStatus is using map to populate the placements.
 			// There is no default order in traversing the map.
-			// When the result of failedResourcePlacement exceeds the limit, the result will be truncated and cannot be
+			// When the result of Placements exceeds the limit, the result will be truncated and cannot be
 			// guaranteed.
 			if maxFailedResourcePlacementLimit == len(tt.wantFailedResourcePlacements) {
 				opt := cmp.Comparer(func(x, y fleetv1beta1.FailedResourcePlacement) bool {
@@ -1735,8 +1735,8 @@ func TestSetBindingStatus(t *testing.T) {
 						}
 						return n1.Namespace < n2.Namespace
 					}),
-					cmpopts.SortSlices(func(f1, f2 fleetv1beta1.DriftedResourcePlacement) bool {
-						return f1.ResourceIdentifier.Kind < f2.ResourceIdentifier.Kind
+					cmp.Comparer(func(f1, f2 fleetv1beta1.DriftedResourcePlacement) bool {
+						return f1.ResourceIdentifier.Kind == f2.ResourceIdentifier.Kind
 					}),
 					cmp.Comparer(func(t1, t2 metav1.Time) bool {
 						if t1.Time.IsZero() || t2.Time.IsZero() {
@@ -1757,13 +1757,13 @@ func TestSetBindingStatus(t *testing.T) {
 
 			resourceCmpOptions := []cmp.Option{
 				cmpopts.SortSlices(func(i, j fleetv1beta1.DriftedResourcePlacement) bool {
-					if i.ResourceIdentifier.Group < j.ResourceIdentifier.Group {
+					if i.Group < j.Group {
 						return true
 					}
-					if i.ResourceIdentifier.Kind < j.ResourceIdentifier.Kind {
+					if i.Kind < j.Kind {
 						return true
 					}
-					return i.ResourceIdentifier.Name < j.ResourceIdentifier.Name
+					return i.Name < j.Name
 				}),
 			}
 			if diff := cmp.Diff(gotDrifted, tt.wantDriftedResourcePlacements, resourceCmpOptions...); diff != "" {
@@ -1804,13 +1804,13 @@ func TestSetBindingStatus(t *testing.T) {
 
 			resourceCmpOptions = []cmp.Option{
 				cmpopts.SortSlices(func(i, j fleetv1beta1.DiffedResourcePlacement) bool {
-					if i.ResourceIdentifier.Group < j.ResourceIdentifier.Group {
+					if i.Group < j.Group {
 						return true
 					}
-					if i.ResourceIdentifier.Kind < j.ResourceIdentifier.Kind {
+					if i.Kind < j.Kind {
 						return true
 					}
-					return i.ResourceIdentifier.Name < j.ResourceIdentifier.Name
+					return i.Name < j.Name
 				}),
 			}
 			if diff := cmp.Diff(gotDiffed, tt.wantDiffedResourcePlacements, resourceCmpOptions...); diff != "" {

pkg/utils/common.go

@@ -555,108 +555,6 @@ func IsFailedResourcePlacementsEqual(oldFailedResourcePlacements, newFailedResou
 	return true
 }
 
-// LessFuncDriftedResourcePlacements is a less function for sorting drifted resource placements
-var LessFuncDriftedResourcePlacements = func(a, b placementv1beta1.DriftedResourcePlacement) bool {
-	var aStr, bStr string
-	if a.Envelope != nil {
-		aStr = fmt.Sprintf(ResourceIdentifierWithEnvelopeIdentifierStringFormat, a.Group, a.Version, a.Kind, a.Namespace, a.Name, a.Envelope.Type, a.Envelope.Namespace, a.Envelope.Name)
-	} else {
-		aStr = fmt.Sprintf(ResourceIdentifierStringFormat, a.Group, a.Version, a.Kind, a.Namespace, a.Name)
-	}
-	if b.Envelope != nil {
-		bStr = fmt.Sprintf(ResourceIdentifierWithEnvelopeIdentifierStringFormat, b.Group, b.Version, b.Kind, b.Namespace, b.Name, b.Envelope.Type, b.Envelope.Namespace, b.Envelope.Name)
-	} else {
-		bStr = fmt.Sprintf(ResourceIdentifierStringFormat, b.Group, b.Version, b.Kind, b.Namespace, b.Name)
-
-	}
-	return aStr < bStr
-}
-
-func IsDriftedResourcePlacementsEqual(oldDriftedResourcePlacements, newDriftedResourcePlacements []placementv1beta1.DriftedResourcePlacement) bool {
-	if len(oldDriftedResourcePlacements) != len(newDriftedResourcePlacements) {
-		return false
-	}
-	sort.Slice(oldDriftedResourcePlacements, func(i, j int) bool {
-		return LessFuncDriftedResourcePlacements(oldDriftedResourcePlacements[i], oldDriftedResourcePlacements[j])
-	})
-	sort.Slice(newDriftedResourcePlacements, func(i, j int) bool {
-		return LessFuncDriftedResourcePlacements(newDriftedResourcePlacements[i], newDriftedResourcePlacements[j])
-	})
-	for i := range oldDriftedResourcePlacements {
-		oldDriftedResourcePlacement := oldDriftedResourcePlacements[i]
-		newDriftedResourcePlacement := newDriftedResourcePlacements[i]
-		if !equality.Semantic.DeepEqual(oldDriftedResourcePlacement.ResourceIdentifier, newDriftedResourcePlacement.ResourceIdentifier) {
-			return false
-		}
-		if !equality.Semantic.DeepEqual(oldDriftedResourcePlacement.ObservationTime, newDriftedResourcePlacement.ObservationTime) {
-			return false
-		}
-		if oldDriftedResourcePlacement.TargetClusterObservedGeneration != newDriftedResourcePlacement.TargetClusterObservedGeneration {
-			return false
-		}
-		if !equality.Semantic.DeepEqual(oldDriftedResourcePlacement.FirstDriftedObservedTime, newDriftedResourcePlacement.FirstDriftedObservedTime) {
-			return false
-		}
-		for j := range oldDriftedResourcePlacement.ObservedDrifts {
-			if !equality.Semantic.DeepEqual(oldDriftedResourcePlacement.ObservedDrifts[j], newDriftedResourcePlacement.ObservedDrifts[j]) {
-				return false
-			}
-		}
-	}
-	return true
-}
-
-// LessFuncDiffedResourcePlacements is a less function for sorting diffed resource placements
-var LessFuncDiffedResourcePlacements = func(a, b placementv1beta1.DiffedResourcePlacement) bool {
-	var aStr, bStr string
-	if a.Envelope != nil {
-		aStr = fmt.Sprintf(ResourceIdentifierWithEnvelopeIdentifierStringFormat, a.Group, a.Version, a.Kind, a.Namespace, a.Name, a.Envelope.Type, a.Envelope.Namespace, a.Envelope.Name)
-	} else {
-		aStr = fmt.Sprintf(ResourceIdentifierStringFormat, a.Group, a.Version, a.Kind, a.Namespace, a.Name)
-	}
-	if b.Envelope != nil {
-		bStr = fmt.Sprintf(ResourceIdentifierWithEnvelopeIdentifierStringFormat, b.Group, b.Version, b.Kind, b.Namespace, b.Name, b.Envelope.Type, b.Envelope.Namespace, b.Envelope.Name)
-	} else {
-		bStr = fmt.Sprintf(ResourceIdentifierStringFormat, b.Group, b.Version, b.Kind, b.Namespace, b.Name)
-
-	}
-	return aStr < bStr
-}
-
-func IsDiffedResourcePlacementsEqual(oldDiffedResourcePlacements, newDiffedResourcePlacements []placementv1beta1.DiffedResourcePlacement) bool {
-	if len(oldDiffedResourcePlacements) != len(newDiffedResourcePlacements) {
-		return false
-	}
-	sort.Slice(oldDiffedResourcePlacements, func(i, j int) bool {
-		return LessFuncDiffedResourcePlacements(oldDiffedResourcePlacements[i], oldDiffedResourcePlacements[j])
-	})
-	sort.Slice(newDiffedResourcePlacements, func(i, j int) bool {
-		return LessFuncDiffedResourcePlacements(newDiffedResourcePlacements[i], newDiffedResourcePlacements[j])
-	})
-	for i := range oldDiffedResourcePlacements {
-		oldDiffedResourcePlacement := oldDiffedResourcePlacements[i]
-		newDiffedResourcePlacement := newDiffedResourcePlacements[i]
-		if !equality.Semantic.DeepEqual(oldDiffedResourcePlacement.ResourceIdentifier, newDiffedResourcePlacement.ResourceIdentifier) {
-			return false
-		}
-		if !equality.Semantic.DeepEqual(oldDiffedResourcePlacement.ObservationTime, newDiffedResourcePlacement.ObservationTime) {
-			return false
-		}
-		if oldDiffedResourcePlacement.TargetClusterObservedGeneration != newDiffedResourcePlacement.TargetClusterObservedGeneration {
-			return false
-		}
-		if !equality.Semantic.DeepEqual(oldDiffedResourcePlacement.FirstDiffedObservedTime, newDiffedResourcePlacement.FirstDiffedObservedTime) {
-			return false
-		}
-		for j := range oldDiffedResourcePlacement.ObservedDiffs {
-			if !equality.Semantic.DeepEqual(oldDiffedResourcePlacement.ObservedDiffs[j], newDiffedResourcePlacement.ObservedDiffs[j]) {
-				return false
-			}
-		}
-	}
-	return true
-}
-
 // IsFleetAnnotationPresent returns true if a key with fleet prefix is present in the annotations map.
 func IsFleetAnnotationPresent(annotations map[string]string) bool {
 	for k := range annotations {