address comments

Signed-off-by: Britania Rodriguez Reyes <[email protected]>

Author: britaniar

pkg/controllers/updaterun/controller.go

@@ -189,6 +189,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim
 		// Stop the updateRun.
 		klog.InfoS("Stopping the updateRun", "state", state, "updatingStageIndex", updatingStageIndex, "updateRun", runObjRef)
 		// TODO(britaniar): Implement the stopping logic for in-progress stages.
+		// TODO(britaniar): Handle before and after stage tasks condition.
 
 		klog.V(2).InfoS("The updateRun is stopped", "updateRun", runObjRef)
 		return runtime.Result{}, r.recordUpdateRunStopped(ctx, updateRun)

pkg/controllers/updaterun/execution.go

@@ -166,13 +166,13 @@ func (r *Reconciler) executeUpdatingStage(
 	for i := 0; i < len(updatingStageStatus.Clusters) && clusterUpdatingCount < maxConcurrency; i++ {
 		clusterStatus := &updatingStageStatus.Clusters[i]
 		clusterUpdateSucceededCond := meta.FindStatusCondition(clusterStatus.Conditions, string(placementv1beta1.ClusterUpdatingConditionSucceeded))
-		if condition.IsConditionStatusTrueIgnoreGeneration(clusterUpdateSucceededCond) {
+		if condition.IsConditionStatusTrueIgnoreGeneration(clusterUpdateSucceededCond) { // Ignoring generation here as once succeeded in a previous Run state, it won't change.
 			// The cluster has been updated successfully.
 			finishedClusterCount++
 			continue
 		}
 		clusterUpdatingCount++
-		if condition.IsConditionStatusFalseIgnoreGeneration(clusterUpdateSucceededCond) {
+		if condition.IsConditionStatusFalseIgnoreGeneration(clusterUpdateSucceededCond) { // Ignoring generation here as once failed, it won't change as the update run is aborted on failure.
 			// The cluster is marked as failed to update, this cluster is counted as updating cluster since it's not finished to avoid processing more clusters than maxConcurrency in this round.
 			failedErr := fmt.Errorf("the cluster `%s` in the stage %s has failed", clusterStatus.ClusterName, updatingStageStatus.StageName)
 			klog.ErrorS(failedErr, "The cluster has failed to be updated", "updateRun", updateRunRef)
@@ -182,7 +182,7 @@ func (r *Reconciler) executeUpdatingStage(
 		// The cluster needs to be processed.
 		clusterStartedCond := meta.FindStatusCondition(clusterStatus.Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted))
 		binding := toBeUpdatedBindingsMap[clusterStatus.ClusterName]
-		if !condition.IsConditionStatusTrueIgnoreGeneration(clusterStartedCond) {
+		if !condition.IsConditionStatusTrueIgnoreGeneration(clusterStartedCond) { // Ignoring generation here as once started in a previous Run state, it won't change.
 			// The cluster has not started updating yet.
 			if !isBindingSyncedWithClusterStatus(resourceSnapshotName, updateRun, binding, clusterStatus) {
 				klog.V(2).InfoS("Found the first cluster that needs to be updated", "cluster", clusterStatus.ClusterName, "stage", updatingStageStatus.StageName, "updateRun", updateRunRef)
@@ -231,9 +231,15 @@ func (r *Reconciler) executeUpdatingStage(
 					}
 				}
 			}
-			markClusterUpdatingStarted(clusterStatus, updateRun.GetGeneration())
+			// The cluster is starting to update. Only mark the cluster as started if not already marked or previously false.
+			// Ignoring generation here as once started in a previous Run state, it won't change.
+			if !condition.IsConditionStatusTrueIgnoreGeneration(clusterStartedCond) {
+				markClusterUpdatingStarted(clusterStatus, updateRun.GetGeneration())
+			}
 			stageUpdatingProgressCond := meta.FindStatusCondition(updatingStageStatus.Conditions, string(placementv1beta1.StageUpdatingConditionProgressing))
-			if finishedClusterCount == 0 && (!condition.IsConditionStatusTrueIgnoreGeneration(stageUpdatingProgressCond)) {
+			// Mark the stage as started if not already marked or previously false (restarted after update run has stopped or waiting).
+			// Ignoring generation here as the progressing generation is updated based on the state of the update run.
+			if finishedClusterCount == 0 && !condition.IsConditionStatusTrueIgnoreGeneration(stageUpdatingProgressCond) {
 				markStageUpdatingStarted(updatingStageStatus, updateRun.GetGeneration())
 			}
 			// Need to continue as we need to process at most maxConcurrency number of clusters in parallel.
@@ -338,8 +344,10 @@ func (r *Reconciler) executeDeleteStage(
 	for i := range existingDeleteStageStatus.Clusters {
 		existingDeleteStageClusterMap[existingDeleteStageStatus.Clusters[i].ClusterName] = &existingDeleteStageStatus.Clusters[i]
 	}
-	// Mark the delete stage as started in case it's not.
-	markStageUpdatingStarted(updateRunStatus.DeletionStageStatus, updateRun.GetGeneration())
+	// Mark the delete stage as started in case it's not. Ignoring generation here as progressing generation is updated based on the state of the update run and could have started in previous Run state.
+	if !condition.IsConditionStatusTrueIgnoreGeneration(meta.FindStatusCondition(existingDeleteStageStatus.Conditions, string(placementv1beta1.StageUpdatingConditionProgressing))) {
+		markStageUpdatingStarted(updateRunStatus.DeletionStageStatus, updateRun.GetGeneration())
+	}
 	for _, binding := range toBeDeletedBindings {
 		bindingSpec := binding.GetBindingSpec()
 		curCluster, exist := existingDeleteStageClusterMap[bindingSpec.TargetCluster]
@@ -352,11 +360,13 @@ func (r *Reconciler) executeDeleteStage(
 		// In validation, we already check the binding must exist in the status.
 		delete(existingDeleteStageClusterMap, bindingSpec.TargetCluster)
 		// Make sure the cluster is not marked as deleted as the binding is still there.
+		// Ignoring generation here, it won't change as the update run is aborted.
 		if condition.IsConditionStatusTrueIgnoreGeneration(meta.FindStatusCondition(curCluster.Conditions, string(placementv1beta1.ClusterUpdatingConditionSucceeded))) {
 			unexpectedErr := controller.NewUnexpectedBehaviorError(fmt.Errorf("the deleted cluster `%s` in the deleting stage still has a binding", bindingSpec.TargetCluster))
 			klog.ErrorS(unexpectedErr, "The cluster in the deleting stage is not removed yet but marked as deleted", "cluster", curCluster.ClusterName, "updateRun", updateRunRef)
 			return false, fmt.Errorf("%w: %s", errStagedUpdatedAborted, unexpectedErr.Error())
 		}
+		// Ignoring generation here as once started in a previous Run state, it won't change.
 		if condition.IsConditionStatusTrueIgnoreGeneration(meta.FindStatusCondition(curCluster.Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted))) {
 			// The cluster status is marked as being deleted.
 			if binding.GetDeletionTimestamp().IsZero() {
@@ -373,15 +383,24 @@ func (r *Reconciler) executeDeleteStage(
 			return false, controller.NewAPIServerError(false, err)
 		}
 		klog.V(2).InfoS("Deleted a binding pointing to a to be deleted cluster", "binding", klog.KObj(binding), "cluster", curCluster.ClusterName, "updateRun", updateRunRef)
-		markClusterUpdatingStarted(curCluster, updateRun.GetGeneration())
+		// Mark the cluster as deleting. Only mark the cluster as started if not already marked or previously false.
+		// Ignoring generation here as once started in a previous Run state, it won't change.
+		if !condition.IsConditionStatusTrueIgnoreGeneration(meta.FindStatusCondition(curCluster.Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted))) {
+			markClusterUpdatingStarted(curCluster, updateRun.GetGeneration())
+		}
 	}
 	// The rest of the clusters in the stage are not in the toBeDeletedBindings so it should be marked as delete succeeded.
 	for _, clusterStatus := range existingDeleteStageClusterMap {
 		// Make sure the cluster is marked as deleted.
+		// Ignoring generation here as once cluster started in a previous Run state, it won't change.
 		if !condition.IsConditionStatusTrueIgnoreGeneration(meta.FindStatusCondition(clusterStatus.Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted))) {
 			markClusterUpdatingStarted(clusterStatus, updateRun.GetGeneration())
 		}
-		markClusterUpdatingSucceeded(clusterStatus, updateRun.GetGeneration())
+		// Make sure the cluster is marked as delete succeeded.
+		// Ignoring generation here as once cluster succeeded in a previous Run state, it won't change.
+		if !condition.IsConditionStatusTrueIgnoreGeneration(meta.FindStatusCondition(clusterStatus.Conditions, string(placementv1beta1.ClusterUpdatingConditionSucceeded))) {
+			markClusterUpdatingSucceeded(clusterStatus, updateRun.GetGeneration())
+		}
 	}
 	klog.InfoS("The delete stage is progressing", "numberOfDeletingClusters", len(toBeDeletedBindings), "updateRun", updateRunRef)
 	if len(toBeDeletedBindings) == 0 {
@@ -445,6 +464,7 @@ func (r *Reconciler) handleStageApprovalTask(
 ) (bool, error) {
 	updateRunRef := klog.KObj(updateRun)
 
+	// Ignoring generation for checking request approved conditions as they could be set in previous Run state.
 	stageTaskApproved := condition.IsConditionStatusTrueIgnoreGeneration(meta.FindStatusCondition(stageTaskStatus.Conditions, string(placementv1beta1.StageTaskConditionApprovalRequestApproved)))
 	if stageTaskApproved {
 		// The stageTask has been approved.
@@ -456,8 +476,7 @@ func (r *Reconciler) handleStageApprovalTask(
 	requestRef := klog.KObj(approvalRequest)
 	if err := r.Client.Create(ctx, approvalRequest); err != nil {
 		if apierrors.IsAlreadyExists(err) {
-			// The approval task already exists.
-			markStageTaskRequestCreated(stageTaskStatus, updateRun.GetGeneration())
+			// The approval task already exists. No need to update request created condition again.
 			if err = r.Client.Get(ctx, client.ObjectKeyFromObject(approvalRequest), approvalRequest); err != nil {
 				klog.ErrorS(err, "Failed to get the already existing approval request", "approvalRequest", requestRef, "stage", updatingStage.Name, "updateRun", updateRunRef)
 				return false, controller.NewAPIServerError(true, err)
@@ -488,14 +507,15 @@ func (r *Reconciler) handleStageApprovalTask(
 				// Approved state should not change once the approval is accepted.
 				klog.V(2).InfoS("The approval request has been approval-accepted, ignoring changing back to unapproved", "approvalRequestTask", requestRef, "stage", updatingStage.Name, "updateRun", updateRunRef)
 			}
+			// Mark the stage task as approved as previously not approved. Will catch the very first check if approved in the beginning of this function.
 			markStageTaskRequestApproved(stageTaskStatus, updateRun.GetGeneration())
 		} else {
 			// retriable error
 			klog.ErrorS(err, "Failed to create the approval request", "approvalRequest", requestRef, "stage", updatingStage.Name, "updateRun", updateRunRef)
 			return false, controller.NewAPIServerError(false, err)
 		}
 	} else {
-		// The approval request has been created for the first time.
+		// The approval request has been created for the first time. Mark the stage task as request created.
 		klog.V(2).InfoS("The approval request has been created", "approvalRequestTask", requestRef, "stage", updatingStage.Name, "updateRun", updateRunRef)
 		markStageTaskRequestCreated(stageTaskStatus, updateRun.GetGeneration())
 		return false, nil
@@ -677,6 +697,7 @@ func markUpdateRunProgressing(updateRun placementv1beta1.UpdateRunObj) {
 func markUpdateRunProgressingIfNotWaitingOrStuck(updateRun placementv1beta1.UpdateRunObj) {
 	updateRunStatus := updateRun.GetUpdateRunStatus()
 	progressingCond := meta.FindStatusCondition(updateRunStatus.Conditions, string(placementv1beta1.StagedUpdateRunConditionProgressing))
+	// Not ignoring generation here as we want to check the latest condition while executing.
 	if condition.IsConditionStatusFalse(progressingCond, updateRun.GetGeneration()) &&
 		(progressingCond.Reason == condition.UpdateRunWaitingReason || progressingCond.Reason == condition.UpdateRunStuckReason) {
 		// The updateRun is waiting or stuck, no need to mark it as started.

pkg/controllers/updaterun/execution_integration_test.go

@@ -954,28 +954,21 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 			validateUpdateRunMetricsEmitted(wantMetrics...)
 		})
 
-		It("Should continue executing stage 1 of the update run when state is Run but wait for afterStage approval", func() {
-			By("Updating updateRun state to Run")
-			updateRun.Spec.State = placementv1beta1.StateRun
-			Expect(k8sClient.Update(ctx, updateRun)).Should(Succeed(), "failed to update the updateRun state")
+		It("Should approve the approval request for 1st stage afterStageTask while stopped and update run stays the same", func() {
+			By("Approving the approvalRequest")
+			approveClusterApprovalRequest(ctx, wantApprovalRequest.Name)
 
-			By("Validating update run is running")
-			// Mark approval request created for the 1st stage approval afterStageTask.
-			meta.SetStatusCondition(&wantStatus.StagesStatus[0].AfterStageTaskStatus[1].Conditions, generateTrueCondition(updateRun, placementv1beta1.StageTaskConditionApprovalRequestCreated))
-			// Mark 1st stage progressing condition as false with waiting reason.
-			meta.SetStatusCondition(&wantStatus.StagesStatus[0].Conditions, generateFalseCondition(updateRun, placementv1beta1.StageUpdatingConditionProgressing))
-			// Mark updateRun progressing condition as true with waiting reason.
-			meta.SetStatusCondition(&wantStatus.Conditions, generateFalseCondition(updateRun, placementv1beta1.StagedUpdateRunConditionProgressing))
+			By("Validating the update run is still stopped")
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
 			By("Checking update run status metrics are emitted")
-			wantMetrics = append(wantMetrics, generateWaitingMetric(updateRun))
 			validateUpdateRunMetricsEmitted(wantMetrics...)
 		})
 
 		It("Should complete the 1st stage once it starts running again when wait time passed and approval request approved then move on to the 2nd stage", func() {
-			By("Approving the approvalRequest")
-			approveClusterApprovalRequest(ctx, wantApprovalRequest.Name)
+			By("Updating updateRun state to Run")
+			updateRun.Spec.State = placementv1beta1.StateRun
+			Expect(k8sClient.Update(ctx, updateRun)).Should(Succeed(), "failed to update the updateRun state")
 
 			By("Validating the approvalRequest has ApprovalAccepted status")
 			Eventually(func() (bool, error) {
@@ -987,7 +980,6 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 			}, timeout, interval).Should(BeTrue(), "failed to validate the approvalRequest approval accepted")
 
 			By("Validating both after stage tasks have completed and 2nd stage has started")
-			meta.SetStatusCondition(&wantStatus.StagesStatus[0].AfterStageTaskStatus[1].Conditions, generateTrueCondition(updateRun, placementv1beta1.StageTaskConditionApprovalRequestCreated))
 			// Timedwait afterStageTask completed.
 			wantStatus.StagesStatus[0].AfterStageTaskStatus[0].Conditions = append(wantStatus.StagesStatus[0].AfterStageTaskStatus[0].Conditions,
 				generateTrueCondition(updateRun, placementv1beta1.StageTaskConditionWaitTimeElapsed))
@@ -1095,8 +1087,6 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 			Expect(k8sClient.Update(ctx, updateRun)).Should(Succeed(), "failed to update the updateRun state")
 
 			By("Validating update run is running")
-			// Mark approval request created for the 2nd stage approval beforeStageTask.
-			meta.SetStatusCondition(&wantStatus.StagesStatus[1].BeforeStageTaskStatus[0].Conditions, generateTrueCondition(updateRun, placementv1beta1.StageTaskConditionApprovalRequestCreated))
 			// Mark 2nd stage progressing condition as false with waiting reason.
 			meta.SetStatusCondition(&wantStatus.StagesStatus[1].Conditions, generateFalseCondition(updateRun, placementv1beta1.StageUpdatingConditionProgressing))
 			// Mark updateRun progressing condition as false with waiting reason.
@@ -1291,7 +1281,6 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 
 		It("Should complete the 2nd stage after wait time passed and approval request approved and move on to the delete stage", func() {
 			By("Validating the 2nd stage has completed and the delete stage has started")
-			meta.SetStatusCondition(&wantStatus.StagesStatus[1].AfterStageTaskStatus[0].Conditions, generateTrueCondition(updateRun, placementv1beta1.StageTaskConditionApprovalRequestCreated))
 			wantStatus.StagesStatus[1].AfterStageTaskStatus[0].Conditions = append(wantStatus.StagesStatus[1].AfterStageTaskStatus[0].Conditions,
 				generateTrueCondition(updateRun, placementv1beta1.StageTaskConditionApprovalRequestApproved))
 			wantStatus.StagesStatus[1].AfterStageTaskStatus[1].Conditions = append(wantStatus.StagesStatus[1].AfterStageTaskStatus[1].Conditions,