API for KEP-4831: Control VPA eviction behavior based on scaling direction and resource

[voelzmo]

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:

API for #4831
Implementation is done in #5599

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[voelzmo]

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 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.

[jbartosik]

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 target and requests and if no restrictions are given, just use the already implemented noopPodEvictionAdmission?

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

[k8s-triage-robot]

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.

[voelzmo]

@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!

[voelzmo]

@jbartosik Can you please take a look? Thanks!

[voelzmo]

@jbartosik Can you try to give this a review, please? Thanks!

[jbartosik]

Since in-place pod updates are now finally a thing (alpha but I hope for beta soon) I'd like to make sure this works well with support for in place updates.

I can see two approaches:

• have one "actuationRequirements" that affect both in-place and eviction actuation
• have separate controls for "evictionRequirements" and "inPlaceUpdateRequirements"

I lean towards single "actuationRequirements" because it's simpler and I don't see how I'd use separate controls for eviction and in place update. What do you think?

[jbartosik]

This looks like #5178; Can you rebase this PR on top of #5178 . It will make review a bit easier

[jbartosik]

I sent #5754

[jbartosik]

Let's decide if we want to change the API for in-place updates before merging this (#5754)

[voelzmo]

Force-pushed an update:

• rebased on current master
• removed the OrEqual ChangeRequirements as discussed in Adjust Eviction Control AEP for in-place updates #5754 (comment)

[jbartosik]

Looks good to me

@mwielgus @gjtempleton could you take a look please

[mwielgus]

/lgtm
/approve

[k8s-ci-robot]

[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

Needs approval from an approver in each of these files:

• 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