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

ProbeTerminationGracePeriod promote to GA #114307

Merged
merged 1 commit into from
Jul 13, 2023
Merged

ProbeTerminationGracePeriod promote to GA #114307

merged 1 commit into from
Jul 13, 2023

Conversation

rphillips
Copy link
Member

@rphillips rphillips commented Dec 6, 2022 •
edited by dims

What type of PR is this?

/kind feature

What this PR does / why we need it:

#99375 introduced Probe-level terminationGracePeriodSeconds to provide a timeout for Liveness probes. The feature was initially targeted for GA in 1.25, but was missed.

The PR promotes ProbeTerminationGracePeriod to GA in 1.28 with a removal of the feature gate in 1.29.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Graduate the `ProbeTerminationGracePeriod` feature gate to GA

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

@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.26 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.26.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Dec 6 09:53:09 UTC 2022.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 6, 2022
@k8s-ci-robot k8s-ci-robot added 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 6, 2022
@rphillips
Copy link
Member Author

/sig node

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 6, 2022
@rphillips rphillips force-pushed the promote_probe_termination_grace_period branch from ccb7913 to 4258a0e Compare December 6, 2022 18:52
@rphillips
Copy link
Member Author

rphillips commented Dec 6, 2022 •
edited

pull-kubernetes-node-e2e-containerd seems to be perma-failing flaking on master branch.

@rphillips
Copy link
Member Author

/test pull-kubernetes-node-e2e-containerd

@SergeyKanzhelev
Copy link
Member

Let's make sure the KEP is approved for 1.27 to go to GA and passed the PRR.

Also, is there any tests that we need to promote to conformance with this change?

@SergeyKanzhelev SergeyKanzhelev moved this from Triage to Waiting on Author in SIG Node PR Triage Dec 10, 2022
@sftim
Copy link
Contributor

sftim commented Dec 14, 2022

Suggestion for changelog entry:

Graduate the `ProbeTerminationGracePeriod` feature gate to GA

@rphillips rphillips force-pushed the promote_probe_termination_grace_period branch 2 times, most recently from 2274c69 to 14ed547 Compare January 3, 2023 16:21
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 3, 2023
@rphillips rphillips force-pushed the promote_probe_termination_grace_period branch from 14ed547 to 83d0fc9 Compare January 3, 2023 17:16
@rphillips rphillips mentioned this pull request Jan 3, 2023
Merged
@bart0sh
Copy link
Contributor

bart0sh commented Jan 4, 2023

/priority important-soon
/triage accepted

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 4, 2023
@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label May 5, 2023
@rphillips rphillips force-pushed the promote_probe_termination_grace_period branch from eb0bf92 to c01612a Compare May 5, 2023 19:07
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 5, 2023
@SergeyKanzhelev
Copy link
Member

oh, we missed this for 1.27? I thought it is done. Can you also send the update for k/enhancements PR with the milestone update?

@rphillips rphillips force-pushed the promote_probe_termination_grace_period branch from c01612a to ae08fe1 Compare May 5, 2023 19:28
@rphillips rphillips mentioned this pull request May 5, 2023
Merged
@rphillips
Copy link
Member Author

@SergeyKanzhelev Done: kubernetes/enhancements#3986

@rphillips
Copy link
Member Author

/retest

@pacoxu
Copy link
Member

pacoxu commented Jun 14, 2023 •
edited

KEP is merged.
/lgtm
better merge with #116091

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

LGTM label has been added.

Git tree hash: aad7496e760018539ee9b2c2b662b9b2fae1f470

@dims
Copy link
Member

dims commented Jun 15, 2023

@rphillips two things to follow up if needed

Gonna approve this for now, please submit a follow up PRs if needed.

/approve
/lgtm

@dims
Copy link
Member

dims commented Jun 15, 2023

/assign @thockin (need approvals for some directories that i don't have karma on)

@rphillips
Copy link
Member Author

@liggitt @smarterclayton could you approve for the top level directories?

@liggitt
Copy link
Member

liggitt commented Jul 13, 2023

/approve

for API package updates

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt, 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 Jul 13, 2023
@k8s-ci-robot k8s-ci-robot merged commit a9e40bd into kubernetes:master Jul 13, 2023
12 of 13 checks passed
SIG Node PR Triage automation moved this from Needs Approver to Done Jul 13, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jul 13, 2023
@gjkim42
Copy link
Member

gjkim42 commented Jul 17, 2023

I guess this may be related to this regression.

gjkim42
gjkim42 reviewed Jul 17, 2023
View reviewed changes
pkg/api/pod/util.go
if !utilfeature.DefaultFeatureGate.Enabled(features.ProbeTerminationGracePeriod) && !probeGracePeriodInUse(oldPodSpec) {
if !probeGracePeriodInUse(oldPodSpec) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the problem.

We should never drop terminationGracePeriodSeconds of a probe.

I'll propose a fix for this.

Copy link
Member

Choose a reason for hiding this comment

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

#119354

Copy link
Member

Choose a reason for hiding this comment

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

you are correct... this block should have gone away entirely

@sftim
Copy link
Contributor

sftim commented Jul 19, 2023

Changelog suggestion

Graduated support for probe-level `terminationGracePeriodSeconds` to general availability.

We graduate features; feature gates change as part of this.

@sftim sftim mentioned this pull request Jul 19, 2023
Open
12 tasks
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

@pacoxu pacoxu pacoxu left review comments

@gjkim42 gjkim42 gjkim42 left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@bart0sh bart0sh Awaiting requested review from bart0sh

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@mrunalp mrunalp Awaiting requested review from mrunalp

@SergeyKanzhelev SergeyKanzhelev Awaiting requested review from SergeyKanzhelev

Assignees

@dims dims

@mrunalp mrunalp

@pacoxu pacoxu

@derekwaynecarr derekwaynecarr

@dchen1107 dchen1107

@SergeyKanzhelev SergeyKanzhelev

@bart0sh bart0sh

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/feature Categorizes issue or PR as related to a new feature. 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/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. 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 PR Triage
Archived in project
SIG Node PR Triage
Done
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

None yet