-
Notifications
You must be signed in to change notification settings - Fork 4k
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
[VPA] KEP-4902: Delete OOM Pods #4902
Changes from 4 commits
45a6608
58b19eb
8a09276
d2859ee
1711a4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
# KEP-4902: Delete OOM Pods | ||
|
||
<!-- toc --> | ||
- [Summary](#summary) | ||
- [Motivation](#motivation) | ||
- [Goals](#goals) | ||
- [Non-Goals](#non-goals) | ||
- [Proposal](#proposal) | ||
- [Design Details](#design-details) | ||
- [Test Plan](#test-plan) | ||
- [Implementation History](#implementation-history) | ||
- [Alternatives](#alternatives) | ||
- [Update the eviction API](#update-the-eviction-api) | ||
<!-- /toc --> | ||
|
||
|
||
## Summary | ||
|
||
The default behaviour of VPA Updater is to evict Pods when new resource | ||
recommendations are available. This works fine for most cases. | ||
A problem that can arise is when there are multiple replicas and a | ||
PodDisruptionBudget (PDB) which allows one disruption. Now the eviction is at | ||
risk, because as soon as multiple replicas run into for example the memory | ||
limit and get killed they will never recover as the eviciton API will not allow | ||
any further disruptions. | ||
|
||
This proposal addresses the problem by allowing users to enable the deletion of | ||
pods as a backup if the eviction fails. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. allowing users to enable the deletion of |
||
|
||
|
||
## Motivation | ||
|
||
The motivation behind the change is to give VPA users a way to recover a | ||
failing deployment, if updated/increased limits would solve the problem. | ||
|
||
### Goals | ||
|
||
- Main: allow cluster administrators and other users to enable deletion of pods | ||
|
||
### Non-Goals | ||
|
||
- Get rid of or work around the existing eviction behaviour | ||
|
||
|
||
## Proposal | ||
|
||
The proposal is to add a new field to the VPA resource and a global flag. | ||
|
||
A new global flag (`--delete-on-eviction-error`) shall be added to the VPA | ||
updater to enable the new feature globally. | ||
|
||
Additionally a new field in the VPA resource | ||
(`Spec.UpdatePolicy.DeleteOnEvictionError`) which takes precedence to the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think |
||
global flag. When unset the value of the flag is taken. This allows cluster | ||
administrators to enable the flag for all VPA resources and at the same time | ||
disable it again for specific deployments, or only enable it for specific | ||
deployments. | ||
|
||
This should give users the most flexible way of configuring this feature to | ||
fit their needs. | ||
|
||
|
||
## Design Details | ||
|
||
When the eviction fails the pod will not just get blindy deleted, but further | ||
checks will occur. Which gives us the following checklist: | ||
- [ ] Was at least one container in the Pod terminated due to being OOM | ||
(`OOMKilled`)? | ||
- [ ] Is at least one container in the Pod currently waiting due to being in | ||
`CrashLoopBackOff`? | ||
|
||
This should make sure to not accidentally disrupt deployments as they might | ||
still heal to a point where eviction then might be possible. | ||
|
||
Suggested implementation is present in [PR | ||
4898](https://github.com/kubernetes/autoscaler/pull/4898). | ||
|
||
### Test Plan | ||
|
||
Add unit tests that cover the new code paths. | ||
|
||
|
||
## Implementation History | ||
|
||
- 2022-05-19: initial version | ||
|
||
|
||
## Alternatives | ||
|
||
### Update the eviction API | ||
|
||
Instead of implementing this change on the client side, the VPA in this case, | ||
it could be implemented on the API side. This would have the advantage that it | ||
would work for all clients. On the other hand this would introduce breaking | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why doing this change in PDB would be a breaking change? I think you could just add a new filed, default being the current behavior and it would be fine. |
||
behaviour and most likely would result in a new api version. | ||
|
||
Also according to some discussions the general stance seems to be: | ||
If you don't like the drain/evict behaviour, just use delete. | ||
(https://github.com/kubernetes/kubernetes/issues/72320, | ||
https://github.com/kubernetes/kubernetes/pull/105296) |
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: extra empty line (also a few times later)