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

Mark pod-startup-liveness-probe-holdoff KEP as implementable #1014

Merged
merged 1 commit into from
May 1, 2019

Conversation

matthyx
Copy link
Contributor

@matthyx matthyx commented Apr 29, 2019

No description provided.

@k8s-ci-robot k8s-ci-robot added 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 sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 29, 2019
@derekwaynecarr
Copy link
Member

Prior to marking implementable, we should clarify if there is a feature gate for this item, and how we will roll the feature out across a set of releases.

@thockin thockin changed the title Mark KEP as implementable Mark probe initializationFailureThreshold KEP as implementable Apr 29, 2019
@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 Apr 29, 2019
@thockin
Copy link
Member

thockin commented Apr 29, 2019

Because of the torrent of excellent KEPs, I have not seen this one.

I am not sure this is the right API -- does it have any meaning to readiness probes or just liveness probes? I don't see how it has utility for readiness.

In kubernetes/kubernetes#27114 (comment) we looked at a different take on this API - it is more precise in its meaning and rather than add yet another behavior modifier to probe, it can reuse the probe structure directly.

If this (or similarly orthogonal -- I am not attached to it) was considered and discarded, please explain why?

@matthyx
Copy link
Contributor Author

matthyx commented Apr 29, 2019

Hi @thockin glad to see you back on this subject.

Well, I thought you agreed since you have remained quiet for some time in the PR (kubernetes/kubernetes#71449).
In the meantime, I have also reached sig-node for their review...

To come back to your proposal for a third probe, do you still think it is simplier to explain to other people why they should implement a StartupProbe in order to prevent LivenessProbe from killing their containers?

To me it made more sense to expand the current LivenessProbe, and while it doesn't really have an effect on ReadinessProbe, I thought for consistency it wouldn't hurt to let it this way :-)

Please let me know how to make sure we don't waste another release cycle for that feature.
Thanks.

@thockin
Copy link
Member

thockin commented Apr 30, 2019

I feel pretty strongly that something like a startupProbe would be net simpler to comprehend than a new field on liveness.

I think you are on EU time, and I feel very guilty that I lost track of this KEP. I feel bad that this could miss the boat because of me, so here's what I proposed to @dchen1107 and @yujuhong -- I'll retitle this KEP to be a bit more generic and approve it. They are both OK with the general idea here. We can haggle over the exact shape of the API in the coming days.

@thockin thockin changed the title Mark probe initializationFailureThreshold KEP as implementable Mark pod-startup-liveness-probe-holdoff KEP as implementable Apr 30, 2019
@thockin thockin self-assigned this Apr 30, 2019
@thockin
Copy link
Member

thockin commented Apr 30, 2019

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 1, 2019
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 1, 2019
@matthyx
Copy link
Contributor Author

matthyx commented May 1, 2019

@thockin thanks a lot for your help, looks like it wasn't merged after your LGTM because you're not an approver for sig-node...

I have updated the design for startupProbe, let's make sure we can still include it in 1.15

@matthyx
Copy link
Contributor Author

matthyx commented May 1, 2019

@thockin I have checked with @justaugustus and we'll need to create an exception request...
Probably before doing so we should agree on the design and if you could help me with the graduation criteria (this is still very cryptic for me)?
I will describe the test plan once we have the implementation...

@thockin
Copy link
Member

thockin commented May 1, 2019

File that exception. I will circle back to this ASAP

Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2019
@derekwaynecarr
Copy link
Member

I am fine w/ iterating on this more during PR process. The general shape and need of the problem is clear.

We will follow-up with exception.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, matthyx, thockin

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 May 1, 2019
@k8s-ci-robot k8s-ci-robot merged commit 5cd9ca7 into kubernetes:master May 1, 2019
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. 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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants