feat: CPU startup boost in master #8813
Conversation
Fix VPA startup boost validation error messages
Fix test failure after rebase
Add e2e tests for CPU startup boost
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
size/XXL
|
/cc adrianmoisey omerap12 |
|
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
left a comment
omerap12
left a comment
There was a problem hiding this comment.
Nice! we already approved this in previous PRs 🥳
/lgtm
/label tide/merge-method-squash
k8s-ci-robot
added
the
tide/merge-method-squash
k8s-ci-robot
added
the
lgtm
|
oh good catch |
k8s-ci-robot
removed
the
lgtm
Update VPA version for startupboost feature
|
I'm good with this, thanks for doing it! |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
/label api-review |
k8s-ci-robot
added
the
api-review
soltysh
left a comment
soltysh
left a comment
There was a problem hiding this comment.
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.
vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go
Outdated
Show resolved
Hide resolved
vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go
Outdated
Show resolved
Hide resolved
| maxAllowedCpu: resource.QuantityValue{}, | ||
| featureGateEnabled: true, | ||
| expectError: fmt.Errorf("boost factor must be >= 1"), | ||
| }, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
CPU startup boost: add tests
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
removed
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kamarabbas99, omerap12 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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?
Which issue(s) this PR fixes:
#7862
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: