address most comments

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

Author: britaniar

pkg/controllers/updaterun/stop.go

@@ -156,9 +156,12 @@ func (r *Reconciler) stopDeleteStage(
 		}
 		// 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.
 		if condition.IsConditionStatusTrue(meta.FindStatusCondition(curCluster.Conditions, string(placementv1beta1.ClusterUpdatingConditionSucceeded)), updateRun.GetGeneration()) {
 			// The cluster status is marked as deleted.
-			continue
+			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())
 		}
 		if condition.IsConditionStatusTrue(meta.FindStatusCondition(curCluster.Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted)), updateRun.GetGeneration()) {
 			// The cluster status is marked as being deleted.
@@ -182,7 +185,19 @@ func (r *Reconciler) stopDeleteStage(
 	}
 
 	klog.V(2).InfoS("The delete stage is stopping", "numberOfDeletingClusters", len(toBeDeletedBindings), "updateRun", updateRunRef)
-	if len(toBeDeletedBindings) == 0 {
+	allDeletingClustersDeleted := true
+	for _, clusterStatus := range updateRunStatus.DeletionStageStatus.Clusters {
+		klog.InfoS("Checking deletion status of cluster in delete stage", "cluster", clusterStatus.ClusterName, "conditions", clusterStatus.Conditions, "updateRun", updateRunRef)
+		if condition.IsConditionStatusTrue(meta.FindStatusCondition(clusterStatus.Conditions,
+			string(placementv1beta1.ClusterUpdatingConditionStarted)), updateRun.GetGeneration()) && !condition.IsConditionStatusTrue(
+			meta.FindStatusCondition(clusterStatus.Conditions, string(placementv1beta1.ClusterUpdatingConditionSucceeded)),
+			updateRun.GetGeneration()) {
+			allDeletingClustersDeleted = false
+			break
+		}
+	}
+
+	if allDeletingClustersDeleted || len(toBeDeletedBindings) == 0 {
 		markStageUpdatingStopped(updateRunStatus.DeletionStageStatus, updateRun.GetGeneration())
 	}
 	return len(toBeDeletedBindings) == 0, nil
@@ -224,13 +239,13 @@ func markStageUpdatingStopped(stageUpdatingStatus *placementv1beta1.StageUpdatin
 }
 
 func checkIfErrorStagedUpdateAborted(err error, updateRun placementv1beta1.UpdateRunObj, updatingStageStatus *placementv1beta1.StageUpdatingStatus) {
-	updateRunStatus := updateRun.GetUpdateRunStatus()
 	if errors.Is(err, errStagedUpdatedAborted) {
 		if updatingStageStatus != nil {
 			klog.InfoS("The update run is aborted due to unrecoverable 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.
+			updateRunStatus := updateRun.GetUpdateRunStatus()
 			markStageUpdatingFailed(updateRunStatus.DeletionStageStatus, updateRun.GetGeneration(), err.Error())
 		}
 	}

pkg/controllers/updaterun/stop_integration_test.go

@@ -231,7 +231,7 @@ var _ = Describe("UpdateRun stop tests", func() {
 			validateUpdateRunMetricsEmitted(wantMetrics...)
 		})
 
-		It("Should be stopping the in middle of cluster updating when update run state is Stop", func() {
+		It("Should be stopping in the middle of cluster updating when update run state is Stop", func() {
 			By("Updating updateRun state to Stop")
 			updateRun = updateClusterStagedUpdateRunState(updateRun.Name, placementv1beta1.StateStop)
 			// Update the test's want status to match the new generation.
@@ -251,7 +251,7 @@ var _ = Describe("UpdateRun stop tests", func() {
 		})
 
 		It("Should wait for cluster to finish updating before update run is completely stopped", func() {
-			By("Validating the 2nd clusterResourceBinding is  updated to Bound")
+			By("Validating the 2nd clusterResourceBinding is updated to Bound")
 			binding := resourceBindings[numTargetClusters-3] // cluster-7
 			validateBindingState(ctx, binding, resourceSnapshot.Name, updateRun, 0)
 
@@ -812,6 +812,10 @@ var _ = Describe("UpdateRun stop tests", func() {
 					if !apierrors.IsNotFound(err) {
 						return fmt.Errorf("get binding %s does not return a not-found error: %w", binding.Name, err)
 					}
+
+					if !binding.DeletionTimestamp.IsZero() {
+						return fmt.Errorf("binding %s is not deleted yet", binding.Name)
+					}
 				}
 				return nil
 			}, timeout, interval).Should(Succeed(), "failed to validate the deletion of the to-be-deleted bindings")