From e01faf07f470b981fb08015d87d722fc25e8070c Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Fri, 27 Sep 2024 09:52:25 -0700 Subject: [PATCH] fix: fix unexpected alert race condition (#921) --------- Co-authored-by: Zhiying Lin Co-authored-by: Britania Rodriguez Reyes --- pkg/controllers/workgenerator/controller.go | 56 +++++-- .../controller_integration_test.go | 147 +++++++++++++----- .../workgenerator/controller_test.go | 62 -------- test/e2e/enveloped_object_placement_test.go | 2 +- 4 files changed, 156 insertions(+), 111 deletions(-) diff --git a/pkg/controllers/workgenerator/controller.go b/pkg/controllers/workgenerator/controller.go index 56a107718..a74ba521e 100644 --- a/pkg/controllers/workgenerator/controller.go +++ b/pkg/controllers/workgenerator/controller.go @@ -118,6 +118,21 @@ func (r *Reconciler) Reconcile(ctx context.Context, req controllerruntime.Reques return controllerruntime.Result{}, err } + // When the binding is in the unscheduled state, rollout controller won't update the condition anymore. + // We treat the unscheduled binding as bound until the rollout controller deletes the binding and here controller still + // updates the status for troubleshooting purpose. + // Requeue until the rollout controller finishes processing the binding. + if resourceBinding.Spec.State == fleetv1beta1.BindingStateBound { + rolloutStartedCondition := resourceBinding.GetCondition(string(fleetv1beta1.ResourceBindingRolloutStarted)) + // Though the bounded binding is not taking the latest resourceSnapshot, we still needs to reconcile the works. + if !condition.IsConditionStatusFalse(rolloutStartedCondition, resourceBinding.Generation) && + !condition.IsConditionStatusTrue(rolloutStartedCondition, resourceBinding.Generation) { + // The rollout controller is still in the processing of updating the condition + klog.V(2).InfoS("Requeue the resource binding until the rollout controller finishes updating the status", "resourceBinding", bindingRef, "generation", resourceBinding.Generation, "rolloutStartedCondition", rolloutStartedCondition) + return controllerruntime.Result{Requeue: true}, nil + } + } + workUpdated := false overrideSucceeded := false // Reset the conditions and failed placements. @@ -224,9 +239,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req controllerruntime.Reques // updateBindingStatusWithRetry sends the update request to API server with retry. func (r *Reconciler) updateBindingStatusWithRetry(ctx context.Context, resourceBinding *fleetv1beta1.ClusterResourceBinding) error { // Retry only for specific errors or conditions + bindingRef := klog.KObj(resourceBinding) err := r.Client.Status().Update(ctx, resourceBinding) if err != nil { - klog.ErrorS(err, "Failed to update the resourceBinding status, will retry", "resourceBinding", klog.KObj(resourceBinding), "resourceBindingStatus", resourceBinding.Status) + klog.ErrorS(err, "Failed to update the resourceBinding status, will retry", "resourceBinding", bindingRef, "resourceBindingStatus", resourceBinding.Status) errAfterRetries := retry.RetryOnConflict(retry.DefaultBackoff, func() error { var latestBinding fleetv1beta1.ClusterResourceBinding if err := r.Client.Get(ctx, client.ObjectKeyFromObject(resourceBinding), &latestBinding); err != nil { @@ -234,29 +250,28 @@ func (r *Reconciler) updateBindingStatusWithRetry(ctx context.Context, resourceB } // Work generator is the only controller that updates conditions excluding rollout started which is updated by rollout controller. if rolloutCond := latestBinding.GetCondition(string(fleetv1beta1.ResourceBindingRolloutStarted)); rolloutCond != nil { - found := false for i := range resourceBinding.Status.Conditions { if resourceBinding.Status.Conditions[i].Type == rolloutCond.Type { // Replace the existing condition resourceBinding.Status.Conditions[i] = *rolloutCond - found = true break } } - if !found { - return controller.NewUnexpectedBehaviorError(fmt.Errorf("found a resourceBinding %v without RolloutStarted condition", klog.KObj(resourceBinding))) - } + } else { + // At least the RolloutStarted condition for the old generation should be set. + // RolloutStarted condition won't be removed by the rollout controller. + klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("found an invalid resourceBinding")), "RolloutStarted condition is not set", "resourceBinding", bindingRef) } - - if err := r.Client.Status().Update(ctx, resourceBinding); err != nil { - klog.ErrorS(err, "Failed to update the resourceBinding status", "resourceBinding", klog.KObj(resourceBinding), "resourceBindingStatus", resourceBinding.Status) + latestBinding.Status = resourceBinding.Status + if err := r.Client.Status().Update(ctx, &latestBinding); err != nil { + klog.ErrorS(err, "Failed to update the resourceBinding status", "resourceBinding", bindingRef, "resourceBindingStatus", latestBinding.Status) return err } - klog.V(2).InfoS("Successfully updated the resourceBinding status", "resourceBinding", klog.KObj(resourceBinding), "resourceBindingStatus", resourceBinding.Status) + klog.V(2).InfoS("Successfully updated the resourceBinding status", "resourceBinding", bindingRef, "resourceBindingStatus", latestBinding.Status) return nil }) if errAfterRetries != nil { - klog.ErrorS(errAfterRetries, "Failed to update resourceBinding status after retries", "resourceBinding", klog.KObj(resourceBinding)) + klog.ErrorS(errAfterRetries, "Failed to update resourceBinding status after retries", "resourceBinding", bindingRef) return errAfterRetries } return nil @@ -935,8 +950,23 @@ func (r *Reconciler) SetupWithManager(mgr controllerruntime.Manager) error { return } } else { - 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 + oldResourceSnapshot := oldWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] + 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) + 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. + // In this edge case, we need to requeue the binding to update the binding status. + if oldResourceSnapshot == newResourceSnapshot { + 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 + } } } // We need to update the binding status in this case diff --git a/pkg/controllers/workgenerator/controller_integration_test.go b/pkg/controllers/workgenerator/controller_integration_test.go index fdf8dfa49..cf2e39925 100644 --- a/pkg/controllers/workgenerator/controller_integration_test.go +++ b/pkg/controllers/workgenerator/controller_integration_test.go @@ -131,9 +131,7 @@ var _ = Describe("Test Work Generator Controller", func() { ResourceSnapshotName: masterSnapshot.Name, TargetCluster: memberClusterName, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) // check the work is not created since the binding state is not bound work := placementv1beta1.Work{} Consistently(func() bool { @@ -146,6 +144,7 @@ var _ = Describe("Test Work Generator Controller", func() { // flip the binding state to bound and check the work is created binding.Spec.State = placementv1beta1.BindingStateBound Expect(k8sClient.Update(ctx, binding)).Should(Succeed()) + updateRolloutStartedGeneration(&binding) // check the work is created Eventually(func() error { return k8sClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf(placementv1beta1.FirstWorkNameFmt, testCRPName), Namespace: memberClusterNamespaceName}, &work) @@ -155,7 +154,7 @@ var _ = Describe("Test Work Generator Controller", func() { verifyBindingStatusSyncedNotApplied(binding, false, true) }) - It("Should only creat work after all the resource snapshots are created", func() { + It("Should only create work after all the resource snapshots are created", func() { // create master resource snapshot with 1 number of resources masterSnapshot := generateResourceSnapshot(1, 2, 0, [][]byte{ testResourceCRD, testNameSpace, testResource, @@ -168,9 +167,7 @@ var _ = Describe("Test Work Generator Controller", func() { ResourceSnapshotName: masterSnapshot.Name, TargetCluster: memberClusterName, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding `%s` created", binding.Name)) + createClusterResourceBinding(&binding, spec) // check the work is not created since we have more resource snapshot to create work := placementv1beta1.Work{} Consistently(func() bool { @@ -181,6 +178,12 @@ var _ = Describe("Test Work Generator Controller", func() { Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) wantStatus := placementv1beta1.ResourceBindingStatus{ Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, { Type: string(placementv1beta1.ResourceBindingOverridden), Status: metav1.ConditionFalse, @@ -221,9 +224,7 @@ var _ = Describe("Test Work Generator Controller", func() { ResourceSnapshotName: masterSnapshot.Name, TargetCluster: memberClusterName, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) // check the work is created work := placementv1beta1.Work{} Eventually(func() error { @@ -262,9 +263,7 @@ var _ = Describe("Test Work Generator Controller", func() { ResourceSnapshotName: masterSnapshot.Name, TargetCluster: memberClusterName, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) }) AfterEach(func() { @@ -344,6 +343,7 @@ var _ = Describe("Test Work Generator Controller", func() { binding.Spec.State = placementv1beta1.BindingStateUnscheduled Expect(k8sClient.Update(ctx, binding)).Should(Succeed()) By(fmt.Sprintf("resource binding %s updated to be unscheduled", binding.Name)) + updateRolloutStartedGeneration(&binding) Consistently(func() error { return k8sClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf(placementv1beta1.FirstWorkNameFmt, testCRPName), Namespace: memberClusterNamespaceName}, &work) }, duration, interval).Should(Succeed(), "controller should not remove work in hub cluster for unscheduled binding") @@ -458,9 +458,7 @@ var _ = Describe("Test Work Generator Controller", func() { ResourceSnapshotName: masterSnapshot.Name, TargetCluster: memberClusterName, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) }) AfterEach(func() { @@ -572,6 +570,7 @@ var _ = Describe("Test Work Generator Controller", func() { binding.Spec.ResourceSnapshotName = masterSnapshot.Name Expect(k8sClient.Update(ctx, binding)).Should(Succeed()) By(fmt.Sprintf("resource binding %s updated", binding.Name)) + updateRolloutStartedGeneration(&binding) // check the binding status till the bound condition is true for the second generation Eventually(func() bool { if err := k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding); err != nil { @@ -677,6 +676,7 @@ var _ = Describe("Test Work Generator Controller", func() { binding.Spec.ResourceSnapshotName = masterSnapshot.Name Expect(k8sClient.Update(ctx, binding)).Should(Succeed()) By(fmt.Sprintf("resource binding %s updated", binding.Name)) + updateRolloutStartedGeneration(&binding) // check the binding status till the bound condition is true for the second binding generation Eventually(func() bool { if err := k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding); err != nil { @@ -726,9 +726,7 @@ var _ = Describe("Test Work Generator Controller", func() { ResourceSnapshotName: masterSnapshot.Name, TargetCluster: memberClusterName, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) // Now create the second resource snapshot secondSnapshot = generateResourceSnapshot(2, 2, 1, [][]byte{ testConfigMap, testPdb, @@ -909,6 +907,7 @@ var _ = Describe("Test Work Generator Controller", func() { Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) binding.Spec.ResourceSnapshotName = masterSnapshot.Name Expect(k8sClient.Update(ctx, binding)).Should(Succeed()) + updateRolloutStartedGeneration(&binding) By(fmt.Sprintf("resource binding %s updated", binding.Name)) // Now create the second resource snapshot secondSnapshot = generateResourceSnapshot(3, 3, 1, [][]byte{ @@ -1000,6 +999,7 @@ var _ = Describe("Test Work Generator Controller", func() { Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) binding.Spec.ResourceSnapshotName = masterSnapshot.Name Expect(k8sClient.Update(ctx, binding)).Should(Succeed()) + updateRolloutStartedGeneration(&binding) By(fmt.Sprintf("resource binding %s updated", binding.Name)) //inspect the work manifest that should have been updated to contain all expectedManifest := []placementv1beta1.Manifest{ @@ -1103,9 +1103,7 @@ var _ = Describe("Test Work Generator Controller", func() { }, }, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) }) AfterEach(func() { @@ -1185,6 +1183,7 @@ var _ = Describe("Test Work Generator Controller", func() { binding.Spec.State = placementv1beta1.BindingStateUnscheduled Expect(k8sClient.Update(ctx, binding)).Should(Succeed()) By(fmt.Sprintf("resource binding %s updated to be unscheduled", binding.Name)) + updateRolloutStartedGeneration(&binding) Consistently(func() error { return k8sClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf(placementv1beta1.FirstWorkNameFmt, testCRPName), Namespace: memberClusterNamespaceName}, &work) }, duration, interval).Should(Succeed(), "controller should not remove work in hub cluster for unscheduled binding") @@ -1218,9 +1217,7 @@ var _ = Describe("Test Work Generator Controller", func() { invalidClusterResourceOverrideSnapshotName, }, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) }) AfterEach(func() { @@ -1242,6 +1239,12 @@ var _ = Describe("Test Work Generator Controller", func() { Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) wantStatus := placementv1beta1.ResourceBindingStatus{ Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, { Type: string(placementv1beta1.ResourceBindingOverridden), Status: metav1.ConditionFalse, @@ -1273,9 +1276,7 @@ var _ = Describe("Test Work Generator Controller", func() { "not-found-snapshot", }, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) }) AfterEach(func() { @@ -1297,6 +1298,12 @@ var _ = Describe("Test Work Generator Controller", func() { Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) wantStatus := placementv1beta1.ResourceBindingStatus{ Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, { Type: string(placementv1beta1.ResourceBindingOverridden), Status: metav1.ConditionFalse, @@ -1324,9 +1331,7 @@ var _ = Describe("Test Work Generator Controller", func() { ResourceSnapshotName: masterSnapshot.Name, TargetCluster: memberClusterName, } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) }) AfterEach(func() { @@ -1407,10 +1412,11 @@ var _ = Describe("Test Work Generator Controller", func() { By(fmt.Sprintf("work %s is created in %s", work.Name, work.Namespace)) // update binding to be unscheduled Expect(k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding)).Should(Succeed()) - rolloutCond := binding.GetCondition(string(placementv1beta1.ResourceBindingRolloutStarted)) binding.Spec.State = placementv1beta1.BindingStateUnscheduled Expect(k8sClient.Update(ctx, binding)).Should(Succeed()) By(fmt.Sprintf("resource binding %s updated to be unscheduled", binding.Name)) + updateRolloutStartedGeneration(&binding) + rolloutCond := binding.GetCondition(string(placementv1beta1.ResourceBindingRolloutStarted)) Consistently(func() error { return k8sClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf(placementv1beta1.FirstWorkNameFmt, testCRPName), Namespace: memberClusterNamespaceName}, &work) }, duration, interval).Should(Succeed(), "controller should not remove work in hub cluster for unscheduled binding") @@ -1438,9 +1444,7 @@ var _ = Describe("Test Work Generator Controller", func() { ResourceSnapshotName: "invalid-resource-snapshot", TargetCluster: "non-found-cluster", } - binding = generateClusterResourceBinding(spec) - Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) - By(fmt.Sprintf("resource binding %s created", binding.Name)) + createClusterResourceBinding(&binding, spec) }) AfterEach(func() { @@ -1478,6 +1482,12 @@ func verifyBindingStatusSyncedNotApplied(binding *placementv1beta1.ClusterResour wantStatus := placementv1beta1.ResourceBindingStatus{ Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, { Type: string(placementv1beta1.ResourceBindingOverridden), Status: metav1.ConditionTrue, @@ -1512,6 +1522,12 @@ func verifyBindStatusAppliedNotAvailable(binding *placementv1beta1.ClusterResour } wantStatus := placementv1beta1.ResourceBindingStatus{ Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, { Type: string(placementv1beta1.ResourceBindingOverridden), Status: metav1.ConditionTrue, @@ -1552,6 +1568,12 @@ func verifyBindStatusAvail(binding *placementv1beta1.ClusterResourceBinding, has } wantStatus := placementv1beta1.ResourceBindingStatus{ Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, { Type: string(placementv1beta1.ResourceBindingOverridden), Status: metav1.ConditionTrue, @@ -1624,6 +1646,12 @@ func verifyBindStatusNotAppliedWithFailedPlacement(binding *placementv1beta1.Clu }, }, Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, { Type: string(placementv1beta1.ResourceBindingOverridden), Status: metav1.ConditionTrue, @@ -1689,6 +1717,12 @@ func verifyBindStatusNotAvailableWithTwoFailedPlacement(binding *placementv1beta }, }, Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, { Type: string(placementv1beta1.ResourceBindingOverridden), Status: metav1.ConditionTrue, @@ -1745,6 +1779,12 @@ func verifyBindStatusNotAvailableWithOneFailedPlacement(binding *placementv1beta }, }, Conditions: []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: binding.GetGeneration(), + }, { Type: string(placementv1beta1.ResourceBindingOverridden), Status: metav1.ConditionTrue, @@ -2071,3 +2111,40 @@ func checkRolloutStartedNotUpdated(rolloutCond *metav1.Condition, binding *place diff := cmp.Diff(rolloutCond, binding.GetCondition(string(placementv1beta1.ResourceBindingRolloutStarted)), cmpConditionOptionWithLTT) Expect(diff).Should(BeEmpty(), fmt.Sprintf("binding(%s) mismatch (-want +got)", binding.Name), diff) } + +func createClusterResourceBinding(binding **placementv1beta1.ClusterResourceBinding, spec placementv1beta1.ResourceBindingSpec) { + *binding = generateClusterResourceBinding(spec) + Expect(k8sClient.Create(ctx, *binding)).Should(Succeed()) + Eventually(func() error { + if err := k8sClient.Get(ctx, types.NamespacedName{Name: (*binding).Name}, *binding); err != nil { + return err + } + (*binding).Status.Conditions = []metav1.Condition{ + { + Type: string(placementv1beta1.ResourceBindingRolloutStarted), + Status: metav1.ConditionTrue, + Reason: condition.RolloutStartedReason, + ObservedGeneration: (*binding).GetGeneration(), + LastTransitionTime: metav1.Now(), + }, + } + return k8sClient.Status().Update(ctx, *binding) + }, timeout, interval).Should(Succeed(), "Failed to update the binding with RolloutStarted condition") + By(fmt.Sprintf("resource binding %s created", (*binding).Name)) +} + +func updateRolloutStartedGeneration(binding **placementv1beta1.ClusterResourceBinding) { + Eventually(func() error { + // Eventually update the binding with the new generation for RolloutStarted condition in case it runs into a conflict error + if err := k8sClient.Get(ctx, types.NamespacedName{Name: (*binding).Name}, *binding); err != nil { + return err + } + // RolloutStarted condition has to be updated to reflect the new generation + for i := range (*binding).Status.Conditions { + if (*binding).Status.Conditions[i].Type == string(placementv1beta1.ResourceBindingRolloutStarted) { + (*binding).Status.Conditions[0].ObservedGeneration = (*binding).GetGeneration() + } + } + return k8sClient.Status().Update(ctx, *binding) + }, timeout, interval).Should(Succeed(), "Failed to update the binding with new generation for RolloutStarted condition") +} diff --git a/pkg/controllers/workgenerator/controller_test.go b/pkg/controllers/workgenerator/controller_test.go index 651157e64..e07952825 100644 --- a/pkg/controllers/workgenerator/controller_test.go +++ b/pkg/controllers/workgenerator/controller_test.go @@ -1734,68 +1734,6 @@ func TestUpdateBindingStatusWithRetry(t *testing.T) { conflictCount: 10, expectError: true, }, - { - name: "does not update status because RolloutStarted not found", - latestBinding: &fleetv1beta1.ClusterResourceBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-binding-4", - Generation: 3, - ResourceVersion: "3", - }, - Spec: fleetv1beta1.ResourceBindingSpec{ - State: fleetv1beta1.BindingStateBound, - TargetCluster: "cluster-1", - ResourceSnapshotName: "snapshot-1", - }, - Status: fleetv1beta1.ResourceBindingStatus{ - Conditions: []metav1.Condition{ - { - Type: string(fleetv1beta1.ResourceBindingRolloutStarted), - Status: metav1.ConditionTrue, - ObservedGeneration: 3, - Reason: condition.RolloutStartedReason, - LastTransitionTime: lastTransitionTime, - }, - }, - }, - }, - resourceBinding: &fleetv1beta1.ClusterResourceBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-binding-4", - Generation: 3, - ResourceVersion: "3", - }, - Spec: fleetv1beta1.ResourceBindingSpec{ - State: fleetv1beta1.BindingStateBound, - TargetCluster: "cluster-1", - ResourceSnapshotName: "snapshot-1", - }, - Status: fleetv1beta1.ResourceBindingStatus{ - Conditions: []metav1.Condition{ - { - Type: string(fleetv1beta1.ResourceBindingOverridden), - Status: metav1.ConditionTrue, - ObservedGeneration: 3, - Reason: condition.OverriddenSucceededReason, - }, - { - Type: string(fleetv1beta1.ResourceBindingWorkSynchronized), - Status: metav1.ConditionTrue, - ObservedGeneration: 3, - Reason: condition.AllWorkSyncedReason, - }, - { - Type: string(fleetv1beta1.ResourceBindingApplied), - Status: metav1.ConditionFalse, - ObservedGeneration: 3, - Reason: condition.WorkNeedSyncedReason, - }, - }, - }, - }, - conflictCount: 1, - expectError: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/test/e2e/enveloped_object_placement_test.go b/test/e2e/enveloped_object_placement_test.go index a94bd49f4..a5408ba63 100644 --- a/test/e2e/enveloped_object_placement_test.go +++ b/test/e2e/enveloped_object_placement_test.go @@ -136,7 +136,7 @@ var _ = Describe("placing wrapped resources using a CRP", func() { It("should update CRP status as success again", func() { crpStatusUpdatedActual := customizedCRPStatusUpdatedActual(crpName, wantSelectedResources, allMemberClusterNames, nil, "2", false) - Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected") + Eventually(crpStatusUpdatedActual, longEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected") }) It("should place the resources on all member clusters again", func() {