diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go index cfc37c6a36e6..3c93338f6716 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go @@ -20,10 +20,11 @@ import ( "fmt" core "k8s.io/api/core/v1" + "k8s.io/klog/v2" + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/limitrange" vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa" - "k8s.io/klog/v2" ) // Provider gets current recommendation, annotations and vpaName for the given pod. @@ -109,5 +110,13 @@ func (p *recommendationProvider) GetContainersResourcesForPod(pod *core.Pod, vpa resourcePolicy = vpa.Spec.ResourcePolicy } containerResources := GetContainersResources(pod, resourcePolicy, *recommendedPodResources, containerLimitRange, false, annotations) + + // Ensure that we are not propagating empty resource key if any. + for _, resource := range containerResources { + if resource.RemoveEmptyResourceKeyIfAny() { + klog.Error("An empty resource key was found and purged for pod=%s/%s with vpa=", pod.Namespace, pod.Name, vpa.Name) + } + } + return containerResources, annotations, nil } 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 73338fde7ce2..f58e98f1a88e 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 @@ -23,6 +23,7 @@ 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" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/limitrange" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test" @@ -60,7 +61,8 @@ func TestUpdateResourceRequests(t *testing.T) { WithContainer(containerName). WithTarget("2", "200Mi"). WithMinAllowed("1", "100Mi"). - WithMaxAllowed("3", "1Gi") + WithMaxAllowed("3", "1Gi"). + WithResourceInTarget("", "666") // Testing that this weird/empty resource will be purged vpa := vpaBuilder.Get() uninitialized := test.Pod().WithName("test_uninitialized"). @@ -309,6 +311,9 @@ func TestUpdateResourceRequests(t *testing.T) { return } + _, foundEmpty := resources[0].Requests[""] + assert.Equal(t, foundEmpty, false, "empty resourceKey have not been purged") + cpuRequest := resources[0].Requests[apiv1.ResourceCPU] assert.Equal(t, tc.expectedCPU.Value(), cpuRequest.Value(), "cpu request doesn't match") diff --git a/vertical-pod-autoscaler/pkg/utils/test/test_recommendation.go b/vertical-pod-autoscaler/pkg/utils/test/test_recommendation.go index 410ca9a91b5b..9c67aa430d26 100644 --- a/vertical-pod-autoscaler/pkg/utils/test/test_recommendation.go +++ b/vertical-pod-autoscaler/pkg/utils/test/test_recommendation.go @@ -18,6 +18,7 @@ package test import ( apiv1 "k8s.io/api/core/v1" + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" ) @@ -25,6 +26,7 @@ import ( type RecommendationBuilder interface { WithContainer(containerName string) RecommendationBuilder WithTarget(cpu, memory string) RecommendationBuilder + WithResource(resource apiv1.ResourceName, value string) RecommendationBuilder WithLowerBound(cpu, memory string) RecommendationBuilder WithUpperBound(cpu, memory string) RecommendationBuilder Get() *vpa_types.RecommendedPodResources @@ -55,6 +57,15 @@ func (b *recommendationBuilder) WithTarget(cpu, memory string) RecommendationBui return &c } +func (b *recommendationBuilder) WithResource(resource apiv1.ResourceName, value string) RecommendationBuilder { + c := *b + if c.target == nil { + c.target = apiv1.ResourceList{} + } + AddResource(c.target, resource, value) + return &c +} + func (b *recommendationBuilder) WithLowerBound(cpu, memory string) RecommendationBuilder { c := *b c.lowerBound = Resources(cpu, memory) diff --git a/vertical-pod-autoscaler/pkg/utils/test/test_utils.go b/vertical-pod-autoscaler/pkg/utils/test/test_utils.go index c21974873cce..b4f103e8a642 100644 --- a/vertical-pod-autoscaler/pkg/utils/test/test_utils.go +++ b/vertical-pod-autoscaler/pkg/utils/test/test_utils.go @@ -26,12 +26,13 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/record" + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" vpa_types_v1beta1 "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1beta1" vpa_lister "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/autoscaling.k8s.io/v1" vpa_lister_v1beta1 "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/autoscaling.k8s.io/v1beta1" - "k8s.io/client-go/listers/core/v1" - "k8s.io/client-go/tools/record" ) var ( @@ -89,6 +90,16 @@ 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 diff --git a/vertical-pod-autoscaler/pkg/utils/test/test_vpa.go b/vertical-pod-autoscaler/pkg/utils/test/test_vpa.go index 3401487d7b9d..c6bbf7fd89bd 100644 --- a/vertical-pod-autoscaler/pkg/utils/test/test_vpa.go +++ b/vertical-pod-autoscaler/pkg/utils/test/test_vpa.go @@ -22,6 +22,7 @@ import ( autoscaling "k8s.io/api/autoscaling/v1" core "k8s.io/api/core/v1" meta "k8s.io/apimachinery/pkg/apis/meta/v1" + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" ) @@ -36,6 +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 WithLowerBound(cpu, memory string) VerticalPodAutoscalerBuilder WithTargetRef(targetRef *autoscaling.CrossVersionObjectReference) VerticalPodAutoscalerBuilder WithUpperBound(cpu, memory string) VerticalPodAutoscalerBuilder @@ -131,6 +133,12 @@ func (b *verticalPodAutoscalerBuilder) WithTarget(cpu, memory string) VerticalPo return &c } +func (b *verticalPodAutoscalerBuilder) WithResourceInTarget(resource core.ResourceName, value string) VerticalPodAutoscalerBuilder { + c := *b + c.recommendation = c.recommendation.WithResource(resource, value) + return &c +} + func (b *verticalPodAutoscalerBuilder) WithLowerBound(cpu, memory string) VerticalPodAutoscalerBuilder { c := *b c.recommendation = c.recommendation.WithLowerBound(cpu, memory) 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 c743d2fd4f47..40cd3d284b9c 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 @@ -172,3 +172,17 @@ func scaleQuantityProportionallyMem(scaledQuantity, scaleBase, scaleResult *reso } return resource.NewQuantity(math.MaxInt64, scaledQuantity.Format), true } + +// RemoveEmptyResourceKeyIfAny ensure that we are not pushing a resource with an empty key. Return true if an empty key was eliminated +func (cr *ContainerResources) RemoveEmptyResourceKeyIfAny() bool { + var found bool + if _, foundEmptyKey := cr.Limits[""]; foundEmptyKey { + delete(cr.Limits, "") + found = true + } + if _, foundEmptyKey := cr.Requests[""]; foundEmptyKey { + delete(cr.Requests, "") + found = true + } + return found +}