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.

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",

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
 	}

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
+}

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)
+		})
+	}
+}