Fix scaleQuantityProportionallyCPU function

[iamzili]

What type of PR is this?

/kind bug
/area vertical-pod-autoscaler

What this PR does / why we need it:

The current implementation of scaleQuantityProportionallyCPU is broken (possibly the scaleQuantityProportionallyMem is broken as well, but I want to fix it in an another PR).

When a LimitRange object with type Pod exists in the namespace, and the algorithm proportionally increases or decreases recommendations, the division may produce an implicit issue. When remainders occur, the sum of the new recommendations does not satisfy the constraints defined in the LimitRange. For example (only the cpu is controlled):

• A LimitRange of type Pod defines min.cpu as 150m.
• request-to-limit ratio is 1:1 for both containers
• A Pod contains two containers with the following CPU recommendations:
  • c1: 15m
  • c2: 100m
• calling scaleQuantityProportionallyCPU(15m, 115m, 150m, noRounding) returns 19m
  • formula used: (15*150)/115
• calling scaleQuantityProportionallyCPU(100m, 115m, 150m, noRounding) returns 130m.
  • formula used: (100*150)/115

The sum of new recommendations is below 150m > (19m + 130m), which means that the pod is not going to be scheduled.

The issue with scaleQuantityProportionallyCPU is that when the division executes, the implementation ignores any remainder.

Special notes for your reviewer:

The change removes the rounding parameter from the function signature because we only using the default behavior, noRounding. Previously, the function behaved as follows:

1. With noRounding, if the formula produces 1234567890m, the function returns 1234567890m.
2. With roundUpToFullUnit, if the formula produces 1234567890m, the function returns 1234568.
3. With roundDownToFullUnit, if the formula produces 1234567890m, the function returns 1234567.

Also the diffs in vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go are somewhat confusing. I removed one duplicated unit test from the file and added a completely new test.

Does this PR introduce a user-facing change?

Yes, but the change is subtle and may not impact users at all. With the new implementation of scaleQuantityProportionallyCPU, if the division produces a remainder, the function adds 1m to the quotient. Using the example specified earlier:

the sum of new recommendations ends up 1m higher (i.e. 151m) than the min.cpu defined in the LimitRange. So from 19m we get 20m, from the 130 we get 131m which results in 151m. So technically we waste 1m but, that shouldn't be a problem. Even the resource.Quantity documentation states that 0.1m rounds up to 1m.

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

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.

[k8s-ci-robot]

Hi @iamzili. Thanks for your PR.

I'm waiting for a github.com 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.

[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/OWNERS

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

[adrianmoisey]

/ok-to-test