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.

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

Add windows node graceful shutdown doc #48474

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

zylxjtu
Copy link
Contributor

@zylxjtu zylxjtu commented Oct 21, 2024

Description

Issue

kubernetes/enhancements#4802
code PR:
kubernetes/kubernetes#127404

Closes: #

@k8s-ci-robot k8s-ci-robot added this to the 1.32 milestone Oct 21, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2024
Copy link

linux-foundation-easycla bot commented Oct 21, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: zylxjtu / name: yuanliang (e4bdcbc)

@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Oct 21, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 21, 2024
Copy link

netlify bot commented Oct 21, 2024

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

Name Link
🔨 Latest commit e4bdcbc
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/673c230fb9eaf600086771ff

Copy link

netlify bot commented Oct 21, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit e4bdcbc
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/673c230f331ff400087ca19d
😎 Deploy Preview https://deploy-preview-48474--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 configuration.

@chanieljdan
Copy link
Contributor

Hi @zylxjtu 👋

Just a reminder to take a look at Documenting for a release - PR Ready for Review to get your PR ready for review ahead of the "Ready for Review" deadline on Tuesday November 19th 2024 18:00 PST.

Thank you!

Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

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

/easycla

@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 Nov 14, 2024
@zylxjtu zylxjtu force-pushed the dev-1.32 branch 2 times, most recently from 63fe604 to 8833042 Compare November 14, 2024 18:47
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 14, 2024
@zylxjtu zylxjtu marked this pull request as ready for review November 14, 2024 18:48
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2024
@zylxjtu zylxjtu requested a review from Arhell November 14, 2024 18:48
@zylxjtu zylxjtu changed the title Add windows node graceful shutdown doc (draft) Add windows node graceful shutdown doc Nov 14, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 14, 2024
Comment on lines 13 to 15
- stage: beta
defaultValue: true
fromVersion: "1.33"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want this in here, until we get to the 1.33 release and make a decision on this in the KEP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed


If Kubelet is not running as a Windows service, it will not be able to set and monitor
the [Preshutdown](https://learn.microsoft.com/en-us/windows/win32/api/winsvc/ns-winsvc-service_preshutdown_info) event,
the node will have to go through the [Non-Graceful Node Shutdown](#non-graceful-node-shutdown) procedure mentioned above.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the node will have to go through the [Non-Graceful Node Shutdown](#non-graceful-node-shutdown) procedure mentioned above.
the node will have to go through the [Non-Graceful Node Shutdown](#non-graceful-node-shutdown) procedure mentioned above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will the user get a log? Will kubelet fail or will it silently continue on in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

- stage: alpha
defaultValue: false
fromVersion: "1.32"
toVersion: "1.32"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean these 4 lines or the line 12?

Copy link
Contributor

Choose a reason for hiding this comment

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

This feature will not go away by the end of 1.32, right?
So line 12 should be removed.

it will then have a registered [service control handler](https://learn.microsoft.com/en-us/windows/win32/services/service-control-handler-function)
to delay the presshutdown event with a given duration.

Windows graceful node shutdown is controlled with the `WindowsGracefulNodeShutdown` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Windows graceful node shutdown is controlled with the `WindowsGracefulNodeShutdown` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
Windows graceful node shutdown is controlled with the `WindowsGracefulNodeShutdown`
[feature gate](/docs/reference/command-line-tools-reference/feature-gates/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update, just wondering what the visual difference will be?

Copy link
Contributor

Choose a reason for hiding this comment

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

From the perspective of the rendered HTML, nothing is different.
From the perspective of docs "source", we discourage long lines, because git diff works
on a line-by-line comparison basis. Changes to long lines are difficult to figure out
thus are difficult to review. For downstream localization teams, they need to track the
upstream changes on a line by line basis. Long lines make their lives suck.

@tengqm
Copy link
Contributor

tengqm commented Nov 19, 2024

/approve
Considering that the upstream code has landed.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 Nov 19, 2024
@marosset
Copy link
Contributor

/lgtm
/hold
in case @jsturtevant wants to re-review

@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 Nov 19, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b023a62ff23b7a5968106914519284cef03a8a7d

@marosset
Copy link
Contributor

/sig windows
/sig node

@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Nov 19, 2024
@jsturtevant
Copy link
Contributor

/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 Nov 21, 2024
@k8s-ci-robot k8s-ci-robot merged commit 5ea7951 into kubernetes:dev-1.32 Nov 21, 2024
6 checks passed

Windows graceful node shutdown is controlled with the `WindowsGracefulNodeShutdown`
[feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
which is introduced in 1.32 as an alpha feature.
Copy link
Member

@carlory carlory Dec 6, 2024

Choose a reason for hiding this comment

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

Hi @zylxjtu, @SergeyKanzhelev If a user wants to use this feature, the GracefulNodeShutdown and WindowsGracefulNodeShutdown feature-gate must be turned on together. Is it correct?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, depends on which platform are you on, you only need to enable one of them

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a bug. If GracefulNodeShutdown is false, the kubelet shouldn't provide graceful node shutdown.

(or, we should document that the feature gate is misnamed and ought to be treated as if it were named LinuxGracefulNodeShutdown).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Development

Successfully merging this pull request may close these issues.

9 participants