From 3608d84374a4ce7ea7defffae5a71812c2b5f9d7 Mon Sep 17 00:00:00 2001 From: Zhiying Lin Date: Tue, 25 Mar 2025 16:38:38 +0800 Subject: [PATCH 1/2] fix: requeue the request when the override has been changed --- pkg/controllers/workgenerator/controller.go | 24 ++++++++++--- test/e2e/placement_cro_test.go | 38 ++++++++++++++++++++ test/e2e/placement_ro_test.go | 40 +++++++++++++++++++++ 3 files changed, 97 insertions(+), 5 deletions(-) diff --git a/pkg/controllers/workgenerator/controller.go b/pkg/controllers/workgenerator/controller.go index 660256e8e..52569cc31 100644 --- a/pkg/controllers/workgenerator/controller.go +++ b/pkg/controllers/workgenerator/controller.go @@ -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 } diff --git a/test/e2e/placement_cro_test.go b/test/e2e/placement_cro_test.go index 9e63f8b37..fba88e80a 100644 --- a/test/e2e/placement_cro_test.go +++ b/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) }) diff --git a/test/e2e/placement_ro_test.go b/test/e2e/placement_ro_test.go index e8fa4bd72..4893e6c2b 100644 --- a/test/e2e/placement_ro_test.go +++ b/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() { From 124eae261e30f70232fc7c0f2dad054762f859b3 Mon Sep 17 00:00:00 2001 From: Zhiying Lin Date: Wed, 26 Mar 2025 11:15:31 +0800 Subject: [PATCH 2/2] address comments --- pkg/controllers/workgenerator/controller.go | 5 +++-- test/e2e/placement_cro_test.go | 2 +- test/e2e/placement_ro_test.go | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/controllers/workgenerator/controller.go b/pkg/controllers/workgenerator/controller.go index 52569cc31..2a04da5b0 100644 --- a/pkg/controllers/workgenerator/controller.go +++ b/pkg/controllers/workgenerator/controller.go @@ -1548,8 +1548,9 @@ func (r *Reconciler) SetupWithManager(mgr controllerruntime.Manager) error { // 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. + // 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 && oldClusterResourceOverrideSnapshotHash == newClusterResourceOverrideSnapshotHash && diff --git a/test/e2e/placement_cro_test.go b/test/e2e/placement_cro_test.go index fba88e80a..d10f25927 100644 --- a/test/e2e/placement_cro_test.go +++ b/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) diff --git a/test/e2e/placement_ro_test.go b/test/e2e/placement_ro_test.go index 4893e6c2b..a40ec1cdd 100644 --- a/test/e2e/placement_ro_test.go +++ b/test/e2e/placement_ro_test.go @@ -168,7 +168,7 @@ var _ = Context("creating resourceOverride (selecting all clusters) to override }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update ro as expected", crpName) }) - It("should update CRP status as expected", func() { + 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)}, }