AEP-7571: Pod-level resources support in VPA

[iamzili]

What type of PR is this?

/kind documentation
/kind feature
/area vertical-pod-autoscaler

What this PR does / why we need it:

Autoscaling Enhancement Proposal (AEP) for pod-level resources support in VPA.

Related ticket from which this AEP originated: Issue

More details about pod-level resources can be found here:

• KEP-2837
• Kubernetes documentation

I'd love to hear your thoughts on this feature.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: iamzili
Once this PR has been reviewed and has the lgtm label, please assign adrianmoisey for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• vertical-pod-autoscaler/enhancements/OWNERS

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]

Hi @iamzili. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[adrianmoisey]

It may be worth linking the KEP here

[iamzili]

I'm linking the KEP and the official blog post a little further down: here

[adrianmoisey]

Just want to sanity check this a little.

typically by aggregating container recommendations

From what I can tell, the metric that metric-server provides is per-container.

So the idea is to leave the recommender as is, making per-container recommendations based on its per-container metric, and let the updater/admission-controller use an aggregated value for the Pod resources.

Is my understanding here right?

[iamzili]

Partially, since the recommender will calculate the pod-level recommendations (from your comment, it seems that the updater/admission controller would do that). My plan is to continue relying on the current approach for collecting and aggregating container-level metrics, as well as for generating per-container recommendations.

The difference introduced by this AEP is that if a pod-level resources stanza is defined at the workload API level, the recommender will also calculate pod-level recommendations, which are simply the sum of the container recommendations. The pod-level recommendations will be stored in the status.recommendation.podRecommendation stanza of the VPA object (new!).

The updater and the admission controller will read from status.recommendation.podRecommendation (and of course from status.recommendation.containerRecommendations) to perform their actions - the updater will evict pods or perform in-place container-level updates, while the admission controller will modify pod specs on the fly.

[adrianmoisey]

Partially, since the recommender will calculate the pod-level recommendations (from your comment, it seems that the updater/admission controller would do that).

I was just making an assumption. If I'm hearing you right, you want the recommender to create the pod recommendation, and store it in the VPA resource, which makes more sense than my assumption.

The difference introduced by this AEP is that if a pod-level resources stanza is defined at the workload API level, the recommender will also calculate pod-level recommendations, which are simply the sum of the container recommendations. The pod-level recommendations will be stored in the status.recommendation.podRecommendation stanza of the VPA object (new!).

The updater and the admission controller will read from status.recommendation.podRecommendation (and of course from status.recommendation.containerRecommendations) to perform their actions - the updater will evict pods or perform in-place container-level updates, while the admission controller will modify pod specs on the fly.

Makes sense!

[adrianmoisey]

Would it be possible to have an example Go Type here?

[adrianmoisey]

Is this a feature flag to assist with GAing the feature, or is it a flag to enable/disable the feature?

[iamzili]

My intention is to use the flag to enable or disable the feature. In other words, the feature should be disabled by default at first, and once the feature matures, it can be enabled by default starting from a specific VPA version.

Could you please clarify what you mean by using the flag for GAing the feature?

[adrianmoisey]

The normal pattern for Kubernetes is to use a feature gate to introduce a new feature. Normally it works like this across many releases:

1. First release - add a feature gate as alpha - defaulted to off
2. Second release - promote to beta - default to on
3. Third release - promote to GA - locked to on
4. A few releases later (3 I think) - remove feature gate logic completely

This is mostly for the kubernetes components to handle roll forward/back gracefully.
I think the main thing it protects is if a user starts using the feature in the beta mode, if they roll back 1 release, that feature would continue to work (ie: the APIs would be valid) since the logic exists in the alpha mode.

[iamzili]

Thanks for the explanation - I appreciate it! Based on your comment, the feature flags (there will be a new one for each component) will serve both purposes, i.e. GAing and enabling/disabling the feature.

[adrianmoisey]

Right, the point of feature gates in Kubernetes is to eventually remove them. enabling/disabling the feature should be driven by the API

[jackfrancis]

Because this AEP has a dependency on the functionality described in https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/5419-pod-level-resources-in-place-resize, can we restate the language as if https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/5419-pod-level-resources-in-place-resize is already implemented, and then add a note that we won't approve this AEP until post-1.35 (when in-place resizing of pod-level resources has been implemented)?

[adrianmoisey]

I thought that this AEP isn't dependant on that feature, it's calling out that we can't do in-place resizing until that KEP is ready

[omerap12]

Let's remove this section, there is no connection between the current AEP and the in-place feature.
This AEP should focus on pod level resources only.

[iamzili]

as @omerap12 suggested, I removed the parts mentioning the In-Place Pod-Level Resources Resize, and kept only a note stating that we should leverage it once it becomes available

Feel free to resolve the conversation if applicable.

[omerap12]

Really thanks for the hard work here Erik!
To my opinion I think we should choose option 2 as the default (control both pod-level and initially set container-level resources) here.
I left couple of notes throughout the proposal.
Can we please remove the in-place feature from this AEP?
This AEP should focus only on pod-level resource so cons like "Applying both pod-level and container-level recommendations requires eviction because is not yet available" are redundant.

[omerap12]

Thanks for catching that! (I forgot I wrote that TBH ) :)

[omerap12]

My formula should be correct, but what happens if after the normalization of the container[i] resources we get a value which is little/bigger than the minAllowed/maxAllowed?
I thought we can do something like that:

- If adjusted[i] < container.minAllowed[i]: set to minAllowed[i]
- If adjusted[i] > container.maxAllowed[i]: set to maxAllowed[i]

And then we need to re-check pod limits after container policy adjustments ( since it might be bigger ).
If we are still exceeding pod limits - what we wanna do here?
cc @adrianmoisey

Sorry if I wasn't clear enough :)

[iamzili]

An individual container limit can't be larger than the pod-level limit, but the aggregated container-level limits can exceed the pod-level limit - Ref.

So, when a new pod-level recommendation is calculated and the limit is set proportionally at the pod level, we also need to check the container-level limits. If a container-level limit is greater than the pod-level limit, it should be set to the same value as the pod-level limit, and the calculated container-level recommendation should be reduced proportionally as well to maintain the original request to limit ratio (similar to how it works when a LimitRange API object is in place).

[omerap12]

Yup. precisely!

[omerap12]

In order for this AEP to merged this has to be filled ( I know it's a WIP but just a remainder ) :)

[iamzili]

the missing test plan has been addressed

[iamzili]

Really thanks for the hard work here Erik! To my opinion I think we should choose option 2 as the default (control both pod-level and initially set container-level resources) here. I left couple of notes throughout the proposal. Can we please remove the in-place feature from this AEP? This AEP should focus only on pod-level resource so cons like "Applying both pod-level and container-level recommendations requires eviction because is not yet available" are redundant.

I would also prefer option 2 (control both pod-level and initially set container-level resources). BTW when do you think a decision will be made to go with this option? We will need to update the AEP to reflect the chosen approach. Once the decision is final, I also plan to add more details.

Furthermore, why are you suggesting that the pod-level resources in-place resize related parts should be removed from this AEP? Since this AEP focuses on the pod-level resources stanza, how it can be mutated (or not) seems relevant from the VPA's perspective

[omerap12]

Really thanks for the hard work here Erik! To my opinion I think we should choose option 2 as the default (control both pod-level and initially set container-level resources) here. I left couple of notes throughout the proposal. Can we please remove the in-place feature from this AEP? This AEP should focus only on pod-level resource so cons like "Applying both pod-level and container-level recommendations requires eviction because is not yet available" are redundant.

I would also prefer option 2 (control both pod-level and initially set container-level resources). BTW when do you think a decision will be made to go with this option? We will need to update the AEP to reflect the chosen approach. Once the decision is final, I also plan to add more details.

Furthermore, why are you suggesting that the pod-level resources in-place resize related parts should be removed from this AEP? Since this AEP focuses on the pod-level resources stanza, how it can be mutated (or not) seems relevant from the VPA's perspective

I see your point - it’s not completely independent, since once Kubernetes supports in-place updates for pod-level resources, the VPA will likely extend that support as well (similar to what we already do for container-level in-place updates).

But, the main scope of this AEP is to define how we provide recommendations for pod-level resources. The actual application of those recommendations - whether in-place or through eviction - is more of an implementation detail and doesn’t directly affect the design decisions in this proposal.
We can add a short note about the current state of in-place updates for pod-level resources (KEP-5419
) and mention that future VPA enhancements will align once that functionality is available.

[jackfrancis]

/release-note-none

[k8s-ci-robot]

@jackfrancis: you can only set the release note label to release-note-none if the release-note block in the PR body text is empty or "none".

Details

In response to this:

/release-note-none

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.

[jackfrancis]

/ok-to-test

[omerap12]

pinging this again, since VPA support is a blocker for Pod Level Resources GA.
/retest
/cc @adrianmoisey @iamzili

[k8s-ci-robot]

@omerap12: GitHub didn't allow me to request PR reviews from the following users: iamzili.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

pinging this again, since VPA support is a blocker for Pod Level Resources GA.
/retest
/cc @adrianmoisey @iamzili

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.

[iamzili]

FYI – I have finished updating the AEP, it is ready for review. Thanks in advance!

cc @omerap12 @adrianmoisey

[omerap12]

Thanks for working on this! these are my initial thoughts

[omerap12]

So we are not changing anything here, right?
To my understanding, this means we calculate the target recommendation for all containers in a pod and then check if the total is different from the current pod resources (by resourceDiff and more).

Maybe we can remove the long paragraph and just keep this one simple sentence?

[omerap12]

So are we are saying we would not support that? (It's fine with me as long is it being documented).

[iamzili]

yes, at this stage (and this may change in the future if the LimitRanger admission controller updates its behavior for pods with a pod-level resources stanza) we should skip the Pod in both the updater and the admission controller when:

1. the Pod template defines pod-level resources, and
2. the Pod is deployed into a namespace that contains container scoped Limitrange objects

However, I don't agree with the current wording in the AEP, as we cannot simply reject Pod creation. Instead, I propose updating the AEP to state that in this situation the updater and the admission controller will skip managing the Pod and emit a log message explaining why (both components), advising the user to move the Pod to a namespace without container level Limitrange object

[omerap12]

Since Auto is deprecated I would like it not to appear in AEPs and such. let's switch it to whatever.

[iamzili]

this is not the VPA mode you are referring to, it is the mode defined under containerPolicies

[omerap12]

Right (https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go#L252) sorry I was confused.

[iamzili]

but I mention the auto vpa mode under the Goals section

[omerap12]

Can we merge this example with the above example? I am afraid this document becomes to excessive

[omerap12]

Why would the updater will not fallback to eviction? How is it differ from containers when we fallback to eviction?

[iamzili]

I'm not sure I fully understand the comment from above:

• what I'm proposing is that at this line, if err is non-nil, the pod is evicted when InPlaceOrRecreate mode is used - in other words "we fallback to eviction"
• what do you mean by "containers when we fall back to eviction"?

[omerap12]

Since this is not the selected option, can we just mention the option and briefly explain why it was not chosen?

[adrianmoisey]

The formal KEP process has an "alternatives" option: https://github.com/kubernetes/enhancements/tree/master/keps/NNNN-kep-template#alternatives

[omerap12]

/cc @ndixita

[adrianmoisey]

Spacing here is a little weird

[adrianmoisey]

The formal KEP process has an "alternatives" option: https://github.com/kubernetes/enhancements/tree/master/keps/NNNN-kep-template#alternatives

[adrianmoisey]

Hmmm, interesting con.
I believe this may go against the existing API though:

autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go

Lines 96 to 104 in 5f6697d

	// Controls how the autoscaler computes recommended resources.
	// The resource policy may be used to set constraints on the recommendations
	// for individual containers.
	// If any individual containers need to be excluded from getting the VPA recommendations, then
	// it must be disabled explicitly by setting mode to "Off" under containerPolicies.
	// If not specified, the autoscaler computes recommended resources for all containers in the pod,
	// without additional constraints.
	// +optional
	ResourcePolicy *PodResourcePolicy `json:"resourcePolicy,omitempty" protobuf:"bytes,3,opt,name=resourcePolicy"`

The current default is to control all containers, regardless of if their resources are set or not

[iamzili]

Yes, but the current default behavior - controlling all container-level resources - does not make sense when pod-level resources are present. For example, this does not look correct, and it is not what users expect (what is the benefit of using pod-level resources here?):

from this:

kind: Pod
apiVersion: v1
metadata:
  namespace: default
  name: mypod
spec:
    resources:
      requests:
        cpu: 200m
        memory: "200Mi"
    containers:
    - name: c1
      image: registry.k8s.io/pause:3.1
    - name: c2
      image: registry.k8s.io/pause:3.1

this should not become:

kind: Pod
apiVersion: v1
metadata:
  namespace: default
  name: mypod
spec:
    resources:
      requests:
        cpu: 200m
        memory: "200Mi"
    containers:
    - name: c1
      image: registry.k8s.io/pause:3.1
      resources:
        requests:
          cpu: 180m
          memory: "180Mi"
    - name: c2
      image: registry.k8s.io/pause:3.1
      resources:
        requests:
          cpu: 20m
          memory: "20Mi"

[adrianmoisey]

Yes, but the current default behavior - controlling all container-level resources - does not make sense when pod-level resources are present. For example, this does not look correct, and it is not what users expect (what is the benefit of using pod-level resources here?):

Right, I agree that this is strange, but, it's what the user asked for (using the existing defaults set for container resources, and opting in to pod level resources)

I think either way, if the behaviour is surprising, we should try change the API such that the behaviour isn't surprising anymore.

What if we had a way to select if you wanted container, pod or both controlled?

May be something living inside of spec.updatePolicy ?

[adrianmoisey]

This seems to contradict the defaults set in the API:

https://github.com/kubernetes/autoscaler/pull/8586/changes#diff-3fe0ec0541bbb21c23bf40a53dcae34f461a2db784c094f4ad86d9a4f4a86d2fR235-R237

1. Add a new spec.resourcePolicy.podPolicies stanza. This stanza is user-modifiable and allows setting constraints for pod-level recommendations:
   - controlledResources: Specifies which resource types are recommended (and possibly applied). Valid values are cpu, memory, or both. If not specified, both resource types are controlled by VPA.

My opinion is this:
Extend the API to include a "podPolicies", which has a "controlledResources" that defaults to nothing.

If a user wants to opt-in to pod level resources, they need to set spec.resourcePolicy.podPolicies.controlledResources to both memory and cpu.

The awkward part here is that container resources are enabled by default and pod resources are disabled by default. We could chat to someone in api-machinery about how we can safely roll this out with both pod and container being set, without breaking backwards compatibility.

[iamzili]

actually this makes sense!

So you are suggesting that the pod-level and container-level resources present in the pod spec would still be relevant, but only because of the request-to-limit ratio. When a user wants VPA to manage pod-level resources, they need to specify this in the new podPolicies.controlledResources stanza (defaults to nothing), is my understanding correct?

[adrianmoisey]

Yup!

[iamzili]

@adrianmoisey and @omerap12 I'm requesting a new review for this AEP.

I did a refactor on this AEP based on the ongoing code implementation PR which is 95% ready. I removed tons of stuff from the documentation, so it is not that dense that it was before, furthermore I reworked the whole mechanism as well.

Thanks in advance!