Skip to content

[ 4802] Graduate Windows node graceful shutdown from alpha to beta #5380

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
Merged
merged 1 commit into from
Jun 18, 2025
Merged

[ 4802] Graduate Windows node graceful shutdown from alpha to beta #5380

merged 1 commit into from
Jun 18, 2025

Conversation

zylxjtu
Copy link
Contributor

@zylxjtu zylxjtu commented Jun 5, 2025

  • One-line PR description:
  • Issue link:
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 5, 2025
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 5, 2025
@zylxjtu zylxjtu mentioned this pull request Jun 5, 2025
Open
9 tasks
msft-linliu
msft-linliu reviewed Jun 5, 2025
View reviewed changes
keps/sig-windows/4802-windows-node-shutdown/README.md Outdated
@@ -267,6 +267,8 @@ Until then, we will cover all the scenerios with e2e tests

* Addresses feedback from alpha testers
* Sufficient E2E and unit testing
* Adding [Windows node level test](https://github.com/kubernetes/kubernetes/pull/129938) , which will include the gracefulshutdown case.
* [Enabling the test in CPAZ cluster](https://github.com/kubernetes-sigs/windows-testing/pull/506)
Copy link

Choose a reason for hiding this comment

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

typo "CPAZ". should be "CAPZ"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@SergeyKanzhelev
Copy link
Member

/hold

to resolve this: #4802 (comment)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2025
@marosset marosset changed the title Graduate 4802 from alpha to beta [ 4802] Graduate Windows node graceful shutdown from alpha to beta Jun 10, 2025
@jmickey
Copy link
Member

jmickey commented Jun 11, 2025

I believe stage should also be updated to reflect the target stage for v1.34.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 11, 2025
SergeyKanzhelev
SergeyKanzhelev reviewed Jun 11, 2025
View reviewed changes
keps/sig-windows/4802-windows-node-shutdown/README.md Outdated
@@ -267,6 +267,8 @@ Until then, we will cover all the scenerios with e2e tests

* Addresses feedback from alpha testers
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Addresses feedback from alpha testers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@marosset
Copy link
Contributor

/approve
For sig-windows since there are is only minor feedback items left to address

SergeyKanzhelev
SergeyKanzhelev approved these changes Jun 13, 2025
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev 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 Jun 13, 2025
@jsturtevant
Copy link
Contributor

@kannon92 @deads2k we've addressed the feedback from sig-node could you take a look at the PRR again? Thanks!

kannon92
kannon92 reviewed Jun 17, 2025
View reviewed changes
Copy link
Contributor

@kannon92 kannon92 left a comment

Choose a reason for hiding this comment

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

Is there a reason why the PRR answers diverge from https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/2712-pod-priority-based-graceful-node-shutdown/README.md#production-readiness-review-questionnaire and https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2000-graceful-node-shutdown#production-readiness-review-questionnaire.

I would expect similar metrics for this. I don't see any metrics for this feature and I'm not really sure how to troubleshoot this feature by reading the questions.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 17, 2025
@zylxjtu
Copy link
Contributor Author

zylxjtu commented Jun 17, 2025

Member

Is there a reason why the PRR answers diverge from https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/2712-pod-priority-based-graceful-node-shutdown/README.md#production-readiness-review-questionnaire and https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2000-graceful-node-shutdown#production-readiness-review-questionnaire.

I would expect similar metrics for this. I don't see any metrics for this feature and I'm not really sure how to troubleshoot this feature by reading the questions.

Send another iteration, I guess KEP 2000 was following older templates and there are comments to remove the explanation part from the KEP during 4802 review.

@zylxjtu zylxjtu requested a review from kannon92 June 17, 2025 23:10
@kannon92
Copy link
Contributor

Member

Is there a reason why the PRR answers diverge from https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/2712-pod-priority-based-graceful-node-shutdown/README.md#production-readiness-review-questionnaire and https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2000-graceful-node-shutdown#production-readiness-review-questionnaire.
I would expect similar metrics for this. I don't see any metrics for this feature and I'm not really sure how to troubleshoot this feature by reading the questions.

Send another iteration, I guess KEP 2000 was following older templates and there are comments to remove the explanation part from the KEP during 4802 review.

I didn't mean the template but the content.

There is still differences between 2712 and 2000 and this KEP.

kannon92
kannon92 reviewed Jun 18, 2025
View reviewed changes
kannon92
kannon92 reviewed Jun 18, 2025
View reviewed changes
kannon92
kannon92 reviewed Jun 18, 2025
View reviewed changes
@zylxjtu zylxjtu requested review from kannon92 and msft-linliu June 18, 2025 16:49
@kannon92
Copy link
Contributor

PRR shadow:

LGTM

/assign @deads2k

@deads2k
Copy link
Contributor

deads2k commented Jun 18, 2025

PRR looks good.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, marosset, SergeyKanzhelev, zylxjtu

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 Jun 18, 2025
@haircommander
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2025
@zylxjtu
Copy link
Contributor Author

zylxjtu commented Jun 18, 2025

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2025
@k8s-ci-robot k8s-ci-robot merged commit e06aa34 into kubernetes:master Jun 18, 2025
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.34 milestone Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyKanzhelev SergeyKanzhelev

@aravindhp aravindhp

@claudiubelu claudiubelu

@kannon92 kannon92

@msft-linliu msft-linliu

Assignees

@deads2k deads2k

@haircommander haircommander

@SergeyKanzhelev SergeyKanzhelev

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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.34
Development

Successfully merging this pull request may close these issues.
10 participants
@zylxjtu @SergeyKanzhelev @jmickey @marosset @jsturtevant @kannon92 @deads2k @k8s-ci-robot @haircommander @msft-linliu