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

kubelet: Add GracefulNodeShutdownPodPolicy config option to graceful node shutdown #108941

Closed
wants to merge 4 commits into from
Closed

kubelet: Add GracefulNodeShutdownPodPolicy config option to graceful node shutdown #108941

wants to merge 4 commits into from

Conversation

bobbypage
Copy link
Member

@bobbypage bobbypage commented Mar 23, 2022 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Graceful node shutdown currently places pods into terminal phase upon shutdown. As discussed in #104531 (comment) and #104531 (comment) some users/distributions would benefit from the ability to toggle this behavior and instead to not put pods into terminal phase on shutdown.

For example, if it's expected that after shutdown the node will reboot, it may be desirable to not place the pods into terminal phase, so that way after reboot the pods will start running again after the node comes back up.

This PR adds a new kubelet configuration option to toggle this behavior GracefulNodeShutdownPodPolicy. The default setting of GracefulNodeShutdownPodPolicy is enabled SetTerminal (which matches current behavior since 1.20) when graceful node shutdown was introduced.

Which issue(s) this PR fixes:

Fixes #108991

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add a new kubelet configuration option `GracefulNodeShutdownPodPolicy` to toggle setting pods to terminal phase during graceful node shutdown.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 23, 2022
@bobbypage bobbypage changed the title wip: kubelet: Add ShutdownSetPodsPhaseToTerminal config option to graceful node shutdown WIP: kubelet: Add ShutdownSetPodsPhaseToTerminal config option to graceful node shutdown Mar 23, 2022
@k8s-ci-robot k8s-ci-robot added area/kubelet area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 23, 2022
@k8s-ci-robot k8s-ci-robot added area/provider/gcp Issues or PRs related to gcp provider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Mar 23, 2022
@ehashman ehashman added this to Triage in SIG Node PR Triage Mar 24, 2022
@ehashman ehashman moved this from Triage to Waiting on Author in SIG Node PR Triage Mar 24, 2022
@bobbypage
Copy link
Member Author

/test pull-kubernetes-node-kubelet-serial-containerd

1 similar comment
@bobbypage
Copy link
Member Author

/test pull-kubernetes-node-kubelet-serial-containerd

@smarterclayton
Copy link
Contributor

Some api doc comments

@bobbypage
kubelet: add new config option GracefulNodeShutdownPodPolicy
f573b64 
Add a new kubelet config option, GracefulNodeShutdownPodPolicy to
control policy of how pod termination is handled during graceful
shutdown.

The two options are `SetTerminal` or `LeaveRunning`. The default is
`SetTerminal` whill place set pod phase to terminal when the node
gracefully shuts down (which is the current default behavior).

The other policy type, `LeaveRunning`, will not change the pod phase during
graceful shutdown.

Also run `make generated_files` to regen api files after kubelet
configuration update.

Signed-off-by: David Porter <david@porter.me>
@bobbypage
Add validation for GracefulNodeShutdownPodPolicy
5ed90d6 
Add validation which checks that GracefulNodeShutdownPodPolicy is set to
a expected value which is currently only `SetTerminal` or
`LeaveRunning`.

Also update the kubelet config validation unit tests.

Signed-off-by: David Porter <david@porter.me>
@bobbypage
kubelet: Use the GracefulNodeShutdownPodPolicy config
f10d2d4 
Pass the config option from the kubelet config to the graceful node
shutdown manager, and only update phase if the option is enabled.

Signed-off-by: David Porter <david@porter.me>
@bobbypage
test: Update node shutdown test for GracefulNodeShutdownPodPolicy
3f1d465 
Refactor the existing test to ensure we can test two scenarios, of
`SetTerminal` or `LeaveRunning` which are two possible
GracefulNodeShutdownPodPolicy policy types.

Modify the check `isPodShutdown` to pass `expectPhaseToBeFailed` and
only check phase if the option is enabled.

Signed-off-by: David Porter <david@porter.me>
@k8s-ci-robot
Copy link
Contributor

@bobbypage: 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-node-e2e-containerd 3f1d465 link true /test pull-kubernetes-node-e2e-containerd
pull-kubernetes-conformance-kind-ipv6-parallel 3f1d465 link false /test pull-kubernetes-conformance-kind-ipv6-parallel
pull-kubernetes-e2e-kind 3f1d465 link true /test pull-kubernetes-e2e-kind
pull-kubernetes-e2e-kind-ipv6 3f1d465 link true /test pull-kubernetes-e2e-kind-ipv6
pull-kubernetes-e2e-gce-ubuntu-containerd 3f1d465 link true /test pull-kubernetes-e2e-gce-ubuntu-containerd
pull-kubernetes-integration 3f1d465 link true /test pull-kubernetes-integration
pull-kubernetes-e2e-gce-100-performance 3f1d465 link true /test pull-kubernetes-e2e-gce-100-performance

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2022
@k8s-ci-robot
Copy link
Contributor

@bobbypage: PR needs rebase.

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.

@pacoxu
Copy link
Member

pacoxu commented Apr 7, 2022

/cc @wzshiming

@pacoxu pacoxu moved this from Needs Reviewer to Waiting on Author in SIG Node PR Triage Apr 7, 2022
@bobbypage bobbypage mentioned this pull request May 4, 2022
Closed
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2022
@endocrimes endocrimes moved this from Waiting on Author to Needs Reviewer in SIG Node PR Triage Jul 28, 2022
@endocrimes endocrimes moved this from Needs Reviewer to Waiting on Author in SIG Node PR Triage Jul 28, 2022
@endocrimes
Copy link
Member

Are we still targeting this for 1.25?

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 27, 2022
@liggitt liggitt added this to Backlog in API Reviews Sep 15, 2022
@liggitt liggitt moved this from Backlog to Assigned in API Reviews Sep 15, 2022
@wzshiming wzshiming mentioned this pull request Sep 16, 2022
Open
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

SIG Node PR Triage automation moved this from Waiting on Author to Done Sep 26, 2022
@liggitt liggitt removed this from Assigned in API Reviews Sep 29, 2022
@dghubble dghubble mentioned this pull request Oct 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@ehashman ehashman ehashman left review comments

@yangjunmyfm192085 yangjunmyfm192085 yangjunmyfm192085 left review comments

@mrunalp mrunalp mrunalp approved these changes

@bart0sh bart0sh Awaiting requested review from bart0sh

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@wzshiming wzshiming Awaiting requested review from wzshiming

Assignees

@liggitt liggitt

@ehashman ehashman

Labels
api-review Categorizes an issue or PR as actively needing an API review. area/code-generation area/kubelet area/provider/gcp Issues or PRs related to gcp provider area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node CI/Test Board
Archive-it
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add kubelet configuration option to graceful shutdown to toggle if pods are put into terminal phase