Code review

0001-Update-vertical-pod-autoscaler-pkg-admission-control.patch

@@ -0,0 +1,28 @@
+From dd8c42bb486ad6aaa96dde56eab34786d10fd418 Mon Sep 17 00:00:00 2001
+From: David Benque <[email protected]>
+Date: Mon, 28 Aug 2023 11:07:24 +0200
+Subject: [PATCH] Update
+ vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go
+
+Co-authored-by: Joachim <[email protected]>
+---
+ .../pod/recommendation/recommendation_provider_test.go         | 3 +--
+ 1 file changed, 1 insertion(+), 2 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 f58e98f1a..36e64fad9 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
+@@ -311,8 +311,7 @@ func TestUpdateResourceRequests(t *testing.T) {
+ 					return
+ 				}
+
+-				_, foundEmpty := resources[0].Requests[""]
+-				assert.Equal(t, foundEmpty, false, "empty resourceKey have not been purged")
++				assert.Contains(t, resources, "", "expected empty resource to be removed")
+
+ 				cpuRequest := resources[0].Requests[apiv1.ResourceCPU]
+ 				assert.Equal(t, tc.expectedCPU.Value(), cpuRequest.Value(), "cpu request doesn't match")
+-- 
+2.37.3
+

vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go

@@ -62,7 +62,7 @@ func TestUpdateResourceRequests(t *testing.T) {
 		WithTarget("2", "200Mi").
 		WithMinAllowed("1", "100Mi").
 		WithMaxAllowed("3", "1Gi").
-		WithResourceInTarget("", "666") // Testing that this weird/empty resource will be purged
+		WithTargetResource("", "666") // Testing that this weird/empty resource will be purged
 	vpa := vpaBuilder.Get()
 
 	uninitialized := test.Pod().WithName("test_uninitialized").

vertical-pod-autoscaler/pkg/utils/test/test_recommendation.go

@@ -18,6 +18,7 @@ package test
 
 import (
 	apiv1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 
 	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
 )
@@ -26,13 +27,15 @@ import (
 type RecommendationBuilder interface {
 	WithContainer(containerName string) RecommendationBuilder
 	WithTarget(cpu, memory string) RecommendationBuilder
-	WithResource(resource apiv1.ResourceName, value string) RecommendationBuilder
+	WithTargetResource(resource apiv1.ResourceName, value string) RecommendationBuilder
 	WithLowerBound(cpu, memory string) RecommendationBuilder
 	WithUpperBound(cpu, memory string) RecommendationBuilder
 	Get() *vpa_types.RecommendedPodResources
 	GetContainerResources() vpa_types.RecommendedContainerResources
 }
 
+// TODO part of this interface is repeated in VerticalPodAutoscalerBuilder, we can probably factorize some code
+
 // Recommendation returns a new RecommendationBuilder.
 func Recommendation() RecommendationBuilder {
 	return &recommendationBuilder{}
@@ -57,12 +60,12 @@ func (b *recommendationBuilder) WithTarget(cpu, memory string) RecommendationBui
 	return &c
 }
 
-func (b *recommendationBuilder) WithResource(resource apiv1.ResourceName, value string) RecommendationBuilder {
+func (b *recommendationBuilder) WithTargetResource(resource apiv1.ResourceName, value string) RecommendationBuilder {
 	c := *b
 	if c.target == nil {
 		c.target = apiv1.ResourceList{}
 	}
-	AddResource(c.target, resource, value)
+	addResource(c.target, resource, value)
 	return &c
 }
 
@@ -103,3 +106,13 @@ func (b *recommendationBuilder) GetContainerResources() vpa_types.RecommendedCon
 		UpperBound:     b.upperBound,
 	}
 }
+
+// addResource add a resource to the given resource list
+func addResource(rl apiv1.ResourceList, resourceName apiv1.ResourceName, value string) apiv1.ResourceList {
+	val, _ := resource.ParseQuantity(value)
+	if rl == nil {
+		rl = apiv1.ResourceList{}
+	}
+	rl[resourceName] = val
+	return rl
+}

vertical-pod-autoscaler/pkg/utils/test/test_utils.go

@@ -74,16 +74,6 @@ func Resources(cpu, mem string) apiv1.ResourceList {
 	return result
 }
 
-// AddResource add a resource to the given resource list
-func AddResource(rl apiv1.ResourceList, resourceName apiv1.ResourceName, value string) apiv1.ResourceList {
-	val, _ := resource.ParseQuantity(value)
-	if rl == nil {
-		rl = apiv1.ResourceList{}
-	}
-	rl[resourceName] = val
-	return rl
-}
-
 // RecommenderAPIMock is a mock of RecommenderAPI
 type RecommenderAPIMock struct {
 	mock.Mock

vertical-pod-autoscaler/pkg/utils/test/test_vpa.go

@@ -37,7 +37,7 @@ type VerticalPodAutoscalerBuilder interface {
 	WithMaxAllowed(cpu, memory string) VerticalPodAutoscalerBuilder
 	WithControlledValues(mode vpa_types.ContainerControlledValues) VerticalPodAutoscalerBuilder
 	WithTarget(cpu, memory string) VerticalPodAutoscalerBuilder
-	WithResourceInTarget(resource core.ResourceName, value string) VerticalPodAutoscalerBuilder
+	WithTargetResource(resource core.ResourceName, value string) VerticalPodAutoscalerBuilder
 	WithLowerBound(cpu, memory string) VerticalPodAutoscalerBuilder
 	WithTargetRef(targetRef *autoscaling.CrossVersionObjectReference) VerticalPodAutoscalerBuilder
 	WithUpperBound(cpu, memory string) VerticalPodAutoscalerBuilder
@@ -50,6 +50,8 @@ type VerticalPodAutoscalerBuilder interface {
 	Get() *vpa_types.VerticalPodAutoscaler
 }
 
+// TODO part of this interface is a repetition of RecommendationBuilder, we can probably factorize some code
+
 // VerticalPodAutoscaler returns a new VerticalPodAutoscalerBuilder.
 func VerticalPodAutoscaler() VerticalPodAutoscalerBuilder {
 	return &verticalPodAutoscalerBuilder{
@@ -136,9 +138,9 @@ func (b *verticalPodAutoscalerBuilder) WithTarget(cpu, memory string) VerticalPo
 	return &c
 }
 
-func (b *verticalPodAutoscalerBuilder) WithResourceInTarget(resource core.ResourceName, value string) VerticalPodAutoscalerBuilder {
+func (b *verticalPodAutoscalerBuilder) WithTargetResource(resource core.ResourceName, value string) VerticalPodAutoscalerBuilder {
 	c := *b
-	c.recommendation = c.recommendation.WithResource(resource, value)
+	c.recommendation = c.recommendation.WithTargetResource(resource, value)
 	return &c
 }