VPA: (InPlaceOrRecreate) Allow updater to actuate InPlaceOrRecreate updates #7962

maxcao13 · 2025-03-22T02:14:02Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Introduces large changes in the updater component to allow InPlaceOrRecreate mode. If the feature gate is enabled and the VPA update mode is InPlaceOrRecreate, the updater will attempt an in place update by first checking a number of preconditions before actuation (e.g., whether we are already in-place resizing,
whether an in-place update may potentially violate disruption(previously eviction) tolerance, etc.). After the preconditions are validated, we send an update signal to the InPlacePodVerticalScaling API with the recommendation, which may or may not fail. Failures are handled in subsequent updater loops.

We need this in order to actually actuate in place updates.

Which issue(s) this PR fixes:

Part of AEP-4016 (InPlaceVerticalScaling/InPlaceOrRecreate)
This PR is part of the larger feature PR in #7673

Depends on:

Note that this PR is merging into the in-place-updates feature branch, which will be merged when this feature is all reviewed and ready.

Special notes for your reviewer:

As for implementation details, patchCalculators have been re-used from the admission-controllers code for the updater in order to calculate recommendations for the updater to actuate. InPlace logic has been mostly stuffed in the eviction package for now because of similarities and ease (user-initated API calls eviction vs. in-place; both cause disruption), maybe we also need a "disruption limiter". It may or may not be useful to refactor this later.

There are some TODOs littered throughout the code. I've limited as many TODOS as possible, but I'm using the rest of the TODOs as review markers to draw attention to parts of the discussion that need community discussion. Otherwise, they are items that should be fixed after 1.33 1287 KEP changes are released, or other future work.

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

[AEP-4016]: https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler/enhancements/4016-in-place-updates-support

maxcao13 · 2025-03-22T02:16:37Z

/hold
unit tests are failing bc we need #7934 to merge

upodroid · 2025-03-22T13:02:50Z

/test pull-kubernetes-e2e-autoscaling-vpa-full

vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go

vertical-pod-autoscaler/pkg/utils/metrics/updater/updater.go

maxcao13 · 2025-03-24T19:26:49Z

FYI:

VPA: fixup vpa-process-yaml.sh script: commit fixes up the script such that it must be a Deployment manifest that triggers the feature-gate adding, and checks if the args field exists first, before adding it if needed.
VPA: Update vpa-rbac.yaml for allowing in place resize requests: commit just adds some rbac I forgot to port over to this PR
fixup! VPA: Allow updater to actuate InPlaceOrRecreate updates: simple refactoring and logic reordering

Verify tests are failing b/c I rebased on the admission PR, which had the same issue which I fixed, but I didn't fix the issue in this PR. Once that's merged, I'll rebase again.

We might want to add a few more that are combined disruption counters, e.g. in-place + eviction totals, but for now just add some separate counters to keep track of what in-place updates are doing.

raywainman

First pass through. Need to do another pass but thought I'd shoot off some questions to start.

Lots of great work here, thanks a lot @maxcao13!

vertical-pod-autoscaler/deploy/vpa-rbac.yaml

vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go

vertical-pod-autoscaler/pkg/updater/logic/updater.go

vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go

vertical-pod-autoscaler/deploy/vpa-rbac.yaml

vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go

vertical-pod-autoscaler/pkg/updater/logic/updater.go

vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go

maxcao13 · 2025-04-07T01:00:17Z

I refactored a lot of the in-place stuff out of the eviction code and separated them in 5a3f674.

I need to write more unit tests for the inplaceRestriction stuff but wanted to push this for now for review.

maxcao13 · 2025-04-10T16:59:12Z

Sorry if it looks like the latest change was big, a lot of the changes were moving things around, renaming, lowercasing some fields, and adding/fixing unit tests.

Other than that, I've refactored the tolerance code a bit to address @raywainman's comments, and changed it so that CanEvict take into accoun about the in-place fallback to evict conditions.

I've added some unit tests to confirm the functionality.

Reviewers, please take a look when you can.

This diff is the list of changes since the last commit to now if that helps: https://github.com/kubernetes/autoscaler/compare/93a37e47308ad58275e909bfeaa0347b2ef6b4ba..34d1df06b3b84611e888dbbf988741bb1f3744dd

raywainman

Looking really good. Generally just some nits. Thanks Max!

vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/calculator.go

vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/resource_updates.go

vertical-pod-autoscaler/pkg/utils/annotations/vpa_inplace_update.go

vertical-pod-autoscaler/pkg/updater/main.go

vertical-pod-autoscaler/pkg/updater/logic/updater.go

vertical-pod-autoscaler/pkg/updater/logic/updater_test.go

vertical-pod-autoscaler/pkg/updater/restriction/pods_inplace_restriction.go

vertical-pod-autoscaler/pkg/updater/inplace/inplace_recommendation_provider.go

Introduces large changes in the updater component to allow InPlaceOrRecreate mode. If the feature gate is enabled and the VPA update mode is InPlaceOrRecreate, the updater will attempt an in place update by first checking a number of preconditions before actuation (e.g., if the pod's qosClass would be changed, whether we are already in-place resizing, whether an in-place update may potentially violate disruption(previously eviction) tolerance, etc.). After the preconditions are validated, we send an update signal to the InPlacePodVerticalScaling API with the recommendation, which may or may not fail. Failures are handled in subsequent updater loops. As for implementation details, patchCalculators have been re-used from the admission-controllers code for the updater in order to calculate recommendations for the updater to actuate. InPlace logic has been mostly stuffed in the eviction package for now because of similarities and ease (user-initated API calls eviction vs. in-place; both cause disruption). It may or may not be useful to refactor this later. Signed-off-by: Max Cao <[email protected]>

Signed-off-by: Max Cao <[email protected]>

The script needs to also check if the yaml input is a Deployment, and no longer needs to check for vpa-component names. Signed-off-by: Max Cao <[email protected]>

Signed-off-by: Max Cao <[email protected]>

This commit refactors inplace logic outside of the pods eviction restriction and separates them into their own files. Also this commit adds PatchResourceTarget to calculators to allow them to explictly specify to the caller which resource/subresource they should be patched to. This commit also creates a utils subpackage in order to prevent dependency cycles in the unit tests, and adds various unit tests. Lastly, this commit adds a rateLimiter specifically for limiting inPlaceResize API calls. Signed-off-by: Max Cao <[email protected]>

raywainman · 2025-04-16T02:18:46Z

Overall LGTM. I would LOVE a second pair of eyes on this just given there is a lot happening here. @adrianmoisey, @omerap12, @voelzmo any chance I could get a second look?

Thanks a lot Max for patiently dealing with my waves of comments :)

vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory.go

omerap12

These are just my initial thoughts—planning to do another pass later.

vertical-pod-autoscaler/pkg/updater/inplace/inplace_updated.go

vertical-pod-autoscaler/pkg/updater/logic/updater.go

vertical-pod-autoscaler/pkg/updater/restriction/pods_inplace_restriction.go

Signed-off-by: Max Cao <[email protected]>

maxcao13 · 2025-04-28T20:22:21Z

Is there anything else that needs to be addressed here?
/unhold

omerap12 · 2025-04-29T07:23:32Z

lgtm, thank you! Excellent work, Max :)
The next PR is for the end-to-end tests, right? I can't recall if we already merged it :)

maxcao13 · 2025-04-29T16:12:49Z

Nope, I'll put that PR up now thanks Omer!

(and it should be the last one before the big PR to main)

omerap12 · 2025-04-29T16:14:59Z

Nope, I'll put that PR up now thanks Omer!

(and it should be the last one before the big PR to main)

Great, Thanks!

maxcao13 · 2025-04-29T19:20:01Z

PR is here: #8072

raywainman · 2025-05-05T21:00:18Z

I know there are still some outstanding TODOs here but in the interest of unblocking this I think this is OK to go now. We can iterate from here. Really great work @maxcao13, thanks again.

/lgtm

/approve

k8s-ci-robot · 2025-05-05T21:00:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maxcao13, raywainman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~vertical-pod-autoscaler/OWNERS~~ [raywainman]

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

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/vertical-pod-autoscaler labels Mar 22, 2025

k8s-ci-robot requested review from kwiesmueller and omerap12 March 22, 2025 02:14

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 22, 2025

maxcao13 mentioned this pull request Mar 22, 2025

VPA: Implement in-place updates support #7673

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2025

adrianmoisey reviewed Mar 23, 2025

View reviewed changes

vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go Outdated Show resolved Hide resolved

adrianmoisey reviewed Mar 23, 2025

View reviewed changes

vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go Outdated Show resolved Hide resolved

omerap12 reviewed Mar 24, 2025

View reviewed changes

vertical-pod-autoscaler/pkg/utils/metrics/updater/updater.go Show resolved Hide resolved

maxcao13 force-pushed the in-place-updates-updater branch from 6dd33cc to 7f9b9c4 Compare March 24, 2025 19:09

VPA: Add metrics gauges for in-place updates

85d5681

We might want to add a few more that are combined disruption counters, e.g. in-place + eviction totals, but for now just add some separate counters to keep track of what in-place updates are doing.

maxcao13 force-pushed the in-place-updates-updater branch from 7f9b9c4 to 114091d Compare March 25, 2025 21:18

raywainman reviewed Mar 27, 2025

View reviewed changes

maxcao13 force-pushed the in-place-updates-updater branch from 114091d to 93a37e4 Compare March 27, 2025 22:57

adrianmoisey reviewed Mar 31, 2025

View reviewed changes

vertical-pod-autoscaler/pkg/updater/eviction/pods_eviction_restriction.go Outdated Show resolved Hide resolved

raywainman reviewed Apr 1, 2025

View reviewed changes

maxcao13 force-pushed the in-place-updates-updater branch from e6356b6 to 5a3f674 Compare April 7, 2025 00:54

maxcao13 force-pushed the in-place-updates-updater branch from 9818035 to 34d1df0 Compare April 10, 2025 16:53

raywainman reviewed Apr 11, 2025

View reviewed changes

maxcao13 added 4 commits April 11, 2025 14:07

VPA: Updater in-place updates unit tests

c3c8177

Signed-off-by: Max Cao <[email protected]>

VPA: fixup vpa-process-yaml.sh script

453e600

The script needs to also check if the yaml input is a Deployment, and no longer needs to check for vpa-component names. Signed-off-by: Max Cao <[email protected]>

VPA: Update vpa-rbac.yaml for allowing in place resize requests

afc3ec3

Signed-off-by: Max Cao <[email protected]>

maxcao13 force-pushed the in-place-updates-updater branch from 0cbc46a to ed9358c Compare April 11, 2025 21:18

raywainman reviewed Apr 16, 2025

View reviewed changes

vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory.go Outdated Show resolved Hide resolved

raywainman modified the milestone: vertical-pod-autoscaler-1.4.0 Apr 16, 2025

omerap12 reviewed Apr 16, 2025

View reviewed changes

address raywainman and omerap12 comments

7819a2b

Signed-off-by: Max Cao <[email protected]>

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2025

maxcao13 mentioned this pull request Apr 29, 2025

VPA: [InPlaceOrRecreate] e2e tests #8072

Merged

k8s-ci-robot assigned raywainman May 5, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2025

k8s-ci-robot merged commit 9440674 into kubernetes:in-place-updates May 5, 2025
7 checks passed

maxcao13 deleted the in-place-updates-updater branch May 5, 2025 21:03

This was referenced May 8, 2025

VPA: Implement in-place updates support #8113

Closed

VPA: Implement in-place updates support #8115

Merged

maxcao13 mentioned this pull request Jul 2, 2025

feat(vpa): Improve handling of in-place resource updates against incompatible clusters #8288

Closed

VPA: (InPlaceOrRecreate) Allow updater to actuate InPlaceOrRecreate updates #7962

VPA: (InPlaceOrRecreate) Allow updater to actuate InPlaceOrRecreate updates #7962

Uh oh!

Conversation

maxcao13 commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Note that this PR is merging into the in-place-updates feature branch, which will be merged when this feature is all reviewed and ready.

Special notes for your reviewer:

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

Uh oh!

maxcao13 commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upodroid commented Mar 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maxcao13 commented Mar 24, 2025

Uh oh!

raywainman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maxcao13 commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxcao13 commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raywainman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raywainman commented Apr 16, 2025

Uh oh!

Uh oh!

omerap12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maxcao13 commented Apr 28, 2025

Uh oh!

omerap12 commented Apr 29, 2025

Uh oh!

maxcao13 commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omerap12 commented Apr 29, 2025

maxcao13 commented Mar 22, 2025 •

edited

Loading

maxcao13 commented Mar 22, 2025 •

edited

Loading

maxcao13 commented Apr 7, 2025 •

edited

Loading

maxcao13 commented Apr 10, 2025 •

edited

Loading

maxcao13 commented Apr 29, 2025 •

edited

Loading

maxcao13 commented Apr 29, 2025 •

edited

Loading