From 1818a9a36a83a88f9c8e9fbe4af837e17053c5b9 Mon Sep 17 00:00:00 2001 From: FillZpp Date: Mon, 30 May 2022 19:16:15 +0800 Subject: [PATCH] A calculation function for StatefulSet maxUnavailable and some tests for it Signed-off-by: FillZpp --- .../statefulset/stateful_set_control.go | 10 +---- .../statefulset/stateful_set_utils.go | 22 ++++++++++- .../statefulset/stateful_set_utils_test.go | 39 ++++++++++++++++++- 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/pkg/controller/statefulset/stateful_set_control.go b/pkg/controller/statefulset/stateful_set_control.go index 5350fc24fd4..d911ca5f0d2 100644 --- a/pkg/controller/statefulset/stateful_set_control.go +++ b/pkg/controller/statefulset/stateful_set_control.go @@ -25,7 +25,6 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilerrors "k8s.io/apimachinery/pkg/util/errors" - intstrutil "k8s.io/apimachinery/pkg/util/intstr" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/tools/record" "k8s.io/klog/v2" @@ -611,18 +610,13 @@ func updateStatefulSetAfterInvariantEstablished( updateMin = int(*set.Spec.UpdateStrategy.RollingUpdate.Partition) // if the feature was enabled and then later disabled, MaxUnavailable may have a value - // other than 1. Ignore the passed in value and Use maxUnavailable as 1 to enforce + // more than 1. Ignore the passed in value and Use maxUnavailable as 1 to enforce // expected behavior when feature gate is not enabled. var err error - maxUnavailable, err = intstrutil.GetValueFromIntOrPercent(intstrutil.ValueOrDefault(set.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable, intstrutil.FromInt(1)), int(replicaCount), false) + maxUnavailable, err = getStatefulSetMaxUnavailable(set.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable, replicaCount) if err != nil { return &status, err } - // maxUnavailable might be zero for small percentage without round up. - // So we have to enforce it not to be less than 1. - if maxUnavailable < 1 { - maxUnavailable = 1 - } } // Collect all targets in the range between the 0 and Spec.Replicas. Count any targets in that range diff --git a/pkg/controller/statefulset/stateful_set_utils.go b/pkg/controller/statefulset/stateful_set_utils.go index 3233a9e5255..3ebfc35f002 100644 --- a/pkg/controller/statefulset/stateful_set_utils.go +++ b/pkg/controller/statefulset/stateful_set_utils.go @@ -24,9 +24,11 @@ import ( "strconv" apps "k8s.io/api/apps/v1" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/intstr" + intstrutil "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/client-go/kubernetes/scheme" "k8s.io/klog/v2" @@ -583,3 +585,21 @@ func (ao ascendingOrdinal) Swap(i, j int) { func (ao ascendingOrdinal) Less(i, j int) bool { return getOrdinal(ao[i]) < getOrdinal(ao[j]) } + +// getStatefulSetMaxUnavailable calculates the real maxUnavailable number according to the replica count +// and maxUnavailable from rollingUpdateStrategy. The number defaults to 1 if the maxUnavailable field is +// not set, and it will be round down to at least 1 if the maxUnavailable value is a percentage. +// Note that API validation has already guaranteed the maxUnavailable field to be >1 if it is an integer +// or 0% < value <= 100% if it is a percentage, so we don't have to consider other cases. +func getStatefulSetMaxUnavailable(maxUnavailable *intstr.IntOrString, replicaCount int) (int, error) { + maxUnavailableNum, err := intstrutil.GetScaledValueFromIntOrPercent(intstrutil.ValueOrDefault(maxUnavailable, intstrutil.FromInt(1)), replicaCount, false) + if err != nil { + return 0, err + } + // maxUnavailable might be zero for small percentage with round down. + // So we have to enforce it not to be less than 1. + if maxUnavailableNum < 1 { + maxUnavailableNum = 1 + } + return maxUnavailableNum, nil +} diff --git a/pkg/controller/statefulset/stateful_set_utils_test.go b/pkg/controller/statefulset/stateful_set_utils_test.go index 0193ba878c6..969bcb8e52e 100644 --- a/pkg/controller/statefulset/stateful_set_utils_test.go +++ b/pkg/controller/statefulset/stateful_set_utils_test.go @@ -30,9 +30,10 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" apps "k8s.io/api/apps/v1" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" podutil "k8s.io/kubernetes/pkg/api/v1/pod" "k8s.io/kubernetes/pkg/controller/history" ) @@ -937,3 +938,39 @@ func newStatefulSetWithLabels(replicas int, name string, uid types.UID, labels m }, } } + +func TestGetStatefulSetMaxUnavailable(t *testing.T) { + testCases := []struct { + maxUnavailable *intstr.IntOrString + replicaCount int + expectedMaxUnavailable int + }{ + // it wouldn't hurt to also test 0 and 0%, even if they should have been forbidden by API validation. + {maxUnavailable: nil, replicaCount: 10, expectedMaxUnavailable: 1}, + {maxUnavailable: intOrStrP(intstr.FromInt(3)), replicaCount: 10, expectedMaxUnavailable: 3}, + {maxUnavailable: intOrStrP(intstr.FromInt(3)), replicaCount: 0, expectedMaxUnavailable: 3}, + {maxUnavailable: intOrStrP(intstr.FromInt(0)), replicaCount: 0, expectedMaxUnavailable: 1}, + {maxUnavailable: intOrStrP(intstr.FromString("10%")), replicaCount: 25, expectedMaxUnavailable: 2}, + {maxUnavailable: intOrStrP(intstr.FromString("100%")), replicaCount: 5, expectedMaxUnavailable: 5}, + {maxUnavailable: intOrStrP(intstr.FromString("50%")), replicaCount: 5, expectedMaxUnavailable: 2}, + {maxUnavailable: intOrStrP(intstr.FromString("10%")), replicaCount: 5, expectedMaxUnavailable: 1}, + {maxUnavailable: intOrStrP(intstr.FromString("1%")), replicaCount: 0, expectedMaxUnavailable: 1}, + {maxUnavailable: intOrStrP(intstr.FromString("0%")), replicaCount: 0, expectedMaxUnavailable: 1}, + } + + for i, tc := range testCases { + t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) { + gotMaxUnavailable, err := getStatefulSetMaxUnavailable(tc.maxUnavailable, tc.replicaCount) + if err != nil { + t.Fatal(err) + } + if gotMaxUnavailable != tc.expectedMaxUnavailable { + t.Errorf("Expected maxUnavailable %v, got pods %v", tc.expectedMaxUnavailable, gotMaxUnavailable) + } + }) + } +} + +func intOrStrP(v intstr.IntOrString) *intstr.IntOrString { + return &v +}