Skip to content

Commit 46434b6

Browse files
zmbergzmberg
committed
fix deleteAvailableLimit bug
Signed-off-by: liheng.zms <[email protected]>
1 parent fa7a1da commit 46434b6

File tree

3 files changed

+33
-35
lines changed

3 files changed

+33
-35
lines changed

‎pkg/controller/cloneset/sync/cloneset_scale.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -110,15 +110,16 @@ func (r *realControl) Scale(
110110
if podsToDelete := util.DiffPods(podsSpecifiedToDelete, podsInPreDelete); len(podsToDelete) > 0 {
111111
newPodsToDelete, oldPodsToDelete := clonesetutils.GroupUpdateAndNotUpdatePods(podsToDelete, updateRevision)
112112
klog.V(3).Infof("CloneSet %s try to delete pods specified. Delete ready limit: %d. New Pods: %v, old Pods: %v.",
113-
controllerKey, diffRes.deleteReadyLimit, util.GetPodNames(newPodsToDelete).List(), util.GetPodNames(oldPodsToDelete).List())
113+
controllerKey, diffRes.deleteAvailableLimit, util.GetPodNames(newPodsToDelete).List(), util.GetPodNames(oldPodsToDelete).List())
114114

115115
podsCanDelete := make([]*v1.Pod, 0, len(podsToDelete))
116116
for _, pod := range podsToDelete {
117-
if !isPodReady(coreControl, pod) {
117+
// Determine pod available, since deleteAvailableLimit is also based on the pod available calculation
118+
if !IsPodAvailable(coreControl, pod, updateCS.Spec.MinReadySeconds) {
118119
podsCanDelete = append(podsCanDelete, pod)
119-
} else if diffRes.deleteReadyLimit > 0 {
120+
} else if diffRes.deleteAvailableLimit > 0 {
120121
podsCanDelete = append(podsCanDelete, pod)
121-
diffRes.deleteReadyLimit--
122+
diffRes.deleteAvailableLimit--
122123
}
123124
}
124125

@@ -136,16 +137,17 @@ func (r *realControl) Scale(
136137
}
137138

138139
klog.V(3).Infof("CloneSet %s begin to scale in %d pods including %d (current rev), delete ready limit: %d",
139-
controllerKey, diffRes.scaleDownNum, diffRes.scaleDownNumOldRevision, diffRes.deleteReadyLimit)
140+
controllerKey, diffRes.scaleDownNum, diffRes.scaleDownNumOldRevision, diffRes.deleteAvailableLimit)
140141

141142
podsPreparingToDelete := r.choosePodsToDelete(updateCS, diffRes.scaleDownNum, diffRes.scaleDownNumOldRevision, notUpdatedPods, updatedPods)
142143
podsToDelete := make([]*v1.Pod, 0, len(podsPreparingToDelete))
143144
for _, pod := range podsPreparingToDelete {
144-
if !isPodReady(coreControl, pod) {
145+
// Determine pod available, since deleteAvailableLimit is also based on the pod available calculation
146+
if !IsPodAvailable(coreControl, pod, updateCS.Spec.MinReadySeconds) {
145147
podsToDelete = append(podsToDelete, pod)
146-
} else if diffRes.deleteReadyLimit > 0 {
148+
} else if diffRes.deleteAvailableLimit > 0 {
147149
podsToDelete = append(podsToDelete, pod)
148-
diffRes.deleteReadyLimit--
150+
diffRes.deleteAvailableLimit--
149151
}
150152
}
151153

‎pkg/controller/cloneset/sync/cloneset_sync_utils.go

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,9 @@ type expectationDiffs struct {
6363
// scaleUpLimit is the limit number of creating Pods when scaling up
6464
// it is limited by scaleStrategy.maxUnavailable
6565
scaleUpLimit int
66-
// deleteReadyLimit is the limit number of ready Pods that can be deleted
66+
// deleteAvailableLimit is the limit number of ready Pods that can be deleted
6767
// it is limited by UpdateStrategy.maxUnavailable
68-
deleteReadyLimit int
68+
deleteAvailableLimit int
6969

7070
// useSurge is the number that temporarily expect to be above the desired replicas
7171
useSurge int
@@ -253,7 +253,7 @@ func calculateDiffsWithExpectation(cs *appsv1alpha1.CloneSet, pods []*v1.Pod, cu
253253
res.scaleDownNumOldRevision = integer.IntMax(currentTotalOldCount-toDeleteOldRevisionCount-expectedTotalOldCount, 0)
254254
}
255255
if toDeleteNewRevisionCount > 0 || toDeleteOldRevisionCount > 0 || res.scaleDownNum > 0 {
256-
res.deleteReadyLimit = integer.IntMax(maxUnavailable+(len(pods)-replicas)-totalUnavailable, 0)
256+
res.deleteAvailableLimit = integer.IntMax(maxUnavailable+(len(pods)-replicas)-totalUnavailable, 0)
257257
}
258258

259259
// The consistency between scale and update will be guaranteed by syncCloneSet and expectations
@@ -281,10 +281,6 @@ func isSpecifiedDelete(cs *appsv1alpha1.CloneSet, pod *v1.Pod) bool {
281281
return false
282282
}
283283

284-
func isPodReady(coreControl clonesetcore.Control, pod *v1.Pod) bool {
285-
return IsPodAvailable(coreControl, pod, 0)
286-
}
287-
288284
func IsPodAvailable(coreControl clonesetcore.Control, pod *v1.Pod, minReadySeconds int32) bool {
289285
state := lifecycle.GetPodLifecycleState(pod)
290286
if state != "" && state != appspub.LifecycleStateNormal {

‎pkg/controller/cloneset/sync/cloneset_sync_utils_test.go

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
8080
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
8181
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
8282
},
83-
expectResult: expectationDiffs{deleteReadyLimit: 1},
83+
expectResult: expectationDiffs{deleteAvailableLimit: 1},
8484
},
8585
{
8686
name: "specified delete 1 pod (all ready) (step 2/3)",
@@ -115,7 +115,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
115115
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
116116
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
117117
},
118-
expectResult: expectationDiffs{deleteReadyLimit: 1},
118+
expectResult: expectationDiffs{deleteAvailableLimit: 1},
119119
},
120120
{
121121
name: "specified delete 2 pod (all ready) (step 2/6)",
@@ -150,7 +150,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
150150
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
151151
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false), // new creation
152152
},
153-
expectResult: expectationDiffs{deleteReadyLimit: 1},
153+
expectResult: expectationDiffs{deleteAvailableLimit: 1},
154154
},
155155
{
156156
name: "specified delete 2 pod (all ready) (step 5/6)",
@@ -185,7 +185,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
185185
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
186186
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
187187
},
188-
expectResult: expectationDiffs{deleteReadyLimit: 2},
188+
expectResult: expectationDiffs{deleteAvailableLimit: 2},
189189
},
190190
{
191191
name: "specified delete 2 pod and replicas to 4 (step 2/3)",
@@ -316,7 +316,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
316316
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
317317
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
318318
},
319-
expectResult: expectationDiffs{deleteReadyLimit: 1, useSurge: 1},
319+
expectResult: expectationDiffs{deleteAvailableLimit: 1, useSurge: 1},
320320
},
321321
{
322322
name: "specified delete with maxSurge (step 4/4)",
@@ -366,7 +366,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
366366
createTestPod(newRevision, appspub.LifecycleStateUpdating, false, false), // new in-place update
367367
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
368368
},
369-
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 0},
369+
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 0},
370370
},
371371
{
372372
name: "update in-place partition=3 with maxSurge (step 4/4)",
@@ -379,7 +379,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
379379
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false), // new in-place update
380380
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
381381
},
382-
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 1},
382+
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 1},
383383
},
384384
{
385385
name: "update recreate partition=3 with maxSurge (step 1/7)",
@@ -417,7 +417,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
417417
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, true), // begin to recreate
418418
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
419419
},
420-
expectResult: expectationDiffs{useSurge: 1, useSurgeOldRevision: 1, deleteReadyLimit: 1, updateNum: 1, updateMaxUnavailable: 2},
420+
expectResult: expectationDiffs{useSurge: 1, useSurgeOldRevision: 1, deleteAvailableLimit: 1, updateNum: 1, updateMaxUnavailable: 2},
421421
},
422422
{
423423
name: "update recreate partition=3 with maxSurge (step 4/7)",
@@ -442,7 +442,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
442442
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
443443
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation for update
444444
},
445-
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 0},
445+
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 0},
446446
},
447447
{
448448
name: "update recreate partition=3 with maxSurge (step 6/7)",
@@ -455,7 +455,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
455455
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false), // new creation
456456
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation for update
457457
},
458-
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 1},
458+
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 1},
459459
},
460460
{
461461
name: "update recreate partition=3 with maxSurge (step 7/7)",
@@ -492,7 +492,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
492492
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
493493
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
494494
},
495-
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 3},
495+
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 3},
496496
},
497497
{
498498
name: "update recreate partition=99% with maxUnavailable=3, maxSurge=2 (step 3/3)",
@@ -529,7 +529,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
529529
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
530530
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
531531
},
532-
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 2},
532+
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 2},
533533
},
534534
{
535535
name: "update recreate partition=99% with maxUnavailable=40%, maxSurge=30% (step 3/3)",
@@ -566,7 +566,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
566566
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
567567
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
568568
},
569-
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteReadyLimit: 1},
569+
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 1, deleteAvailableLimit: 1},
570570
},
571571
{
572572
name: "update recreate partition=99% with maxUnavailable=30%, maxSurge=30% (step 3/3)",
@@ -656,7 +656,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
656656
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
657657
},
658658
revisionConsistent: true,
659-
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 2, deleteReadyLimit: 2, updateNum: 1, updateMaxUnavailable: 2},
659+
expectResult: expectationDiffs{scaleDownNum: 1, scaleDownNumOldRevision: 2, deleteAvailableLimit: 2, updateNum: 1, updateMaxUnavailable: 2},
660660
},
661661
{
662662
name: "disable rollback feature-gate",
@@ -705,7 +705,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
705705
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
706706
createTestPod(newRevision, appspub.LifecycleStateNormal, true, true),
707707
},
708-
expectResult: expectationDiffs{deleteReadyLimit: 1},
708+
expectResult: expectationDiffs{deleteAvailableLimit: 1},
709709
},
710710
{
711711
name: "[scalingExcludePreparingDelete=false] specific delete a pod with lifecycle hook (step 2/4)",
@@ -745,7 +745,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
745745
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
746746
createTestPod(newRevision, appspub.LifecycleStateNormal, true, true),
747747
},
748-
expectResult: expectationDiffs{deleteReadyLimit: 1},
748+
expectResult: expectationDiffs{deleteAvailableLimit: 1},
749749
},
750750
{
751751
name: "[scalingExcludePreparingDelete=true] specific delete a pod with lifecycle hook (step 2/4)",
@@ -791,7 +791,7 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
791791
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
792792
createTestPod(newRevision, appspub.LifecycleStateNormal, true, true),
793793
},
794-
expectResult: expectationDiffs{deleteReadyLimit: 1},
794+
expectResult: expectationDiffs{deleteAvailableLimit: 1},
795795
},
796796
{
797797
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) {
826826
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false), // it has been changed to normal by managePreparingDelete
827827
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false),
828828
},
829-
expectResult: expectationDiffs{scaleDownNum: 1, deleteReadyLimit: 1},
829+
expectResult: expectationDiffs{scaleDownNum: 1, deleteAvailableLimit: 1},
830830
},
831831
{
832832
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) {
859859
createTestPod(newRevision, appspub.LifecycleStateNormal, true, false),
860860
createTestPod(newRevision, appspub.LifecycleStateNormal, true, true),
861861
},
862-
expectResult: expectationDiffs{deleteReadyLimit: 2},
862+
expectResult: expectationDiffs{deleteAvailableLimit: 2},
863863
},
864864
{
865865
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) {
950950
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
951951
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
952952
},
953-
expectResult: expectationDiffs{scaleDownNum: 2, scaleDownNumOldRevision: 5, deleteReadyLimit: 2, updateNum: 3, updateMaxUnavailable: 2},
953+
expectResult: expectationDiffs{scaleDownNum: 2, scaleDownNumOldRevision: 5, deleteAvailableLimit: 2, updateNum: 3, updateMaxUnavailable: 2},
954954
},
955955
{
956956
name: "[UpdateStrategyPaused=true] create 0 newRevision pods with maxSurge=3,maxUnavailable=0",

0 commit comments

Comments
 (0)