Read containerStatus.resources to calculate container requests and limits when InPlaceVerticalScaling feature is enabled

[laoj2]

Which component are you using?:

vertical-pod-autoscaler

/area vertical-pod-autoscaler

Is your feature request designed to solve a problem? If so describe the problem this feature should solve.:

When In-Place Update of Pod Resources feature is enabled (see KEP for details):

• Pod.Spec.Containers[i].Resources becomes purely a declaration, denoting the desired state of Pod resources.
• Pod.Status.ContainerStatuses[i].Resources (new field, type v1.ResourceRequirements) shows the actual resources held by the Pod and its Containers for running containers, and the allocated resources for non-running containers. (Same applies to initContainers).

We need to modify VPA's code so that when InPlaceVerticalScaling is enabled in a cluster, the resource requests and limits should be read from the container status field (Pod.Status.ContainerStatuses[i].Resources). Otherwise, we should fallback to the resource requests defined in the pod spec (Pod.Spec.Containers[i].Resources).

[bernot-dev]

Makes sense to me. Is this a "help wanted" issue, or do you plan to put up a PR?

[omerap12]

Thanks for this!
/triage accepted

[laoj2]

Makes sense to me. Is this a "help wanted" issue, or do you plan to put up a PR?

I plan to put a few PRs to fix it. Thanks!

[omerap12]

/assign @laoj2

[voelzmo]

@laoj2 Thanks for the issue, this really is an interesting problem!

Can you elaborate a bit in which cases the values in Pod.Spec.Containers[i].Resources differ from Pod.Status.ContainerStatuses[i].Resources? My understanding is that this would e.g. be the case when an in-place update failed to be executed for some reason, or it takes "longer than expected", for some definition of "longer" and "expected". Is there any other scenario where this might happen?

If that's the only case, my understanding is that we should have this covered (by checking the conditions PodResizeInProgress and PodResizePending, as defined in https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler/enhancements/4016-in-place-updates-support#in-place-updates) and we then should fall back to evicting the Pod.

I'm not sure if there is any SLA on how long an update can be in PodResizeInProgress: true, but if it can take more than 1 minute, we might not want to read the resources from Pod.Status.ContainerStatuses[i].Resources, but instead trust that the update will either happen eventually or our fallback-to-eviction logic will pick this up when too much time has passed.

[laoj2]

Thanks, @voelzmo. These are very good points. And yes. That matches my understanding as well: Pod.Spec.Containers[i].Resources should differ from Pod.Status.ContainerStatuses[i].Resources while the resize request is in progress or has failed. I think the new InPlaceOrRecreate mode should handle VPA-triggered resizes well in most cases as well. But it may not handle non-VPA-triggered resizes well. For example:

Scenario 1:

• User manually resizes a container in-place
• Resize takes "too long" for some reason (e.g., not enough resources in the node)
• Workload also has a VPA in Recreate mode configured on the same workload. VPA will only see the "patched" resource values, not the real resources for the duration of the resize.

Scenario 2:
A change to Pod.Spec.Containers[i].Resources triggers an update pod event, so our informer handlers would catch the change quickly enough, potentially triggering unwanted side effects? Like the OOM observer:

autoscaler/vertical-pod-autoscaler/pkg/recommender/input/oom/observer.go

Lines 150 to 159 in 27d00f4

	for _, containerStatus := range newPod.Status.ContainerStatuses {
	if containerStatus.RestartCount > 0 &&
	containerStatus.LastTerminationState.Terminated != nil &&
	containerStatus.LastTerminationState.Terminated.Reason == "OOMKilled" {
	oldStatus := findStatus(containerStatus.Name, oldPod.Status.ContainerStatuses)
	if oldStatus != nil && containerStatus.RestartCount > oldStatus.RestartCount {
	oldSpec := findSpec(containerStatus.Name, oldPod.Spec.Containers)
	if oldSpec != nil {
	memory := oldSpec.Resources.Requests[apiv1.ResourceMemory]

(Chances of this happening are too small, though? Given all conditions that need to be true to trigger this code path).

---

I'm not completely familiar with the code, so would love some input here too if you know this off the top of your head: even for VPA-triggered resizes (https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler/enhancements/4016-in-place-updates-support#in-place-updates), would VPA tolerate these periods of incorrect resource requests gracefully?

[voelzmo]

I think in those cases where an update is already in progress, VPA shouldn't look at the status, but rather consider the soon-to-be set requests. This might be a moot point, though, probably there would be some early exit for the case when the PodResizeInProgress condition is true and it isn't considered to take "too long".

The second case is interesting, I didn't think about those cases. For those scenarios I think it definitely makes sense to look at the actual requests from the status rather than the requests in the spec.

Regarding

even for VPA-triggered resizes, would VPA tolerate these periods of incorrect resource requests gracefully?

Can you help me understand which scenario you're thinking about here? Like I wrote at the beginning of this comment: I think as soon as the condition PodResizeInProgress is true and a time limit hasn't been reached, VPA should just skip this Pod. If the limit has been reached, just fall back to eviction.

[laoj2]

Can you help me understand which scenario you're thinking about here? Like I wrote at the beginning of this comment: I think as soon as the condition PodResizeInProgress is true and a time limit hasn't been reached, VPA should just skip this Pod. If the limit has been reached, just fall back to eviction.

Thanks for the insights! The way I'm thinking about this is that even when VPA falls back to Recreate mode, due to a failed resize or a resize that's taking too long, it still needs to read from the "actual" resources, so that the fallback works correctly.

The same idea applies for failed/long resizes of manually-resized pods for VPAs in Recreate/Auto mode.