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

Updates kep-2238 for v1.25 #3408

Merged
merged 1 commit into from
Jun 27, 2022
Merged

Updates kep-2238 for v1.25 #3408

merged 1 commit into from
Jun 27, 2022

Conversation

psschwei
Copy link
Contributor

@psschwei psschwei commented Jun 17, 2022 •
edited
Loading

Signed-off-by: Paul S. Schweigert [email protected]

  • One-line PR description: updating KEP to new template
  • Issue link:
  • Other comments:

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 17, 2022
@psschwei psschwei force-pushed the kep-2238-update branch 3 times, most recently from 123e781 to 273b626 Compare June 17, 2022 19:56
@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 Jun 17, 2022
@psschwei psschwei changed the title [WIP] Updates kep-2238 for beta [WIP] Updates kep-2238 for v1.25 Jun 17, 2022
psschwei
psschwei commented Jun 17, 2022
View reviewed changes
keps/sig-node/2238-liveness-probe-grace-period/kep.yaml
Comment on lines -17 to -18
prr-approvers:
- "@johnbelamaric"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per comments here

Copy link
Member

Choose a reason for hiding this comment

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

ok

@psschwei psschwei mentioned this pull request Jun 17, 2022
Closed
12 tasks
@psschwei psschwei force-pushed the kep-2238-update branch 2 times, most recently from cc58c83 to 258542b Compare June 20, 2022 15:27
mikebrow
mikebrow reviewed Jun 20, 2022
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

couple comments / questions

keps/sig-node/2238-liveness-probe-grace-period/README.md Outdated Show resolved Hide resolved
keps/sig-node/2238-liveness-probe-grace-period/README.md Outdated Show resolved Hide resolved
keps/sig-node/2238-liveness-probe-grace-period/README.md Show resolved Hide resolved
@johnbelamaric
Copy link
Member

No separate PRR is needed, this just requires your SIG approval.

@psschwei psschwei force-pushed the kep-2238-update branch 2 times, most recently from a14c432 to c087470 Compare June 21, 2022 16:05
@psschwei psschwei changed the title [WIP] Updates kep-2238 for v1.25 Updates kep-2238 for v1.25 Jun 21, 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 Jun 21, 2022
@mrunalp
Copy link
Contributor

mrunalp commented Jun 21, 2022

/lgtm
/approve

@mrunalp
Copy link
Contributor

mrunalp commented Jun 21, 2022

/assign @dchen1107

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2022
mikebrow
mikebrow approved these changes Jun 21, 2022
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM one possible small change to line 159 .. We expect that the `livenessProbe.terminationGracePeriodSeconds` (or `startupProbe.*`) will not be greater than the pod-level `terminationGracePeriodSeconds`, but we will not explicitly validate this. is this still the case?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2022
@psschwei
Copy link
Contributor Author

LGTM one possible small change to line 159 .. We expect that the `livenessProbe.terminationGracePeriodSeconds` (or `startupProbe.*`) will not be greater than the pod-level `terminationGracePeriodSeconds`, but we will not explicitly validate this. is this still the case?

This is still the case, there is no current validation for whether the probe-level value is less than the pod-level value.

@mikebrow
Copy link
Member

LGTM one possible small change to line 159 .. We expect that the `livenessProbe.terminationGracePeriodSeconds` (or `startupProbe.*`) will not be greater than the pod-level `terminationGracePeriodSeconds`, but we will not explicitly validate this. is this still the case?

This is still the case, there is no current validation for whether the probe-level value is less than the pod-level value.

nod.. kk suggest at least adding a log to notify when that happens (the probe's grace period is ignored due to pod level) Overriding the ability of the container to gracefully finish after first signal due to pod override could cause loss of data. Maybe a soft validation?

@psschwei
Copy link
Contributor Author

Maybe a soft validation?

I'm not even opposed to a hard validation, but not sure if we're allowed to make that kind of change at this stage...

@psschwei
updates kep-2238 for beta
6270ce1 
Signed-off-by: Paul S. Schweigert <[email protected]>
mikebrow
mikebrow approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM

keps/sig-node/2238-liveness-probe-grace-period/README.md
@@ -149,7 +158,8 @@ This field is not valid for readiness probes. We will add validation to ensure

We expect that the `livenessProbe.terminationGracePeriodSeconds` (or
`startupProbe.*`) will not be greater than the pod-level
`terminationGracePeriodSeconds`, but we will not explicitly validate this.
`terminationGracePeriodSeconds`, but we will not explicitly validate this (we will
Copy link
Member

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 22, 2022
@psschwei psschwei mentioned this pull request Jun 22, 2022
Merged
@Priyankasaggu11929
Copy link
Member

Hello, 1.25 Enhancements Lead here 👋. With Enhancements Freeze now in effect, this enhancement has been removed from the milestone.

Looks like we're just missing with a SIG approval on this PR

Feel free to file an exception to add this back to the release. If you plan to do so, please file this as early as possible.

Thanks!

@derekwaynecarr
Copy link
Member

I was out of office last week, and feel bad for missing this in interim.

The updates lgtm for sig-node.

/lgtm
/approve
/hold

If @Priyankasaggu11929 and release team agree to exception, we can remove hold.

@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 27, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, mikebrow, mrunalp, psschwei

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 27, 2022
@cici37 cici37 added this to the v1.25 milestone Jun 27, 2022
@cici37
Copy link
Contributor

cici37 commented Jun 27, 2022

Hi, Thanks for raising the exception request. The exception request has been approved by 1.25 release team and your updated deadline to make any changes to your KEP is 18:00 PST Thursday 30th June 2022.
/milestone v1.25
/unhold

@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 27, 2022
@k8s-ci-robot k8s-ci-robot merged commit 4e095b9 into kubernetes:master Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnbelamaric johnbelamaric johnbelamaric left review comments

@mikebrow mikebrow mikebrow approved these changes

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

Assignees

@mrunalp mrunalp

@derekwaynecarr derekwaynecarr

@dchen1107 dchen1107

@mikebrow mikebrow

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/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.
9 participants
@psschwei @johnbelamaric @mrunalp @mikebrow @Priyankasaggu11929 @derekwaynecarr @k8s-ci-robot @cici37 @dchen1107