fix: requeue the request when the override has been changed

Author: zhiying-lin

pkg/controllers/workgenerator/controller.go

@@ -144,7 +144,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req controllerruntime.Reques
 
 	workUpdated := false
 	overrideSucceeded := false
-	// list all the corresponding works
+	// list all the correspo/nding works
 	works, syncErr := r.listAllWorksAssociated(ctx, &resourceBinding)
 	if syncErr == nil {
 		// generate and apply the workUpdated works if we have all the works
@@ -1530,16 +1530,30 @@ func (r *Reconciler) SetupWithManager(mgr controllerruntime.Manager) error {
 					newResourceSnapshot := newWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel]
 					if oldResourceSnapshot == "" || newResourceSnapshot == "" {
 						klog.ErrorS(controller.NewUnexpectedBehaviorError(errors.New("found an invalid work without parent-resource-snapshot-index")),
-							"Could not find the parent resource snapshot index label", "oldWork", klog.KObj(oldWork), "oldResourceSnapshotLabelValue", oldResourceSnapshot,
-							"newWork", klog.KObj(newWork), "newResourceSnapshotLabelValue", newResourceSnapshot)
+							"Could not find the parent resource snapshot index label", "oldWork", klog.KObj(oldWork), "oldWorkLabels", oldWork.Labels,
+							"newWork", klog.KObj(newWork), "newWorkLabels", newWork.Labels)
 						return
 					}
-					// There is an edge case that, the work spec is the same but from different resourceSnapshots.
-					// WorkGenerator will update the work because of the label changes, but the generation is the same.
+					oldClusterResourceOverrideSnapshotHash := oldWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation]
+					newClusterResourceOverrideSnapshotHash := newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation]
+					oldResourceOverrideSnapshotHash := oldWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation]
+					newResourceOverrideSnapshotHash := newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation]
+					if oldClusterResourceOverrideSnapshotHash == "" || newClusterResourceOverrideSnapshotHash == "" ||
+						oldResourceOverrideSnapshotHash == "" || newResourceOverrideSnapshotHash == "" {
+						klog.ErrorS(controller.NewUnexpectedBehaviorError(errors.New("found an invalid work without override-snapshot-hash")),
+							"Could not find the override-snapshot-hash annotation", "oldWork", klog.KObj(oldWork), "oldWorkAnnotations", oldWork.Annotations,
+							"newWork", klog.KObj(newWork), "newWorkAnnotations", newWork.Annotations)
+						return
+					}
+
+					// There is an edge case that, the work spec is the same but from different resourceSnapshots or resourceOverrideSnapshots.
+					// WorkGenerator will update the work because of the label/annotation changes, but the generation is the same.
 					// When the normal update happens, the controller will set the applied condition as false and wait
 					// until the work condition has been changed.
 					// In this edge case, we need to requeue the binding to update the binding status.
-					if oldResourceSnapshot == newResourceSnapshot {
+					if oldResourceSnapshot == newResourceSnapshot &&
+						oldClusterResourceOverrideSnapshotHash == newClusterResourceOverrideSnapshotHash &&
+						oldResourceOverrideSnapshotHash == newResourceOverrideSnapshotHash {
 						klog.V(2).InfoS("The work applied or available condition stayed as true, no need to reconcile", "oldWork", klog.KObj(oldWork), "newWork", klog.KObj(newWork))
 						return
 					}

test/e2e/placement_cro_test.go

@@ -144,6 +144,44 @@ var _ = Context("creating clusterResourceOverride (selecting all clusters) to ov
 		checkIfOverrideAnnotationsOnAllMemberClusters(true, want)
 	})
 
+	It("update cro attached to this CRP only and no updates on the namespace", func() {
+		Eventually(func() error {
+			cro := &placementv1alpha1.ClusterResourceOverride{}
+			if err := hubClient.Get(ctx, types.NamespacedName{Name: croName}, cro); err != nil {
+				return err
+			}
+			cro.Spec.Policy.OverrideRules = append(cro.Spec.Policy.OverrideRules, placementv1alpha1.OverrideRule{
+				ClusterSelector: &placementv1beta1.ClusterSelector{
+					ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{
+						{
+							LabelSelector: &metav1.LabelSelector{
+								MatchLabels: map[string]string{
+									"invalid-key": "invalid-value",
+								},
+							},
+						},
+					},
+				},
+				OverrideType: placementv1alpha1.DeleteOverrideType,
+			})
+			return hubClient.Update(ctx, cro)
+		}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update cro as expected", crpName)
+	})
+
+	It("should update CRP status on demand as expected", func() {
+		wantCRONames := []string{fmt.Sprintf(placementv1alpha1.OverrideSnapshotNameFmt, croName, 2)}
+		crpStatusUpdatedActual := crpStatusWithOverrideUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, "0", wantCRONames, nil)
+		Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
+	})
+
+	// This check will ignore the annotation of resources.
+	It("should place the selected resources on member clusters", checkIfPlacedWorkResourcesOnAllMemberClusters)
+
+	It("should have new override annotation value on the placed resources", func() {
+		want := map[string]string{croTestAnnotationKey: croTestAnnotationValue1}
+		checkIfOverrideAnnotationsOnAllMemberClusters(true, want)
+	})
+
 	It("delete the cro attached to this CRP", func() {
 		cleanupClusterResourceOverride(croName)
 	})

test/e2e/placement_ro_test.go

@@ -143,6 +143,46 @@ var _ = Context("creating resourceOverride (selecting all clusters) to override
 		want := map[string]string{roTestAnnotationKey: roTestAnnotationValue1}
 		checkIfOverrideAnnotationsOnAllMemberClusters(false, want)
 	})
+
+	It("update ro attached to this CRP only and no update on the configmap itself", func() {
+		Eventually(func() error {
+			ro := &placementv1alpha1.ResourceOverride{}
+			if err := hubClient.Get(ctx, types.NamespacedName{Name: roName, Namespace: roNamespace}, ro); err != nil {
+				return err
+			}
+			ro.Spec.Policy.OverrideRules = append(ro.Spec.Policy.OverrideRules, placementv1alpha1.OverrideRule{
+				ClusterSelector: &placementv1beta1.ClusterSelector{
+					ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{
+						{
+							LabelSelector: &metav1.LabelSelector{
+								MatchLabels: map[string]string{
+									"invalid-key": "invalid-value",
+								},
+							},
+						},
+					},
+				},
+				OverrideType: placementv1alpha1.DeleteOverrideType,
+			})
+			return hubClient.Update(ctx, ro)
+		}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update ro as expected", crpName)
+	})
+
+	It("should update CRP status as expected", func() {
+		wantRONames := []placementv1beta1.NamespacedName{
+			{Namespace: roNamespace, Name: fmt.Sprintf(placementv1alpha1.OverrideSnapshotNameFmt, roName, 2)},
+		}
+		crpStatusUpdatedActual := crpStatusWithOverrideUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, "0", nil, wantRONames)
+		Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
+	})
+
+	// This check will ignore the annotation of resources.
+	It("should place the selected resources on member clusters", checkIfPlacedWorkResourcesOnAllMemberClusters)
+
+	It("should have override annotations on the configmap", func() {
+		want := map[string]string{roTestAnnotationKey: roTestAnnotationValue1}
+		checkIfOverrideAnnotationsOnAllMemberClusters(false, want)
+	})
 })
 
 var _ = Context("creating resourceOverride with multiple jsonPatchOverrides to override configMap", Ordered, func() {