Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Promote StatefulSet MinReadySeconds to GA #35539

Merged
merged 1 commit into from
Aug 14, 2022
Merged

Promote StatefulSet MinReadySeconds to GA #35539

merged 1 commit into from
Aug 14, 2022

Conversation

atiratree
Copy link
Member

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jul 29, 2022
@atiratree
Copy link
Member Author

@ravisantoshgudimetla @soltysh

@atiratree atiratree mentioned this pull request Jul 29, 2022
Closed
12 tasks
@netlify
Copy link

netlify bot commented Jul 29, 2022 •
edited
Loading

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 3556301
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/62e7f0e30897560009b95fa1
😎 Deploy Preview https://deploy-preview-35539--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@divya-mohan0209 divya-mohan0209 added this to the 1.25 milestone Jul 29, 2022
divya-mohan0209
divya-mohan0209 reviewed Jul 29, 2022
View reviewed changes
Copy link
Contributor

@divya-mohan0209 divya-mohan0209 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2022
@tengqm
Copy link
Contributor

tengqm commented Jul 29, 2022

This should target 'dev-1.25' branch rather than 'main'.

@divya-mohan0209
Copy link
Contributor

Oh yes, my bad. Thanks for the check, @tengqm ! :)
/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2022
sftim
sftim reviewed Jul 29, 2022
View reviewed changes
content/en/docs/concepts/workloads/controllers/statefulset.md
available as soon as it is ready). To learn more about when a Pod is considered ready, see
[Container Probes](/docs/concepts/workloads/pods/pod-lifecycle/#container-probes).
This defaults to 0 (the Pod will be considered available as soon as it is ready). To learn more about when
a Pod is considered ready, see [Container Probes](/docs/concepts/workloads/pods/pod-lifecycle/#container-probes).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make it clear that minReadySeconds is really there to phase the rollout of new or updated StatefulSets (whereas probe thresholds are the right thing to use if you want to, eg, see that the Pod has had a healthcheck every 2 seconds and that there have been 30 successful ones, which implies that the Pod is ready for traffic).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, but we also say that here https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#rolling-updates.

Please let me know if you are satisfied with the current state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to repeat that detail under the Minimum Ready Seconds heading, in case people didn't arrive via https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#rolling-updates.

Copy link
Member Author

@atiratree atiratree Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about having a link to the Rolling Updates so we do not have to define the strategy again? commit updated..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim -- do you still have concerns, or can we approve this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim looks like this was merged without the comment being addressed, do we need any further followup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to fix that, but that fix can happen after v1.25 ships.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about having a link to the Rolling Updates so we do not have to define the strategy again?

I can post an update, but what are problems with this approach? ^

@soltysh
Copy link
Contributor

soltysh commented Aug 1, 2022

This should be against dev-1.25 branch, since that's covering a feature going GA in 1.25

@atiratree atiratree force-pushed the concepts.statefulset.minreadyseconds branch from d535f22 to 3556301 Compare August 1, 2022 15:27
@k8s-ci-robot 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 Aug 1, 2022
@atiratree atiratree changed the base branch from main to dev-1.25 August 1, 2022 15:28
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 1, 2022
@atiratree
Copy link
Member Author

Thanks for the review. I tried to resolve it and changed the branch.

@atiratree atiratree mentioned this pull request Aug 1, 2022
Closed
soltysh
soltysh approved these changes Aug 1, 2022
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
from tech pov

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: cb13306d16474bfa1e04b210ccbd39013a75c0a6

@reylejano
Copy link
Member

/assign @kcmartin

@atiratree atiratree force-pushed the concepts.statefulset.minreadyseconds branch from 3556301 to 47fc402 Compare August 5, 2022 15:45
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2022
@netlify
Copy link

netlify bot commented Aug 5, 2022 •
edited
Loading

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit 9d7efb1
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/62ed3cc0ee93ef00080173d6

@atiratree atiratree force-pushed the concepts.statefulset.minreadyseconds branch from 47fc402 to e50094c Compare August 5, 2022 15:47
@atiratree atiratree force-pushed the concepts.statefulset.minreadyseconds branch from e50094c to 9d7efb1 Compare August 5, 2022 15:52
@atiratree
Copy link
Member Author

@sftim ping regarding #35539 (comment)

@tengqm
Copy link
Contributor

tengqm commented Aug 14, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3cf8c98d569ed4b38f00673b7efe1db90df80d9d

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, tengqm

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2022
@k8s-ci-robot k8s-ci-robot merged commit 1faec91 into kubernetes:dev-1.25 Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@onlydole onlydole Awaiting requested review from onlydole

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@divya-mohan0209 divya-mohan0209 Awaiting requested review from divya-mohan0209

@kcmartin kcmartin Awaiting requested review from kcmartin

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@soltysh soltysh

@tengqm tengqm

@kcmartin kcmartin

@divya-mohan0209 divya-mohan0209

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.25
Development

Successfully merging this pull request may close these issues.
8 participants
@atiratree @tengqm @divya-mohan0209 @soltysh @k8s-ci-robot @reylejano @kcmartin @sftim