rebase and address comments

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

Author: britaniar

pkg/controllers/updaterun/controller_integration_test.go

@@ -812,26 +812,17 @@ func generateFalseProgressingCondition(obj client.Object, condType any, succeede
 
 func generateTrueStageTaskCondition(obj client.Object, condType any, isBeforeStage bool) metav1.Condition {
 	reason, typeStr := "", ""
-	switch cond := condType.(type) {
-	case placementv1beta1.StageTaskConditionType:
-		switch cond {
-		case placementv1beta1.StageTaskConditionWaitTimeElapsed:
-			reason = condition.AfterStageTaskWaitTimeElapsedReason
-		case placementv1beta1.StageTaskConditionApprovalRequestCreated:
-			if isBeforeStage {
-				reason = condition.BeforeStageTaskApprovalRequestCreatedReason
-			} else {
-				reason = condition.AfterStageTaskApprovalRequestCreatedReason
-			}
-		case placementv1beta1.StageTaskConditionApprovalRequestApproved:
-			if isBeforeStage {
-				reason = condition.BeforeStageTaskApprovalRequestApprovedReason
-			} else {
-				reason = condition.AfterStageTaskApprovalRequestApprovedReason
-			}
-		}
-		typeStr = string(cond)
+	cond := condType.(placementv1beta1.StageTaskConditionType)
+	switch cond {
+	case placementv1beta1.StageTaskConditionWaitTimeElapsed:
+		reason = condition.AfterStageTaskWaitTimeElapsedReason
+	case placementv1beta1.StageTaskConditionApprovalRequestCreated:
+		reason = condition.StageTaskApprovalRequestCreatedReason
+	case placementv1beta1.StageTaskConditionApprovalRequestApproved:
+		reason = condition.StageTaskApprovalRequestApprovedReason
 	}
+	typeStr = string(cond)
+
 	return metav1.Condition{
 		Status:             metav1.ConditionTrue,
 		Type:               typeStr,

pkg/controllers/updaterun/execution.go

@@ -167,9 +167,6 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 	Context("Cluster staged update run should update clusters one by one", Ordered, func() {
 		var wantApprovalRequest *placementv1beta1.ClusterApprovalRequest
 		BeforeAll(func() {
-			By("Update the strategy with an approval before stage task and after stage tasks")
-			Expect(k8sClient.Update(ctx, updateStrategy)).To(Succeed())
-
 			By("Creating a new clusterStagedUpdateRun")
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
@@ -180,7 +177,7 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 			wantStatus = generateExecutionNotStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
-			By("Validating the first approvalRequest has been created")
+			By("Validating the first beforeStage approvalRequest has been created")
 			wantApprovalRequest = &placementv1beta1.ClusterApprovalRequest{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: updateRun.Status.StagesStatus[0].BeforeStageTaskStatus[0].ApprovalRequestName,
@@ -319,7 +316,7 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 			meta.SetStatusCondition(&binding.Status.Conditions, generateTrueCondition(binding, placementv1beta1.ResourceBindingAvailable))
 			Expect(k8sClient.Status().Update(ctx, binding)).Should(Succeed(), "failed to update the binding status")
 
-			By("Validating the 5th cluster has succeeded and 1st has completed and is waiting for next after stage tasks")
+			By("Validating the 5th cluster has succeeded and 1st stage has completed and is waiting for AfterStageTasks")
 			// 5th cluster succeeded.
 			wantStatus.StagesStatus[0].Clusters[4].Conditions = append(wantStatus.StagesStatus[0].Clusters[4].Conditions, generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionSucceeded))
 			// Now waiting for after stage tasks of 1st stage.
@@ -355,9 +352,6 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 			approveClusterApprovalRequest(ctx, wantApprovalRequest.Name)
 
 			By("Validating both after stage tasks have completed and 2nd stage has started")
-			// Timedwait afterStageTask completed.
-			wantStatus.StagesStatus[0].AfterStageTaskStatus[0].Conditions = append(wantStatus.StagesStatus[0].AfterStageTaskStatus[0].Conditions,
-				generateTrueCondition(updateRun, placementv1beta1.StageTaskConditionWaitTimeElapsed))
 			// Approval afterStageTask completed.
 			wantStatus.StagesStatus[0].AfterStageTaskStatus[1].Conditions = append(wantStatus.StagesStatus[0].AfterStageTaskStatus[1].Conditions,
 				generateTrueCondition(updateRun, placementv1beta1.StageTaskConditionApprovalRequestApproved))
@@ -926,7 +920,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
 			wantStatus = generateExecutionNotStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
-			By("Validating the approvalRequest has been created")
+			By("Validating the first beforeStage approvalRequest has been created")
 			wantApprovalRequest = &placementv1beta1.ClusterApprovalRequest{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: updateRun.Status.StagesStatus[0].BeforeStageTaskStatus[0].ApprovalRequestName,
@@ -1617,7 +1611,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 			By("Validating the initialization succeeded and the execution started")
-			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, policySnapshot, updateStrategy)
+			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy)
 			initialized.StagesStatus[0].BeforeStageTaskStatus[0].Conditions = append(initialized.StagesStatus[0].BeforeStageTaskStatus[0].Conditions,
 				generateTrueStageTaskCondition(updateRun, placementv1beta1.StageTaskConditionApprovalRequestCreated, true))
 			wantStatus = generateExecutionNotStartedStatus(updateRun, initialized)

pkg/controllers/updaterun/execution_integration_test.go

@@ -17,12 +17,16 @@ limitations under the License.
 package updaterun
 
 import (
+	"context"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
 	"github.com/kubefleet-dev/kubefleet/pkg/utils/condition"
@@ -398,3 +402,63 @@ func TestBuildApprovalRequestObject(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckBeforeStageTasksStatus_NegativeCases(t *testing.T) {
+	tests := []struct {
+		name       string
+		stageIndex int
+		updateRun  *placementv1beta1.ClusterStagedUpdateRun
+	}{
+		// Negative test cases only
+		{
+			name:       "should return err if before stage task is TimedWait",
+			stageIndex: 0,
+			updateRun: &placementv1beta1.ClusterStagedUpdateRun{
+				Status: placementv1beta1.UpdateRunStatus{
+					UpdateStrategySnapshot: &placementv1beta1.UpdateStrategySpec{
+						Stages: []placementv1beta1.StageConfig{
+							{
+								Name: "stage-0",
+								BeforeStageTasks: []placementv1beta1.StageTask{
+									{
+										Type: placementv1beta1.StageTaskTypeTimedWait,
+									},
+								},
+							},
+						},
+					},
+					StagesStatus: []placementv1beta1.StageUpdatingStatus{
+						{
+							StageName: "stage-0",
+							BeforeStageTaskStatus: []placementv1beta1.StageTaskStatus{
+								{
+									Type: placementv1beta1.StageTaskTypeTimedWait,
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			objects := []client.Object{tt.updateRun}
+			scheme := runtime.NewScheme()
+			_ = placementv1beta1.AddToScheme(scheme)
+			fakeClient := fake.NewClientBuilder().
+				WithScheme(scheme).
+				WithObjects(objects...).
+				WithStatusSubresource(objects...).
+				Build()
+			r := Reconciler{
+				Client: fakeClient,
+			}
+			ctx := context.Background()
+			_, err := r.checkBeforeStageTasksStatus(ctx, tt.stageIndex, tt.updateRun)
+			if err == nil {
+				t.Fatalf("checkBeforeStageTasksStatus() expected error but got nil")
+			}
+		})
+	}
+}

pkg/controllers/updaterun/execution_test.go

@@ -587,7 +587,7 @@ var _ = Describe("Updaterun initialization tests", func() {
 					Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 					By("Validating the initialization succeeded")
-					generateSucceededInitializationStatus(crp, updateRun, policySnapshot, updateStrategy, clusterResourceOverride)
+					generateSucceededInitializationStatus(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy, clusterResourceOverride)
 				})
 			})
 

pkg/controllers/updaterun/initialization_integration_test.go

@@ -194,23 +194,20 @@ const (
 	// ClusterUpdatingSucceededReason is the reason string of condition if the cluster updating succeeded.
 	ClusterUpdatingSucceededReason = "ClusterUpdatingSucceeded"
 
-	// BeforeStageTaskApprovalRequestApprovedReason is the reason string of condition if the approval request for before stage task has been approved.
-	BeforeStageTaskApprovalRequestApprovedReason = "BeforeStageTaskApprovalRequestApproved"
+	// StageTaskApprovalRequestApprovedReason is the reason string of condition if the approval request for before or after stage task has been approved.
+	StageTaskApprovalRequestApprovedReason = "StageTaskApprovalRequestApproved"
 
-	// BeforeStageTaskApprovalRequestCreatedReason is the reason string of condition if the approval request for before stage task has been created.
-	BeforeStageTaskApprovalRequestCreatedReason = "BeforeStageTaskApprovalRequestCreated"
-
-	// AfterStageTaskApprovalRequestApprovedReason is the reason string of condition if the approval request for after stage task has been approved.
-	AfterStageTaskApprovalRequestApprovedReason = "AfterStageTaskApprovalRequestApproved"
-
-	// AfterStageTaskApprovalRequestCreatedReason is the reason string of condition if the approval request for after stage task has been created.
-	AfterStageTaskApprovalRequestCreatedReason = "AfterStageTaskApprovalRequestCreated"
+	// StageTaskApprovalRequestCreatedReason is the reason string of condition if the approval request for beofre or after stage task has been created.
+	StageTaskApprovalRequestCreatedReason = "StageTaskApprovalRequestCreated"
 
 	// AfterStageTaskWaitTimeElapsedReason is the reason string of condition if the wait time for after stage task has elapsed.
 	AfterStageTaskWaitTimeElapsedReason = "AfterStageTaskWaitTimeElapsed"
 
 	// ApprovalRequestApprovalAcceptedReason is the reason string of condition if the approval of the approval request has been accepted.
 	ApprovalRequestApprovalAcceptedReason = "ApprovalRequestApprovalAccepted"
+
+	// UpdateRunWaitingMessageFmt is the message format string of condition if the staged update run is waiting for stage tasks in a stage to complete.
+	UpdateRunWaitingMessageFmt = "The updateRun is waiting for %s tasks in stage %s to complete"
 )
 
 // A group of condition reason & message string which is used to populate the ClusterResourcePlacementEviction condition.

pkg/utils/condition/reason.go

@@ -2040,35 +2040,19 @@ func updateRunStageRolloutSucceedConditions(generation int64) []metav1.Condition
 	}
 }
 
-func updateRunStageTaskSucceedConditions(generation int64, taskType placementv1beta1.StageTaskType, isBeforeStage bool) []metav1.Condition {
+func updateRunStageTaskSucceedConditions(generation int64, taskType placementv1beta1.StageTaskType) []metav1.Condition {
 	if taskType == placementv1beta1.StageTaskTypeApproval {
-		if isBeforeStage {
-			return []metav1.Condition{
-				{
-					Type:               string(placementv1beta1.StageTaskConditionApprovalRequestCreated),
-					Status:             metav1.ConditionTrue,
-					Reason:             condition.BeforeStageTaskApprovalRequestCreatedReason,
-					ObservedGeneration: generation,
-				},
-				{
-					Type:               string(placementv1beta1.StageTaskConditionApprovalRequestApproved),
-					Status:             metav1.ConditionTrue,
-					Reason:             condition.BeforeStageTaskApprovalRequestApprovedReason,
-					ObservedGeneration: generation,
-				},
-			}
-		}
 		return []metav1.Condition{
 			{
 				Type:               string(placementv1beta1.StageTaskConditionApprovalRequestCreated),
 				Status:             metav1.ConditionTrue,
-				Reason:             condition.AfterStageTaskApprovalRequestCreatedReason,
+				Reason:             condition.StageTaskApprovalRequestCreatedReason,
 				ObservedGeneration: generation,
 			},
 			{
 				Type:               string(placementv1beta1.StageTaskConditionApprovalRequestApproved),
 				Status:             metav1.ConditionTrue,
-				Reason:             condition.AfterStageTaskApprovalRequestApprovedReason,
+				Reason:             condition.StageTaskApprovalRequestApprovedReason,
 				ObservedGeneration: generation,
 			},
 		}
@@ -2200,15 +2184,15 @@ func buildStageUpdatingStatuses(
 			if task.Type == placementv1beta1.StageTaskTypeApproval {
 				stagesStatus[i].AfterStageTaskStatus[j].ApprovalRequestName = fmt.Sprintf(placementv1beta1.AfterStageApprovalTaskNameFmt, updateRun.GetName(), stage.Name)
 			}
-			stagesStatus[i].AfterStageTaskStatus[j].Conditions = updateRunStageTaskSucceedConditions(updateRun.GetGeneration(), task.Type, false)
+			stagesStatus[i].AfterStageTaskStatus[j].Conditions = updateRunStageTaskSucceedConditions(updateRun.GetGeneration(), task.Type)
 		}
 		stagesStatus[i].BeforeStageTaskStatus = make([]placementv1beta1.StageTaskStatus, len(stage.BeforeStageTasks))
 		for j, task := range stage.BeforeStageTasks {
 			stagesStatus[i].BeforeStageTaskStatus[j].Type = task.Type
 			if task.Type == placementv1beta1.StageTaskTypeApproval {
 				stagesStatus[i].BeforeStageTaskStatus[j].ApprovalRequestName = fmt.Sprintf(placementv1beta1.BeforeStageApprovalTaskNameFmt, updateRun.GetName(), stage.Name)
 			}
-			stagesStatus[i].BeforeStageTaskStatus[j].Conditions = updateRunStageTaskSucceedConditions(updateRun.GetGeneration(), task.Type, true)
+			stagesStatus[i].BeforeStageTaskStatus[j].Conditions = updateRunStageTaskSucceedConditions(updateRun.GetGeneration(), task.Type)
 		}
 		stagesStatus[i].Conditions = updateRunStageRolloutSucceedConditions(updateRun.GetGeneration())
 	}

test/e2e/actuals_test.go

@@ -215,7 +215,16 @@ var _ = Describe("test CRP rollout with staged update run", func() {
 			validateAndApproveClusterApprovalRequests(updateRunNames[1], envCanary)
 		})
 
-		It("Should rollout resources to member-cluster-1 and member-cluster-3 too and complete the cluster staged update run successfully", func() {
+		It("Should not rollout resources to prod stage until approved", func() {
+			checkIfRemovedWorkResourcesFromMemberClustersConsistently([]*framework.Cluster{allMemberClusters[0], allMemberClusters[2]})
+		})
+
+		It("Should rollout resources to all the members after approval and complete the cluster staged update run successfully", func() {
+			validateAndApproveClusterApprovalRequests(updateRunNames[0], envProd)
+
+			By("Should rollout resources to member-cluster-1 first because of its name")
+			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun([]*framework.Cluster{allMemberClusters[0]})
+
 			csurSucceededActual := clusterStagedUpdateRunStatusSucceededActual(updateRunNames[1], resourceSnapshotIndex2nd, policySnapshotIndex1st, len(allMemberClusters), defaultApplyStrategy, &strategy.Spec, [][]string{{allMemberClusterNames[1]}, {allMemberClusterNames[0], allMemberClusterNames[2]}}, nil, nil, nil)
 			Eventually(csurSucceededActual, updateRunEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to validate updateRun %s succeeded", updateRunNames[1])
 			By("Verify that new the configmap is updated on all member clusters")

test/e2e/cluster_staged_updaterun_test.go

@@ -137,11 +137,16 @@ var _ = Describe("test RP rollout with staged update run", Label("resourceplacem
 			validateAndApproveNamespacedApprovalRequests(updateRunNames[0], testNamespace, envCanary)
 		})
 
-		It("Should rollout resources to member-cluster-1 first because of its name", func() {
-			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun([]*framework.Cluster{allMemberClusters[0]})
+		It("Should not rollout resources to prod stage until approved", func() {
+			checkIfRemovedConfigMapFromMemberClustersConsistently([]*framework.Cluster{allMemberClusters[0], allMemberClusters[2]})
 		})
 
-		It("Should rollout resources to all the members and complete the staged update run successfully", func() {
+		It("Should rollout resources to all the members after approval and complete the staged update run successfully", func() {
+			validateAndApproveNamespacedApprovalRequests(updateRunNames[0], testNamespace, envProd)
+
+			By("Should rollout resources to member-cluster-1 first because of its name")
+			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun([]*framework.Cluster{allMemberClusters[0]})
+
 			surSucceededActual := stagedUpdateRunStatusSucceededActual(updateRunNames[0], testNamespace, resourceSnapshotIndex1st, policySnapshotIndex1st, len(allMemberClusters), defaultApplyStrategy, &strategy.Spec, [][]string{{allMemberClusterNames[1]}, {allMemberClusterNames[0], allMemberClusterNames[2]}}, nil, nil, nil)
 			Eventually(surSucceededActual, updateRunEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to validate updateRun %s/%s succeeded", testNamespace, updateRunNames[0])
 			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun(allMemberClusters)
@@ -309,6 +314,9 @@ var _ = Describe("test RP rollout with staged update run", Label("resourceplacem
 		It("Should rollout resources to all the members after approval and complete the staged update run successfully", func() {
 			validateAndApproveNamespacedApprovalRequests(updateRunNames[0], testNamespace, envProd)
 
+			By("Should rollout resources to member-cluster-1 first because of its name")
+			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun([]*framework.Cluster{allMemberClusters[0]})
+
 			surSucceededActual := stagedUpdateRunStatusSucceededActual(updateRunNames[0], testNamespace, resourceSnapshotIndex1st, policySnapshotIndex1st, len(allMemberClusters), defaultApplyStrategy, &strategy.Spec, [][]string{{allMemberClusterNames[1]}, {allMemberClusterNames[0], allMemberClusterNames[2]}}, nil, nil, nil)
 			Eventually(surSucceededActual, updateRunEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to validate updateRun %s/%s succeeded", testNamespace, updateRunNames[0])
 			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun(allMemberClusters)