Skip to content

Commit

Permalink
Merge pull request #3340 from bskiba/fix-limit-capping
Browse files Browse the repository at this point in the history 
Only cap to limit in RequestOnly mode
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jul 22, 2020
2 parents 2d189bb + b0a64f9 commit ebbc578
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 30 deletions.
11 changes: 1 addition & 10 deletions ...utoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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) {
Expand Down
9 changes: 9 additions & 0 deletions vertical-pod-autoscaler/pkg/utils/vpa/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
76 changes: 76 additions & 0 deletions vertical-pod-autoscaler/pkg/utils/vpa/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
9 changes: 6 additions & 3 deletions vertical-pod-autoscaler/pkg/utils/vpa/capping.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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...)
}
}
}

Expand Down
103 changes: 86 additions & 17 deletions vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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) {
Expand Down Expand Up @@ -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
}{
Expand Down Expand Up @@ -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{
{
Expand All @@ -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"])
Expand Down

0 comments on commit ebbc578

Please sign in to comment.