From 45a6608ec4befdf92bb41e2807b60ee0993cf33f Mon Sep 17 00:00:00 2001
From: Christoph Manns <christoph.manns@ionos.com>
Date: Fri, 20 May 2022 15:35:06 +0200
Subject: [PATCH 1/5] [VPA] Enhancement proposal: Delete OOM Pods
---
.../4902-delete-oom-pods/README.md | 75 +++++++++++++++++++
1 file changed, 75 insertions(+)
create mode 100644 vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
diff --git a/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
new file mode 100644
index 000000000000..0fa06410ea2c
--- /dev/null
+++ b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
@@ -0,0 +1,75 @@
+# 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.
+
+## 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 to enable deletion of pods
+- Secondary: configurable deletion threshold to tune the deletion behaviour
+
+### Non-Goals
+
+- Get rid of or work around the existing eviction behaviour
+
+## Proposal
+
+The proposal is to add `--experimental-deletion` to the VPA to enable deletion
+of pods. Currently only as an experimental, or beta feature.
+To add a bit of configuration an additional an additional flag,
+`--experimental-deletion-threshold`, should be addedd.
+
+## Design Details
+
+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
+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)
From 58b19eb005733c2c83994fef8ec5cdd6cddaa08c Mon Sep 17 00:00:00 2001
From: Christoph Manns <2413490+RuriRyan@users.noreply.github.com>
Date: Fri, 27 May 2022 09:42:35 +0200
Subject: [PATCH 2/5] Update
vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
Co-authored-by: Marco Voelz <voelzmo@users.noreply.github.com>
---
.../enhancements/4902-delete-oom-pods/README.md | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
index 0fa06410ea2c..e9dc9e017971 100644
--- a/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
+++ b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
@@ -44,7 +44,7 @@ failing deployment, if updated/increased limits would solve the problem.
The proposal is to add `--experimental-deletion` to the VPA to enable deletion
of pods. Currently only as an experimental, or beta feature.
-To add a bit of configuration an additional an additional flag,
+To add a bit of configuration an additional flag,
`--experimental-deletion-threshold`, should be addedd.
## Design Details
From 8a09276e4aa11067f66c4d99aec458a4fd650851 Mon Sep 17 00:00:00 2001
From: Christoph Manns <christoph.manns@ionos.com>
Date: Fri, 27 May 2022 09:50:10 +0200
Subject: [PATCH 3/5] Rename flags & further explain the threshold flag
---
.../enhancements/4902-delete-oom-pods/README.md | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
index e9dc9e017971..df5253dcfa5d 100644
--- a/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
+++ b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
@@ -42,10 +42,14 @@ failing deployment, if updated/increased limits would solve the problem.
## Proposal
-The proposal is to add `--experimental-deletion` to the VPA to enable deletion
-of pods. Currently only as an experimental, or beta feature.
+The proposal is to add `--delete-on-eviction-error` to the VPA to enable
+deletion of pods.
+
To add a bit of configuration an additional flag,
-`--experimental-deletion-threshold`, should be addedd.
+`--delete-on-eviction-error-threshold`, should be addedd. This value compared
+to the amount of restarts a pod has gone through. The deletion wil only be
+allowed if the amount of restarts exceeds the threshold. This is to further
+ensure that only pods get deleted that are consistently crashing.
## Design Details
From d2859eec964a9605b33314ef6aac58d2ec28018c Mon Sep 17 00:00:00 2001
From: Christoph Manns <christoph.manns@ionos.com>
Date: Mon, 20 Jun 2022 16:41:55 +0200
Subject: [PATCH 4/5] extend proposal, add design details
---
.../4902-delete-oom-pods/README.md | 39 ++++++++++++++-----
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
index df5253dcfa5d..4dfe90eaa645 100644
--- a/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
+++ b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
@@ -13,6 +13,7 @@
- [Update the eviction API](#update-the-eviction-api)
<!-- /toc -->
+
## Summary
The default behaviour of VPA Updater is to evict Pods when new resource
@@ -26,6 +27,7 @@ any further disruptions.
This proposal addresses the problem by allowing users to enable the deletion of
pods as a backup if the eviction fails.
+
## Motivation
The motivation behind the change is to give VPA users a way to recover a
@@ -33,26 +35,43 @@ failing deployment, if updated/increased limits would solve the problem.
### Goals
-- Main: allow cluster administrators to enable deletion of pods
-- Secondary: configurable deletion threshold to tune the deletion behaviour
+- 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 `--delete-on-eviction-error` to the VPA to enable
-deletion of pods.
+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
+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.
-To add a bit of configuration an additional flag,
-`--delete-on-eviction-error-threshold`, should be addedd. This value compared
-to the amount of restarts a pod has gone through. The deletion wil only be
-allowed if the amount of restarts exceeds the threshold. This is to further
-ensure that only pods get deleted that are consistently crashing.
## 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).
@@ -60,10 +79,12 @@ Suggested implementation is present in [PR
Add unit tests that cover the new code paths.
+
## Implementation History
- 2022-05-19: initial version
+
## Alternatives
### Update the eviction API
From 1711a4bb1527076c8dc33aeaa27b546474ad3441 Mon Sep 17 00:00:00 2001
From: Christoph Manns <christoph.manns@ionos.com>
Date: Thu, 21 Jul 2022 15:50:37 +0200
Subject: [PATCH 5/5] further improvements
---
.../4902-delete-oom-pods/README.md | 25 +++++++++++--------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
index 4dfe90eaa645..691383ab05a2 100644
--- a/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
+++ b/vertical-pod-autoscaler/enhancements/4902-delete-oom-pods/README.md
@@ -13,7 +13,6 @@
- [Update the eviction API](#update-the-eviction-api)
<!-- /toc -->
-
## Summary
The default behaviour of VPA Updater is to evict Pods when new resource
@@ -25,8 +24,7 @@ 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.
-
+OOMing pods as a backup if the eviction fails.
## Motivation
@@ -41,17 +39,16 @@ failing deployment, if updated/increased limits would solve the problem.
- 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.
+A new global flag (`--delete-ooming-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
-global flag. When unset the value of the flag is taken. This allows cluster
+(`Spec.UpdatePolicy.DeleteOomingOnEvictionError `) which takes precedence to
+the 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.
@@ -59,7 +56,6 @@ 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
@@ -69,6 +65,10 @@ checks will occur. Which gives us the following checklist:
- [ ] Is at least one container in the Pod currently waiting due to being in
`CrashLoopBackOff`?
+Additionally deletion should only occur when an OOM event was recorded and if
+it is planned to actually increase the memory limit. Those information are
+already present in the updater, they just need to be made available.
+
This should make sure to not accidentally disrupt deployments as they might
still heal to a point where eviction then might be possible.
@@ -84,7 +84,6 @@ Add unit tests that cover the new code paths.
- 2022-05-19: initial version
-
## Alternatives
### Update the eviction API
@@ -94,6 +93,12 @@ 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
behaviour and most likely would result in a new api version.
+Another possibility would be to allow the eviction of disrupted workloads, but
+this might be hard to decide for the API.
+It would need to allow the eviction if the container is currently not running.
+This could be introduced as an additional flag in the PDB, to allow this
+behaviour.
+
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,