KEP-2599: Promote STS minReadySeconds to GA #3354

ravisantoshgudimetla · 2022-06-07T22:14:18Z

One-line PR description:
Promote STS minReadySeconds to GA

Issue link:
Add minReadySeconds to Statefulsets #2599

Other comments:

soltysh

@ravisantoshgudimetla a couple of nits

keps/sig-apps/2599-minreadyseconds-for-statefulsets/README.md

soltysh · 2022-06-08T18:21:39Z

keps/sig-apps/2599-minreadyseconds-for-statefulsets/README.md

+
+Following e2e tests are added to statefulsets.
+- minReadySeconds when present should be honored
+- AvailableReplicas should get updated accordingly when minReadySeconds is set


IIUC those tests already exists, also from template:

For Beta and GA, add links to added tests together with links to k8s-triage for those tests: https://storage.googleapis.com/k8s-triage/index.html

soltysh · 2022-06-08T18:22:13Z

keps/sig-apps/2599-minreadyseconds-for-statefulsets/README.md

@@ -273,6 +306,8 @@ API validation, behavioral change of StatefulSet with feature gate enabled and d

 #### Beta -> GA Graduation
 - 2 examples of end users using this field
+  - The latest version of OpenShift is using this field for some control plane components


Link with pointers, is possible?

keps/sig-apps/2599-minreadyseconds-for-statefulsets/README.md

soltysh · 2022-06-08T18:23:09Z

keps/sig-apps/2599-minreadyseconds-for-statefulsets/kep.yaml

@@ -7,7 +7,7 @@ participating-sigs:
  - "sig-apps"
 status: implementable
 creation-date: 2021-04-07
-last-updated: 2021-04-12
+last-updated: 2022-06-07


add @deads2k to prr-approvers below

wojtek-t · 2022-06-09T05:29:39Z

keps/prod-readiness/sig-apps/2599.yaml

-  approver: "@ehashman"  
+  approver: "@ehashman"
+stable:
+  approver: "@deads2k"


David already has quite a lot PRRs this cycle - please switch to me and I will take a look soon.

wojtek-t

Couple clarifying comments from me.

keps/sig-apps/2599-minreadyseconds-for-statefulsets/README.md

wojtek-t · 2022-06-09T09:13:33Z

keps/sig-apps/2599-minreadyseconds-for-statefulsets/README.md

+Following e2e tests are added to statefulsets.
+- StatefulSet MinReadySeconds should be honored when enabled: [test grid](https://testgrid.k8s.io/sig-apps#gce)
+- StatefulSet AvailableReplicas should get updated accordingly when MinReadySeconds is enabled: [test grid](https://testgrid.k8s.io/sig-apps#gce)
+


Comments for lines that I can't comment on:

L356: "we will set minReadySeconds to 0" - who exactly is "we"? Do we really set it?
Or do we implicitly default it on the controller side?

L357: "The default value of AvailableReplicas would be value of existing field in Status sub-resource called ReadyReplicas" - do we really default that field? This seems wrong.
It's just controller should start setting this field (together with other status fields).

L362: kube-apiserver will not clear automatically the field; it may be cleared if someone overwrites the object (not using patch). What happens in practice?

L431: please link?
[also - are they testing enablement/disablement (so switching on/off the feature in the middle of the test) or rather how it works with feature enabled and separately disabled]

L356: "we will set minReadySeconds to 0" - who exactly is "we"? Do we really set it?

minReadySeconds is an int32 whose default value is 0. We don't explicitly set it in defaults.go, if that is what you're asking

"The default value of AvailableReplicas would be value of existing field in Status sub-resource called ReadyReplicas" - do we really default that field? This seems wrong.

I think by default you mean the value which will be present in defaults.go. No, we don't set the value there.
If minReadySeconds is 0, the default behavior of the controller is to set all the ready replicas as available replicas.

L362: kube-apiserver will not clear automatically the field; it may be cleared if someone overwrites the object (not using patch). What happens in practice?

The values continue to exist but won't be honored by the controller. I updated the statement to reflect it.

[also - are they testing enablement/disablement (so switching on/off the feature in the middle of the test) or rather how it works with feature enabled and separately disabled]

Not in the middle of tests but run test with feature enabled and then run the same test when the feature is disabled. Added the same in test plan section. Do you want anything else to be added?

I think by default you mean the value which will be present in defaults.go. No, we don't set the value there.
If minReadySeconds is 0, the default behavior of the controller is to set all the ready replicas as available replicas.

Please adjust then to say that "it's the controller that sets this field to the same value as ReadyReplicas.
This is different then defaulting (e.g. it's asynchronous).

Not in the middle of tests but run test with feature enabled and then run the same test when the feature is disabled. Added the same in test plan section. Do you want anything else to be added?

Which is not really the enablement/disablement - we're testing the feature on and off.
It's a bit too late to ask for it, so I won't ask for adding such tests now, but please clarify in the doc that you're not testing enablement/disablement, but rather the feature being on and off.

Please adjust then to say that "it's the controller that sets this field to the same value as ReadyReplicas.

I did that in the last commit.

It's a bit too late to ask for it, so I won't ask for adding such tests now, but please clarify in the doc that you're not testing enablement/disablement, but rather the feature being on and off.

Sure. I updated the wording. If you have an example of how to enable/disable, I can try fitting it into this release. I was following the doc at https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#new-field-in-existing-api-version

soltysh

2 minor nits, but overall
/lgtm
/approve

soltysh · 2022-06-13T10:59:50Z

keps/sig-apps/2599-minreadyseconds-for-statefulsets/README.md

  - If `minReadySeconds` is equal to 0 -- in this case user wont see any 
-    difference in behavior
+    difference in behavior. The `AvailableReplicas` will be set to `ReadyReplicas`


Nit 2 lines above:
s/in this case user wont see any/in this case user won't see any/g

soltysh · 2022-06-13T11:00:14Z

keps/sig-apps/2599-minreadyseconds-for-statefulsets/kep.yaml

 reviewers:
  - "@soltysh"
 approvers:
  - "@soltysh"
 prr-approvers:
  - "@ehashman"
+  - "@deads2k"


wojtek-t per above change

I don't care about it - it's a duplicate with the file in prod-readiness dir.
I'm planning to get to it at some point and remove this field from everywhere..

wojtek-t · 2022-06-13T13:23:18Z

/lgtm
/approve PRR

/hold - to address the comment from @soltysh

soltysh

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2022-06-13T13:49:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ravisantoshgudimetla, soltysh, wojtek-t

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:

~~keps/prod-readiness/OWNERS~~ [wojtek-t]
~~keps/sig-apps/OWNERS~~ [soltysh]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 7, 2022

k8s-ci-robot requested a review from kow3ns June 7, 2022 22:14

k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jun 7, 2022

k8s-ci-robot requested a review from soltysh June 7, 2022 22:14

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Jun 7, 2022

ravisantoshgudimetla changed the title ~~Promote STS minReadySeconds to GA~~ KEP-2599: Promote STS minReadySeconds to GA Jun 7, 2022

wojtek-t assigned deads2k Jun 8, 2022

soltysh reviewed Jun 8, 2022

View reviewed changes

ravisantoshgudimetla force-pushed the promote-minReadySec-sts-ga branch from ab4f9ae to d7ccec4 Compare June 8, 2022 22:43

wojtek-t reviewed Jun 9, 2022

View reviewed changes

wojtek-t self-assigned this Jun 9, 2022

wojtek-t reviewed Jun 9, 2022

View reviewed changes

marosset mentioned this pull request Jun 9, 2022

Add minReadySeconds to Statefulsets #2599

Closed

12 tasks

ravisantoshgudimetla force-pushed the promote-minReadySec-sts-ga branch from d7ccec4 to b9c992a Compare June 9, 2022 20:12

soltysh approved these changes Jun 13, 2022

View reviewed changes

k8s-ci-robot assigned soltysh Jun 13, 2022

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

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 13, 2022

Promote STS minReadySeconds to GA

5bcb2a9

ravisantoshgudimetla force-pushed the promote-minReadySec-sts-ga branch from 4e91610 to 5bcb2a9 Compare June 13, 2022 13:32

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2022

soltysh approved these changes Jun 13, 2022

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 13, 2022

k8s-ci-robot merged commit 5cde0fc into kubernetes:master Jun 13, 2022

k8s-ci-robot added this to the v1.25 milestone Jun 13, 2022

ravisantoshgudimetla mentioned this pull request Jul 1, 2022

Promote minReadySeconds to GA kubernetes/kubernetes#110896

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2599: Promote STS minReadySeconds to GA #3354

KEP-2599: Promote STS minReadySeconds to GA #3354

ravisantoshgudimetla commented Jun 7, 2022

soltysh left a comment

soltysh Jun 8, 2022

soltysh Jun 8, 2022

soltysh Jun 8, 2022

wojtek-t Jun 9, 2022

wojtek-t left a comment

wojtek-t Jun 9, 2022

ravisantoshgudimetla Jun 9, 2022

wojtek-t Jun 10, 2022

ravisantoshgudimetla Jun 10, 2022

soltysh left a comment

soltysh Jun 13, 2022

soltysh Jun 13, 2022

wojtek-t Jun 13, 2022

wojtek-t commented Jun 13, 2022

soltysh left a comment

k8s-ci-robot commented Jun 13, 2022

KEP-2599: Promote STS minReadySeconds to GA #3354

KEP-2599: Promote STS minReadySeconds to GA #3354

Conversation

ravisantoshgudimetla commented Jun 7, 2022

soltysh left a 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

wojtek-t left a 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

soltysh left a 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

wojtek-t commented Jun 13, 2022

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 13, 2022