AEP for support of in-place updates for VPA #5755

jbartosik · 2023-05-11T13:40:11Z

/kind documentation
/kind feature

Add Autoscaling Enhancement Proposal to add basic support for in place updates to VPA.

See also #5754 for changes I propose to eviction control enhancement proposal to work better with in place updates

#4016

@voelzmo @kgolab @pbetkier @wangchen615 please take a look

k8s-triage-robot · 2023-05-11T15:24:23Z

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

Awesome, thanks for getting this Enhancement Proposal rolling! I have a few suggestions for changing the text, but mostly questions around the two UpdateModes and how we're handling errors and the fallback to eviction.

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md

voelzmo · 2023-05-12T12:19:46Z

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md

+For VPAs in `InPlaceOnly` and `InPlaceOrRecreate` modes VPA Admission Controller will apply updates to starting pods,
+like it does for VPAs in `Initial`, `Auto`, and `Recreate` modes.
+
+### Applying Disruption-free Updates


Are we doing something about the case where an in-place update becomes infeasible, because the Pod doesn't fit on this Node anymore? Otherwise it could happen that using in-place updates creates more problems for people's workloads.

We could e.g. react to events on the Pod, which should be generated by kubelet

kubelet will generate Events on the Pod whenever a resize is accepted or rejected

I wasn't sure where/how to find which events should be generated in this case, though?

FWIW I think VPA should react to both Infeasible (this is rather easy) and also to long-lasting Deferred status: "it fits on this node" might require evicting all other Pods and it might take a long time to actually achieve this (IIUC there is no controller which will just do it for VPA).

I'd like to make sure that a long-lasting Deferred status doesn't block VPA from actuating the recommendation to other Pods in the workload.

Does the "VPA updater will consider that the update failed if" part I added answer your question? Or were you thinking about something else (scale down in place but then can't scale up in place to the old size)?

@kgolab @voelzmo if you think I should change the doc please let me know and I'll add changes to #5877

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md

kgolab · 2023-05-16T11:52:04Z

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md

+
+In `InPlaceOrRecreate` mode (but not in `InPlaceOnly` mode) VPA updater will evict pod to actuate a recommendation if it
+attempted to apply the recommendation in place and failed.
+


I'd like to have the order of updater actions explicitly stated.
I expected it would attempt in-place updates first but a) it's only my assumption b) I'm not sure it always would be the case.
I can see at least the following possibilities:

all resources can be updated in-place and the change matches eviction criteria: attempt in-place, evict on error?

some resources can be updated in-place, some not and the change matches eviction criteria: would VPA evict already, w/o attempting in-place update (this seems contradictory to "if necessary execute only partial updates" earlier), does this depend on which resource matches eviction criteria, other?

all or some resources can be updated in-place but the change doesn't match eviction criteria: in-place only?

no possibility for in-place update: go to the existing updater flow.

We also have priority ordering in updater, maybe it's worth adding a sentence how it would be changed?

I'm not sure I understand what you mean by "order of updater actions". Do you mean order of operations in updaters RunOnce?

Or do you mean since now there are up to 3 different things we could attempt for any given pod, in which of them we actually do?

The things we could do:

Apply full update by eviction,

Apply full update in-place, trigger some container restarts,

Apply partial update in-place, don't restart any containers

We definitely need to try 2. before attempting 1. Attempt 1. only if 2. failed.

If both 2. and 3. are an option I think we should do 2. (there's some reason we didn't do the smaller disruption-free update earlier so let's move quickly).

I'm not updating AEP yet because I'm not sure what to put there.

for InPlaceOrRecreate and 3. for InPlaceOnly?

for both InPlaceOrRecreate and InPlaceOnly

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md

kgolab · 2023-05-16T11:57:08Z

@jbartosik , thanks for starting this! I added some comments on top of @voelzmo reviews (thanks!) - nothing major but I think it's worth discussing some finer points to avoid misunderstanding (or getting wrong expectations).

jbartosik · 2023-05-17T15:16:32Z

@kgolab @voelzmo thank you for your comments. I replied to all and I think most are resolved. Please take a look

iakat · 2023-06-11T17:11:14Z

Hi @kgolab @voelzmo, could you please see if another round of reviews is appropriate?

voelzmo

/lgtm

Thanks for addressing all the comments! I've just added a few comments regarding link formatting for your consideration, otherwise this looks good to me!

voelzmo · 2023-06-12T12:15:11Z

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md

+(like applying different recommendations during pod initialization) will be introduced as separate enhancement
+proposals.
+
+[in-place update feature](https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources)


Suggested change

[in-place update feature](https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources)

[in-place update feature]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources

Done in #5877

voelzmo · 2023-06-12T12:15:37Z

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md

+proposals.
+
+[in-place update feature](https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources)
+[available in Kubernetes 1.27.](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#api-change-3)


Suggested change

[available in Kubernetes 1.27.](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#api-change-3)

[available in Kubernetes 1.27.]: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#api-change-3

Done in #5877

voelzmo · 2023-06-12T13:22:05Z

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md

+[`UpdatePriorityCalculator.AddPod`]: https://github.com/kubernetes/autoscaler/blob/114a35961a85efdf3f36859350764e5e2c0c7013/vertical-pod-autoscaler/pkg/updater/priority/update_priority_calculator.go#L81
+[by default 12h]: https://github.com/kubernetes/autoscaler/blob/114a35961a85efdf3f36859350764e5e2c0c7013/vertical-pod-autoscaler/pkg/updater/priority/update_priority_calculator.go#L35
+[by default 10%]: https://github.com/kubernetes/autoscaler/blob/114a35961a85efdf3f36859350764e5e2c0c7013/vertical-pod-autoscaler/pkg/updater/priority/update_priority_calculator.go#L33
+[Outside recommendation range]: https://github.com/kubernetes/autoscaler/blob/114a35961a85efdf3f36859350764e5e2c0c7013/vertical-pod-autoscaler/pkg/updater/priority/priority_processor.go#L73


Suggested change

[Outside recommendation range]: https://github.com/kubernetes/autoscaler/blob/114a35961a85efdf3f36859350764e5e2c0c7013/vertical-pod-autoscaler/pkg/updater/priority/priority_processor.go#L73

[Outside recommended range]: https://github.com/kubernetes/autoscaler/blob/114a35961a85efdf3f36859350764e5e2c0c7013/vertical-pod-autoscaler/pkg/updater/priority/priority_processor.go#L73

Done in #5877

k8s-ci-robot · 2023-06-12T13:27:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbartosik, 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~~ [jbartosik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

iakat · 2023-06-12T13:34:18Z

Thank you all for your work on this 🤗

Excited to try it out!

jbartosik added area/vertical-pod-autoscaler kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels May 11, 2023

k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. labels May 11, 2023

jbartosik mentioned this pull request May 11, 2023

Adjust Eviction Control AEP for in-place updates #5754

Closed

AEP for support of in-place updates for VPA

dd5f6b5

jbartosik force-pushed the vpa-in-place-aep branch from c7ebf6c to dd5f6b5 Compare May 12, 2023 12:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 2023

voelzmo suggested changes May 12, 2023

View reviewed changes

kgolab reviewed May 16, 2023

View reviewed changes

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md Outdated Show resolved Hide resolved

kgolab reviewed May 16, 2023

View reviewed changes

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md Outdated Show resolved Hide resolved

kgolab reviewed May 16, 2023

View reviewed changes

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md Outdated Show resolved Hide resolved

kgolab reviewed May 16, 2023

View reviewed changes

vertical-pod-autoscaler/enhancements/4016-in-place-updates-support/README.md Show resolved Hide resolved

AEP 4016 - initial version - review round 1

22a8e35

voelzmo approved these changes Jun 12, 2023

View reviewed changes

k8s-ci-robot assigned voelzmo Jun 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2023

k8s-ci-robot merged commit 9c3d2e0 into kubernetes:master Jun 12, 2023

jbartosik mentioned this pull request Jun 21, 2023

Apply fixes to in place support VPA AEP #5877

Merged

jbartosik deleted the vpa-in-place-aep branch June 28, 2023 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AEP for support of in-place updates for VPA #5755

AEP for support of in-place updates for VPA #5755

jbartosik commented May 11, 2023

k8s-triage-robot commented May 11, 2023

voelzmo left a comment

voelzmo May 12, 2023

kgolab May 16, 2023

jbartosik May 17, 2023

jbartosik Jun 21, 2023

kgolab May 16, 2023

jbartosik May 17, 2023

pbetkier Jun 5, 2023

jbartosik Jun 21, 2023

kgolab commented May 16, 2023

jbartosik commented May 17, 2023

iakat commented Jun 11, 2023

voelzmo left a comment

voelzmo Jun 12, 2023

jbartosik Jun 21, 2023

voelzmo Jun 12, 2023

jbartosik Jun 21, 2023

voelzmo Jun 12, 2023

jbartosik Jun 21, 2023

k8s-ci-robot commented Jun 12, 2023

iakat commented Jun 12, 2023 •

edited

Loading


		In `InPlaceOrRecreate` mode (but not in `InPlaceOnly` mode) VPA updater will evict pod to actuate a recommendation if it
		attempted to apply the recommendation in place and failed.

	[in-place update feature](https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources)
	[in-place update feature]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources

	[available in Kubernetes 1.27.](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#api-change-3)
	[available in Kubernetes 1.27.]: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#api-change-3

	[Outside recommendation range]: https://github.com/kubernetes/autoscaler/blob/114a35961a85efdf3f36859350764e5e2c0c7013/vertical-pod-autoscaler/pkg/updater/priority/priority_processor.go#L73
	[Outside recommended range]: https://github.com/kubernetes/autoscaler/blob/114a35961a85efdf3f36859350764e5e2c0c7013/vertical-pod-autoscaler/pkg/updater/priority/priority_processor.go#L73

AEP for support of in-place updates for VPA #5755

AEP for support of in-place updates for VPA #5755

Conversation

jbartosik commented May 11, 2023

k8s-triage-robot commented May 11, 2023

voelzmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgolab commented May 16, 2023

jbartosik commented May 17, 2023

iakat commented Jun 11, 2023

voelzmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 12, 2023

iakat commented Jun 12, 2023 • edited Loading

iakat commented Jun 12, 2023 •

edited

Loading