feat: CPU startup boost in master

[kamarabbas99]

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

The CPU startup boost changes were done on experimental branch, moving this to master.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Users can configure a startupBoost policy in the VPA spec. 

Which issue(s) this PR fixes:

#7862

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

- [KEP]: (https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler/enhancements/7862-cpu-startup-boost#aep-7862-cpu-startup-boost)

[kamarabbas99]

/cc adrianmoisey omerap12

[k8s-triage-robot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[omerap12]

Nice! we already approved this in previous PRs 🥳
/lgtm
/label tide/merge-method-squash

[omerap12]

oh good catch
/lgtm cancel

[adrianmoisey]

I'm good with this, thanks for doing it!
/lgtm

[omerap12]

/approve

[soltysh]

/label api-review

[soltysh]

I left there a few API related questions, but I didn't get too deep into the logic itself, other than just the validation bits.

[soltysh]

This is the only error case you're testing, I'd add additional ones for type and factor|quantity specified, and probably also one for invalid type, even though that should be caught at the api server validation, but it doesn't hurt to have a test case covering that.

[soltysh]

The other thing, the WithCPUStartupBoost method only modifies the VPA.spec, but doesn't change the container level StartupBoost, it would be good to also cover that case in test, to ensure the latter takes priority.

[soltysh]

What is the relationship between the two, I missed that in the AEP-7862 and the code seems to be working on one or the other, depending on the location?

[kamarabbas99]

Added some more error cases in #8828. should it error out when both quantity and Factor are set ? Currently it doesnt.

contianer level startup boost has higher priority, added some test cases to cover that.

[soltysh]

Great, just tagged #8828, but it would be also nice to update AEP-7862 with this information.

[kamarabbas99]

Actually AEP does mention it.

The new StartupBoost parameter will be added to both:

[VerticalPodAutoscalerSpec]: Will allow users to specify the default CPU startup boost for all containers of the pod targeted by the VPA object.
[ContainerResourcePolicy]: Will allow users to optionally customize the startup boost behavior for individual containers.

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[alextarasov-spot]

Hello, I’m not sure if this is the right place to report this issue, so I’ll share it here. If there’s a more appropriate channel, please let me know.

I cloned the experimental-cpu-boost-v2 branch to test the new feature, and I’m concerned about the following behavior:

• Boost works for one replica
• Unboost does not work with a single replica, meaning the pod stays stuck with the boosted CPU request.

1 pods_inplace_restriction.go:112] "Checking if pod can be unboosted" pod="vpa-test/nginx-all-containers-5d649d7c56-t7bqj" durationPassed=true hasAnnotation=true
1 pods_restriction_factory.go:212] "Too few replicas" kind="ReplicaSet" object="vpa-test/nginx-all-containers-5d649d7c56" livePods=1 requiredPods=2 globalMinReplicas=2

The Minimum number of replicas to perform an update is 2 by default

autoscaler/vertical-pod-autoscaler/pkg/updater/main.go

Line 61 in 4b40a55

minReplicas = flag.Int("min-replicas", 2,

This can be confusing, as boosting is allowed with a single replica, but unboosting will never occur in that case.

[kamarabbas99]

@alextarasov-spot thanks for catching this!
sent #8854 to address this.

[omerap12]

@alextarasov-spot , great catch! thanks for this!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kamarabbas99, omerap12
Once this PR has been reviewed and has the lgtm label, please ask for approval from adrianmoisey. 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/OWNERS [omerap12]
• vertical-pod-autoscaler/enhancements/OWNERS
• vertical-pod-autoscaler/pkg/apis/OWNERS [omerap12]

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

[alextarasov-spot]

@kamarabbas99 @omerap12 thank you guys for the quick reaction!

If you don't mind, I will share another issue I found.

Input data

• resources request

resources:
            requests:
              cpu: "100m"

• VPA definition

spec:
  resourcePolicy:
    containerPolicies:
    - containerName: nginx
      controlledValues: RequestsAndLimits
  startupBoost:
    cpu:
      duration: 15s
      quantity: 200m
      type: Quantity
  ......
  ......
  updatePolicy:
    updateMode: InPlaceOrRecreate
status:
  recommendation:
    containerRecommendations:
    - containerName: nginx
      target:
        cpu: 105m

• If there are no Limits defined

• AND the VPA’s controlledValues is RequestsAndLimits - the vpa-admission-controller calculates:

  • the limit as {Boost Quantity} + {Deployment Request} = 300m
  • the request as {VPA Target} + {Boost Quantity} = 305m

• here it calculates the Limit

  autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/resource_updates.go

  Line 254 in 4b40a55

  boostedLimit, err := c.calculateBoostedCPU(recommendedLimit, originalLimit, startupBoostPolicy)

• if the limit is not set, it takes the original CPU request

  autoscaler/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/patch/resource_updates.go

  Line 208 in 4b40a55

  if baseCPU.IsZero() {

Which leads to the following error:

Error creating: Pod "nginx-with-probes-6fb6445758-ldw5m"
  is invalid: spec.containers[0].resources.requests: Invalid value: "305m": must be
  less than or equal to cpu limit of 300m'

[kamarabbas99]

@alextarasov-spot did you manually set the target? Because I thought when controlledValues is RequestsAndLimits, the recommender will set limits as well.

[omerap12]

@kamarabbas99 I think it would be helpful to document this use case in the AEP (where no limits are defined for a pod).
In situations where no original limit is set, shouldn’t the limit be based on the boosted request when no original limit exists?

[omerap12]

Having no limit does make sense :) as long as we document this.

[alextarasov-spot]

@kamarabbas99

@alextarasov-spot did you manually set the target? Because I thought when controlledValues is RequestsAndLimits, the recommender will set limits as well.

Yes, I manually configured the VPA target. If RequestsAndLimits is set, the VPA admission controller will calculate the limit, but it doesn't factor in the boosted value. I think that when there are no original limits set AND a boost is configured, AND the controlledValues is RequestsAndLimits, the admission controller should calculate the limit based on the boosted value and not on the original request value

I think that @omerap12 meant the same:

shouldn’t the limit be based on the boosted request when no original limit exists?

[kamarabbas99]

@alextarasov-spot I am addressing this in #8863 PTAL!