Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions vertical-pod-autoscaler/docs/features.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,20 @@ Note: Due to the conversion to binary units and decimal place rounding, the huma
To enable this feature, set the `--humanize-memory` flag to true when running the VPA recommender:
```bash
--humanize-memory=true
```

## CPU Recommendation Rounding
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yup! Thank you :)


VPA can now 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:

```bash
--round-cpu-millicores=50
```
9 changes: 5 additions & 4 deletions vertical-pod-autoscaler/pkg/recommender/logic/recommender.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ var (
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.`)
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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@adrianmoisey adrianmoisey Jan 12, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

They would select the mebibyte's to round up to

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup!

Copy link
Member

Choose a reason for hiding this comment

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

Should we specify that it is always rounded up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree: 6645e94

)

// PodResourceRecommender computes resource recommendation for a Vpa object.
Expand Down Expand Up @@ -189,10 +190,10 @@ func MapToListOfRecommendedContainerResources(resources RecommendedPodResources)
for _, name := range containerNames {
containerResources = append(containerResources, vpa_types.RecommendedContainerResources{
ContainerName: name,
Target: model.ResourcesAsResourceList(resources[name].Target, *humanizeMemory),
LowerBound: model.ResourcesAsResourceList(resources[name].LowerBound, *humanizeMemory),
UpperBound: model.ResourcesAsResourceList(resources[name].UpperBound, *humanizeMemory),
UncappedTarget: model.ResourcesAsResourceList(resources[name].Target, *humanizeMemory),
Target: model.ResourcesAsResourceList(resources[name].Target, *humanizeMemory, *roundCPUMillicores),
LowerBound: model.ResourcesAsResourceList(resources[name].LowerBound, *humanizeMemory, *roundCPUMillicores),
UpperBound: model.ResourcesAsResourceList(resources[name].UpperBound, *humanizeMemory, *roundCPUMillicores),
UncappedTarget: model.ResourcesAsResourceList(resources[name].Target, *humanizeMemory, *roundCPUMillicores),
})
}
recommendation := &vpa_types.RecommendedPodResources{
Expand Down
22 changes: 21 additions & 1 deletion vertical-pod-autoscaler/pkg/recommender/model/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package model

import (
"fmt"
"math"

apiv1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -81,7 +82,7 @@ func ScaleResource(amount ResourceAmount, factor float64) ResourceAmount {
}

// ResourcesAsResourceList converts internal Resources representation to ResourcesList.
func ResourcesAsResourceList(resources Resources, humanizeMemory bool) apiv1.ResourceList {
func ResourcesAsResourceList(resources Resources, humanizeMemory bool, roundCPUMillicores int) apiv1.ResourceList {
result := make(apiv1.ResourceList)
for key, resourceAmount := range resources {
var newKey apiv1.ResourceName
Expand All @@ -90,6 +91,15 @@ func ResourcesAsResourceList(resources Resources, humanizeMemory bool) apiv1.Res
case ResourceCPU:
newKey = apiv1.ResourceCPU
quantity = QuantityFromCPUAmount(resourceAmount)
if roundCPUMillicores != 1 && !quantity.IsZero() {
roundedValues, err := RoundUpToScale(resourceAmount, roundCPUMillicores)
if err != nil {
klog.V(4).InfoS("Error rounding CPU value; leaving unchanged", "rawValue", resourceAmount, "scale", roundCPUMillicores, "error", err)
} else {
klog.V(4).InfoS("Successfully rounded CPU value", "rawValue", resourceAmount, "roundedValue", roundedValues)
}
quantity = QuantityFromCPUAmount(roundedValues)
}
case ResourceMemory:
newKey = apiv1.ResourceMemory
quantity = QuantityFromMemoryAmount(resourceAmount)
Expand Down Expand Up @@ -166,6 +176,16 @@ func HumanizeMemoryQuantity(bytes int64) string {
}
}

// RoundUpToScale rounds the value to the nearest multiple of scale, rounding up
func RoundUpToScale(value ResourceAmount, scale int) (ResourceAmount, error) {
if scale <= 0 {
return value, fmt.Errorf("scale must be greater than zero")
}
scale64 := int64(scale)
roundedValue := int64(math.Ceil(float64(value)/float64(scale64))) * scale64
return ResourceAmount(roundedValue), nil
}

// PodID contains information needed to identify a Pod within a cluster.
type PodID struct {
// Namespaces where the Pod is defined.
Expand Down
81 changes: 72 additions & 9 deletions vertical-pod-autoscaler/pkg/recommender/model/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,75 +28,81 @@ type ResourcesAsResourceListTestCase struct {
name string
resources Resources
humanize bool
roundCPU int
resourceList apiv1.ResourceList
}

func TestResourcesAsResourceList(t *testing.T) {
testCases := []ResourcesAsResourceListTestCase{
{
name: "basic resources without humanize",
name: "basic resources without humanize and no cpu rounding",
resources: Resources{
ResourceCPU: 1000,
ResourceMemory: 1000,
},
humanize: false,
roundCPU: 1,
resourceList: apiv1.ResourceList{
apiv1.ResourceCPU: *resource.NewMilliQuantity(1000, resource.DecimalSI),
apiv1.ResourceMemory: *resource.NewQuantity(1000, resource.DecimalSI),
},
},
{
name: "basic resources with humanize",
name: "basic resources with humanize and cpu rounding to 1",
resources: Resources{
ResourceCPU: 1000,
ResourceMemory: 262144000, // 250Mi
},
humanize: true,
roundCPU: 1,
resourceList: apiv1.ResourceList{
apiv1.ResourceCPU: *resource.NewMilliQuantity(1000, resource.DecimalSI),
apiv1.ResourceMemory: resource.MustParse("250.00Mi"),
},
},
{
name: "large memory value with humanize",
name: "large memory value with humanize and cpu rounding to 3",
resources: Resources{
ResourceCPU: 1000,
ResourceMemory: 839500000, // 800.61Mi
},
humanize: true,
roundCPU: 3,
resourceList: apiv1.ResourceList{
apiv1.ResourceCPU: *resource.NewMilliQuantity(1000, resource.DecimalSI),
apiv1.ResourceCPU: *resource.NewMilliQuantity(1002, resource.DecimalSI),
apiv1.ResourceMemory: resource.MustParse("800.61Mi"),
},
},
{
name: "zero values without humanize",
name: "zero values without humanize and cpu rounding to 2",
resources: Resources{
ResourceCPU: 0,
ResourceMemory: 0,
},
humanize: false,
roundCPU: 2,
resourceList: apiv1.ResourceList{
apiv1.ResourceCPU: *resource.NewMilliQuantity(0, resource.DecimalSI),
apiv1.ResourceMemory: *resource.NewQuantity(0, resource.DecimalSI),
},
},
{
name: "large memory value without humanize",
name: "large memory value without humanize and cpu rounding to 13",
resources: Resources{
ResourceCPU: 1000,
ResourceCPU: 1231241,
ResourceMemory: 839500000,
},
humanize: false,
roundCPU: 13,
resourceList: apiv1.ResourceList{
apiv1.ResourceCPU: *resource.NewMilliQuantity(1000, resource.DecimalSI),
apiv1.ResourceCPU: *resource.NewMilliQuantity(1231243, resource.DecimalSI),
apiv1.ResourceMemory: *resource.NewQuantity(839500000, resource.DecimalSI),
},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
result := ResourcesAsResourceList(tc.resources, tc.humanize)
result := ResourcesAsResourceList(tc.resources, tc.humanize, tc.roundCPU)
if !result[apiv1.ResourceCPU].Equal(tc.resourceList[apiv1.ResourceCPU]) {
t.Errorf("expected %v, got %v", tc.resourceList[apiv1.ResourceCPU], result[apiv1.ResourceCPU])
}
Expand Down Expand Up @@ -194,6 +200,63 @@ func TestHumanizeMemoryQuantity(t *testing.T) {
}
}

type TestRoundUpToScaleTestCase struct {
name string
value ResourceAmount
scale int
expected ResourceAmount
expectedErr string
}

func TestRoundUpToScale(t *testing.T) {
testsCases := []TestRoundUpToScaleTestCase{
{
name: "Round up to nearest 7",
value: ResourceAmount(100),
scale: 7,
expected: ResourceAmount(105),
expectedErr: "",
},
{
name: "Exact multiple of 10",
value: ResourceAmount(100),
scale: 10,
expected: ResourceAmount(100),
expectedErr: "",
},
{
name: "Zero value with scale 5",
value: ResourceAmount(0),
scale: 5,
expected: ResourceAmount(0),
expectedErr: "",
},
{
name: "Negative scale value",
value: ResourceAmount(100),
scale: -5,
expected: ResourceAmount(100),
expectedErr: "scale must be greater than zero",
},
{
name: "Scale is zero",
value: ResourceAmount(100),
scale: 0,
expected: ResourceAmount(100),
expectedErr: "scale must be greater than zero",
},
}
for _, tc := range testsCases {
t.Run(tc.name, func(t *testing.T) {
result, err := RoundUpToScale(tc.value, tc.scale)
assert.Equal(t, tc.expected, result)
if tc.expectedErr != "" {
assert.Equal(t, tc.expectedErr, err.Error())
}
})
}
}

type CPUAmountFromCoresTestCase struct {
name string
cores float64
Expand Down
Loading