Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 24, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6fd2f5
Revert "fix deleteAvailableLimit bug (#1481)"
zmberg Jan 11, 2024
1597112
add cloneset scale ut
zmberg Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions pkg/controller/cloneset/sync/cloneset_scale.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading