Address review feedback from raywainman

kubernetes · Feb 10, 2025 · b7b30d4 · b7b30d4
1 parent 3314cf9
commit b7b30d4
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 23 deletions.
diff --git a/vertical-pod-autoscaler/pkg/recommender/main.go b/vertical-pod-autoscaler/pkg/recommender/main.go
@@ -118,10 +118,12 @@ const (
 	defaultResyncPeriod               time.Duration = 10 * time.Minute
 )
 
-func main() {
+func init() {
 	flag.Var(&maxAllowedCPU, "container-recommendation-max-allowed-cpu", "Maximum amount of CPU that will be recommended for a container. VerticalPodAutoscaler-level maximum allowed takes precedence over the global maximum allowed.")
 	flag.Var(&maxAllowedMemory, "container-recommendation-max-allowed-memory", "Maximum amount of memory that will be recommended for a container. VerticalPodAutoscaler-level maximum allowed takes precedence over the global maximum allowed.")
+}
 
+func main() {
 	commonFlags := common.InitCommonFlags()
 	klog.InitFlags(nil)
 	common.InitLoggingFlags()
@@ -227,14 +229,7 @@ func run(healthCheck *metrics.HealthCheck, commonFlag *common.CommonFlags) {
 		postProcessors = append(postProcessors, &routines.IntegerCPUPostProcessor{})
 	}
 
-	var globalMaxAllowed apiv1.ResourceList
-	if !maxAllowedCPU.Quantity.IsZero() {
-		setGlobalMaxAllowed(&globalMaxAllowed, apiv1.ResourceCPU, maxAllowedCPU.Quantity)
-	}
-	if !maxAllowedMemory.Quantity.IsZero() {
-		setGlobalMaxAllowed(&globalMaxAllowed, apiv1.ResourceMemory, maxAllowedMemory.Quantity)
-	}
-
+	globalMaxAllowed := initGlobalMaxAllowed()
 	// CappingPostProcessor, should always come in the last position for post-processing
 	postProcessors = append(postProcessors, routines.NewCappingRecommendationProcessor(globalMaxAllowed))
 	var source input_metrics.PodMetricsLister
@@ -331,10 +326,14 @@ func run(healthCheck *metrics.HealthCheck, commonFlag *common.CommonFlags) {
 	}
 }
 
-func setGlobalMaxAllowed(globalMaxAllowed *apiv1.ResourceList, key apiv1.ResourceName, value resource.Quantity) {
-	if *globalMaxAllowed == nil {
-		*globalMaxAllowed = make(map[apiv1.ResourceName]resource.Quantity, 2)
+func initGlobalMaxAllowed() apiv1.ResourceList {
+	result := make(apiv1.ResourceList)
+	if !maxAllowedCPU.Quantity.IsZero() {
+		result[apiv1.ResourceCPU] = maxAllowedCPU.Quantity
+	}
+	if !maxAllowedMemory.Quantity.IsZero() {
+		result[apiv1.ResourceMemory] = maxAllowedMemory.Quantity
 	}
 
-	(*globalMaxAllowed)[key] = value
+	return result
 }
diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/capping.go b/vertical-pod-autoscaler/pkg/utils/vpa/capping.go
@@ -204,15 +204,13 @@ func applyVPAPolicyForContainer(containerName string,
 				maxAllowed = containerPolicy.MaxAllowed
 			}
 
-			if globalMaxAllowed != nil {
-				if maxAllowed == nil {
-					maxAllowed = globalMaxAllowed
-				} else {
-					// Set resources from the global maxAllowed if the VPA maxAllowed is missing them.
-					for resourceName, quantity := range globalMaxAllowed {
-						if _, ok := maxAllowed[resourceName]; !ok {
-							maxAllowed[resourceName] = quantity
-						}
+			if maxAllowed == nil {
+				maxAllowed = globalMaxAllowed
+			} else {
+				// Set resources from the global maxAllowed if the VPA maxAllowed is missing them.
+				for resourceName, quantity := range globalMaxAllowed {
+					if _, ok := maxAllowed[resourceName]; !ok {
+						maxAllowed[resourceName] = quantity
 					}
 				}
 			}

diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go
@@ -411,7 +411,7 @@ func TestApplyVPAPolicy(t *testing.T) {
 			},
 		},
 		{
-			Name:              "resource policy is nil and global max allowed is set",
+			Name:              "resource policy is nil and global max allowed is set for cpu and memory",
 			PodRecommendation: recommendation,
 			ResourcePolicy:    nil,
 			GlobalMaxAllowed: apiv1.ResourceList{
@@ -484,6 +484,46 @@ func TestApplyVPAPolicy(t *testing.T) {
 				},
 			},
 		},
+		{
+			Name:              "resource policy has max allowed for cpu and global max allowed is set for memory",
+			PodRecommendation: recommendation,
+			ResourcePolicy: &vpa_types.PodResourcePolicy{
+				ContainerPolicies: []vpa_types.ContainerResourcePolicy{
+					{
+						ContainerName: "foo",
+						MaxAllowed: apiv1.ResourceList{
+							apiv1.ResourceCPU: resource.MustParse("40m"),
+						},
+					},
+				},
+			},
+			GlobalMaxAllowed: apiv1.ResourceList{
+				apiv1.ResourceMemory: resource.MustParse("40Mi"),
+			},
+			Expected: &vpa_types.RecommendedPodResources{
+				ContainerRecommendations: []vpa_types.RecommendedContainerResources{
+					{
+						ContainerName: "foo",
+						Target: apiv1.ResourceList{
+							apiv1.ResourceCPU:    resource.MustParse("40m"),
+							apiv1.ResourceMemory: resource.MustParse("40Mi"),
+						},
+						LowerBound: apiv1.ResourceList{
+							apiv1.ResourceCPU:    resource.MustParse("31m"),
+							apiv1.ResourceMemory: resource.MustParse("31Mi"),
+						},
+						UpperBound: apiv1.ResourceList{
+							apiv1.ResourceCPU:    resource.MustParse("40m"),
+							apiv1.ResourceMemory: resource.MustParse("40Mi"),
+						},
+						UncappedTarget: apiv1.ResourceList{
+							apiv1.ResourceCPU:    resource.MustParse("42m"),
+							apiv1.ResourceMemory: resource.MustParse("42Mi"),
+						},
+					},
+				},
+			},
+		},
 		{
 			Name:              "resource policy has max allowed for cpu and global max allowed is set for cpu and memory",
 			PodRecommendation: recommendation,