autoscaler
autoscaler copied to clipboard
kubernetes
Implement KEP-4831: Control VPA eviction behavior based on scaling direction and resource
Which component this PR applies to?
vertical-pod-autoscaler
What type of PR is this?
/kind feature /kind api-change
What this PR does / why we need it
Implements KEP-4831: Control VPA eviction behavior based on scaling direction and resource
Which issue(s) this PR fixes:
Implements #4831
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
While looking for the best place to put this into the Updater, I found https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/pkg/updater/priority/pod_eviction_admission.go which was pulled in years ago with https://github.com/kubernetes/autoscaler/pull/1002 but apparently never used afterwards. Beata (after merging) even commented that the default implementation isn't even used: https://github.com/kubernetes/autoscaler/pull/1002#pullrequestreview-130869322
I guess this is probably a good place to implement the filtering we want to do based on the comparison of target and requests and if no restrictions are given, just use the already implemented noopPodEvictionAdmission?
There's also https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go but that seems to be concerned with limiting eviction based on minimum available replicas and ensuring this also works for Jobs and other Pod owners in the same way.
@jbartosik WDYT, should I just make use of the PodEvictionAdmission linked above? From a flow perspective, this seems to be a good fit, as it is used in the priority calculator, where Pod requests and target recommendation are compared to figure out if a Pod should be evicted and in which order.
While looking for the best place to put this into the Updater, I found https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/pkg/updater/priority/pod_eviction_admission.go which was pulled in years ago with #1002 but apparently never used afterwards. Beata (after merging) even commented that the default implementation isn't even used: #1002 (review)
I guess this is probably a good place to implement the filtering we want to do based on the comparison of
targetandrequestsand if no restrictions are given, just use the already implementednoopPodEvictionAdmission?
Yes, adding a new type that implements PodEvictionAdmission and passing it to Updater seems to be the best way to implement this KEP.
I'd implement it so when there are no restrictions it just allows the pod
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: voelzmo
Once this PR has been reviewed and has the lgtm label, please assign kgolab for approval by writing /assign @kgolab in a comment. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
This PR may require API review.
If so, when the changes are ready, complete the pre-review checklist and request an API review.
Status of requested reviews is tracked in the API Review project.
@jbartosik I've added an implementation PR using this API in #5599, so this API change is good to be reviewed from my side. Can you please take a look? Thanks!
Let's decide if we want to change the API for in-place updates before merging this (https://github.com/kubernetes/autoscaler/pull/5754)
Force-pushed an update:
- rebased on current master
- removed the
OrEqualChangeRequirements as discussed in https://github.com/kubernetes/autoscaler/pull/5754#issuecomment-1647577540
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: mwielgus, voelzmo
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~vertical-pod-autoscaler/OWNERS~~ [mwielgus]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment