From c18cf2e2bfb9eb991b29da9652122101e45b21c9 Mon Sep 17 00:00:00 2001 From: Luis Ramirez Date: Wed, 2 Jun 2021 16:26:22 -0400 Subject: [PATCH] Separate limits scaling between CPU & memory Treating them both the same would cause issues when the ratio between the requests and the limits is a floating-point value, suggesting a millivalue as the limit for memory. --- .../recommendation_provider_test.go | 2 +- .../pkg/utils/vpa/capping.go | 12 +-- .../pkg/utils/vpa/capping_test.go | 6 +- .../utils/vpa/limit_and_request_scaling.go | 48 ++++++++-- .../vpa/limit_and_request_scaling_test.go | 90 ++++++++++++++++++- 5 files changed, 142 insertions(+), 16 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go index 6091b24725f6..73338fde7ce2 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go @@ -256,7 +256,7 @@ func TestUpdateResourceRequests(t *testing.T) { expectedCPU: resource.MustParse("1Ei"), expectedMem: resource.MustParse("1Ei"), expectedCPULimit: resource.NewMilliQuantity(math.MaxInt64, resource.DecimalExponent), - expectedMemLimit: resource.NewMilliQuantity(math.MaxInt64, resource.DecimalExponent), + expectedMemLimit: resource.NewQuantity(math.MaxInt64, resource.DecimalExponent), annotations: vpa_api_util.ContainerToAnnotationsMap{ containerName: []string{ "cpu: failed to keep limit to request ratio; capping limit to int64", diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/capping.go b/vertical-pod-autoscaler/pkg/utils/vpa/capping.go index 26df99459542..d6acc673ac47 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/capping.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/capping.go @@ -335,8 +335,8 @@ func getBoundaryRecommendation(recommendation apiv1.ResourceList, container apiv if boundaryLimit == nil { return apiv1.ResourceList{} } - boundaryCpu := GetBoundaryRequest(container.Resources.Requests.Cpu(), container.Resources.Limits.Cpu(), boundaryLimit.Cpu(), defaultLimit.Cpu()) - boundaryMem := GetBoundaryRequest(container.Resources.Requests.Memory(), container.Resources.Limits.Memory(), boundaryLimit.Memory(), defaultLimit.Memory()) + boundaryCpu := GetBoundaryRequest(apiv1.ResourceCPU, container.Resources.Requests.Cpu(), container.Resources.Limits.Cpu(), boundaryLimit.Cpu(), defaultLimit.Cpu()) + boundaryMem := GetBoundaryRequest(apiv1.ResourceMemory, container.Resources.Requests.Memory(), container.Resources.Limits.Memory(), boundaryLimit.Memory(), defaultLimit.Memory()) return apiv1.ResourceList{ apiv1.ResourceCPU: *boundaryCpu, apiv1.ResourceMemory: *boundaryMem, @@ -373,9 +373,9 @@ func applyPodLimitRange(resources []vpa_types.RecommendedContainerResources, request := (*fieldGetter(resources[i]))[resourceName] var cappedContainerRequest *resource.Quantity if resourceName == apiv1.ResourceMemory { - cappedContainerRequest, _ = scaleQuantityProportionally(&request, &sumRecommendation, &minLimit, roundUpToFullUnit) + cappedContainerRequest, _ = scaleQuantityProportionallyMem(&request, &sumRecommendation, &minLimit, roundUpToFullUnit) } else { - cappedContainerRequest, _ = scaleQuantityProportionally(&request, &sumRecommendation, &minLimit, noRounding) + cappedContainerRequest, _ = scaleQuantityProportionallyCPU(&request, &sumRecommendation, &minLimit, noRounding) } (*fieldGetter(resources[i]))[resourceName] = *cappedContainerRequest } @@ -397,9 +397,9 @@ func applyPodLimitRange(resources []vpa_types.RecommendedContainerResources, limit := (*fieldGetter(resources[i]))[resourceName] var cappedContainerRequest *resource.Quantity if resourceName == apiv1.ResourceMemory { - cappedContainerRequest, _ = scaleQuantityProportionally(&limit, &sumLimit, &targetTotalLimit, roundDownToFullUnit) + cappedContainerRequest, _ = scaleQuantityProportionallyMem(&limit, &sumLimit, &targetTotalLimit, roundDownToFullUnit) } else { - cappedContainerRequest, _ = scaleQuantityProportionally(&limit, &sumLimit, &targetTotalLimit, noRounding) + cappedContainerRequest, _ = scaleQuantityProportionallyCPU(&limit, &sumLimit, &targetTotalLimit, noRounding) } (*fieldGetter(resources[i]))[resourceName] = *cappedContainerRequest } diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go index bf4e89f39cd7..a791d1373912 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go @@ -375,7 +375,7 @@ func TestApplyCapsToLimitRange(t *testing.T) { ContainerName: "container", Target: apiv1.ResourceList{ apiv1.ResourceCPU: resource.MustParse("1000m"), - apiv1.ResourceMemory: resource.MustParse("500000000000m"), + apiv1.ResourceMemory: resource.MustParse("500000000"), }, }, }, @@ -895,13 +895,13 @@ func TestCapPodMemoryWithUnderByteSplit(t *testing.T) { { ContainerName: "container-1", Target: apiv1.ResourceList{ - apiv1.ResourceMemory: *resource.NewQuantity(1431655766, resource.BinarySI), + apiv1.ResourceMemory: *resource.NewQuantity(1431655765, resource.BinarySI), }, }, { ContainerName: "container-2", Target: apiv1.ResourceList{ - apiv1.ResourceMemory: *resource.NewQuantity(2863311531, resource.BinarySI), + apiv1.ResourceMemory: *resource.NewQuantity(2863311530, resource.BinarySI), }, }, }, diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/limit_and_request_scaling.go b/vertical-pod-autoscaler/pkg/utils/vpa/limit_and_request_scaling.go index 9bcfe65fcd72..7262dd6d2e79 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/limit_and_request_scaling.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/limit_and_request_scaling.go @@ -85,7 +85,15 @@ func getProportionalResourceLimit(resourceName core.ResourceName, originalLimit, result := *recommendedRequest return &result, "" } - result, capped := scaleQuantityProportionally( /*scaledQuantity=*/ originalLimit /*scaleBase=*/, originalRequest /*scaleResult=*/, recommendedRequest, noRounding) + if resourceName == core.ResourceCPU { + result, capped := scaleQuantityProportionallyCPU( /*scaledQuantity=*/ originalLimit /*scaleBase=*/, originalRequest /*scaleResult=*/, recommendedRequest, noRounding) + if !capped { + return result, "" + } + return result, fmt.Sprintf( + "%v: failed to keep limit to request ratio; capping limit to int64", resourceName) + } + result, capped := scaleQuantityProportionallyMem( /*scaledQuantity=*/ originalLimit /*scaleBase=*/, originalRequest /*scaleResult=*/, recommendedRequest, noRounding) if !capped { return result, "" } @@ -95,7 +103,7 @@ func getProportionalResourceLimit(resourceName core.ResourceName, originalLimit, // GetBoundaryRequest returns the boundary (min/max) request that can be specified with // preserving the original limit to request ratio. Returns nil if no boundary exists -func GetBoundaryRequest(originalRequest, originalLimit, boundaryLimit, defaultLimit *resource.Quantity) *resource.Quantity { +func GetBoundaryRequest(resourceName core.ResourceName, originalRequest, originalLimit, boundaryLimit, defaultLimit *resource.Quantity) *resource.Quantity { if originalLimit == nil || originalLimit.Value() == 0 && defaultLimit != nil { originalLimit = defaultLimit } @@ -107,7 +115,14 @@ func GetBoundaryRequest(originalRequest, originalLimit, boundaryLimit, defaultLi if originalRequest == nil || originalRequest.Value() == 0 { return boundaryLimit } - result, _ := scaleQuantityProportionally(originalRequest /* scaledQuantity */, originalLimit /*scaleBase*/, boundaryLimit /*scaleResult*/, noRounding) + + // Determine which scaling function to use based on resource type. + var result *resource.Quantity + if resourceName == core.ResourceCPU { + result, _ = scaleQuantityProportionallyCPU(originalRequest /* scaledQuantity */, originalLimit /*scaleBase*/, boundaryLimit /*scaleResult*/, noRounding) + return result + } + result, _ = scaleQuantityProportionallyMem(originalRequest /* scaledQuantity */, originalLimit /*scaleBase*/, boundaryLimit /*scaleResult*/, noRounding) return result } @@ -119,9 +134,9 @@ const ( roundDownToFullUnit ) -// scaleQuantityProportionally returns value which has the same proportion to scaledQuantity as scaleResult has to scaleBase +// scaleQuantityProportionallyCPU returns a value in milliunits which has the same proportion to scaledQuantity as scaleResult has to scaleBase. // It also returns a bool indicating if it had to cap result to MaxInt64 milliunits. -func scaleQuantityProportionally(scaledQuantity, scaleBase, scaleResult *resource.Quantity, rounding roundingMode) (*resource.Quantity, bool) { +func scaleQuantityProportionallyCPU(scaledQuantity, scaleBase, scaleResult *resource.Quantity, rounding roundingMode) (*resource.Quantity, bool) { originalMilli := big.NewInt(scaledQuantity.MilliValue()) scaleBaseMilli := big.NewInt(scaleBase.MilliValue()) scaleResultMilli := big.NewInt(scaleResult.MilliValue()) @@ -141,3 +156,26 @@ func scaleQuantityProportionally(scaledQuantity, scaleBase, scaleResult *resourc } return resource.NewMilliQuantity(math.MaxInt64, scaledQuantity.Format), true } + +// scaleQuantityProportionallyMem returns a value in whole units which has the same proportion to scaledQuantity as scaleResult has to scaleBase. +// It also returns a bool indicating if it had to cap result to MaxInt64 units. +func scaleQuantityProportionallyMem(scaledQuantity, scaleBase, scaleResult *resource.Quantity, rounding roundingMode) (*resource.Quantity, bool) { + originalValue := big.NewInt(scaledQuantity.Value()) + scaleBaseValue := big.NewInt(scaleBase.Value()) + scaleResultValue := big.NewInt(scaleResult.Value()) + var scaledOriginal big.Int + scaledOriginal.Mul(originalValue, scaleResultValue) + scaledOriginal.Div(&scaledOriginal, scaleBaseValue) + if scaledOriginal.IsInt64() { + result := resource.NewQuantity(scaledOriginal.Int64(), scaledQuantity.Format) + if rounding == roundUpToFullUnit { + result.RoundUp(resource.Scale(0)) + } + if rounding == roundDownToFullUnit { + result.Sub(*resource.NewMilliQuantity(999, result.Format)) + result.RoundUp(resource.Scale(0)) + } + return result, false + } + return resource.NewQuantity(math.MaxInt64, scaledQuantity.Format), true +} diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/limit_and_request_scaling_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/limit_and_request_scaling_test.go index 43dc9b222eee..6bd8a8f8873e 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/limit_and_request_scaling_test.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/limit_and_request_scaling_test.go @@ -31,7 +31,7 @@ func mustParseToPointer(str string) *resource.Quantity { return &val } -func TestGetProportionalResourceLimit(t *testing.T) { +func TestGetProportionalResourceLimitCPU(t *testing.T) { tests := []struct { name string originalLimit *resource.Quantity @@ -81,6 +81,13 @@ func TestGetProportionalResourceLimit(t *testing.T) { recommendedRequest: mustParseToPointer("10"), expectLimit: mustParseToPointer("10"), }, + { + name: "handle non-integer ratios", + originalLimit: mustParseToPointer("2"), + originalRequest: mustParseToPointer("1.50"), + recommendedRequest: mustParseToPointer("1"), + expectLimit: mustParseToPointer("1.333"), + }, { name: "go over milli cap", originalLimit: mustParseToPointer("10G"), @@ -104,3 +111,84 @@ func TestGetProportionalResourceLimit(t *testing.T) { }) } } + +func TestGetProportionalResourceLimitMem(t *testing.T) { + tests := []struct { + name string + originalLimit *resource.Quantity + originalRequest *resource.Quantity + recommendedRequest *resource.Quantity + defaultLimit *resource.Quantity + expectLimit *resource.Quantity + expectAnnotation bool + }{ + { + name: "scale proportionally", + originalLimit: mustParseToPointer("2"), + originalRequest: mustParseToPointer("1"), + recommendedRequest: mustParseToPointer("10"), + expectLimit: mustParseToPointer("20"), + }, + { + name: "scale proportionally with default", + originalRequest: mustParseToPointer("1"), + recommendedRequest: mustParseToPointer("10"), + defaultLimit: mustParseToPointer("2"), + expectLimit: mustParseToPointer("20"), + }, + { + name: "no original limit", + originalRequest: mustParseToPointer("1"), + recommendedRequest: mustParseToPointer("10"), + expectLimit: nil, + }, + { + name: "no original request", + originalLimit: mustParseToPointer("2"), + recommendedRequest: mustParseToPointer("10"), + expectLimit: mustParseToPointer("10"), + }, + { + name: "no recommendation", + originalRequest: mustParseToPointer("1"), + recommendedRequest: mustParseToPointer("0"), + defaultLimit: mustParseToPointer("2"), + expectLimit: nil, + }, + { + name: "limit equal to request", + originalLimit: mustParseToPointer("1"), + originalRequest: mustParseToPointer("1"), + recommendedRequest: mustParseToPointer("10"), + expectLimit: mustParseToPointer("10"), + }, + { + name: "handle non-integer ratios", + originalLimit: mustParseToPointer("200"), + originalRequest: mustParseToPointer("150"), + recommendedRequest: mustParseToPointer("100"), + expectLimit: mustParseToPointer("133"), + }, + { + name: "go over milli cap", + originalLimit: mustParseToPointer("10G"), + originalRequest: mustParseToPointer("1m"), + recommendedRequest: mustParseToPointer("10G"), + expectLimit: resource.NewQuantity(math.MaxInt64, resource.DecimalExponent), + expectAnnotation: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gotLimit, gotAnnotation := getProportionalResourceLimit(core.ResourceMemory, tc.originalLimit, tc.originalRequest, tc.recommendedRequest, tc.defaultLimit) + if tc.expectLimit == nil { + assert.Nil(t, gotLimit) + } else { + if assert.NotNil(t, gotLimit) { + assert.Equal(t, gotLimit.MilliValue(), tc.expectLimit.MilliValue()) + } + } + assert.Equal(t, gotAnnotation != "", tc.expectAnnotation) + }) + } +}