Skip to content

Decouple Startup CPU Boost from VPA modes #8175

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laoj2
Copy link
Contributor

@laoj2 laoj2 commented May 27, 2025

What type of PR is this?

/kind documentation
/kind feature

What this PR does / why we need it:

Changes to AEP-7862:

  • Decouple startup boost config from regular VPA, so the API is simplified, in case users want to only use CPU boost without regular VPA updates (we no longer require a new VPA mode + have a way to specify the startup boost config at the VPA/workload level).
  • Added more yaml examples

#7862

@k8s-ci-robot k8s-ci-robot added 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. area/vertical-pod-autoscaler needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 27, 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 May 27, 2025
@laoj2
Copy link
Contributor Author

laoj2 commented May 27, 2025

/assign @omerap12
/assign @adrianmoisey

Comment on lines +98 to +101
* [Optional] `StartupBoost.CPU.Mode`: whether CPU boost is enabled (`"Auto"`)
or not (`"Off"`). If not specified, it defaults to `"Auto"`.
Copy link
Member

Choose a reason for hiding this comment

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

Could you give a reason why the term Auto was selected here?
I'd assume this field is effectively a boolean, so I'd assume On/Off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Wanted to make this similar to regular VPA, but doesn't really make much sense. Changed it to On/Off. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Please look at this: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md

It says:

"Think twice about bool fields. Many ideas start as boolean but eventually trend towards
a small set of mutually exclusive options. Plan for future expansions by describing the
policy options explicitly as a string type alias (e.g. TerminationMessagePolicy)."

I learned this from @soltysh in this thread

Copy link
Member

Choose a reason for hiding this comment

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

Good call out, thanks Omer!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the guidance here then to go back to Auto?

Copy link
Member

Choose a reason for hiding this comment

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

What if we copied ControlledResources?
I wonder if StartupBoost.Resources (or StartupBoostResources?) could be a list that takes memory or cpu in the future, but for now it takes cpu only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting idea. I'm wondering how we could use it for the use case where we define the "top-level" CPU boost, but want to disable it for an individual container. In the current proposal, it would look like this:

apiVersion: "autoscaling.k8s.io/v1"
kind: VerticalPodAutoscaler
metadata:
  name: example-vpa
spec:
  targetRef:
    apiVersion: "apps/v1"
    kind: Deployment
    name: example
  startupBoost:
    cpu:
      factor: 2.0
  resourcePolicy:
    containerPolicies:
      - containerName: "disable-cpu-boost-for-this-container"
        startupBoost:
          mode: "Off"

So, would we still need an "off"/"none" StartupBoostResources if we want to use this other field, instead? e.g.:

apiVersion: "autoscaling.k8s.io/v1"
kind: VerticalPodAutoscaler
metadata:
  name: example-vpa
spec:
  targetRef:
    apiVersion: "apps/v1"
    kind: Deployment
    name: example
  startupBoost:
    cpu:
      factor: 2.0
  resourcePolicy:
    containerPolicies:
      - containerName: "disable-cpu-boost-for-this-container"
        startupBoost:
          startupBoostResources:
             - "Off"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi folks, in the meantime, I changed this back to Auto. But let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure about "Auto", it seems to be like it's just "Enabled/On", what is automatic about it?

@laoj2 laoj2 force-pushed the aep-7862-fixes-to-api branch from 954bbbc to 0b42a31 Compare May 28, 2025 17:18
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: laoj2
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:

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

@laoj2 laoj2 requested review from adrianmoisey and omerap12 May 28, 2025 20:18
Comment on lines +98 to +101
* [Optional] `StartupBoost.CPU.Mode`: whether CPU boost is enabled (`"Auto"`)
or not (`"Off"`). If not specified, it defaults to `"Auto"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the guidance here then to go back to Auto?

@laoj2 laoj2 force-pushed the aep-7862-fixes-to-api branch from 0b42a31 to ea041d5 Compare June 6, 2025 16:07
@laoj2 laoj2 requested a review from raywainman June 6, 2025 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. needs-ok-to-test Indicates a PR that requires an org member to verify it 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.

5 participants