From b0a64f90bdbbbbfa35b1b70f429f9aac8d96cbd0 Mon Sep 17 00:00:00 2001 From: "Beata Lach (Skiba)" Date: Tue, 21 Jul 2020 18:29:46 +0200 Subject: [PATCH] Only cap to limit in RequestOnly mode --- .../recommendation/recommendation_provider.go | 11 +- vertical-pod-autoscaler/pkg/utils/vpa/api.go | 9 ++ .../pkg/utils/vpa/api_test.go | 76 +++++++++++++ .../pkg/utils/vpa/capping.go | 9 +- .../pkg/utils/vpa/capping_test.go | 103 +++++++++++++++--- 5 files changed, 178 insertions(+), 30 deletions(-) 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 ba22f2d39c9a..7f9bc504d5a8 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 @@ -60,7 +60,7 @@ func GetContainersResources(pod *core.Pod, vpaResourcePolicy *vpa_types.PodResou if limitRange != nil { defaultLimit = limitRange.Default } - containerControlledValues := GetContainerControlledValues(container.Name, vpaResourcePolicy) + containerControlledValues := vpa_api_util.GetContainerControlledValues(container.Name, vpaResourcePolicy) if containerControlledValues == vpa_types.ContainerControlledValuesRequestsAndLimits { proportionalLimits, limitAnnotations := vpa_api_util.GetProportionalLimit(container.Resources.Limits, container.Resources.Requests, recommendation.Target, defaultLimit) if proportionalLimits != nil { @@ -74,15 +74,6 @@ func GetContainersResources(pod *core.Pod, vpaResourcePolicy *vpa_types.PodResou return resources } -// GetContainerControlledValues returns controlled resource values -func GetContainerControlledValues(name string, vpaResourcePolicy *vpa_types.PodResourcePolicy) vpa_types.ContainerControlledValues { - containerPolicy := vpa_api_util.GetContainerResourcePolicy(name, vpaResourcePolicy) - if containerPolicy == nil || containerPolicy.ControlledValues == nil { - return vpa_types.ContainerControlledValuesRequestsAndLimits - } - return *containerPolicy.ControlledValues -} - // GetContainersResourcesForPod returns recommended request for a given pod and associated annotations. // The returned slice corresponds 1-1 to containers in the Pod. func (p *recommendationProvider) GetContainersResourcesForPod(pod *core.Pod, vpa *vpa_types.VerticalPodAutoscaler) ([]vpa_api_util.ContainerResources, vpa_api_util.ContainerToAnnotationsMap, error) { diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api.go b/vertical-pod-autoscaler/pkg/utils/vpa/api.go index 7eefef7dfce8..67e73d664d9a 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api.go @@ -164,6 +164,15 @@ func GetContainerResourcePolicy(containerName string, policy *vpa_types.PodResou return defaultPolicy } +// GetContainerControlledValues returns controlled resource values +func GetContainerControlledValues(name string, vpaResourcePolicy *vpa_types.PodResourcePolicy) vpa_types.ContainerControlledValues { + containerPolicy := GetContainerResourcePolicy(name, vpaResourcePolicy) + if containerPolicy == nil || containerPolicy.ControlledValues == nil { + return vpa_types.ContainerControlledValuesRequestsAndLimits + } + return *containerPolicy.ControlledValues +} + // CreateOrUpdateVpaCheckpoint updates the status field of the VPA Checkpoint API object. // If object doesn't exits it is created. func CreateOrUpdateVpaCheckpoint(vpaCheckpointClient vpa_api.VerticalPodAutoscalerCheckpointInterface, diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go index 999af9065507..19db3ab27877 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go @@ -195,3 +195,79 @@ func TestGetContainerResourcePolicy(t *testing.T) { assert.Equal(t, &containerPolicy2, GetContainerResourcePolicy("container2", &policy)) assert.Equal(t, &defaultPolicy, GetContainerResourcePolicy("container3", &policy)) } + +func TestGetContainerControlledResources(t *testing.T) { + requestsAndLimits := vpa_types.ContainerControlledValuesRequestsAndLimits + requestsOnly := vpa_types.ContainerControlledValuesRequestsOnly + for _, tc := range []struct { + name string + containerName string + policy *vpa_types.PodResourcePolicy + expected vpa_types.ContainerControlledValues + }{ + { + name: "default policy is RequestAndLimits", + containerName: "any", + policy: nil, + expected: vpa_types.ContainerControlledValuesRequestsAndLimits, + }, { + name: "container default policy is RequestsAndLimits", + containerName: "any", + policy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{{ + ContainerName: vpa_types.DefaultContainerResourcePolicy, + ControlledValues: &requestsAndLimits, + }}, + }, + expected: vpa_types.ContainerControlledValuesRequestsAndLimits, + }, { + name: "container default policy is RequestsOnly", + containerName: "any", + policy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{{ + ContainerName: vpa_types.DefaultContainerResourcePolicy, + ControlledValues: &requestsOnly, + }}, + }, + expected: vpa_types.ContainerControlledValuesRequestsOnly, + }, { + name: "RequestAndLimits is used when no policy for given container specified", + containerName: "other", + policy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{{ + ContainerName: "some", + ControlledValues: &requestsOnly, + }}, + }, + expected: vpa_types.ContainerControlledValuesRequestsAndLimits, + }, { + name: "RequestsOnly specified explicitly", + containerName: "some", + policy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{{ + ContainerName: "some", + ControlledValues: &requestsOnly, + }}, + }, + expected: vpa_types.ContainerControlledValuesRequestsOnly, + }, { + name: "RequestsAndLimits specified explicitly overrides default", + containerName: "some", + policy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{{ + ContainerName: vpa_types.DefaultContainerResourcePolicy, + ControlledValues: &requestsOnly, + }, { + ContainerName: "some", + ControlledValues: &requestsAndLimits, + }}, + }, + expected: vpa_types.ContainerControlledValuesRequestsAndLimits, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := GetContainerControlledValues(tc.containerName, tc.policy) + assert.Equal(t, got, tc.expected) + }) + } +} diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/capping.go b/vertical-pod-autoscaler/pkg/utils/vpa/capping.go index 985a32d4245f..ae481710238e 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/capping.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/capping.go @@ -109,6 +109,7 @@ func getCappedRecommendationForContainer( } // containerPolicy can be nil (user does not have to configure it). containerPolicy := GetContainerResourcePolicy(container.Name, policy) + containerControlledValues := GetContainerControlledValues(container.Name, policy) cappedRecommendations := containerRecommendation.DeepCopy() @@ -123,9 +124,11 @@ func getCappedRecommendationForContainer( cappingAnnotations = append(cappingAnnotations, annotations...) } // TODO: If limits and policy are conflicting, set some condition on the VPA. - annotations = capRecommendationToContainerLimit(recommendation, container) - if genAnnotations { - cappingAnnotations = append(cappingAnnotations, annotations...) + if containerControlledValues == vpa_types.ContainerControlledValuesRequestsOnly { + annotations = capRecommendationToContainerLimit(recommendation, container) + if genAnnotations { + cappingAnnotations = append(cappingAnnotations, annotations...) + } } } diff --git a/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go b/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go index 6c8d2eb5a97d..ec67e05ef62a 100644 --- a/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go +++ b/vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go @@ -26,6 +26,11 @@ import ( "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test" ) +var ( + requestsAndLimits vpa_types.ContainerControlledValues = vpa_types.ContainerControlledValuesRequestsAndLimits + requestsOnly vpa_types.ContainerControlledValues = vpa_types.ContainerControlledValuesRequestsOnly +) + func TestRecommendationNotAvailable(t *testing.T) { pod := test.Pod().WithName("pod1").AddContainer(test.BuildTestContainer("ctr-name", "", "")).Get() podRecommendation := vpa_types.RecommendedPodResources{ @@ -47,7 +52,7 @@ func TestRecommendationNotAvailable(t *testing.T) { assert.Empty(t, res.ContainerRecommendations) } -func TestRecommendationCappedToLimit(t *testing.T) { +func TestRecommendationToLimitCapping(t *testing.T) { pod := test.Pod().WithName("pod1").AddContainer(test.BuildTestContainer("ctr-name", "", "")).Get() pod.Spec.Containers[0].Resources.Limits = apiv1.ResourceList{ @@ -59,31 +64,87 @@ func TestRecommendationCappedToLimit(t *testing.T) { { ContainerName: "ctr-name", Target: apiv1.ResourceList{ - apiv1.ResourceCPU: *resource.NewScaledQuantity(10, 1), + apiv1.ResourceCPU: *resource.NewScaledQuantity(2, 1), apiv1.ResourceMemory: *resource.NewScaledQuantity(8000, 1), }, UpperBound: apiv1.ResourceList{ - apiv1.ResourceCPU: *resource.NewScaledQuantity(2, 1), + apiv1.ResourceCPU: *resource.NewScaledQuantity(10, 1), apiv1.ResourceMemory: *resource.NewScaledQuantity(9000, 1), }, }, }, } - policy := vpa_types.PodResourcePolicy{} - res, annotations, err := NewCappingRecommendationProcessor(&fakeLimitRangeCalculator{}).Apply(&podRecommendation, &policy, nil, pod) - assert.Nil(t, err) - assert.Equal(t, apiv1.ResourceList{ - apiv1.ResourceCPU: *resource.NewScaledQuantity(3, 1), - apiv1.ResourceMemory: *resource.NewScaledQuantity(7000, 1), - }, res.ContainerRecommendations[0].Target) + requestsAndLimits := vpa_types.ContainerControlledValuesRequestsAndLimits + requestsOnly := vpa_types.ContainerControlledValuesRequestsOnly + for _, tc := range []struct { + name string + policy vpa_types.PodResourcePolicy + expectedTarget apiv1.ResourceList + expectedUpperBound apiv1.ResourceList + expectedAnnotation bool + }{ + { + name: "no capping for default policy", + policy: vpa_types.PodResourcePolicy{}, + expectedTarget: apiv1.ResourceList{ + apiv1.ResourceCPU: *resource.NewScaledQuantity(2, 1), + apiv1.ResourceMemory: *resource.NewScaledQuantity(8000, 1), + }, + expectedUpperBound: apiv1.ResourceList{ + apiv1.ResourceCPU: *resource.NewScaledQuantity(10, 1), + apiv1.ResourceMemory: *resource.NewScaledQuantity(9000, 1), + }, + }, { + name: "no capping for RequestsAndLimits policy", + policy: vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{{ + ContainerName: vpa_types.DefaultContainerResourcePolicy, + ControlledValues: &requestsAndLimits, + }}, + }, + expectedTarget: apiv1.ResourceList{ + apiv1.ResourceCPU: *resource.NewScaledQuantity(2, 1), + apiv1.ResourceMemory: *resource.NewScaledQuantity(8000, 1), + }, + expectedUpperBound: apiv1.ResourceList{ + apiv1.ResourceCPU: *resource.NewScaledQuantity(10, 1), + apiv1.ResourceMemory: *resource.NewScaledQuantity(9000, 1), + }, + }, { + name: "capping for RequestsOnly policy", + policy: vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{{ + ContainerName: vpa_types.DefaultContainerResourcePolicy, + ControlledValues: &requestsOnly, + }}, + }, + expectedTarget: apiv1.ResourceList{ + apiv1.ResourceCPU: *resource.NewScaledQuantity(2, 1), + apiv1.ResourceMemory: *resource.NewScaledQuantity(7000, 1), + }, + expectedUpperBound: apiv1.ResourceList{ + apiv1.ResourceCPU: *resource.NewScaledQuantity(3, 1), + apiv1.ResourceMemory: *resource.NewScaledQuantity(7000, 1), + }, + expectedAnnotation: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + res, annotations, err := NewCappingRecommendationProcessor(&fakeLimitRangeCalculator{}).Apply(&podRecommendation, &tc.policy, nil, pod) + assert.Nil(t, err) + assert.Equal(t, tc.expectedTarget, res.ContainerRecommendations[0].Target) - assert.Contains(t, annotations, "ctr-name") - assert.Contains(t, annotations["ctr-name"], "memory capped to container limit") + if tc.expectedAnnotation { + assert.Contains(t, annotations, "ctr-name") + assert.Contains(t, annotations["ctr-name"], "memory capped to container limit") + } else { + assert.NotContains(t, annotations, "ctr-name") + } - assert.Equal(t, apiv1.ResourceList{ - apiv1.ResourceCPU: *resource.NewScaledQuantity(2, 1), - apiv1.ResourceMemory: *resource.NewScaledQuantity(7000, 1), - }, res.ContainerRecommendations[0].UpperBound) + assert.Equal(t, tc.expectedUpperBound, res.ContainerRecommendations[0].UpperBound) + + }) + } } func TestRecommendationCappedToMinMaxPolicy(t *testing.T) { @@ -605,11 +666,13 @@ func TestApplyPodLimitRange(t *testing.T) { } func TestApplyLimitRangeMinToRequest(t *testing.T) { + requestsOnly := vpa_types.ContainerControlledValuesRequestsOnly tests := []struct { name string resources vpa_types.RecommendedPodResources pod apiv1.Pod limitRange apiv1.LimitRangeItem + policy *vpa_types.PodResourcePolicy expect vpa_types.RecommendedPodResources expectAnnotations []string }{ @@ -703,6 +766,12 @@ func TestApplyLimitRangeMinToRequest(t *testing.T) { apiv1.ResourceMemory: resource.MustParse("500M"), }, }, + policy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{{ + ContainerName: vpa_types.DefaultContainerResourcePolicy, + ControlledValues: &requestsOnly, + }}, + }, expect: vpa_types.RecommendedPodResources{ ContainerRecommendations: []vpa_types.RecommendedContainerResources{ { @@ -724,7 +793,7 @@ func TestApplyLimitRangeMinToRequest(t *testing.T) { t.Run(tc.name, func(t *testing.T) { calculator := fakeLimitRangeCalculator{containerLimitRange: tc.limitRange} processor := NewCappingRecommendationProcessor(&calculator) - processedRecommendation, annotations, err := processor.Apply(&tc.resources, nil, nil, &tc.pod) + processedRecommendation, annotations, err := processor.Apply(&tc.resources, tc.policy, nil, &tc.pod) assert.NoError(t, err) assert.Contains(t, annotations, "container") assert.ElementsMatch(t, tc.expectAnnotations, annotations["container"])