Revert "fix deleteAvailableLimit bug (#1481)" (#1487)

* Revert "fix deleteAvailableLimit bug (#1481)" This fix is incorrect. This reverts commit f4e238f. Signed-off-by: liheng.zms <[email protected]> * add cloneset scale ut Signed-off-by: liheng.zms <[email protected]> --------- Signed-off-by: liheng.zms <[email protected]>
openkruise · Jan 24, 2024 · 19854e8 · 19854e8
1 parent 8af135d
commit 19854e8
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 35 deletions.
diff --git a/pkg/controller/cloneset/sync/cloneset_scale.go b/pkg/controller/cloneset/sync/cloneset_scale.go
@@ -110,16 +110,15 @@ func (r *realControl) Scale(
 	if podsToDelete := util.DiffPods(podsSpecifiedToDelete, podsInPreDelete); len(podsToDelete) > 0 {
 		newPodsToDelete, oldPodsToDelete := clonesetutils.GroupUpdateAndNotUpdatePods(podsToDelete, updateRevision)
 		klog.V(3).Infof("CloneSet %s try to delete pods specified. Delete ready limit: %d. New Pods: %v, old Pods: %v.",
-			controllerKey, diffRes.deleteAvailableLimit, util.GetPodNames(newPodsToDelete).List(), util.GetPodNames(oldPodsToDelete).List())
+			controllerKey, diffRes.deleteReadyLimit, util.GetPodNames(newPodsToDelete).List(), util.GetPodNames(oldPodsToDelete).List())
 
 		podsCanDelete := make([]*v1.Pod, 0, len(podsToDelete))
 		for _, pod := range podsToDelete {
-			// Determine pod available, since deleteAvailableLimit is also based on the pod available calculation
-			if !IsPodAvailable(coreControl, pod, updateCS.Spec.MinReadySeconds) {
+			if !isPodReady(coreControl, pod) {
 				podsCanDelete = append(podsCanDelete, pod)
-			} else if diffRes.deleteAvailableLimit > 0 {
+			} else if diffRes.deleteReadyLimit > 0 {
 				podsCanDelete = append(podsCanDelete, pod)
-				diffRes.deleteAvailableLimit--
+				diffRes.deleteReadyLimit--
 			}
 		}
 
@@ -137,17 +136,16 @@ func (r *realControl) Scale(
 		}
 
 		klog.V(3).Infof("CloneSet %s begin to scale in %d pods including %d (current rev), delete ready limit: %d",
-			controllerKey, diffRes.scaleDownNum, diffRes.scaleDownNumOldRevision, diffRes.deleteAvailableLimit)
+			controllerKey, diffRes.scaleDownNum, diffRes.scaleDownNumOldRevision, diffRes.deleteReadyLimit)
 
 		podsPreparingToDelete := r.choosePodsToDelete(updateCS, diffRes.scaleDownNum, diffRes.scaleDownNumOldRevision, notUpdatedPods, updatedPods)
 		podsToDelete := make([]*v1.Pod, 0, len(podsPreparingToDelete))
 		for _, pod := range podsPreparingToDelete {
-			// Determine pod available, since deleteAvailableLimit is also based on the pod available calculation
-			if !IsPodAvailable(coreControl, pod, updateCS.Spec.MinReadySeconds) {
+			if !isPodReady(coreControl, pod) {
 				podsToDelete = append(podsToDelete, pod)
-			} else if diffRes.deleteAvailableLimit > 0 {
+			} else if diffRes.deleteReadyLimit > 0 {
 				podsToDelete = append(podsToDelete, pod)
-				diffRes.deleteAvailableLimit--
+				diffRes.deleteReadyLimit--
 			}
 		}
 

diff --git a/pkg/controller/cloneset/sync/cloneset_scale_test.go b/pkg/controller/cloneset/sync/cloneset_scale_test.go
@@ -609,8 +609,8 @@ func TestScale(t *testing.T) {
 				}
 				return generatePods(obj, 5)
 			},
-			expectedPodsLen:  3,
-			expectedModified: true,
+			expectedPodsLen:  5,
+			expectedModified: false,
 		},
 		{
 			name: "cloneSet(replicas=3,maxUnavailable=20%,partition=nil,maxSurge=nil,minReadySeconds=0), specified delete pod-0, pods=5, and scale replicas 5 -> 3",

diff --git a/pkg/controller/cloneset/sync/cloneset_sync_utils.go b/pkg/controller/cloneset/sync/cloneset_sync_utils.go
@@ -63,9 +63,9 @@ type expectationDiffs struct {
 	// scaleUpLimit is the limit number of creating Pods when scaling up
 	// it is limited by scaleStrategy.maxUnavailable
 	scaleUpLimit int
-	// deleteAvailableLimit is the limit number of ready Pods that can be deleted
+	// deleteReadyLimit is the limit number of ready Pods that can be deleted
 	// it is limited by UpdateStrategy.maxUnavailable
-	deleteAvailableLimit int
+	deleteReadyLimit int
 
 	// useSurge is the number that temporarily expect to be above the desired replicas
 	useSurge int
@@ -253,7 +253,7 @@ func calculateDiffsWithExpectation(cs *appsv1alpha1.CloneSet, pods []*v1.Pod, cu
 		res.scaleDownNumOldRevision = integer.IntMax(currentTotalOldCount-toDeleteOldRevisionCount-expectedTotalOldCount, 0)
 	}
 	if toDeleteNewRevisionCount > 0 || toDeleteOldRevisionCount > 0 || res.scaleDownNum > 0 {
-		res.deleteAvailableLimit = integer.IntMax(maxUnavailable+(len(pods)-replicas)-totalUnavailable, 0)
+		res.deleteReadyLimit = integer.IntMax(maxUnavailable+(len(pods)-replicas)-totalUnavailable, 0)
 	}
 
 	// The consistency between scale and update will be guaranteed by syncCloneSet and expectations
@@ -281,6 +281,10 @@ func isSpecifiedDelete(cs *appsv1alpha1.CloneSet, pod *v1.Pod) bool {
 	return false
 }
 
+func isPodReady(coreControl clonesetcore.Control, pod *v1.Pod) bool {
+	return IsPodAvailable(coreControl, pod, 0)
+}
+
 func IsPodAvailable(coreControl clonesetcore.Control, pod *v1.Pod, minReadySeconds int32) bool {
 	state := lifecycle.GetPodLifecycleState(pod)
 	if state != "" && state != appspub.LifecycleStateNormal {

diff --git a/pkg/controller/cloneset/sync/cloneset_sync_utils_test.go b/pkg/controller/cloneset/sync/cloneset_sync_utils_test.go
@@ -80,7 +80,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 			},
-			expectResult: expectationDiffs{deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{deleteReadyLimit: 1},
 		},
 		{
 			name: "specified delete 1 pod (all ready) (step 2/3)",
@@ -115,7 +115,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 			},
-			expectResult: expectationDiffs{deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{deleteReadyLimit: 1},
 		},
 		{
 			name: "specified delete 2 pod (all ready) (step 2/6)",
@@ -150,7 +150,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false), // new creation
 			},
-			expectResult: expectationDiffs{deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{deleteReadyLimit: 1},
 		},
 		{
 			name: "specified delete 2 pod (all ready) (step 5/6)",
@@ -185,7 +185,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 			},
-			expectResult: expectationDiffs{deleteAvailableLimit: 2},
+			expectResult: expectationDiffs{deleteReadyLimit: 2},
 		},
 		{
 			name: "specified delete 2 pod and replicas to 4 (step 2/3)",
@@ -316,7 +316,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 			},
-			expectResult: expectationDiffs{deleteAvailableLimit: 1, useSurge: 1},
+			expectResult: expectationDiffs{deleteReadyLimit: 1, useSurge: 1},
 		},
 		{
 			name: "specified delete with maxSurge (step 4/4)",
@@ -366,7 +366,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateUpdating, false, false), // new in-place update
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false),   // new creation
 			},
-			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 0},
+			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 0},
 		},
 		{
 			name: "update in-place partition=3 with maxSurge (step 4/4)",
@@ -379,7 +379,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),  // new in-place update
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
 			},
-			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 1},
 		},
 		{
 			name: "update recreate partition=3 with maxSurge (step 1/7)",
@@ -417,7 +417,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(oldRevision, appspub.LifecycleStateNormal, true, true),   // begin to recreate
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
 			},
-			expectResult: expectationDiffs{useSurge: 1, useSurgeOldRevision: 1, deleteAvailableLimit: 1, updateNum: 1, updateMaxUnavailable: 2},
+			expectResult: expectationDiffs{useSurge: 1, useSurgeOldRevision: 1, deleteReadyLimit: 1, updateNum: 1, updateMaxUnavailable: 2},
 		},
 		{
 			name: "update recreate partition=3 with maxSurge (step 4/7)",
@@ -442,7 +442,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation for update
 			},
-			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 0},
+			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 0},
 		},
 		{
 			name: "update recreate partition=3 with maxSurge (step 6/7)",
@@ -455,7 +455,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),  // new creation
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation for update
 			},
-			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 1},
 		},
 		{
 			name: "update recreate partition=3 with maxSurge (step 7/7)",
@@ -492,7 +492,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
 			},
-			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 3},
+			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 3},
 		},
 		{
 			name: "update recreate partition=99% with maxUnavailable=3, maxSurge=2 (step 3/3)",
@@ -529,7 +529,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
 			},
-			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 2},
+			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 2},
 		},
 		{
 			name: "update recreate partition=99% with maxUnavailable=40%, maxSurge=30% (step 3/3)",
@@ -566,7 +566,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
 			},
-			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 1},
 		},
 		{
 			name: "update recreate partition=99% with maxUnavailable=30%, maxSurge=30% (step 3/3)",
@@ -656,7 +656,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 			},
 			revisionConsistent: true,
-			expectResult:       expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 2, deleteAvailableLimit: 2, updateNum: 1, updateMaxUnavailable: 2},
+			expectResult:       expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 2, deleteReadyLimit: 2, updateNum: 1, updateMaxUnavailable: 2},
 		},
 		{
 			name: "disable rollback feature-gate",
@@ -705,7 +705,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, true),
 			},
-			expectResult: expectationDiffs{deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{deleteReadyLimit: 1},
 		},
 		{
 			name: "[scalingExcludePreparingDelete=false] specific delete a pod with lifecycle hook (step 2/4)",
@@ -745,7 +745,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, true),
 			},
-			expectResult: expectationDiffs{deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{deleteReadyLimit: 1},
 		},
 		{
 			name:      "[scalingExcludePreparingDelete=true] specific delete a pod with lifecycle hook (step 2/4)",
@@ -791,7 +791,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, true),
 			},
-			expectResult: expectationDiffs{deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{deleteReadyLimit: 1},
 		},
 		{
 			name:      "[scalingExcludePreparingDelete=true] specific delete a pod with lifecycle hook and then cancel (step 2/5)",
@@ -826,7 +826,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false), // it has been changed to normal by managePreparingDelete
 				createTestPod(newRevision, appspub.LifecycleStateNormal, false, false),
 			},
-			expectResult: expectationDiffs{scaleDownNum: 1, deleteAvailableLimit: 1},
+			expectResult: expectationDiffs{scaleDownNum: 1, deleteReadyLimit: 1},
 		},
 		{
 			name:      "[scalingExcludePreparingDelete=true] specific delete a pod with lifecycle hook and then cancel (step 5/5)",
@@ -859,7 +859,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(newRevision, appspub.LifecycleStateNormal, true, true),
 			},
-			expectResult: expectationDiffs{deleteAvailableLimit: 2},
+			expectResult: expectationDiffs{deleteReadyLimit: 2},
 		},
 		{
 			name:      "[scalingExcludePreparingDelete=true] specific scale down with lifecycle hook, then scale up pods (step 3/6)",
@@ -950,7 +950,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
 				createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
 				createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
 			},
-			expectResult: expectationDiffs{scaleDownNum: 2, scaleDownNumOldRevision: 5, deleteAvailableLimit: 2, updateNum: 3, updateMaxUnavailable: 2},
+			expectResult: expectationDiffs{scaleDownNum: 2, scaleDownNumOldRevision: 5, deleteReadyLimit: 2, updateNum: 3, updateMaxUnavailable: 2},
 		},
 		{
 			name: "[UpdateStrategyPaused=true] create 0 newRevision pods with maxSurge=3,maxUnavailable=0",