AEP-8515: Add support for setting a custom request-to-limit ratio at the VPA object level

[iamzili]

What type of PR is this?

/kind documentation
/kind feature

What this PR does / why we need it:

Autoscaling Enhancement Proposal (AEP) to add support for setting a custom request-to-limit ratio at the VPA object level.

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

#8515

[k8s-ci-robot]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

[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.

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]

Thanks for the AEP!
I've left 2 small comments, but will definitely need to come back for a more thorough review.

[adrianmoisey]

Would QuantityPercentage not be the same as factor?
ie: QuantityPercentage of 10% is the same as factor of 1.1? (assuming that factor wasn't stored as an int)

[iamzili]

Good point - I was thinking the same: simplify things by dropping QuantityPercentage and renaming QuantityValue to Quantity.

When I first thought about this feature, I thought it might be useful to give users more options to define the ratio's magnitude. But this will just make VPA more complex.

[iamzili]

I'm also considering changing the way the sub-fields are defined, based on AEP-7862, to make the VPA CRD more consistent. For example:

from:

RequestToLimitRatio:
  cpu:
    Type: Factor
    Value: 2
  memory:
    Type: Quantity
    Value: 100Mi

to

RequestToLimitRatio:
  cpu:
    factor: 2
  memory:
    quantity: 100Mi

btw after reviewing AEP-7862, I think consistency is broken there when startupBoost is defined under containerPolicies:

startupBoost:
  cpu:
    type: "Quantity"
    quantity: "4"

vs

startupBoost:
  cpu:
    factor: 1

We should define a common specification to be used for both AEPs.

[adrianmoisey]

Yup, I think this change makes sense. And good call out on the inconstancy in the startup boost feature.

[adrianmoisey]

Wait, hold on, where is the consistency broken? type default to Factor.
So the second example you pasted implies the following:

startupBoost:
  cpu:
    type: "Factor"
    factor: 1

This is consistent with the other methods, unless I'm missing something.

[iamzili]

you're right! I missed the defaulting part from AEP 7863.

But the consistency between the AEP I created and AEP 7863 is still broken because:

from startupBoost:

Type: "Factor"
Factor: 1

from RequestToLimitRatio:

Type: "Factor"
Value: 1 # do I need to rename the key to "Factor"?

If you ask me, I think the startupBoost spec is more aligned with the VPA object. Since the startupBoost AEP is already merged into the main branch, I assume we need to follow the same approach in the RequestToLimitRatio AEP.

[adrianmoisey]

Correct, there should be consistency between this AEP and the startupBoost AEP

[iamzili]

consistency fixed

[adrianmoisey]

What happens in a scenario like this:

1. A user has a pod with in-place set. And where requests are equal to limits (and the QoS class is Guaranteed)
2. The user modifies the VPA to add a RequestToLimitRatio, making limits double requests

What should the behaviour be? Should the VPA kill the pod to allow a new one be created with a different QoS class?

[iamzili]

I totally forgot about QoS classes, thanks for bringing this up!

Since the qosClass field is immutable, we cannot rely on in-place updates, the Pod(s) need to be recreated if the QoS class changes.

To clarify, changing the ratio using this new feature will not evict the Pod(s) immediately. It will rely on the current Updater's behavior like when the current resources.requests differ significantly from the new recommendation.

[adrianmoisey]

I guess my concern is actually more of "how will the VPA handle the RequestToLimitRatio changing". Either from the default to a specified ratio, or from one ratio to another.
I think it's worth calling this out in the AEP

[iamzili]

added more examples

[tspearconquest]

Thanks for tagging me in Slack to take a look, and for this AEP!
I added a few questions and ideas.

[tspearconquest]

Please test extremely high factors like 1 million in addition to more typical factors like 1, 5, 10.

[iamzili]

why do we need to test something like this?

[tspearconquest]

We don't necessarily need to given the response above that limits aren't used for scheduling but it might turn up something interesting in how Kubernetes handles it. You never know what bug might lurk with limits that have never been tested before.

[omerap12]

@tspearconquest , what do you want to test here? if you wanna test a pod with 1 million factor this is unrelated to the VPA work. In tests we need to make sure the VPA behaves as planned.

[tspearconquest]

what do you want to test here?

Integer overflows on the recommender status fields.

if you wanna test a pod with 1 million factor this is unrelated to the VPA work.

Okay, no problem.

In tests we need to make sure the VPA behaves as planned.

No argument here. I'm probably overthinking this, as I don't know Go code very well and don't have the level of understanding you all do with regards to how the recommender status fields and the VPA pods would work with a value exceeding the int64 limit of ~8 exbibytes when converted to an integer.

[omerap12]

Yeah valid points, in the HPA we do the same tests but as a unit tests.
I guess we can make some unit tests around that as well.

[omerap12]

I suggest we first agree on this AEP and then we can mark this for an api-review.

[iamzili]

I suggest we first agree on this AEP and then we can mark this for an api-review.

Is this step for the core maintainers, or can I help with it to move things forward somehow?

[omerap12]

I suggest we first agree on this AEP and then we can mark this for an api-review.

Once the maintainers are ok with that we will move this to the api-machinery folks for review.

[omerap12]

Instead of leaving this as a TODO, please add some coverage here. The E2E test should be straightforward - just verify the limit across a few different scenarios. A couple of simple cases would be enough.

[iamzili]

Done, added some e2e tests.

[adrianmoisey]

/ok-to-test

[adrianmoisey]

To be in line with CPU Startup Boost feature, this needs to be a required field. See #8608

(Sorry for changing it out from under you)

[iamzili]

no problem, updated

[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 gjtempleton for approval. For more information see the Code Review Process.

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

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

[omerap12]

/api-review cancel
So we can modify this AEP

[omerap12]

/remove-label api-review

[omerap12]

nit from me.
@adrianmoisey if you are good with this we can make this for an api-review.

[iamzili]

nit from me. @adrianmoisey if you are good with this we can make this for an api-review.

Just a reminder for @adrianmoisey: according to Omer, the PR can be marked for an API review. Could you please double-check?

[adrianmoisey]

Overall, LGTM. I left a few minor nits.

I have another question: what happens if a user's Pod doesn't have limits set, but they have RequestToLimitRatio configured?

Should the VPA set a limit for them?
Or should the VPA ignore limits (much like it does now?)

In my opinion, once this question is answered, let's ask for API review (since this question may change the API, but none of my other nits will)

[adrianmoisey]

Nit: technically it's not the workload API that defines this, it's the Pod.

[iamzili]

addressed in a commit

[adrianmoisey]

These fields aren't technically immutable, hence being allowed to adjust them.
They are mutable, but since they change the Podspec, new Pods are created.

[iamzili]

addressed in a commit

[adrianmoisey]

for example to non-disruptively scale workloads beyond what the their requests/limits would predict

I'm not too sure what this means, can you explain it?

[iamzili]

Sure! my intention was to emphasize that by leveraging the InPlaceOrRecreate mode together with the new RequestToLimitRatio stanza, users may change the request-to-limit ratio without evicting Pods (i.e. in a non-disruptive way). Of course, this does not apply in every case, for example:

as far as I know, decreasing a Pod's memory limit while its actual memory usage exceeds the new limit will simply cause the kubelet to reject the in-place update.

Should I be more specific about this in the AEP?

[adrianmoisey]

I don't think it's really worth mentioning.
The in-place AEP speaks about what happens during an in-place resize. All this AEP is doing is potentially changing the actual limit recommendation.

[iamzili]

You convinced me, I removed that part

[adrianmoisey]

Oh, another consideration is the Pod's QoS. Generally we don't change the QoS, I also think that the in-place doesn't yet allowing the change of the QoS level.

[omerap12]

Oh, another consideration is the Pod's QoS. Generally we don't change the QoS, I also think that the in-place doesn't yet allowing the change of the QoS level.

True. we don't change the QoS

[iamzili]

Overall, LGTM. I left a few minor nits.

I have another question: what happens if a user's Pod doesn't have limits set, but they have RequestToLimitRatio configured?

Should the VPA set a limit for them? Or should the VPA ignore limits (much like it does now?)

In my opinion, once this question is answered, let's ask for API review (since this question may change the API, but none of my other nits will)

Thanks for the review! Regarding your question, my personal opinion is that even if a Pod does not have a limit, when the new RequestToLimitRatio field is used, VPA (both the admission controller and updater) should calculate the limits. I believe this is what users would expect when RequestToLimitRatio is configured, although my assumption could be wrong.

[adrianmoisey]

Thanks for the review! Regarding your question, my personal opinion is that even if a Pod does not have a limit, when the new RequestToLimitRatio field is used, VPA (both the admission controller and updater) should calculate the limits. I believe this is what users would expect when RequestToLimitRatio is configured, although my assumption could be wrong.

This is my gut feel too.

I think it's worth including in the AEP.
Also worth adding to the description of that field in the API, and including in documentation.

[iamzili]

Oh, another consideration is the Pod's QoS. Generally we don't change the QoS, I also think that the in-place doesn't yet allowing the change of the QoS level.

Yes, in-place updates do not allow changing a Pod's QoS, as this results in an error.

This means that after implementing this AEP, when using the InPlaceOrRecreate mode, the updater will first attempt to apply patches in-place. If the resize error occurs, it will evict the Pod, which can then be recreated with a new QoS - for example if the RequestToLimitRatio factor is changed from 1.2 to 1 (i.e. the request-to-limit ratio becomes 1:1). This is also stated in the AEP.

[iamzili]

Thanks for the review! Regarding your question, my personal opinion is that even if a Pod does not have a limit, when the new RequestToLimitRatio field is used, VPA (both the admission controller and updater) should calculate the limits. I believe this is what users would expect when RequestToLimitRatio is configured, although my assumption could be wrong.

This is my gut feel too.

I think it's worth including in the AEP. Also worth adding to the description of that field in the API, and including in documentation.

Yes, I will definitely mention this. @omerap12 what do you think?

[adrianmoisey]

Oh, another consideration is the Pod's QoS. Generally we don't change the QoS, I also think that the in-place doesn't yet allowing the change of the QoS level.

Yes, in-place updates do not allow changing a Pod's QoS, as this results in an error.

This means that after implementing this AEP, when using the InPlaceOrRecreate mode, the updater will first attempt to apply patches in-place. If the resize error occurs, it will evict the Pod, which can then be recreated with a new QoS - for example if the RequestToLimitRatio factor is changed from 1.2 to 1 (i.e. the request-to-limit ratio becomes 1:1). This is also stated in the AEP.

Oh, I see, over here: https://github.com/kubernetes/autoscaler/pull/8516/files#diff-5540c5d94390214fe8bf4289331809652e480a07c1d29e6ac8087c21cd5d21a8R145-R146
Thanks

[iamzili]

Oh, another consideration is the Pod's QoS. Generally we don't change the QoS, I also think that the in-place doesn't yet allowing the change of the QoS level.

Yes, in-place updates do not allow changing a Pod's QoS, as this results in an error.
This means that after implementing this AEP, when using the InPlaceOrRecreate mode, the updater will first attempt to apply patches in-place. If the resize error occurs, it will evict the Pod, which can then be recreated with a new QoS - for example if the RequestToLimitRatio factor is changed from 1.2 to 1 (i.e. the request-to-limit ratio becomes 1:1). This is also stated in the AEP.

Oh, I see, over here: https://github.com/kubernetes/autoscaler/pull/8516/files#diff-5540c5d94390214fe8bf4289331809652e480a07c1d29e6ac8087c21cd5d21a8R145-R146 Thanks

fyi - I just updated that part slightly in commit

[omerap12]

Thanks for the review! Regarding your question, my personal opinion is that even if a Pod does not have a limit, when the new RequestToLimitRatio field is used, VPA (both the admission controller and updater) should calculate the limits. I believe this is what users would expect when RequestToLimitRatio is configured, although my assumption could be wrong.

This is my gut feel too.
I think it's worth including in the AEP. Also worth adding to the description of that field in the API, and including in documentation.

Yes, I will definitely mention this. @omerap12 what do you think?

I concur :)

[iamzili]

Thanks for the review! Regarding your question, my personal opinion is that even if a Pod does not have a limit, when the new RequestToLimitRatio field is used, VPA (both the admission controller and updater) should calculate the limits. I believe this is what users would expect when RequestToLimitRatio is configured, although my assumption could be wrong.

This is my gut feel too.

I think it's worth including in the AEP. Also worth adding to the description of that field in the API, and including in documentation.

addressed in 3dd47b2