Re-write parts of AEP-4016 (VPA with In-Place Updates) to align with the new In-Place Updates KEP changes #7813

raywainman · 2025-02-07T16:30:05Z

What type of PR is this?

/kind documentation
/kind feature

What this PR does / why we need it:

Many changes have happened to the In-Place Updates KEP since 2023, we need to refresh AEP-4016 to align with the new changes so that we can proceed with implementation.

Which issue(s) this PR fixes:

Part of #7673

Does this PR introduce a user-facing change?

NONE - internal documentation only

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

All information linked in the AEP.

k8s-ci-robot · 2025-02-07T16:30:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

raywainman · 2025-02-07T16:33:00Z

/assign maxcao13

k8s-ci-robot · 2025-02-07T16:33:03Z

@raywainman: GitHub didn't allow me to assign the following users: maxcao13.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign maxcao13

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.

raywainman · 2025-02-07T16:33:25Z

cc @maxcao13

raywainman · 2025-02-07T16:39:46Z

@maxcao13 We can consider cutting the "disruption-less" updates as part of our initial MVP. This essentially means that we have the current Auto mode + in-place updates.

Would love your thoughts :)

raywainman · 2025-02-07T16:40:00Z

/assign @laoj2

k8s-ci-robot · 2025-02-07T16:40:03Z

@raywainman: GitHub didn't allow me to assign the following users: laoj2.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @laoj2

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.

raywainman · 2025-02-07T19:19:45Z

cc @jpawelczak
cc @laoj2

maxcao13

Thanks for writing this up! Some comments and questions.

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

maxcao13 · 2025-02-07T18:58:20Z

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

+In the `InPlaceOrRecreate` modes VPA updater will attempt to apply updates that require container
+restart in place. It will update them under the same conditions that would trigger update with
+`Recreate` mode. That is it will apply disruptive updates if:


I'd say the updater should only attempt to apply these "disruptive" updates if the container restart resize policy is RestartContainer. VPA shouldn't actively try to do this otherwise. At least that's what I understood from the previous AEP description.

Do you mean: if the container restart resize policy is RestartContainer, instead?

Yes! My mistake, I've edited the original comment,

As discussed on Slack, I don't think VPA needs to care about the resize policy, I think it should simply attempt the resize operation if the user set it that way and let the rest of the Kubernetes machinery take care of actually applying the update.

Does that sound OK?

I added a small section about the interaction between VPA and the ResizePolicy setting, hopefully that clarifies things a bit?

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

laoj2 · 2025-02-07T20:20:08Z

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

+In the `InPlaceOrRecreate` modes VPA updater will attempt to apply updates that require container
+restart in place. It will update them under the same conditions that would trigger update with
+`Recreate` mode. That is it will apply disruptive updates if:


Do you mean: if the container restart resize policy is RestartContainer, instead?

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

adrianmoisey · 2025-02-08T13:06:01Z

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

+### 1. Applying Updates During Pod Admission
+
+For VPAs using the new `InPlaceOrRecreate` mode, the VPA Admission Controller will apply updates to
+starting pods just as it does for VPAs in `Initial`, `Auto`, and `Recreate` modes.


I'm not entirely sure if this AEP is the correct place to make this comment, but the promise of Auto was that it would one day support in-place, see https://github.com/kubernetes/autoscaler/blob/78c8173b979316f892327022d53369760b000210/vertical-pod-autoscaler/docs/api.md#updatemode

We've been promising users that if they use Auto today, one day in the future the VPA will start resizing their pods for them without recreating them.
It seems that this may not be the case since the container runtime can't guarantee it.

Do we need to start thinking about what to do with Auto in the future?

Again, I'm unsure if this really fits into the context of this AEP, but I thought I'd bring it up.

My 2 cents on this are that, switching Auto to be InPlace or some variation of InPlace would be a breaking change that should be in something like VPA 2.0. It may not "break" someone's configuration, but the change probably won't be the same expected behaviour in VPA 0.x and 1.x when the updater was freely evicting pods in Auto mode. Although, what I'm talking about is probably a discussion for the future timeline of VPA in general.

Changing the behavior of Auto, good idea. Major version bump to accompany that, also a good idea.

With the discussion around changing the behavior of auto, we're talking about a future where in-place vertical scaling is GA and not under a feature gate, right? IMHO it would feel a bit weird to make the default behavior of VPA depend on a beta feature gate.

With the discussion around changing the behavior of auto, we're talking about a future where in-place vertical scaling is GA and not under a feature gate, right?

Correct. Since the work on in-place is starting to happen now, and we are potentially diverging on the promise given to users, it seems appropriate to discuss Auto now, just so it's considered in our discussions as we move forward with the in-place rollout.

Specified in the AEP that the intention is that this eventually graduates into the Auto mode.

raywainman · 2025-02-19T20:13:07Z

Thanks all! I've applied changes based on the feedback and some discussions we've on Slack.

Would love another look when folks have some time, thanks again!

voelzmo

Looks great! Not instantly approving to give others a chance for a second look.

adrianmoisey · 2025-02-20T08:25:30Z

/hold

EDIT: Oh, hold is already on this PR

omerap12

Looks great! Thanks @raywainman!

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

adrianmoisey · 2025-02-20T11:16:21Z

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

+* If `CanEvict` is false.
+* If any of the `EvictionRequirements` on the VPA are not true.
+
+These additional resizes can be attempted because the eviction fallback would fail anyway.


Just want to clarify this...
So an in-place update will be attempted, if that in-place fails, the VPA will not attempt an eviction?
Almost as if it was InPlaceOrNotAtAll ?

It will attempt an eviction but the eviction won't go through because the actual condition will prevent it from happening. Let me clarify that in the text.

adrianmoisey · 2025-02-20T11:16:58Z

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

+These additional resizes can be attempted because the eviction fallback would fail anyway.
+
+The VPA updater will evict a pod to actuate a recommendation if it attempted to apply the
+recommendation in place and failed.


These two sentences seem to conflict with each other

Clarified.

The idea here is that we can be slightly more relaxed with in-place resizes and still allow VPA to proceed with eviction when the operation fails. The eviction would get blocked anyway since these conditions themselves prevent eviction from happening. Hopefully that makes sense.

adrianmoisey · 2025-02-20T11:18:32Z

The content /lgtm

I left some mostly minor comments, and one clarification on wording

laoj2 · 2025-02-20T15:03:31Z

/lgtm

Looks great, thanks @raywainman!

k8s-ci-robot · 2025-02-20T15:03:37Z

@laoj2: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Looks great, thanks @raywainman!

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.

maxcao13

This looks great and clears up much of my concerns with the previous iteration. Thanks so much @raywainman. I think we'll be able to close #7722 from this too.

raywainman · 2025-02-20T20:58:01Z

cc @tallclair

adrianmoisey · 2025-02-21T11:23:03Z

/lgtm

Thank you!

omerap12 · 2025-02-21T11:29:58Z

/lgtm for me too

k8s-ci-robot · 2025-02-21T11:30:27Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

raywainman · 2025-02-21T14:46:29Z

Thanks all! Will merge this now. We can make further updates in new PRs as needed.

raywainman · 2025-02-21T14:46:35Z

/unhold

Re-write parts of AEP-4016 to align with the new KEP changes

39a5851

k8s-ci-robot requested review from omerap12 and voelzmo February 7, 2025 16:30

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 Feb 7, 2025

raywainman marked this pull request as ready for review February 7, 2025 16:32

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2025

k8s-ci-robot requested review from jbartosik and kgolab February 7, 2025 16:32

raywainman mentioned this pull request Feb 7, 2025

Questions on AEP-4016: Support for in place updates in VPA #7722

Open

maxcao13 reviewed Feb 7, 2025

View reviewed changes

laoj2 reviewed Feb 7, 2025

View reviewed changes

Small edits based on comments

10e2339

adrianmoisey reviewed Feb 8, 2025

View reviewed changes

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

adrianmoisey reviewed Feb 8, 2025

View reviewed changes

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

adrianmoisey reviewed Feb 8, 2025

View reviewed changes

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

adrianmoisey reviewed Feb 8, 2025

View reviewed changes

Edits to address comments from sftim

1969f47

fix typo

4ea61cb

voelzmo approved these changes Feb 20, 2025

View reviewed changes

omerap12 reviewed Feb 20, 2025

View reviewed changes

adrianmoisey reviewed Feb 20, 2025

View reviewed changes

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

adrianmoisey reviewed Feb 20, 2025

View reviewed changes

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

adrianmoisey reviewed Feb 20, 2025

View reviewed changes

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

adrianmoisey reviewed Feb 20, 2025

View reviewed changes

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

adrianmoisey reviewed Feb 20, 2025

View reviewed changes

maxcao13 reviewed Feb 20, 2025

View reviewed changes

raywainman added 3 commits February 20, 2025 20:02

URL and formatting updates

6214833

more anchor fixes

047e6ad

added clarification on eviction after in-place failure

a4d2d6c

k8s-ci-robot assigned adrianmoisey Feb 21, 2025

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

omerap12 approved these changes Feb 21, 2025

View reviewed changes

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

k8s-ci-robot merged commit 0a34bf5 into kubernetes:master Feb 21, 2025
7 checks passed

Re-write parts of AEP-4016 (VPA with In-Place Updates) to align with the new In-Place Updates KEP changes #7813

Re-write parts of AEP-4016 (VPA with In-Place Updates) to align with the new In-Place Updates KEP changes #7813

Conversation

raywainman commented Feb 7, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Feb 7, 2025

raywainman commented Feb 7, 2025

k8s-ci-robot commented Feb 7, 2025

raywainman commented Feb 7, 2025

raywainman commented Feb 7, 2025

raywainman commented Feb 7, 2025

k8s-ci-robot commented Feb 7, 2025

raywainman commented Feb 7, 2025

maxcao13 left a comment

Choose a reason for hiding this comment

maxcao13 Feb 7, 2025 • edited Loading

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

Choose a reason for hiding this comment

adrianmoisey Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raywainman commented Feb 19, 2025

voelzmo left a comment

Choose a reason for hiding this comment

adrianmoisey commented Feb 20, 2025 • edited Loading

omerap12 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

adrianmoisey commented Feb 20, 2025

laoj2 commented Feb 20, 2025

k8s-ci-robot commented Feb 20, 2025

maxcao13 left a comment

Choose a reason for hiding this comment

raywainman commented Feb 20, 2025

adrianmoisey commented Feb 21, 2025

omerap12 commented Feb 21, 2025

k8s-ci-robot commented Feb 21, 2025

raywainman commented Feb 21, 2025

raywainman commented Feb 21, 2025

maxcao13 Feb 7, 2025 •

edited

Loading

adrianmoisey Feb 12, 2025 •

edited

Loading

adrianmoisey commented Feb 20, 2025 •

edited

Loading