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

Re-enable Kubelet Pod Readiness Probes on Termination and Pod probes should be handled by pod worker #110191

Merged
merged 4 commits into from Jun 7, 2022
Merged

Re-enable Kubelet Pod Readiness Probes on Termination and Pod probes should be handled by pod worker #110191

merged 4 commits into from Jun 7, 2022

Conversation

rphillips
Copy link
Member

@rphillips rphillips commented May 24, 2022 •
edited by liggitt

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

This PR builds off of @smarterclayton 's #107893 PR to fix kubelet: Pod probes should be handled by pod worker. The pod refactor in 2021 disabled all probes on pod deletion. @deads2k also reported to me an issue where Pods were failing to report readiness status on termination. Readiness probes are important for controllers and load balancers to know when to take a pod out of service. Not reporting readiness can cause a load balancer (or controller) to react slower to a terminating pod producing 500s and other ill effects. This PR now reports the correct readiness status during termination.

#107893 needed a test case, which this PR adds.

Which issue(s) this PR fixes:

Fixes #110309

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix 1.22 regression so pods will now post their readiness during termination.

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. kind/bug Categorizes issue or PR as related to a bug. 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-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. area/kubelet area/test 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-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 24, 2022
@rphillips rphillips force-pushed the fixes/probe_shutdowns_readiness branch 3 times, most recently from 5040a2e to 0d29ef4 Compare May 24, 2022 14:55
@harche
Copy link
Contributor

harche commented May 24, 2022

/test pull-kubernetes-node-crio-e2e

@openshift-ci-robot openshift-ci-robot mentioned this pull request May 24, 2022
Merged
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 24, 2022
@rphillips rphillips force-pushed the fixes/probe_shutdowns_readiness branch from 0d29ef4 to 8783744 Compare May 24, 2022 16:17
@rphillips
Copy link
Member Author

cc @aojea

@rphillips rphillips force-pushed the fixes/probe_shutdowns_readiness branch 2 times, most recently from fdf99b1 to de7fa7b Compare May 24, 2022 16:51
@rphillips rphillips changed the title WIP: Fix pod shutdown readiness Re-enable Kubelet Pod Readiness Probes on Termination and Pod probes should be handled by pod worker May 24, 2022
@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 May 24, 2022
@rphillips rphillips mentioned this pull request May 24, 2022
Closed
@rphillips
Copy link
Member Author

/priority important-soon

@rphillips
Copy link
Member Author

@bobbypage Thank you for the review.

I updated the tests to the constants you references and added the gomega.Consistently tweak.

I removed the graceperiod optimization since I do not know of any limitations to it.

@bobbypage
Copy link
Member

Thanks for all the fixes and updates!

/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 6, 2022
@bobbypage
Copy link
Member

/assign @mrunalp

@rphillips
Copy link
Member Author

/assign @aojea and @SergeyKanzhelev

@k8s-ci-robot
Copy link
Contributor

@rphillips: GitHub didn't allow me to assign the following users: and.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @aojea and @SergeyKanzhelev

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.

@aojea
Copy link
Member

aojea commented Jun 7, 2022

/approve

Thank you very much

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, mrunalp, rphillips

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 7, 2022
@k8s-ci-robot k8s-ci-robot merged commit 79cef12 into kubernetes:master Jun 7, 2022
SIG Node CI/Test Board automation moved this from PRs - Needs Reviewer to Done Jun 7, 2022
SIG Node PR Triage automation moved this from Needs Reviewer to Done Jun 7, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 7, 2022
This was referenced Jun 7, 2022
Merged
Merged
Merged
@bobbypage bobbypage mentioned this pull request Jun 7, 2022
Closed
k8s-ci-robot added a commit that referenced this pull request Jun 8, 2022
@k8s-ci-robot
Merge pull request #110416 from aojea/automated-cherry-pick-of-#11019…
bf2a7ff 
…1-upstream-release-1.24

Automated cherry pick of #110191: kubelet: Pod probes should be handled by pod worker
k8s-ci-robot added a commit that referenced this pull request Jun 8, 2022
@k8s-ci-robot
Merge pull request #110418 from aojea/automated-cherry-pick-of-#11019…
af521aa 
…1-upstream-release-1.22

Automated cherry pick of #110191: kubelet: Pod probes should be handled by pod worker
k8s-ci-robot added a commit that referenced this pull request Jun 8, 2022
@k8s-ci-robot
Merge pull request #110417 from aojea/automated-cherry-pick-of-#11019…
df140a7 
…1-upstream-release-1.23

Automated cherry pick of #110191: kubelet: Pod probes should be handled by pod worker
@sftim sftim mentioned this pull request Oct 29, 2022
Open
This was referenced Jan 10, 2023
Closed
Closed
@aojea aojea mentioned this pull request Apr 29, 2023
Closed
@IvanUkhov IvanUkhov mentioned this pull request Jan 4, 2024
Open
@rphillips rphillips mentioned this pull request Jan 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobbypage bobbypage bobbypage left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@aojea aojea aojea left review comments

@249043822 249043822 249043822 left review comments

@mrunalp mrunalp mrunalp approved these changes

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@bowei bowei Awaiting requested review from bowei

Assignees

@mrunalp mrunalp

@bobbypage bobbypage

@aojea aojea

@SergeyKanzhelev SergeyKanzhelev

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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/network Categorizes an issue or PR as relevant to SIG Network. 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
Done
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Done
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

Kubelet Readiness Probes do not run during pod termination