AEP-8818: InPlace Update Mode

[omerap12]

What type of PR is this?

/kind documentation

What this PR does / why we need it:

AEP for #8720

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

NONE

[k8s-ci-robot]

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

[omerap12]

/cc @adrianmoisey @maxcao13

[omerap12]

/kind api-review

[k8s-ci-robot]

@omerap12: The label(s) kind/api-review cannot be applied, because the repository doesn't have them.

Details

In response to this:

/kind api-review

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.

[omerap12]

/label kind/api-review

[k8s-ci-robot]

@omerap12: The label(s) /label kind/api-review cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, ci-short, ci-extended, ci-full. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

Details

In response to this:

/label kind/api-review

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.

[omerap12]

/label api-review

[iamzili]

/lgtm

@omerap12 If you have a draft PR that makes sense to review, then let me know Omer.

[k8s-ci-robot]

@iamzili: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

@omerap12 If you have a draft PR that makes sense to review, then let me know Omer.

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]

I'll take a look at this next week @omerap12 sorry for the delay 🥲

[adrianmoisey]

It may be worth linking to the issue/PR that discusses this.
Also, is this technically out of scope of the InPlace mode?
It's a problem we'll face for InPlaceOrRecreate also

[omerap12]

I will link the PR.

Also, is this technically out of scope of the InPlace mode?

I don’t think so - this AEP should define a clear solution for it.

It’s a problem we’ll face for InPlaceOrRecreate as well.

Agreed, but since InPlaceOrRecreate can recreate Pods, we should take a different approach there.

Good catch flagging this. We should adjust it before it’s merged into k/k. SIG Node will likely put this guarded behind a feature gate, but we’ll need to account for that as well (e.g. someone enable this feature gate).

[omerap12]

Added

[adrianmoisey]

It’s a problem we’ll face for InPlaceOrRecreate as well.

Agreed, but since InPlaceOrRecreate can recreate Pods, we should take a different approach there.

Yeah, good point, I guess it needs to handle the error, rather than saving it. May be that can happen in the AEP for the InPlaceOrRecreate mode

[omerap12]

Agree.

[iamzili]

when thinking about the data structure that will store the infeasible attempts, one situation came to mind that I think we haven't discussed yet:

What if we record an infeasible attempt for a pod, and then for some reason, the pod is moved to another node (e.g. due to a node drain or node-pressure eviction), where the required resources suddenly become available? In that case, the updater would not try to apply the previously infeasible attempt to the pod, right?

[adrianmoisey]

What if we record an infeasible attempt for a pod, and then for some reason, the pod is moved to another node (e.g. due to a node drain or node-pressure eviction), where the required resources suddenly become available? In that case, the updater would not try to apply the previously infeasible attempt to the pod, right?

I had thought about this, and had decided that pods can't move nodes. My assumption was that a pod can be destroyed and a new one recreated on a new node, however, may be I'm wrong?

EDIT: oh, what about statefulsets? those names don't really change. May be we need to store the UID rather than the pod?

[iamzili]

EDIT: oh, what about statefulsets? those names don't really change. May be we need to store the UID rather than the pod?

we are fine with statefulsets, as the pod name + namespace together form a unique key. We are using the same approach in the clusterState.

[adrianmoisey]

EDIT: oh, what about statefulsets? those names don't really change. May be we need to store the UID rather than the pod?

we are fine with statefulsets, as the pod name + namespace together form a unique key. We are using the same approach in the clusterState.

Not sure I understand, a Pod named "my-first-statefulset-0" can be recreated and land on a new (larger) node, and still be called "my-first-statefulset-0", in which case we may not resize it, since it's still stored in the map (ie: the problem you described in a previous comment)

[iamzili]

EDIT: oh, what about statefulsets? those names don't really change. May be we need to store the UID rather than the pod?

we are fine with statefulsets, as the pod name + namespace together form a unique key. We are using the same approach in the clusterState.

Not sure I understand, a Pod named "my-first-statefulset-0" can be recreated and land on a new (larger) node, and still be called "my-first-statefulset-0", in which case we may not resize it, since it's still stored in the map (ie: the problem you described in a previous comment)

ah, now I understand what you meant 🙂 Good point! You are right - this issue that I described, and that you also thought about, is really a problem for statefulset pods, since deployment pods get a completely new pod name when they are evicted and scheduled onto a new node.

[omerap12]

We can rely on the pod’s UID, which is guaranteed to be unique.

[adrianmoisey]

/kind api-change

(Is this correct?)

[natasha41575]

all result in waiting—VPA never falls back to eviction

We are thinking about creating a proposal for IPPR-triggered scheduler eviction, so I'd like to confirm my understanding about the Deferred condition handling here. What I am envisioning is something like this:

• VPA has a recommendation that could theoretically fit on the node, but that space is currently being occupied by some lower-priority pods. It initiates an in-place pod resize request (assume InPlaceOnly mode).
• Kubelet marks the resize as Deferred in the pod status.
• The scheduler watches for Deferred resizes, and preempts a lower-priority pod accordingly.
• After the lower-priority pod is removed, kubelet triggers a retry of the deferred resize and it succeeds.

From VPA perspective, does this sound like a use case that the current InPlaceOnly proposal already supports?

cc @tallclair

[omerap12]

Yes. The main point is this with Deferred, we are only waiting (with Infeasible, there is more logic involved e.g. remembering earlier recommendations etc ).

We are thinking about creating a proposal for IPPR-triggered scheduler eviction

Has the KEP been created already? I would like to review it :)

[natasha41575]

Thanks! I will CC you in the KEP.

[adrianmoisey]

Can you also CC me? Thanks!

[soltysh]

@omerap12 and @adrianmoisey asked me to check this from API review pov, and from that perspective this looks good 👍

[soltysh]

Nit: I'd suggest inverting the conditions here (yes, I'm aware this is just a design doc 😉 but I won't likely review the actual PR, so I'm leaving that comment here), but that will make the code easier to understand. Same thing applies to identical check few lines below. Iow:

cr, present := ip.podToReplicaCreatorMap[getPodID(pod)]
if !present {
	return utils.InPlaceDeferred
}
...
singleGroupStats, present := ip.creatorToSingleGroupStatsMap[cr]
if pod.Status.Phase == apiv1.PodPending {
	return utils.InPlaceDeferred
}
if !present {
	return utils.InPlaceDeferred
}
...

[omerap12]

Thanks for the review. fixed :)

[adrianmoisey]

/lgtm

Thanks! This is amazing

[omerap12]

/unhold

[adrianmoisey]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianmoisey, omerap12, soltysh

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/enhancements/OWNERS [adrianmoisey]

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