API: maxUnavailable for StatefulSet #82162

krmayankk · 2019-08-30T07:12:11Z

Implements https://github.com/kubernetes/enhancements/pull/1010/files

MaxUnavailable for StatefulSets, allows faster RollingUpdate by taking down more than 1 pod at a time. The number of pods you want to take down during a RollingUpdate is configurable using maxUnavailable parameter.

fejta-bot · 2019-08-30T07:57:24Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

krmayankk · 2019-08-30T22:52:22Z

/assign @lavalamp

lavalamp · 2019-09-03T16:38:45Z

staging/src/k8s.io/api/apps/v1beta2/types.go

+	// The maximum number of pods that can be unavailable during the update.
+	// Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%).
+	// Absolute number is calculated from percentage by rounding down.
+	// Defaults to 1.


May it be zero?

added a comment, it cannot be zero @lavalamp

staging/src/k8s.io/api/apps/v1beta2/types.go

lavalamp · 2019-09-03T16:40:53Z

Needs validation code.

krmayankk · 2019-09-19T19:24:08Z

Needs validation code.

added validation code

lavalamp · 2019-09-19T23:59:38Z

pkg/apis/apps/validation/validation.go

+				spec.UpdateStrategy.RollingUpdate.MaxUnavailable != nil &&
+				spec.UpdateStrategy.RollingUpdate.MaxUnavailable.IntValue() > 1 {
+				allErrs = append(allErrs, field.Invalid(fldPath.Child("updateStrategy").Child("rollingUpdate").Child("maxUnavailable"), spec.UpdateStrategy.RollingUpdate.MaxUnavailable,
+					fmt.Sprintf("with value greater than 1 only allowed for podManagementPolicy '%s'", apps.ParallelPodManagement)))


Should also prevent using a percentage in this case, no?

+1, any reason not to prevent setting it at all with ordered policy?

this was very early API days when we were debating this for whether to implement for OrderedReady or not. Finally we decided to add for both, unit tests cover both scenarios. PTAL

lavalamp · 2019-09-20T00:01:18Z

API looks OK now. Feature gate?

I guess all of the implementation goes in the controller?

krmayankk · 2019-09-26T17:39:18Z

@lavalamp yes all of the implementation goes into the controller. Can we merge the api with feature gate first and then do the implementation since its alpha and always off ?(just makes it easier to do the changes)

lavalamp · 2019-09-26T17:46:49Z

IIRC I'm OK with this if it comes with a feature gate.

…

On Thu, Sep 26, 2019 at 10:41 AM krmayankk ***@***.***> wrote: @lavalamp <https://github.com/lavalamp> yes all of the implementation goes into the controller. Can we merge the api with feature gate first and then do the implementation since its alpha and always off ?(just makes it easier to do the changes) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#82162?email_source=notifications&email_token=AAE6BFQIXMLCQCVMS63FDADQLTX5LA5CNFSM4ISKBZCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7WMNUA#issuecomment-535611088>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE6BFQ2BMEKUKUTU4UUKR3QLTX5LANCNFSM4ISKBZCA> .

lavalamp · 2022-03-25T22:29:34Z

pkg/apis/apps/v1beta2/defaults_test.go

 func TestSetDefaultStatefulSet(t *testing.T) {
 	defaultLabels := map[string]string{"foo": "bar"}
+	defaultMaxUnavailable := intstr.FromInt(1)
+	notTheDefaultMaxUnavailable := intstr.FromInt(3)


same -- please remove

lavalamp · 2022-03-25T22:30:51Z

pkg/apis/apps/validation/validation_test.go

+					Type: apps.RollingUpdateStatefulSetStrategyType,
+					RollingUpdate: &apps.RollingUpdateStatefulSetStrategy{
+						Partition: 2,
+						MaxUnavailable: func() *intstr.IntOrString {


replace with a call to the function you defined.

lavalamp · 2022-03-25T22:33:04Z

pkg/controller/statefulset/stateful_set_control.go

+	updateMin := 0
+	maxUnavailable := 1
+	if set.Spec.UpdateStrategy.RollingUpdate != nil {
+		updateMin = int(*set.Spec.UpdateStrategy.RollingUpdate.Partition)


Partition could be nil?

Maybe I don't understand how this is supposed to work but it seems to me that this doesn't work if this is a non-zero value?

this is all existing old code
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/statefulset/stateful_set_control.go#L578-L583

partition is defaulted to 0 always and its not under any feature flag, and hence that would be reason its not nil

if partition is non zero, the pods are deleted starting from highest ordinal to the ordinal equal to the partition number, all pods less than partition are left untouched

eg. maxUnavailable =2, total replicas 6 partition=3, so the pods deleted are 5,4(because maxUnavailable) first and eventually 3 also. 2, 1 and 0 remain untouched

In that case you need to decide if maxUnavailable is per-partition or total, state that in the documentation, and probably fix the logic below.

maxUnavailable is total , we dont want to proceed with any update, if the total count of unavailable pods within 0 to replicas is greater than or equal to maxUnavailable

lavalamp · 2022-03-25T22:49:42Z

pkg/controller/statefulset/stateful_set_control.go

+	// to their ordinal for termination. Delete those pods and count the successful deletions. Update the status with the correct
+	// number of deletions.
+	unavailablePods := 0
+	for target := len(replicas) - 1; target >= updateMin; target-- {


If updateMin is not zero, this could miss counting some unhealthy pods?

the invariant has been established till this point(https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/statefulset/stateful_set_control.go#L497-L501), that all pods are running and ready before we attempt any deletion. See https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/statefulset/stateful_set_control.go#L444 and https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/statefulset/stateful_set_control.go#L452
and https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/statefulset/stateful_set_control.go#L497-L501 so technically we could leave this check as target >=updateMin since we from nowards are only deleting pods from replicas to updateMin(partition) in the next loop. For extra security, i will make it 0. @lavalamp

lavalamp · 2022-03-25T22:52:43Z

staging/src/k8s.io/api/apps/v1/types.go

+	// Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%).
+	// Absolute number is calculated from percentage by rounding up. This can not be 0.
+	// Defaults to 1. This field is alpha-level and is only honored by servers that enable the
+	// MaxUnavailableStatefulSet feature.


This comment should explain how this field interacts with the Partition field.

...The partition field comment is also not helpful and could be expanded if you know what it is supposed to mean.

it does not . maxUnavailable only determines at rolling update time, how many can we make go down, and partition limits the ordinals that can go down to the range [ replicas: partition]

i will fix these comments, partition one is completely useless, i just noticed :-)

lavalamp · 2022-03-25T23:08:59Z

api/openapi-spec/swagger.json

@@ -11487,6 +11491,555 @@
      ],
      "type": "object"
    },
+    "io.k8s.api.flowcontrol.v1alpha1.FlowDistinguisherMethod": {


You must need a rebase or something, your PR is not adding this field.

weird i am not sure what is causing this to be generated. i removed this file and ran code generation again

lavalamp · 2022-03-25T23:09:56Z

api/openapi-spec/v3/apis__flowcontrol.apiserver.k8s.io__v1alpha1_openapi.json

@@ -0,0 +1,4093 @@
+{


Your PR should not be adding this file.

lavalamp · 2022-03-25T23:34:26Z

pkg/controller/statefulset/stateful_set_control_test.go

+		}
+
+		if tc == apps.OrderedReadyPodManagement {
+			// only one pod gets created at a time due to OrderedReady


This "loop with huge nested if" is not a good way to do a table driven test BTW. Better to extract logic to e.g. functions which are in the test cases and can just be run. I'm not sure if I'm asking for a change or not because I'm still trying to understand the test.

in my original PR, these were two different tests , one for OrderedReadyPodManagement and one for ParallelPodManagement. Since there was lot of duplication between two, i changed it to single test

k8s-ci-robot · 2022-03-28T17:11:42Z

@krmayankk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-device-plugin-gpu	739289444d5fa5f92d111759083d1479e388117f	link	`/test pull-kubernetes-e2e-gce-device-plugin-gpu`
pull-kubernetes-bazel-test	32e82c5ca9c79f3d87bfea1e158f667a48d68489	link	`/test pull-kubernetes-bazel-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

lavalamp · 2022-03-28T18:22:46Z

staging/src/k8s.io/api/apps/v1/types.go

+	// Defaults to 1. This field is alpha-level and is only honored by servers that enable the
+	// MaxUnavailableStatefulSet feature. The field applies to all pods in the range 0 to
+	// Replicas-1. That means if there is any unavailable pod in the range 0 to Replicas-1, it
+	// will be counted towards MaxUnavailable.


Both these comments are huge improvements, thanks.

lavalamp · 2022-03-28T18:30:13Z

/approve

For API.

I can't vouch for the controller tests, you'll need to get an lgtm from someone who can.

With the code split I'm no longer worried about breaking regular non-alpha users.

k8s-ci-robot · 2022-03-28T18:31:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kow3ns, krmayankk, lavalamp, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [lavalamp]
~~pkg/apis/OWNERS~~ [lavalamp]
~~pkg/controller/statefulset/OWNERS~~ [kow3ns,lavalamp,soltysh]
~~pkg/features/OWNERS~~ [kow3ns,lavalamp,soltysh]
~~pkg/generated/openapi/OWNERS~~ [lavalamp]
~~pkg/registry/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/api/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ravisantoshgudimetla · 2022-03-28T23:14:29Z

/lgtm

from controller standpoint. We can improve the tests in a separate PR later.

k8s-ci-robot requested review from david-mcmahon and m1093782566 August 30, 2019 07:15

k8s-ci-robot assigned lavalamp Aug 30, 2019

krmayankk force-pushed the maxun branch from 958617e to 7a49f3c Compare August 30, 2019 22:54

lavalamp reviewed Sep 3, 2019

View reviewed changes

staging/src/k8s.io/api/apps/v1beta2/types.go Show resolved Hide resolved

krmayankk force-pushed the maxun branch from 7a49f3c to e63371a Compare September 16, 2019 05:57

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 16, 2019

lavalamp reviewed Sep 19, 2019

View reviewed changes

krmayankk mentioned this pull request Oct 3, 2019

maxUnavailable for StatefulSets kubernetes/enhancements#961

Open

8 tasks

lavalamp reviewed Mar 25, 2022

View reviewed changes

krmayankk force-pushed the maxun branch from 98dfac0 to 03d6f30 Compare March 25, 2022 22:57

lavalamp reviewed Mar 25, 2022

View reviewed changes

krmayankk force-pushed the maxun branch 2 times, most recently from b51a150 to e957da8 Compare March 26, 2022 04:00

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2022

krmayankk force-pushed the maxun branch from e957da8 to f22c193 Compare March 27, 2022 03:06

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2022

krmayankk force-pushed the maxun branch from f22c193 to c0eb04d Compare March 28, 2022 16:18

Mayank Kumar added 2 commits March 28, 2022 11:01

API: maxUnavailable for StatefulSet

357203d

add maxUnavailable implementation and UT

2733b66

krmayankk force-pushed the maxun branch from c0eb04d to 2733b66 Compare March 28, 2022 18:01

lavalamp reviewed Mar 28, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2022

k8s-ci-robot merged commit f85ff4b into kubernetes:master Mar 29, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 29, 2022

tengqm mentioned this pull request Mar 29, 2022

Document the MaxUnavailableStatefulSet feature kubernetes/website#32596

Merged

krmayankk deleted the maxun branch March 30, 2022 08:34

github-actions bot mentioned this pull request Apr 5, 2022

Week Ending April 3, 2022 dev-obs/actus#414

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: maxUnavailable for StatefulSet #82162

API: maxUnavailable for StatefulSet #82162

krmayankk commented Aug 30, 2019 •

edited

fejta-bot commented Aug 30, 2019

krmayankk commented Aug 30, 2019

lavalamp Sep 3, 2019

krmayankk Sep 19, 2019

lavalamp commented Sep 3, 2019

krmayankk commented Sep 19, 2019

lavalamp Sep 19, 2019

tnozicka Oct 19, 2020

krmayankk Nov 14, 2020

lavalamp commented Sep 20, 2019

krmayankk commented Sep 26, 2019

lavalamp commented Sep 26, 2019 via email

lavalamp Mar 25, 2022

lavalamp Mar 25, 2022

lavalamp Mar 25, 2022

lavalamp Mar 25, 2022

krmayankk Mar 25, 2022 •

edited

lavalamp Mar 25, 2022

krmayankk Mar 27, 2022

lavalamp Mar 25, 2022

krmayankk Mar 27, 2022

lavalamp Mar 25, 2022

krmayankk Mar 26, 2022

krmayankk Mar 27, 2022

lavalamp Mar 25, 2022

krmayankk Mar 26, 2022

lavalamp Mar 25, 2022

lavalamp Mar 25, 2022

krmayankk Mar 26, 2022 •

edited

k8s-ci-robot commented Mar 28, 2022 •

edited

lavalamp Mar 28, 2022

lavalamp commented Mar 28, 2022

k8s-ci-robot commented Mar 28, 2022

ravisantoshgudimetla commented Mar 28, 2022 •

edited

API: maxUnavailable for StatefulSet #82162

API: maxUnavailable for StatefulSet #82162

Conversation

krmayankk commented Aug 30, 2019 • edited

fejta-bot commented Aug 30, 2019

krmayankk commented Aug 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Sep 3, 2019

krmayankk commented Sep 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Sep 20, 2019

krmayankk commented Sep 26, 2019

lavalamp commented Sep 26, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmayankk Mar 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmayankk Mar 26, 2022 • edited

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 28, 2022 • edited

Choose a reason for hiding this comment

lavalamp commented Mar 28, 2022

k8s-ci-robot commented Mar 28, 2022

ravisantoshgudimetla commented Mar 28, 2022 • edited

krmayankk commented Aug 30, 2019 •

edited

krmayankk Mar 25, 2022 •

edited

krmayankk Mar 26, 2022 •

edited

k8s-ci-robot commented Mar 28, 2022 •

edited

ravisantoshgudimetla commented Mar 28, 2022 •

edited