From 19854e846a268f118884a3682ba3c6578a728196 Mon Sep 17 00:00:00 2001 From: berg Date: Wed, 24 Jan 2024 11:09:37 +0800 Subject: [PATCH] Revert "fix deleteAvailableLimit bug (#1481)" (#1487) * Revert "fix deleteAvailableLimit bug (#1481)" This fix is incorrect. This reverts commit f4e238fd8d45ff81b0c102cf58228daa66c0d5bc. Signed-off-by: liheng.zms * add cloneset scale ut Signed-off-by: liheng.zms --------- Signed-off-by: liheng.zms --- .../cloneset/sync/cloneset_scale.go | 18 ++++----- .../cloneset/sync/cloneset_scale_test.go | 4 +- .../cloneset/sync/cloneset_sync_utils.go | 10 +++-- .../cloneset/sync/cloneset_sync_utils_test.go | 40 +++++++++---------- 4 files changed, 37 insertions(+), 35 deletions(-) diff --git a/pkg/controller/cloneset/sync/cloneset_scale.go b/pkg/controller/cloneset/sync/cloneset_scale.go index acd06bbe66..c78399e38c 100644 --- 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 index 4f01e3447f..e11a07c8ae 100644 --- 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 index 3ba1e95641..9a7bd19649 100644 --- 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 index 18bd5ee1f8..50dd0926b5 100644 --- 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",