-
Notifications
You must be signed in to change notification settings - Fork 4.2k
AEP-7862: Make API changes for CPU Startup boost #8349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
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. |
Welcome @kamarabbas99! |
Hi @kamarabbas99. 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 Once the patch is verified, the new status will be reflected by the 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kamarabbas99 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 |
/unassign laoj2 omerap12 |
/cc laoj2 omerap12 |
72835f9
to
847756a
Compare
/ok-to-test |
vertical-pod-autoscaler/enhancements/7862-cpu-startup-boost/README.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this and updating the AEP.
We should also include the expected Go struct definition. From what I understand, it should look like this:
// +enum
type CPUBoostType string
const (
FactorType CPUBoostType = "Factor"
QuantityType CPUBoostType = "Quantity"
)
type CPUStartupBoost struct {
// +unionDiscriminator
// +required
Type CPUBoostType `json:"type"`
// +unionMember=Factor
// +optional
FactorValue *int32 `json:"factorValue,omitempty"`
// +unionMember=Quantity
// +optional
Quantity *resource.Quantity `json:"quantity,omitempty"`
// +optional
Duration *metav1.Duration `json:"duration,omitempty"`
}
These changes are based on discussion in kubernetes#8175 (comment) https://kubernetes.slack.com/archives/C0EG7JC6T/p1751910562504969
847756a
to
017738e
Compare
/lgtm I'll let @adrianmoisey or @omerap12 approve since they were heavily involved in the discussion. |
`existing VPA recommendation for that container` (if any) OR the | ||
`CPU resources configured in the pod spec`. | ||
down the CPU resources to the appropriate non-boosted value, which is determined by the VPA `updatePolicy`: | ||
* If `updatePolicy` is `Auto`, `Recreate` or `InPlaceOrRecreate`, the VPA Updater will apply the VPA recommendation, even if it's higher than the boosted value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the recommendation is higher than the boosted value, during the boost period?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the boost period, the VPA updater ignores new recommendations, and the pod continues to run with its boosted CPU value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, makes sense. Is it worth calling that out in the AEP?
I think there are 3 things worth calling out explicitly:
- the feature will base the boosted value on the recommendation at time of admission
- no resizing will take place during boost
- during unboost, the pod will be resized to whatever the recommendation is at that moment in time
At least, those bullet points are how I understand the AEP to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, it's also important to callout that the feature will ignore the recommendation if the VPA updateMode=Off
(or containerScalingMode=Off
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized we do mention the updateMode
already :), is it worth it mentioning containerScalingMode too?
@@ -394,7 +424,7 @@ spec: | |||
startupBoost: | |||
cpu: | |||
type: "Quantity" | |||
value: "4" | |||
quantity: "4" | |||
``` | |||
|
|||
## Implementation History |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you also mention the updates in the history log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall /lgtm. please address @adrianmoisey and @laoj2 comments :)
What type of PR is this?
/kind documentation
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
These changes are based on discussion in
#8175 (comment) https://kubernetes.slack.com/archives/C0EG7JC6T/p1751910562504969