Skip to content

Conversation

@laoj2
Copy link
Contributor

@laoj2 laoj2 commented Feb 24, 2025

What type of PR is this?

/kind documentation
/kind feature

What this PR does / why we need it:

Autoscaling Enhancement Proposal to add basic support for boosting CPU of workloads in VPA, leveraging in-place pod resizes.

Would love your thoughts on this feature. This can be a great addition on top of AEP-4016

#7862

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. area/vertical-pod-autoscaler labels Feb 24, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 24, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @laoj2. 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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 24, 2025
@laoj2
Copy link
Contributor Author

laoj2 commented Feb 24, 2025

/assign @raywainman
/assign @voelzmo

cc @jpawelczak

@raywainman
Copy link
Member

raywainman commented Feb 25, 2025

cc @maxcao13 who might have some good insights and thoughts here

@raywainman
Copy link
Member

/assign @maxcao13

@k8s-ci-robot
Copy link
Contributor

@raywainman: GitHub didn't allow me to assign the following users: maxcao13.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @maxcao13

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.

Copy link
Member

@omerap12 omerap12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CPU issue is well known (especially in Java). I’m wondering if there are programming languages with a similar problem but with memory (e.g needing a lot of memory at startup and then using less over time)

@adrianmoisey
Copy link
Member

The CPU issue is well known (especially in Java). I’m wondering if there are programming languages with a similar problem but with memory (e.g needing a lot of memory at startup and then using less over time)

Memory decrease isn't supported yet: https://github.com/kubernetes/enhancements/tree/758ea034908515a934af09d03a927b24186af04c/keps/sig-node/1287-in-place-update-pod-resources#memory-limit-decreases

@omerap12
Copy link
Member

The CPU issue is well known (especially in Java). I’m wondering if there are programming languages with a similar problem but with memory (e.g needing a lot of memory at startup and then using less over time)

Memory decrease isn't supported yet: https://github.com/kubernetes/enhancements/tree/758ea034908515a934af09d03a927b24186af04c/keps/sig-node/1287-in-place-update-pod-resources#memory-limit-decreases

Thanks for the reference!

@raywainman raywainman changed the title Support CPU Startup Boost in VPA AEP-7862: Support CPU Startup Boost in VPA Mar 14, 2025
@laoj2 laoj2 requested review from adrianmoisey and omerap12 March 20, 2025 18:41
@adrianmoisey
Copy link
Member

Good work! I'm at the point where I'm happy to approve this

@jpawelczak
Copy link

Could we use phased implementation for the feature?

I would recommend to start from CPU Boost with the startup/readiness probe as the 1st phase.

Meanwhile, we could discuss in more details about memory boost as downscaling mem may result in having OOM (very disruptive in nature resulting in container restart)

@laoj2
Copy link
Contributor Author

laoj2 commented Mar 21, 2025

Could we use phased implementation for the feature?

I would recommend to start from CPU Boost with the startup/readiness probe as the 1st phase.

Yes, we could do that. It's probably gonna be simpler to start the implementation disregarding the StartupBoost.CPU.Duration field. This doesn't change the design, but it means that until we implement the support for duration, this will likely only make sense for workloads with probes as you said.

Meanwhile, we could discuss in more details about memory boost as downscaling mem may result in having OOM (very disruptive in nature resulting in container restart)

Yes, there's nothing much we can do until the in-place pod resize feature supports memory downsize: https://github.com/kubernetes/enhancements/tree/758ea034908515a934af09d03a927b24186af04c/keps/sig-node/1287-in-place-update-pod-resources#memory-limit-decreases

@raywainman
Copy link
Member

LGTM as well

/lgtm

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 21, 2025
@raywainman
Copy link
Member

@adrianmoisey I'll let you have approver honors when you are ready :)

Copy link
Member

@omerap12 omerap12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm from me too :)

@k8s-ci-robot
Copy link
Contributor

@laoj2: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-e2e-autoscaling-vpa-full 455d290 link false /test pull-kubernetes-e2e-autoscaling-vpa-full

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@adrianmoisey
Copy link
Member

/approve
/retest

(I assume we may need tests to pass before this will merge?)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianmoisey, laoj2, omerap12

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2025
@k8s-ci-robot k8s-ci-robot merged commit 15295c7 into kubernetes:master Mar 23, 2025
6 of 7 checks passed
@laoj2 laoj2 deleted the cpu-boost-kep branch March 24, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants