Deprecate descheduler.alpha.kubernetes.io/evict in favor of descheduler.beta.kubernetes.io/evict with Boolean logic #1673
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
Hi @tiraboschi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
tiraboschi
force-pushed
the
evict_bool
branch
3 times, most recently
from
efaeeaa to
ad6b457
Compare
| return found | ||
| // HaveEvictAnnotation checks if the pod have evict annotation and its value | ||
| func HaveEvictAnnotation(pod *v1.Pod) (bool, bool) { | ||
| svalue, found := pod.ObjectMeta.Annotations[evictPodAnnotationKey] |
There was a problem hiding this comment.
thoughts on using strconv.ParseBool?
There was a problem hiding this comment.
I think it depends on how much we want to push in terms of backward compatibility: the current code is checking only the presence of the annotation.
strconv.ParseBool will return a syntaxError for values (like an empty string) that fails to be cleanly converted to a Bool. Using strconv.ParseBool and catching the error to consider it as a True to mimic backward compatibility is also an option.
There was a problem hiding this comment.
Shouldn't we consider "graduating" this feature to be descheduler.beta.kubernetes.io/evict and leave the alpha as is ?
There was a problem hiding this comment.
Yes, this is also a great suggestion.
|
/retest |
2 similar comments
|
/retest |
|
/retest |
tiraboschi
force-pushed
the
evict_bool
branch
2 times, most recently
from
7cb9d67 to
674df88
Compare
tiraboschi
changed the title
tiraboschi
force-pushed
the
evict_bool
branch
2 times, most recently
from
66531b7 to
e1155ac
Compare
k8s-ci-robot
added
the
needs-rebase
…er.beta.kubernetes.io/evict with Boolean logic In the past the descheduler was only checking the presence of descheduler.alpha.kubernetes.io/evict annotation to force a pod to be an eviction candidate. descheduler.alpha.kubernetes.io/evict=false was producing the same effect of descheduler.alpha.kubernetes.io/evict=true. If the user wants to exclude a specific pod from the descheduler actions, the suggested solution was to re-create it setting an higher priority. This could potentially work for best effort pods that can be easily recreated but it's not a viable solution for other use cases like VMs (executed in pod) that can be temporarily excluded from the descheduler action without the need to reboot them. Let's deprecate descheduler.alpha.kubernetes.io/evict introducing descheduler.beta.kubernetes.io/evict with proper Boolean logic. Signed-off-by: Simone Tiraboschi <[email protected]>
k8s-ci-robot
removed
the
needs-rebase
In the past the descheduler was only checking the presence of descheduler.alpha.kubernetes.io/evict annotation to force a pod to be an eviction candidate.
descheduler.alpha.kubernetes.io/evict=false was producing the same effect of descheduler.alpha.kubernetes.io/evict=true.
If the user wants to exclude a specific pod from the descheduler actions, the suggested solution was to re-create it
setting an higher priority.
This could potentially work for best effort pods that can be easily recreated but it's not a viable solution for other use
cases like VMs (executed in pod) that can be temporarily excluded from the descheduler action without the need to reboot them.
Let's deprecate descheduler.alpha.kubernetes.io/evict introducing descheduler.beta.kubernetes.io/evict with proper Boolean logic.
Something like this has been already discussed in the past, see for instance:
#422
and the accepted conclusion was to set a custom (possibly non-preempting) priority classes with an higher priority for the pods that we want to exclude form the descheduling.
But I think that we still have a valid use case that is not covered by that alternative.
A priority class can only be set ad pod creation time and never amend while an annotation can be added and removed at any point in time during the lifecycle of a pod.
Now let's image that we have a long living pod (such for instance a VM running inside a pod) and the owner wants to dynamically exclude it from eviction risk only during specific period of time or he simply wants to amed his choice without the need to reboot the VM. Currently we cannot achieve that without recreating the pod while it would be really simple and intuitive with an annotation.
And the Node-pressure Eviction mechanism can still act as a safety net for corner cases where descheduler.alpha.kubernetes.io/evict=false is abused.
Fixes: #1659