feat(recommender): add round cpu millicores #7682

omerap12 · 2025-01-09T15:04:18Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds the --round-cpu-millicores flag to the VPA recommender, enabling user-configurable rounding of CPU recommendations.

Which issue(s) this PR fixes:

Fixes #7678

Special notes for your reviewer:

Does this PR introduce a user-facing change?

`--round-cpu-millicores` flag to the VPA recommender

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

Documentation updated to include the `--round-cpu-millicores` flag in the recommender feature set.

Signed-off-by: Omer Aplatony <[email protected]>

vertical-pod-autoscaler/docs/features.md

adrianmoisey · 2025-01-10T12:44:11Z

vertical-pod-autoscaler/pkg/recommender/logic/recommender.go

 	humanizeMemory             = flag.Bool("humanize-memory", false, "Convert memory values in recommendations to the highest appropriate SI unit with up to 2 decimal places for better readability.")
+	roundCPUMillicores         = flag.Int("round-cpu-millicores", 1, `CPU recommendation rounding factor in millicores. The CPU value will be rounded to the nearest multiple of this factor.`)


I'm curious what others think, but I like the idea of these two options being similar.
humanize-memory + humanize-cpu
or
round-memory-mebibyte + round-cpu-millicores

I haven't though through if this is possible or not. So I'm unsure if my idea is a good one or not, but for some reason keeping the options similar just feels like it may be a good user experience.

Do you mean you want to combine these two flags into a single one?

That is another option...
--humanize=cpu,memory

Or two flags:

--humanize-memory=true/false --humanize-cpu=true/false

I'm not strongly opinionated on the direction we go, I do think more opinions may be useful

but the cpu rounding depends on the user input, as it rounds to the nearest multiple of a specified factor. So, I'm not sure how the boolean flag is helpful here - unless you're suggesting that we should always round to a specific factor.

unless you're suggesting that we should always round to a specific factor.

☝️ this

Does it make sense to have the "memory rounding feature" and the "cpu rounding feature" to operate in similar ways? Either ask a user to input a factor, or to use the humanize feature for both

They would select the mebibyte's to round up to

(I don't know if this is a good idea or not, to be honest)

But if the user would want GiB? or TiB (not sure it's possible but still )..

Aaaahhhhhh, ok! I see the problem now.
They are both "rounding", but the difference between a few millicore, or a few memory units (GiB, TiB) is quite different.

adrianmoisey · 2025-01-10T12:45:27Z

vertical-pod-autoscaler/docs/features.md

 --humanize-memory=true
+```
+
+## CPU Recommendation Rounding


+1 for adding docs as the feature is added! This is a habit we need to be getting into

yup! Thank you :)

Co-authored-by: Adrian Moisey <[email protected]>

adrianmoisey · 2025-01-12T14:54:45Z

Don't forget to run vpa-generate-flags.go

Signed-off-by: Omer Aplatony <[email protected]>

adrianmoisey · 2025-01-14T06:46:48Z

/milestone vertical-pod-autoscaler-1.4.0

adrianmoisey · 2025-01-17T06:51:32Z

/lgtm

alekc · 2025-01-28T11:05:58Z

Just fiy, there are some bugs around memory implementation of humanize, see #7770 (might affect this one as well if it has followed the same approach)

adrianmoisey · 2025-01-28T11:16:18Z

Just fiy, there are some bugs around memory implementation of humanize, see #7770 (might affect this one as well if it has followed the same approach)

Yup, thanks. Omer (author of this PR and the assignee of #7770) is aware of the issue.

omerap12 · 2025-01-28T12:26:24Z

Just fiy, there are some bugs around memory implementation of humanize, see #7770 (might affect this one as well if it has followed the same approach)

I know about this bug, but this change is simpler and doesn’t use the humanize memory method.

raywainman

Small documentation nit then this is good to go, thanks Omer! :)

raywainman · 2025-01-31T17:34:21Z

vertical-pod-autoscaler/pkg/recommender/logic/recommender.go

 	lowerBoundMemoryPercentile = flag.Float64("recommendation-lower-bound-memory-percentile", 0.5, `Memory usage percentile that will be used for the lower bound on memory recommendation.`)
 	upperBoundMemoryPercentile = flag.Float64("recommendation-upper-bound-memory-percentile", 0.95, `Memory usage percentile that will be used for the upper bound on memory recommendation.`)
 	humanizeMemory             = flag.Bool("humanize-memory", false, "Convert memory values in recommendations to the highest appropriate SI unit with up to 2 decimal places for better readability.")
+	roundCPUMillicores         = flag.Int("round-cpu-millicores", 1, `CPU recommendation rounding factor in millicores. The CPU value will be rounded to the nearest multiple of this factor.`)


Should we specify that it is always rounded up?

Yes, I agree: 6645e94

Signed-off-by: Omer Aplatony <[email protected]>

omerap12 · 2025-01-31T18:09:38Z

Small documentation nit then this is good to go, thanks Omer! :)

No problem :)

adrianmoisey · 2025-01-31T18:19:02Z

vertical-pod-autoscaler/docs/features.md

+VPA can provide CPU recommendations rounded to user-specified values, making it easier to interpret and configure resources. This feature is controlled by the `--round-cpu-millicores` flag in the recommender component.
+
+When enabled, CPU recommendations will be:
+- Rounded to the nearest multiple of the specified millicore value
+- Applied to target, lower bound, and upper bound recommendations
+
+For example, with `--round-cpu-millicores=50`, a CPU recommendation of `79m` would be rounded to `100m`, and a recommendation of `34m` would be rounded to `50m`.
+
+To enable this feature, set the --round-cpu-millicores flag when running the VPA recommender:


Does it also make sense to say "rounded up" in this section? Just to drive home that it's up, not down.

Added in a6a1e1c

Signed-off-by: Omer Aplatony <[email protected]>

raywainman · 2025-01-31T19:33:34Z

/lgtm

/approve

Thanks!

k8s-ci-robot · 2025-01-31T19:33:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: omerap12, raywainman

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~vertical-pod-autoscaler/OWNERS~~ [raywainman]

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

feat(recommender): add round cpu millicores

8e26139

Signed-off-by: Omer Aplatony <[email protected]>

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 9, 2025

k8s-ci-robot requested a review from jbartosik January 9, 2025 15:04

k8s-ci-robot added the area/vertical-pod-autoscaler label Jan 9, 2025

k8s-ci-robot requested a review from kgolab January 9, 2025 15:04

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 9, 2025

Fixed error log

64a921c

Signed-off-by: Omer Aplatony <[email protected]>

adrianmoisey reviewed Jan 10, 2025

View reviewed changes

vertical-pod-autoscaler/docs/features.md Outdated Show resolved Hide resolved

adrianmoisey reviewed Jan 10, 2025

View reviewed changes

fixed docs

06504c2

Co-authored-by: Adrian Moisey <[email protected]>

Add flag to doc

57580b9

Signed-off-by: Omer Aplatony <[email protected]>

k8s-ci-robot added this to the vertical-pod-autoscaler-1.4.0 milestone Jan 14, 2025

omerap12 requested a review from adrianmoisey January 16, 2025 07:22

k8s-ci-robot assigned adrianmoisey Jan 17, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2025

raywainman reviewed Jan 31, 2025

View reviewed changes

edit round up description

6645e94

Signed-off-by: Omer Aplatony <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2025

adrianmoisey reviewed Jan 31, 2025

View reviewed changes

specify rounded up in docs

a6a1e1c

Signed-off-by: Omer Aplatony <[email protected]>

k8s-ci-robot assigned raywainman Jan 31, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2025

k8s-ci-robot merged commit 8e980c0 into kubernetes:master Jan 31, 2025
6 of 7 checks passed

adrianmoisey mentioned this pull request Feb 25, 2025

VPA CPU rounding argument not found #7866

Closed

voelzmo mentioned this pull request May 9, 2025

Fix firstBucketSize for CPU histogram #7554

Open

ialidzhikov mentioned this pull request May 23, 2025

VPA 1.4.0 release gardener/gardener#12027

Closed

		humanizeMemory = flag.Bool("humanize-memory", false, "Convert memory values in recommendations to the highest appropriate SI unit with up to 2 decimal places for better readability.")
		roundCPUMillicores = flag.Int("round-cpu-millicores", 1, `CPU recommendation rounding factor in millicores. The CPU value will be rounded to the nearest multiple of this factor.`)

feat(recommender): add round cpu millicores #7682

feat(recommender): add round cpu millicores #7682

Uh oh!

Conversation

omerap12 commented Jan 9, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianmoisey Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianmoisey commented Jan 12, 2025

Uh oh!

adrianmoisey commented Jan 14, 2025

Uh oh!

adrianmoisey commented Jan 17, 2025

Uh oh!

alekc commented Jan 28, 2025

Uh oh!

adrianmoisey commented Jan 28, 2025

Uh oh!

omerap12 commented Jan 28, 2025

Uh oh!

raywainman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omerap12 commented Jan 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raywainman commented Jan 31, 2025

Uh oh!

k8s-ci-robot commented Jan 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adrianmoisey Jan 12, 2025 •

edited

Loading