-
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
KEP-3017: rewrite PodHealthyPolicy KEP into UnhealthyPodEvictionPolicy #3677
KEP-3017: rewrite PodHealthyPolicy KEP into UnhealthyPodEvictionPolicy #3677
Conversation
atiratree
commented
Dec 8, 2022
- One-line PR description: changes to reflect the implementation that was considerably altered as compared to this KEP
- Issue link: PodHealthyPolicy for PodDisruptionBudget #3017
- Other comments:
- to reflect the implementation
659e930
to
612c538
Compare
content changes lgtm, I would actually keep the directory name the same and avoid renaming the files and losing history |
612c538
to
ecbac28
Compare
+1, I have removed the rename and keeping the old name |
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.
The changes LGTM. Thanks for making these changes @atiratree
/lgtm |
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.
/approve
|
||
Currently, we only allow evicting Running pods in case there are enough pods healthy | ||
(`.status.currentHealthy` is at least equal to `.status.DesiredHealthy`). | ||
This is to give the application best change to achieve availability and prevent data loss |
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.
This is to give the application best change to achieve availability and prevent data loss | |
This is to give the application best chance to achieve availability and prevent data loss |
// Clients making eviction decisions should disallow eviction of unhealthy pods | ||
// if they encounter an unrecognized policy in this field. | ||
// | ||
// This field is alpha-level. The eviction API uses this field when |
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.
Nit: when updating this for beta, I'd just drop this information, it's important to be in the API, but not so in enhancement, and you'll have to remember to update it every time 😉
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: atiratree, liggitt, ravisantoshgudimetla, soltysh 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 |