fix: requeue the request when the override has been changed (#1097)

Author: zhiying-lin

pkg/controllers/workgenerator/controller.go

@@ -1530,16 +1530,31 @@ 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.
-					// When the normal update happens, the controller will set the applied condition as false and wait
-					// until the work condition has been changed.
+					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 override update happens, the rollout controller will set the applied condition as false
+					// and wait for the workGenerator to update it. The workGenerator will wait for the work status change,
+					// but here the status didn't change as the work's spec didn't change
 					// 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

@@ -130,7 +130,7 @@ var _ = Context("creating clusterResourceOverride (selecting all clusters) to ov
 		}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update cro as expected", crpName)
 	})
 
-	It("should update CRP status on demand as expected", func() {
+	It("should refresh the CRP status even as there is no change on the resources", func() {
 		wantCRONames := []string{fmt.Sprintf(placementv1alpha1.OverrideSnapshotNameFmt, croName, 1)}
 		crpStatusUpdatedActual := crpStatusWithOverrideUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, "0", wantCRONames, nil)
 		Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
@@ -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 refresh the CRP status even as there is no change on the resources", 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() {