From 349b9243cd72c68aaba19a0a3e6f204ec6e6cf09 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 23 Dec 2025 13:15:22 -0800 Subject: [PATCH 1/7] fix: return unexpected error if no. of updating cluster > maxConcurrency Signed-off-by: Arvind Thirumurugan --- pkg/controllers/updaterun/validation.go | 30 +++++++++++++++--- pkg/controllers/updaterun/validation_test.go | 33 +++++++++++++++++++- 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/pkg/controllers/updaterun/validation.go b/pkg/controllers/updaterun/validation.go index ffa5ea3c2..1963d6630 100644 --- a/pkg/controllers/updaterun/validation.go +++ b/pkg/controllers/updaterun/validation.go @@ -136,8 +136,9 @@ func (r *Reconciler) validateStagesStatus( func validateUpdateStagesStatus(existingStageStatus []placementv1beta1.StageUpdatingStatus, updateRun placementv1beta1.UpdateRunObj) (int, int, error) { updatingStageIndex := -1 lastFinishedStageIndex := -1 + updateRunStatus := updateRun.GetUpdateRunStatus() // Remember the newly computed stage status. - newStageStatus := updateRun.GetUpdateRunStatus().StagesStatus + newStageStatus := updateRunStatus.StagesStatus // Make sure the number of stages in the updateRun are still the same. if len(existingStageStatus) != len(newStageStatus) { mismatchErr := fmt.Errorf("the number of stages in the updateRun has changed, new: %d, existing: %d", len(newStageStatus), len(existingStageStatus)) @@ -164,9 +165,12 @@ func validateUpdateStagesStatus(existingStageStatus []placementv1beta1.StageUpda return -1, -1, fmt.Errorf("%w: %s", errStagedUpdatedAborted, mismatchErr.Error()) } } - - var err error - updatingStageIndex, lastFinishedStageIndex, err = validateClusterUpdatingStatus(curStage, updatingStageIndex, lastFinishedStageIndex, &existingStageStatus[curStage], updateRun) + // Calculate maxConcurrency for the current stage. + maxConcurrency, err := calculateMaxConcurrencyValue(updateRunStatus, curStage) + if err != nil { + return -1, -1, err + } + updatingStageIndex, lastFinishedStageIndex, err = validateClusterUpdatingStatus(curStage, updatingStageIndex, lastFinishedStageIndex, &existingStageStatus[curStage], maxConcurrency, updateRun) if err != nil { return -1, -1, err } @@ -181,6 +185,7 @@ func validateUpdateStagesStatus(existingStageStatus []placementv1beta1.StageUpda func validateClusterUpdatingStatus( curStage, updatingStageIndex, lastFinishedStageIndex int, stageStatus *placementv1beta1.StageUpdatingStatus, + maxConcurrency int, updateRun placementv1beta1.UpdateRunObj, ) (int, int, error) { stageSucceedCond := meta.FindStatusCondition(stageStatus.Conditions, string(placementv1beta1.StageUpdatingConditionSucceeded)) @@ -234,7 +239,22 @@ func validateClusterUpdatingStatus( return -1, -1, fmt.Errorf("%w: %s", errStagedUpdatedAborted, unexpectedErr.Error()) } updatingStageIndex = curStage - // TODO(arvindth): add validation to ensure updating cluster count should not exceed maxConcurrency. + // Collect the updating clusters. + var updatingClusters []string + for j := range stageStatus.Clusters { + clusterStartedCond := meta.FindStatusCondition(stageStatus.Clusters[j].Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted)) + clusterFinishedCond := meta.FindStatusCondition(stageStatus.Clusters[j].Conditions, string(placementv1beta1.ClusterUpdatingConditionSucceeded)) + // cluster is updating if it has started but not yet finished, we also consider failed clusters as updating clusters in execution. + if condition.IsConditionStatusTrue(clusterStartedCond, updateRun.GetGeneration()) && + !(condition.IsConditionStatusTrue(clusterFinishedCond, updateRun.GetGeneration()) || condition.IsConditionStatusFalse(clusterFinishedCond, updateRun.GetGeneration())) { + updatingClusters = append(updatingClusters, stageStatus.Clusters[j].ClusterName) + } + } + if len(updatingClusters) > maxConcurrency { + unexpectedErr := controller.NewUnexpectedBehaviorError(fmt.Errorf("the number of updating clusters `%d` in the updating stage `%s` exceeds maxConcurrency `%d`", len(updatingClusters), stageStatus.StageName, maxConcurrency)) + klog.ErrorS(unexpectedErr, "The number of updating clusters in the updating stage exceeds maxConcurrency", "updateRun", klog.KObj(updateRun)) + return -1, -1, fmt.Errorf("%w: %s", errStagedUpdatedAborted, unexpectedErr.Error()) + } } return updatingStageIndex, lastFinishedStageIndex, nil } diff --git a/pkg/controllers/updaterun/validation_test.go b/pkg/controllers/updaterun/validation_test.go index d6ad8215d..91232df94 100644 --- a/pkg/controllers/updaterun/validation_test.go +++ b/pkg/controllers/updaterun/validation_test.go @@ -40,6 +40,7 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { updatingStageIndex int lastFinishedStageIndex int stageStatus *placementv1beta1.StageUpdatingStatus + maxConcurrency int wantErr error wantUpdatingStageIndex int wantLastFinishedStageIndex int @@ -163,10 +164,39 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { }, }, }, + maxConcurrency: 2, wantErr: nil, wantUpdatingStageIndex: 0, wantLastFinishedStageIndex: -1, }, + { + name: "validateClusterUpdatingStatus should return error if number of updating clusters exceeds maxConcurrency", + curStage: 0, + updatingStageIndex: -1, + lastFinishedStageIndex: -1, + stageStatus: &placementv1beta1.StageUpdatingStatus{ + StageName: "test-stage", + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.StageUpdatingConditionProgressing)}, + Clusters: []placementv1beta1.ClusterUpdatingStatus{ + { + ClusterName: "cluster-1", + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted)}, + }, + { + ClusterName: "cluster-2", + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted)}, + }, + { + ClusterName: "cluster-3", + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted)}, + }, + }, + }, + maxConcurrency: 1, + wantErr: wrapErr(true, fmt.Errorf("the number of updating clusters `3` in the updating stage `test-stage` exceeds maxConcurrency `1`")), + wantUpdatingStageIndex: -1, + wantLastFinishedStageIndex: -1, + }, { name: "validateClusterUpdatingStatus should return -1 as the updatingStageIndex if no stage is updating", curStage: 0, @@ -188,6 +218,7 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { StageName: "test-stage", Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.StageUpdatingConditionProgressing)}, }, + maxConcurrency: 1, wantErr: nil, wantUpdatingStageIndex: 2, wantLastFinishedStageIndex: 1, @@ -213,7 +244,7 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { gotUpdatingStageIndex, gotLastFinishedStageIndex, err := - validateClusterUpdatingStatus(test.curStage, test.updatingStageIndex, test.lastFinishedStageIndex, test.stageStatus, updateRun) + validateClusterUpdatingStatus(test.curStage, test.updatingStageIndex, test.lastFinishedStageIndex, test.stageStatus, test.maxConcurrency, updateRun) if test.wantErr == nil { if err != nil { t.Fatalf("validateClusterUpdatingStatus() got error = %+v, want error = nil", err) From 3297a88d64e6f5ac8d667918899e5bf03d7f147b Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 31 Dec 2025 14:59:40 -0800 Subject: [PATCH 2/7] address comment Signed-off-by: Arvind Thirumurugan --- pkg/controllers/updaterun/validation.go | 3 +-- pkg/controllers/updaterun/validation_test.go | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/controllers/updaterun/validation.go b/pkg/controllers/updaterun/validation.go index 1963d6630..b5352d369 100644 --- a/pkg/controllers/updaterun/validation.go +++ b/pkg/controllers/updaterun/validation.go @@ -245,8 +245,7 @@ func validateClusterUpdatingStatus( clusterStartedCond := meta.FindStatusCondition(stageStatus.Clusters[j].Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted)) clusterFinishedCond := meta.FindStatusCondition(stageStatus.Clusters[j].Conditions, string(placementv1beta1.ClusterUpdatingConditionSucceeded)) // cluster is updating if it has started but not yet finished, we also consider failed clusters as updating clusters in execution. - if condition.IsConditionStatusTrue(clusterStartedCond, updateRun.GetGeneration()) && - !(condition.IsConditionStatusTrue(clusterFinishedCond, updateRun.GetGeneration()) || condition.IsConditionStatusFalse(clusterFinishedCond, updateRun.GetGeneration())) { + if condition.IsConditionStatusTrue(clusterStartedCond, updateRun.GetGeneration()) && !(condition.IsConditionStatusTrue(clusterFinishedCond, updateRun.GetGeneration())) { updatingClusters = append(updatingClusters, stageStatus.Clusters[j].ClusterName) } } diff --git a/pkg/controllers/updaterun/validation_test.go b/pkg/controllers/updaterun/validation_test.go index 91232df94..4191985b1 100644 --- a/pkg/controllers/updaterun/validation_test.go +++ b/pkg/controllers/updaterun/validation_test.go @@ -160,7 +160,7 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { }, { ClusterName: "cluster-2", - Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted)}, + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted), generateFalseCondition(updateRun, placementv1beta1.ClusterUpdatingConditionSucceeded)}, }, }, }, @@ -184,11 +184,11 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { }, { ClusterName: "cluster-2", - Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted)}, + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted), generateFalseCondition(updateRun, placementv1beta1.ClusterUpdatingConditionSucceeded)}, }, { ClusterName: "cluster-3", - Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted)}, + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted), generateFalseCondition(updateRun, placementv1beta1.ClusterUpdatingConditionSucceeded)}, }, }, }, From db699f48c340d3f395cf61214a3181afe82a236d Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 31 Dec 2025 15:03:30 -0800 Subject: [PATCH 3/7] add UT Signed-off-by: Arvind Thirumurugan --- pkg/controllers/updaterun/validation_test.go | 24 ++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/pkg/controllers/updaterun/validation_test.go b/pkg/controllers/updaterun/validation_test.go index 4191985b1..7a0d43df7 100644 --- a/pkg/controllers/updaterun/validation_test.go +++ b/pkg/controllers/updaterun/validation_test.go @@ -169,6 +169,30 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { wantUpdatingStageIndex: 0, wantLastFinishedStageIndex: -1, }, + { + name: "determineUpdatignStage should not return error if there are multiple clusters have succeeded in an updating stage", + curStage: 0, + updatingStageIndex: -1, + lastFinishedStageIndex: -1, + stageStatus: &placementv1beta1.StageUpdatingStatus{ + StageName: "test-stage", + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.StageUpdatingConditionProgressing)}, + Clusters: []placementv1beta1.ClusterUpdatingStatus{ + { + ClusterName: "cluster-1", + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted), generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionSucceeded)}, + }, + { + ClusterName: "cluster-2", + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionStarted), generateTrueCondition(updateRun, placementv1beta1.ClusterUpdatingConditionSucceeded)}, + }, + }, + }, + maxConcurrency: 2, + wantErr: nil, + wantUpdatingStageIndex: 0, + wantLastFinishedStageIndex: -1, + }, { name: "validateClusterUpdatingStatus should return error if number of updating clusters exceeds maxConcurrency", curStage: 0, From 7b2317cfc49833565deabaf8ccae3d167177c46e Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 31 Dec 2025 15:07:31 -0800 Subject: [PATCH 4/7] minor fix Signed-off-by: Arvind Thirumurugan --- pkg/controllers/updaterun/validation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/updaterun/validation_test.go b/pkg/controllers/updaterun/validation_test.go index 7a0d43df7..daadccd1e 100644 --- a/pkg/controllers/updaterun/validation_test.go +++ b/pkg/controllers/updaterun/validation_test.go @@ -170,7 +170,7 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { wantLastFinishedStageIndex: -1, }, { - name: "determineUpdatignStage should not return error if there are multiple clusters have succeeded in an updating stage", + name: "determineUpdatignStage should not return error if multiple clusters have succeeded in an updating stage", curStage: 0, updatingStageIndex: -1, lastFinishedStageIndex: -1, From 597cfec2776dba2b452484bd0152f3f5dad45b2d Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 6 Jan 2026 15:19:32 -0800 Subject: [PATCH 5/7] address minor comment Signed-off-by: Arvind Thirumurugan --- pkg/controllers/updaterun/validation.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controllers/updaterun/validation.go b/pkg/controllers/updaterun/validation.go index b5352d369..ac5595cd1 100644 --- a/pkg/controllers/updaterun/validation.go +++ b/pkg/controllers/updaterun/validation.go @@ -240,17 +240,17 @@ func validateClusterUpdatingStatus( } updatingStageIndex = curStage // Collect the updating clusters. - var updatingClusters []string + updatingClusterCount := 0 for j := range stageStatus.Clusters { clusterStartedCond := meta.FindStatusCondition(stageStatus.Clusters[j].Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted)) clusterFinishedCond := meta.FindStatusCondition(stageStatus.Clusters[j].Conditions, string(placementv1beta1.ClusterUpdatingConditionSucceeded)) // cluster is updating if it has started but not yet finished, we also consider failed clusters as updating clusters in execution. if condition.IsConditionStatusTrue(clusterStartedCond, updateRun.GetGeneration()) && !(condition.IsConditionStatusTrue(clusterFinishedCond, updateRun.GetGeneration())) { - updatingClusters = append(updatingClusters, stageStatus.Clusters[j].ClusterName) + updatingClusterCount++ } } - if len(updatingClusters) > maxConcurrency { - unexpectedErr := controller.NewUnexpectedBehaviorError(fmt.Errorf("the number of updating clusters `%d` in the updating stage `%s` exceeds maxConcurrency `%d`", len(updatingClusters), stageStatus.StageName, maxConcurrency)) + if updatingClusterCount > maxConcurrency { + unexpectedErr := controller.NewUnexpectedBehaviorError(fmt.Errorf("the number of updating clusters `%d` in the updating stage `%s` exceeds maxConcurrency `%d`", updatingClusterCount, stageStatus.StageName, maxConcurrency)) klog.ErrorS(unexpectedErr, "The number of updating clusters in the updating stage exceeds maxConcurrency", "updateRun", klog.KObj(updateRun)) return -1, -1, fmt.Errorf("%w: %s", errStagedUpdatedAborted, unexpectedErr.Error()) } From 681bdfb239d0738daee3231894277f1c2d45d1c2 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 6 Jan 2026 15:57:01 -0800 Subject: [PATCH 6/7] address comments Signed-off-by: Arvind Thirumurugan --- pkg/controllers/updaterun/execution.go | 2 +- pkg/controllers/updaterun/validation.go | 2 +- pkg/controllers/updaterun/validation_test.go | 22 ++++++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/updaterun/execution.go b/pkg/controllers/updaterun/execution.go index e8a48d4db..4f9f88a68 100644 --- a/pkg/controllers/updaterun/execution.go +++ b/pkg/controllers/updaterun/execution.go @@ -87,7 +87,7 @@ func (r *Reconciler) execute( } maxConcurrency, err := calculateMaxConcurrencyValue(updateRunStatus, updatingStageIndex) if err != nil { - return false, 0, err + return false, 0, fmt.Errorf("%w: %s", errStagedUpdatedAborted, err.Error()) } waitTime, err = r.executeUpdatingStage(ctx, updateRun, updatingStageIndex, toBeUpdatedBindings, maxConcurrency) // The execution has not finished yet. diff --git a/pkg/controllers/updaterun/validation.go b/pkg/controllers/updaterun/validation.go index ac5595cd1..fdf0c5391 100644 --- a/pkg/controllers/updaterun/validation.go +++ b/pkg/controllers/updaterun/validation.go @@ -168,7 +168,7 @@ func validateUpdateStagesStatus(existingStageStatus []placementv1beta1.StageUpda // Calculate maxConcurrency for the current stage. maxConcurrency, err := calculateMaxConcurrencyValue(updateRunStatus, curStage) if err != nil { - return -1, -1, err + return -1, -1, fmt.Errorf("%w: %s", errStagedUpdatedAborted, err.Error()) } updatingStageIndex, lastFinishedStageIndex, err = validateClusterUpdatingStatus(curStage, updatingStageIndex, lastFinishedStageIndex, &existingStageStatus[curStage], maxConcurrency, updateRun) if err != nil { diff --git a/pkg/controllers/updaterun/validation_test.go b/pkg/controllers/updaterun/validation_test.go index daadccd1e..da373283d 100644 --- a/pkg/controllers/updaterun/validation_test.go +++ b/pkg/controllers/updaterun/validation_test.go @@ -145,6 +145,28 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { wantUpdatingStageIndex: -1, wantLastFinishedStageIndex: -1, }, + { + name: "determineUpdatignStage should not return error if there are multiple clusters in an updating stage with no condition set (execution not started)", + curStage: 0, + updatingStageIndex: -1, + lastFinishedStageIndex: -1, + stageStatus: &placementv1beta1.StageUpdatingStatus{ + StageName: "test-stage", + Conditions: []metav1.Condition{generateTrueCondition(updateRun, placementv1beta1.StageUpdatingConditionProgressing)}, + Clusters: []placementv1beta1.ClusterUpdatingStatus{ + { + ClusterName: "cluster-1", + }, + { + ClusterName: "cluster-2", + }, + }, + }, + maxConcurrency: 2, + wantErr: nil, + wantUpdatingStageIndex: 0, + wantLastFinishedStageIndex: -1, + }, { name: "determineUpdatignStage should not return error if there are multiple clusters updating in an updating stage", curStage: 0, From 91b853f29a53c243c11d51dde646e4193ffa4a92 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 7 Jan 2026 13:46:08 -0800 Subject: [PATCH 7/7] address comments Signed-off-by: Arvind Thirumurugan --- pkg/controllers/updaterun/validation_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/updaterun/validation_test.go b/pkg/controllers/updaterun/validation_test.go index da373283d..14fa2be14 100644 --- a/pkg/controllers/updaterun/validation_test.go +++ b/pkg/controllers/updaterun/validation_test.go @@ -162,7 +162,7 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { }, }, }, - maxConcurrency: 2, + maxConcurrency: 1, wantErr: nil, wantUpdatingStageIndex: 0, wantLastFinishedStageIndex: -1, @@ -210,7 +210,7 @@ func TestValidateClusterUpdatingStatus(t *testing.T) { }, }, }, - maxConcurrency: 2, + maxConcurrency: 1, wantErr: nil, wantUpdatingStageIndex: 0, wantLastFinishedStageIndex: -1,