From 26be87ae0de98256cedf25d110b879e3614500b6 Mon Sep 17 00:00:00 2001 From: Abner-1 Date: Wed, 6 Nov 2024 14:18:42 +0800 Subject: [PATCH] define partition as number of non-updated pods should be reversed Signed-off-by: Abner-1 --- apis/apps/v1beta1/statefulset_types.go | 4 +--- config/crd/bases/apps.kruise.io_statefulsets.yaml | 4 +--- .../bases/apps.kruise.io_uniteddeployments.yaml | 4 +--- pkg/controller/statefulset/stateful_set_utils.go | 11 ++++++++++- .../statefulset/stateful_set_utils_test.go | 15 +++++++++------ .../statefulset/stateful_update_utils.go | 11 ++++++----- .../statefulset/stateful_update_utils_test.go | 3 ++- 7 files changed, 30 insertions(+), 22 deletions(-) diff --git a/apis/apps/v1beta1/statefulset_types.go b/apis/apps/v1beta1/statefulset_types.go index 15befe58af..32d408b00d 100644 --- a/apis/apps/v1beta1/statefulset_types.go +++ b/apis/apps/v1beta1/statefulset_types.go @@ -87,9 +87,7 @@ type VolumeClaimUpdateStrategy struct { // RollingUpdateStatefulSetStrategy is used to communicate parameter for RollingUpdateStatefulSetStrategyType. type RollingUpdateStatefulSetStrategy struct { - // Partition indicates the ordinal at which the StatefulSet should be partitioned by default. - // But if unorderedUpdate has been set: - // - Partition indicates the number of pods with non-updated revisions when rolling update. + // Partition indicates the number of pods the StatefulSet should be partitioned by default. // - It means controller will update $(replicas - partition) number of pod. // Default value is 0. // +optional diff --git a/config/crd/bases/apps.kruise.io_statefulsets.yaml b/config/crd/bases/apps.kruise.io_statefulsets.yaml index 67adfc8c4b..8794b66135 100644 --- a/config/crd/bases/apps.kruise.io_statefulsets.yaml +++ b/config/crd/bases/apps.kruise.io_statefulsets.yaml @@ -784,9 +784,7 @@ spec: type: integer partition: description: |- - Partition indicates the ordinal at which the StatefulSet should be partitioned by default. - But if unorderedUpdate has been set: - - Partition indicates the number of pods with non-updated revisions when rolling update. + Partition indicates the number of pods the StatefulSet should be partitioned by default. - It means controller will update $(replicas - partition) number of pod. Default value is 0. format: int32 diff --git a/config/crd/bases/apps.kruise.io_uniteddeployments.yaml b/config/crd/bases/apps.kruise.io_uniteddeployments.yaml index 3595596280..e03c3c0474 100644 --- a/config/crd/bases/apps.kruise.io_uniteddeployments.yaml +++ b/config/crd/bases/apps.kruise.io_uniteddeployments.yaml @@ -407,9 +407,7 @@ spec: type: integer partition: description: |- - Partition indicates the ordinal at which the StatefulSet should be partitioned by default. - But if unorderedUpdate has been set: - - Partition indicates the number of pods with non-updated revisions when rolling update. + Partition indicates the number of pods the StatefulSet should be partitioned by default. - It means controller will update $(replicas - partition) number of pod. Default value is 0. format: int32 diff --git a/pkg/controller/statefulset/stateful_set_utils.go b/pkg/controller/statefulset/stateful_set_utils.go index 2d21d13ccf..25b405f3ae 100644 --- a/pkg/controller/statefulset/stateful_set_utils.go +++ b/pkg/controller/statefulset/stateful_set_utils.go @@ -514,7 +514,16 @@ func isCurrentRevisionNeeded(set *appsv1beta1.StatefulSet, updateRevision string return ordinal < getStartOrdinal(set)+int(set.Status.CurrentReplicas) } if set.Spec.UpdateStrategy.RollingUpdate.UnorderedUpdate == nil { - return ordinal < getStartOrdinal(set)+int(*set.Spec.UpdateStrategy.RollingUpdate.Partition) + unreservedPodsNum := 0 + // assume all pods [0, idx) are created and only reserved pods are nil + idx := ordinal - getStartOrdinal(set) + for i := 0; i < idx; i++ { + if replicas[i] != nil { + unreservedPodsNum++ + } + } + // if all pods [0, idx] are current revision + return unreservedPodsNum+1 <= int(*set.Spec.UpdateStrategy.RollingUpdate.Partition) } var noUpdatedReplicas int diff --git a/pkg/controller/statefulset/stateful_set_utils_test.go b/pkg/controller/statefulset/stateful_set_utils_test.go index 346cb981c3..41ac1dddf4 100644 --- a/pkg/controller/statefulset/stateful_set_utils_test.go +++ b/pkg/controller/statefulset/stateful_set_utils_test.go @@ -1250,7 +1250,8 @@ func TestIsCurrentRevisionNeeded(t *testing.T) { updateRevision: updatedRevisionHash, ordinal: 0, replicas: func() []*corev1.Pod { - pods := newReplicas(2, 2, currentRevisionHash) + pods := []*corev1.Pod{nil, nil} + pods = append(pods, newReplicas(2, 2, currentRevisionHash)...) return pods }(), expectedRes: true, @@ -1701,12 +1702,12 @@ func TestIsCurrentRevisionNeeded(t *testing.T) { }, // with startOrdinal and reservedIds - // TODO Abner-1: These cases will be discussed and may be changed + // fixes https://github.com/openkruise/kruise/issues/1813 { // reservedId 1, replicas 3, partition 2 // 3: updated revision // 0: current revision - // => 2: should be updated revision + // => 2: should be current revision name: "Ordinals start 0, reservedId 1, partition 1, create pod2", statefulSet: &appsv1beta1.StatefulSet{ Spec: appsv1beta1.StatefulSetSpec{ @@ -1731,16 +1732,17 @@ func TestIsCurrentRevisionNeeded(t *testing.T) { ordinal: 2, replicas: func() []*corev1.Pod { pods := newReplicas(0, 1, currentRevisionHash) + pods = append(pods, nil, nil) pods = append(pods, newReplicas(3, 1, updatedRevisionHash)...) return pods }(), - expectedRes: false, + expectedRes: true, }, { // start ordinal 2, reservedId 3, replicas 3, partition 2 // 5: updated revision // 2: current revision - // => 4: should be updated revision + // => 4: should be current revision name: "Ordinals start 2, reservedId 1, partition 1, create pod2", statefulSet: &appsv1beta1.StatefulSet{ Spec: appsv1beta1.StatefulSetSpec{ @@ -1765,10 +1767,11 @@ func TestIsCurrentRevisionNeeded(t *testing.T) { ordinal: 4, replicas: func() []*corev1.Pod { pods := newReplicas(2, 1, currentRevisionHash) + pods = append(pods, nil, nil) pods = append(pods, newReplicas(5, 1, updatedRevisionHash)...) return pods }(), - expectedRes: false, + expectedRes: true, }, } diff --git a/pkg/controller/statefulset/stateful_update_utils.go b/pkg/controller/statefulset/stateful_update_utils.go index 20fb2b3448..c25ef77e8f 100644 --- a/pkg/controller/statefulset/stateful_update_utils.go +++ b/pkg/controller/statefulset/stateful_update_utils.go @@ -30,9 +30,14 @@ func sortPodsToUpdate(rollingUpdateStrategy *appsv1beta1.RollingUpdateStatefulSe updateMin = int(*rollingUpdateStrategy.Partition) } + maxUpdate := int(totalReplicas) - updateMin + if maxUpdate <= 0 { + return []int{} + } + if rollingUpdateStrategy == nil || rollingUpdateStrategy.UnorderedUpdate == nil { var indexes []int - for target := len(replicas) - 1; target >= updateMin; target-- { + for target := len(replicas) - 1; target >= updateMin && len(indexes) < maxUpdate; target-- { if replicas[target] == nil { continue } @@ -42,10 +47,6 @@ func sortPodsToUpdate(rollingUpdateStrategy *appsv1beta1.RollingUpdateStatefulSe } priorityStrategy := rollingUpdateStrategy.UnorderedUpdate.PriorityStrategy - maxUpdate := int(totalReplicas) - updateMin - if maxUpdate <= 0 { - return []int{} - } var updatedIdxs []int var waitUpdateIdxs []int diff --git a/pkg/controller/statefulset/stateful_update_utils_test.go b/pkg/controller/statefulset/stateful_update_utils_test.go index 9ef953827f..3a2d2df81b 100644 --- a/pkg/controller/statefulset/stateful_update_utils_test.go +++ b/pkg/controller/statefulset/stateful_update_utils_test.go @@ -87,6 +87,7 @@ func TestSortPodsToUpdate(t *testing.T) { expected: []int{2, 1, 0}, }, { + // change the case because we define partition as number of pods with non-updated revision strategy: &appsv1beta1.RollingUpdateStatefulSetStrategy{Partition: func() *int32 { var i int32 = 2; return &i }()}, updateRevision: "r1", totalReplicas: 3, @@ -97,7 +98,7 @@ func TestSortPodsToUpdate(t *testing.T) { nil, {ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{apps.ControllerRevisionHashLabelKey: "r0"}}}, }, - expected: []int{4, 2}, + expected: []int{4}, }, { strategy: &appsv1beta1.RollingUpdateStatefulSetStrategy{