-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
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. |
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? |
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). 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. |
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. |
Thanks! /lgtm |
@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 |
@thockin I have checked with @justaugustus and we'll need to create an exception request... |
File that exception. I will circle back to this ASAP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
/approve
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 |
[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 |
No description provided.