Skip to content

Commit

Permalink
Revert "fix deleteAvailableLimit bug (openkruise#1481)" (openkruise#1487
Browse files Browse the repository at this point in the history 
)

* Revert "fix deleteAvailableLimit bug (openkruise#1481)"

This fix is incorrect.

This reverts commit f4e238f.


* add cloneset scale ut


---------
  • Loading branch information
@zmberg
zmberg authored Jan 24, 2024
1 parent 18a2f36 commit 310e39f
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 35 deletions.
18 changes: 8 additions & 10 deletions pkg/controller/cloneset/sync/cloneset_scale.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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--
}
}

Expand All @@ -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--
}
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/cloneset/sync/cloneset_scale_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
10 changes: 7 additions & 3 deletions pkg/controller/cloneset/sync/cloneset_sync_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
40 changes: 20 additions & 20 deletions pkg/controller/cloneset/sync/cloneset_sync_utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand All @@ -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)",
Expand Down Expand Up @@ -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)",
Expand All @@ -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)",
Expand All @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 310e39f

Please sign in to comment.