fixes

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

Author: britaniar

pkg/controllers/updaterun/execution.go

@@ -61,22 +61,24 @@ func (r *Reconciler) execute(
 	updateRun placementv1beta1.UpdateRunObj,
 	updatingStageIndex int,
 	toBeUpdatedBindings, toBeDeletedBindings []placementv1beta1.BindingObj,
-) (finished bool, waitTime time.Duration, err error) {
+) (finished bool, waitTime time.Duration, execErr error) {
 	updateRunStatus := updateRun.GetUpdateRunStatus()
 	var updatingStageStatus *placementv1beta1.StageUpdatingStatus
 
 	// Set up defer function to handle errStagedUpdatedAborted.
-	defer checkIfErrorStagedUpdateAborted(err, updateRun, updatingStageStatus)
+	defer func() {
+		checkIfErrorStagedUpdateAborted(execErr, updateRun, updatingStageStatus)
+	}()
 
 	// Mark updateRun as progressing if it's not already marked as waiting or stuck.
 	// This avoids triggering an unnecessary in-memory transition from stuck (waiting) -> progressing -> stuck (waiting),
 	// which would update the lastTransitionTime even though the status hasn't effectively changed.
 	markUpdateRunProgressingIfNotWaitingOrStuck(updateRun)
 	if updatingStageIndex < len(updateRunStatus.StagesStatus) {
 		updatingStageStatus = &updateRunStatus.StagesStatus[updatingStageIndex]
-		approved, err := r.checkBeforeStageTasksStatus(ctx, updatingStageIndex, updateRun)
-		if err != nil {
-			return false, 0, err
+		approved, execErr := r.checkBeforeStageTasksStatus(ctx, updatingStageIndex, updateRun)
+		if execErr != nil {
+			return false, 0, execErr
 		}
 		if !approved {
 			markStageUpdatingWaiting(updatingStageStatus, updateRun.GetGeneration(), "Not all before-stage tasks are completed, waiting for approval")
@@ -87,13 +89,13 @@ func (r *Reconciler) execute(
 		if err != nil {
 			return false, 0, err
 		}
-		waitTime, err = r.executeUpdatingStage(ctx, updateRun, updatingStageIndex, toBeUpdatedBindings, maxConcurrency)
+		waitTime, execErr = r.executeUpdatingStage(ctx, updateRun, updatingStageIndex, toBeUpdatedBindings, maxConcurrency)
 		// The execution has not finished yet.
-		return false, waitTime, err
+		return false, waitTime, execErr
 	}
 	// All the stages have finished, now start the delete stage.
-	finished, err = r.executeDeleteStage(ctx, updateRun, toBeDeletedBindings)
-	return finished, clusterUpdatingWaitTime, err
+	finished, execErr = r.executeDeleteStage(ctx, updateRun, toBeDeletedBindings)
+	return finished, clusterUpdatingWaitTime, execErr
 }
 
 // checkBeforeStageTasksStatus checks if the before stage tasks have finished.

pkg/controllers/updaterun/stop.go

@@ -25,7 +25,9 @@ func (r *Reconciler) stop(
 	var updatingStageStatus *placementv1beta1.StageUpdatingStatus
 
 	// Set up defer function to handle errStagedUpdatedAborted.
-	defer checkIfErrorStagedUpdateAborted(stopErr, updateRun, updatingStageStatus)
+	defer func() {
+		checkIfErrorStagedUpdateAborted(stopErr, updateRun, updatingStageStatus)
+	}()
 
 	markUpdateRunStopping(updateRun)
 
@@ -197,6 +199,7 @@ func checkIfErrorStagedUpdateAborted(err error, updateRun placementv1beta1.Updat
 	updateRunStatus := updateRun.GetUpdateRunStatus()
 	if errors.Is(err, errStagedUpdatedAborted) {
 		if updatingStageStatus != nil {
+			klog.InfoS("The update run is aborted due to unexpected behavior in updating stage, marking the stage as failed", "stage", updatingStageStatus.StageName, "updateRun", klog.KObj(updateRun))
 			markStageUpdatingFailed(updatingStageStatus, updateRun.GetGeneration(), err.Error())
 		} else {
 			// Handle deletion stage case.

test/e2e/actuals_test.go

@@ -2068,16 +2068,12 @@ func updateRunStageTaskSucceedConditions(generation int64, taskType placementv1b
 }
 
 func updateRunSucceedConditions(generation int64) []metav1.Condition {
-	initializeCondGeneration := generation
-	if generation > 1 {
-		initializeCondGeneration = 1
-	}
 	return []metav1.Condition{
 		{
 			Type:               string(placementv1beta1.StagedUpdateRunConditionInitialized),
 			Status:             metav1.ConditionTrue,
 			Reason:             condition.UpdateRunInitializeSucceededReason,
-			ObservedGeneration: initializeCondGeneration,
+			ObservedGeneration: generation,
 		},
 		{
 			Type:               string(placementv1beta1.StagedUpdateRunConditionProgressing),

test/e2e/cluster_staged_updaterun_test.go

@@ -1668,7 +1668,7 @@ var _ = Describe("test CRP rollout with staged update run", func() {
 		})
 	})
 
-	Context("Test resource rollout with staged update run by update run states - (Initialize -> Run)", Ordered, func() {
+	FContext("Test resource rollout with staged update run by update run states - (Initialize -> Run -> Stop -> Run)", Ordered, func() {
 		updateRunNames := []string{}
 		var strategy *placementv1beta1.ClusterStagedUpdateStrategy
 
@@ -1758,9 +1758,43 @@ var _ = Describe("test CRP rollout with staged update run", func() {
 			validateAndApproveClusterApprovalRequests(updateRunNames[0], envCanary, placementv1beta1.AfterStageApprovalTaskNameFmt, placementv1beta1.AfterStageTaskLabelValue)
 		})
 
-		It("Should rollout resources to all the members and complete the cluster staged update run successfully", func() {
+		It("Should not rollout to all member clusters while waiting for beforeStageTask approval for prod stage", func() {
+			By("Validating not rolled out to member-cluster-1 and member-cluster-3 yet")
+			checkIfRemovedWorkResourcesFromMemberClustersConsistently([]*framework.Cluster{allMemberClusters[0], allMemberClusters[2]})
+			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun([]*framework.Cluster{allMemberClusters[1]})
+
+			By("Validating crp status as member-cluster-2 updated only")
+			crpStatusUpdatedActual := crpStatusWithExternalStrategyActual(nil, "", false, allMemberClusterNames, []string{"", resourceSnapshotIndex1st, ""}, []bool{false, true, false}, nil, nil)
+			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
+		})
+
+		It("Should not rollout to all member clusters after stopping update run", func() {
+			By("Updating update run state to Stop")
+			updateClusterStagedUpdateRunState(updateRunNames[0], placementv1beta1.StateStop)
+
+			By("Validating not rolled out to member-cluster-1 and member-cluster-3 yet")
+			checkIfRemovedWorkResourcesFromMemberClustersConsistently([]*framework.Cluster{allMemberClusters[0], allMemberClusters[2]})
+			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun([]*framework.Cluster{allMemberClusters[1]})
+
+			By("Validating crp status as member-cluster-2 updated")
+			crpStatusUpdatedActual := crpStatusWithExternalStrategyActual(nil, "", false, allMemberClusterNames, []string{"", resourceSnapshotIndex1st, ""}, []bool{false, true, false}, nil, nil)
+			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
+		})
+
+		It("Should complete rollout to all member after update run state is Run and beforeStageTask approval", func() {
+			By("Validating not rolled out to member-cluster-1 and member-cluster-3 yet")
+			checkIfRemovedWorkResourcesFromMemberClustersConsistently([]*framework.Cluster{allMemberClusters[0], allMemberClusters[2]})
+
+			// Update the update run state back to Run.
+			By("Updating the update run state back to Run")
+			updateClusterStagedUpdateRunState(updateRunNames[0], placementv1beta1.StateRun)
+
 			validateAndApproveClusterApprovalRequests(updateRunNames[0], envProd, placementv1beta1.BeforeStageApprovalTaskNameFmt, placementv1beta1.BeforeStageTaskLabelValue)
 
+			By("All member clusters should have work resources placed")
+			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun([]*framework.Cluster{allMemberClusters[0], allMemberClusters[1], allMemberClusters[2]})
+
+			By("Validating update run has succeeded after resuming")
 			csurSucceededActual := clusterStagedUpdateRunStatusSucceededActual(updateRunNames[0], resourceSnapshotIndex1st, policySnapshotIndex1st, len(allMemberClusters), defaultApplyStrategy, &strategy.Spec, [][]string{{allMemberClusterNames[1]}, {allMemberClusterNames[0], allMemberClusterNames[2]}}, nil, nil, nil, true)
 			Eventually(csurSucceededActual, updateRunEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to validate updateRun %s succeeded", updateRunNames[0])
 			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun(allMemberClusters)