KEP-3017: Pod Healthy Policy for PDBs #3018
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
sig/apps
| approvers: | ||
| - "@liggitt" | ||
| - "@soltysh" | ||
| - "@wojtek-t" |
There was a problem hiding this comment.
You need to also open PRR file later to include KEP in the milestone (together with marking it as implementable): https://github.com/kubernetes/enhancements/tree/master/keps/prod-readiness/sig-apps
Please assing to me.
| // covered by a PodDisruptionBudget. | ||
| type PodHealthyPolicy string | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Does it make sense to consider the other end? That is having MinReadySeconds and PodAvailable as PodHealthyPolicy. The application would then end up with stricter constraints for eviction.
There was a problem hiding this comment.
I'm not sure I follow this. How would MinReadySeconds and PodAvailable solve the issue about when pods should be considered healthy enough to be covered by PDBs?
There was a problem hiding this comment.
This was just a hypothetical scenario, not concerning the original issue.
mortent
force-pushed
the
PodEvictionPolicy
branch
from
c9eb2fb
to
41821aa
Compare
| // whether they are Ready or not. Any pods that are in the Running | ||
| // phase will be counted when computing "disruptionsAllowed" and | ||
| // will be subject to the PDB for eviction. | ||
| PodRunning PodHealthyPolicy = "PodRunning" |
There was a problem hiding this comment.
That's the current default, maybe add an info that empty or this means the same.
There was a problem hiding this comment.
Currently the disruption controller and the eviction api don't agree on what is needed for a pod to be healthy, so we don't have a policy here that reflect the current behavior. The idea is that by specifying a policy, users can get consistent behavior between the two.
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
lgtm
|
/assign @wojtek-t |
|
|
||
| ###### Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested? | ||
|
|
||
| N/A |
There was a problem hiding this comment.
Every feature, especially the ones that are extending the API have to be update->downgrade->upgrde tested.
There was a problem hiding this comment.
Updated this to describe a manual test to verify this, mostly based on looking at other keps. If automated tests are feasible, that would obviously be better.
|
|
||
| ###### What specific metrics should inform a rollback? | ||
|
|
||
| N/A |
There was a problem hiding this comment.
I can easily imagine a buggy implementatio and e.g. disruption-controller crashing if the field is set or sth.
There was a problem hiding this comment.
Yeah, added more details here.
|
|
||
| N/A | ||
|
|
||
| ### Monitoring Requirements |
There was a problem hiding this comment.
Not required for Alpha, but I recommend you to think about it when implementing Alpha. I will be asking for that for sure during Beta review.
There was a problem hiding this comment.
Yeah, I will make sure this is addressed for beta.
mortent
force-pushed
the
PodEvictionPolicy
branch
from
41821aa
to
335ce7d
Compare
k8s-ci-robot
removed
lgtm
k8s-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mortent, soltysh, wojtek-t 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 |
k8s-ci-robot
added
the
approved
There was a problem hiding this comment.
Tracking Issue description，Provide supporting details for a feature in development labels:kind/feature
pod ?What would you like to be added?
pls description the issues.Feature requests are unlikely to make progress as issues. Please consider engaging with SIGs on slack and mailing lists, instead.A proposal that works through the design along with the implications of the change can be opened as a enhancementskepses-proposals-keps Why is this needed?
Add PodHealthyPolicy to PodDisruptionBudget