Merge pull request #676 from tkulczynski/request-limit

Make requests smaller or equal to limits

vertical-pod-autoscaler/admission-controller/logic/recommendation_provider.go

@@ -41,76 +41,20 @@ func NewRecommendationProvider(vpaLister vpa_lister.VerticalPodAutoscalerLister)
 	return &recommendationProvider{vpaLister: vpaLister}
 }
 
-// getRecomendedResources overwrites pod resources Request field with recommended values.
+// getRecomendedResources returns the recommended resources Request for each container in the given pod in the same order they are specified in the pod.Spec.
 func getRecomendedResources(pod *v1.Pod, podRecommendation vpa_types.RecommendedPodResources, policy vpa_types.PodResourcePolicy) []v1.ResourceList {
 	res := make([]v1.ResourceList, len(pod.Spec.Containers))
 	for i, container := range pod.Spec.Containers {
-		containerRecommendation := getRecommendationForContainer(podRecommendation, container)
+		recommendation, err := vpa_api_util.GetCappedRecommendationForContainer(container, &podRecommendation, &policy)
-		if containerRecommendation == nil {
+		if err != nil {
+			glog.V(2).Infof("%v", err)
 			continue
 		}
-		containerPolicy := getContainerPolicy(container.Name, &policy)
+		res[i] = recommendation
-		applyVPAPolicy(containerRecommendation, containerPolicy)
-		res[i] = make(v1.ResourceList)
-		for resource, recommended := range containerRecommendation.Target {
-			requested, exists := container.Resources.Requests[resource]
-			if exists {
-				// overwriting existing resource spec
-				glog.V(2).Infof("updating resources request for pod %v container %v resource %v old value: %v new value: %v",
-					pod.Name, container.Name, resource, requested, recommended)
-			} else {
-				// adding new resource spec
-				glog.V(2).Infof("updating resources request for pod %v container %v resource %v old value: none new value: %v",
-					pod.Name, container.Name, resource, recommended)
-			}
-
-			res[i][resource] = recommended
-		}
 	}
 	return res
 }
 
-// applyVPAPolicy updates recommendation if recommended resources exceed limits defined in VPA resources policy
-func applyVPAPolicy(recommendation *vpa_types.RecommendedContainerResources, policy *vpa_types.ContainerResourcePolicy) {
-	for resourceName, recommended := range recommendation.Target {
-		if policy == nil {
-			continue
-		}
-		min, found := policy.MinAllowed[resourceName]
-		if found && !min.IsZero() && recommended.Value() < min.Value() {
-			glog.Warningf("recommendation outside of policy bounds : min value : %v recommended : %v",
-				min.Value(), recommended)
-			recommendation.Target[resourceName] = min
-		}
-		max, found := policy.MaxAllowed[resourceName]
-		if found && !max.IsZero() && recommended.Value() > max.Value() {
-			glog.Warningf("recommendation outside of policy bounds : max value : %v recommended : %v",
-				max.Value(), recommended)
-			recommendation.Target[resourceName] = max
-		}
-	}
-}
-
-func getRecommendationForContainer(recommendation vpa_types.RecommendedPodResources, container v1.Container) *vpa_types.RecommendedContainerResources {
-	for i, containerRec := range recommendation.ContainerRecommendations {
-		if containerRec.Name == container.Name {
-			return &recommendation.ContainerRecommendations[i]
-		}
-	}
-	return nil
-}
-
-func getContainerPolicy(containerName string, policy *vpa_types.PodResourcePolicy) *vpa_types.ContainerResourcePolicy {
-	if policy != nil {
-		for i, container := range policy.ContainerPolicies {
-			if containerName == container.Name {
-				return &policy.ContainerPolicies[i]
-			}
-		}
-	}
-	return nil
-}
-
 func (p *recommendationProvider) getMatchingVPA(pod *v1.Pod) *vpa_types.VerticalPodAutoscaler {
 	configs, err := p.vpaLister.VerticalPodAutoscalers(pod.Namespace).List(labels.Everything())
 	if err != nil {

vertical-pod-autoscaler/pkg/utils/vpa/capping.go

@@ -0,0 +1,96 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package api
+
+import (
+	"fmt"
+
+	apiv1 "k8s.io/api/core/v1"
+	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/poc.autoscaling.k8s.io/v1alpha1"
+)
+
+// GetCappedRecommendationForContainer returns a recommendation extracted for the given container, adjusted to obey policy and limits.
+func GetCappedRecommendationForContainer(
+	container apiv1.Container,
+	podRecommendation *vpa_types.RecommendedPodResources,
+	policy *vpa_types.PodResourcePolicy) (apiv1.ResourceList, error) {
+	containerRecommendation := getRecommendationForContainer(podRecommendation, container)
+	if containerRecommendation == nil {
+		return nil, fmt.Errorf("no recommendation available for container name %v", container.Name)
+	}
+	// containerPolicy can be nil (user does not have to configure it).
+	containerPolicy := getContainerPolicy(container.Name, policy)
+	applyVPAPolicy(containerRecommendation, containerPolicy)
+	// TODO: If limits and policy are conflicting, set some condition on the VPA.
+	capRecommendationToContainerLimit(containerRecommendation, container)
+	res := make(apiv1.ResourceList)
+	for resource, recommended := range containerRecommendation.Target {
+		res[resource] = recommended
+	}
+	return res, nil
+}
+
+// capRecommendationToContainerLimit makes sure recommendation is not above current limit for the container.
+func capRecommendationToContainerLimit(recommendation *vpa_types.RecommendedContainerResources, container apiv1.Container) {
+	// Iterate over limits set in the container. Unset means Infinite limit.
+	for resourceName, limit := range container.Resources.Limits {
+		target, found := recommendation.Target[resourceName]
+		if found && target.MilliValue() > limit.MilliValue() {
+			recommendation.Target[resourceName] = limit
+		}
+	}
+}
+
+// applyVPAPolicy updates recommendation if recommended resources are outside of limits defined in VPA resources policy
+func applyVPAPolicy(recommendation *vpa_types.RecommendedContainerResources, policy *vpa_types.ContainerResourcePolicy) {
+	if policy == nil {
+		return
+	}
+	for resourceName, recommended := range recommendation.Target {
+		min, found := policy.MinAllowed[resourceName]
+		if found && !min.IsZero() && recommended.MilliValue() < min.MilliValue() {
+			recommendation.Target[resourceName] = min
+		}
+		max, found := policy.MaxAllowed[resourceName]
+		if found && !max.IsZero() && recommended.MilliValue() > max.MilliValue() {
+			recommendation.Target[resourceName] = max
+		}
+	}
+}
+
+func getRecommendationForContainer(recommendation *vpa_types.RecommendedPodResources, container apiv1.Container) *vpa_types.RecommendedContainerResources {
+	if recommendation != nil {
+		for i, containerRec := range recommendation.ContainerRecommendations {
+			if containerRec.Name == container.Name {
+				recommendationCopy := recommendation.ContainerRecommendations[i]
+				return &recommendationCopy
+			}
+		}
+	}
+	return nil
+}
+
+func getContainerPolicy(containerName string, policy *vpa_types.PodResourcePolicy) *vpa_types.ContainerResourcePolicy {
+	if policy != nil {
+		for i, container := range policy.ContainerPolicies {
+			if containerName == container.Name {
+				return &policy.ContainerPolicies[i]
+			}
+		}
+	}
+	return nil
+}

vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go

@@ -0,0 +1,144 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package api
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	apiv1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/poc.autoscaling.k8s.io/v1alpha1"
+)
+
+func TestRecommendationNotAvailable(t *testing.T) {
+	container := apiv1.Container{Name: "ctr-name"}
+	podRecommendation := vpa_types.RecommendedPodResources{
+		ContainerRecommendations: []vpa_types.RecommendedContainerResources{
+			{
+				Name: "ctr-name-other",
+				Target: apiv1.ResourceList{
+					apiv1.ResourceCPU:    *resource.NewScaledQuantity(100, 1),
+					apiv1.ResourceMemory: *resource.NewScaledQuantity(50000, 1),
+				},
+			},
+		},
+	}
+	policy := vpa_types.PodResourcePolicy{}
+
+	res, err := GetCappedRecommendationForContainer(container, &podRecommendation, &policy)
+	assert.Nil(t, res)
+	assert.NotNil(t, err)
+}
+
+func TestRecommendationChosenForProperContainer(t *testing.T) {
+	container := apiv1.Container{Name: "ctr-name"}
+	podRecommendation := vpa_types.RecommendedPodResources{
+		ContainerRecommendations: []vpa_types.RecommendedContainerResources{
+			{
+				Name: "ctr-name-other",
+				Target: apiv1.ResourceList{
+					apiv1.ResourceCPU:    *resource.NewScaledQuantity(100, 1),
+					apiv1.ResourceMemory: *resource.NewScaledQuantity(50000, 1),
+				},
+			},
+			{
+				Name: "ctr-name",
+				Target: apiv1.ResourceList{
+					apiv1.ResourceCPU:    *resource.NewScaledQuantity(10, 1),
+					apiv1.ResourceMemory: *resource.NewScaledQuantity(5000, 1),
+				},
+			},
+		},
+	}
+	policy := vpa_types.PodResourcePolicy{}
+
+	res, err := GetCappedRecommendationForContainer(container, &podRecommendation, &policy)
+	assert.Nil(t, err)
+	assert.Equal(t, res, apiv1.ResourceList{
+		apiv1.ResourceCPU:    *resource.NewScaledQuantity(10, 1),
+		apiv1.ResourceMemory: *resource.NewScaledQuantity(5000, 1),
+	})
+}
+
+func TestRecommendationCappedToLimit(t *testing.T) {
+	container := apiv1.Container{
+		Name: "ctr-name",
+		Resources: apiv1.ResourceRequirements{
+			Limits: apiv1.ResourceList{
+				apiv1.ResourceCPU:    *resource.NewScaledQuantity(3, 1),
+				apiv1.ResourceMemory: *resource.NewScaledQuantity(7000, 1),
+			},
+		},
+	}
+	podRecommendation := vpa_types.RecommendedPodResources{
+		ContainerRecommendations: []vpa_types.RecommendedContainerResources{
+			{
+				Name: "ctr-name",
+				Target: apiv1.ResourceList{
+					apiv1.ResourceCPU:    *resource.NewScaledQuantity(10, 1),
+					apiv1.ResourceMemory: *resource.NewScaledQuantity(5000, 1),
+				},
+			},
+		},
+	}
+	policy := vpa_types.PodResourcePolicy{}
+
+	res, err := GetCappedRecommendationForContainer(container, &podRecommendation, &policy)
+	assert.Nil(t, err)
+	assert.Equal(t, res, apiv1.ResourceList{
+		apiv1.ResourceCPU:    *resource.NewScaledQuantity(3, 1),
+		apiv1.ResourceMemory: *resource.NewScaledQuantity(5000, 1),
+	})
+}
+
+func TestRecommendationCappedToMinMaxPolicy(t *testing.T) {
+	container := apiv1.Container{Name: "ctr-name"}
+	podRecommendation := vpa_types.RecommendedPodResources{
+		ContainerRecommendations: []vpa_types.RecommendedContainerResources{
+			{
+				Name: "ctr-name",
+				Target: apiv1.ResourceList{
+					apiv1.ResourceCPU:    *resource.NewScaledQuantity(10, 1),
+					apiv1.ResourceMemory: *resource.NewScaledQuantity(5000, 1),
+				},
+			},
+		},
+	}
+	policy := vpa_types.PodResourcePolicy{
+		ContainerPolicies: []vpa_types.ContainerResourcePolicy{
+			{
+				Name: "ctr-name",
+				MinAllowed: apiv1.ResourceList{
+					apiv1.ResourceCPU:    *resource.NewScaledQuantity(40, 1),
+					apiv1.ResourceMemory: *resource.NewScaledQuantity(4000, 1),
+				},
+				MaxAllowed: apiv1.ResourceList{
+					apiv1.ResourceCPU:    *resource.NewScaledQuantity(45, 1),
+					apiv1.ResourceMemory: *resource.NewScaledQuantity(4500, 1),
+				},
+			},
+		},
+	}
+
+	res, err := GetCappedRecommendationForContainer(container, &podRecommendation, &policy)
+	assert.Nil(t, err)
+	assert.Equal(t, res, apiv1.ResourceList{
+		apiv1.ResourceCPU:    *resource.NewScaledQuantity(40, 1),
+		apiv1.ResourceMemory: *resource.NewScaledQuantity(4500, 1),
+	})
+}

vertical-pod-autoscaler/updater/priority/update_priority_calculator.go

@@ -20,12 +20,11 @@ import (
 	"math"
 	"sort"
 
-	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/poc.autoscaling.k8s.io/v1alpha1"
+	"github.com/golang/glog"
-
 	apiv1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
-
+	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/poc.autoscaling.k8s.io/v1alpha1"
-	"github.com/golang/glog"
+	vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa"
 )
 
 const (
@@ -90,36 +89,25 @@ func (calc *UpdatePriorityCalculator) getUpdatePriority(pod *apiv1.Pod, recommen
 	var priority float64
 
 	for _, podContainer := range pod.Spec.Containers {
-		cr := getContainerRecommendation(podContainer.Name, recommendation)
+		recommendedRequest, err := vpa_api_util.GetCappedRecommendationForContainer(podContainer, recommendation, calc.resourcesPolicy)
-		if cr == nil {
+		if err != nil {
 			glog.V(2).Infof("no recommendation for container %v in pod %v", podContainer.Name, pod.Name)
 			continue
 		}
-
+		for resourceName, recommended := range recommendedRequest {
-		containerPolicy := getContainerPolicy(podContainer.Name, calc.resourcesPolicy)
+			var requested *resource.Quantity
-
-		for resourceName, recommended := range cr.Target {
-			var requested, min, max *resource.Quantity
 
 			if request, ok := podContainer.Resources.Requests[resourceName]; ok {
 				requested = &request
 			}
-			if containerPolicy != nil {
+			resourceDiff := getPercentageDiff(requested, &recommended)
-				if minAllowed, ok := containerPolicy.MinAllowed[resourceName]; ok {
-					min = &minAllowed
-				}
-				if maxAllowed, ok := containerPolicy.MaxAllowed[resourceName]; ok {
-					max = &maxAllowed
-				}
-			}
-			resourceDiff := getPercentageDiff(requested, min, max, &recommended)
 			priority += math.Abs(resourceDiff)
 		}
 	}
 	return priority
 }
 
-func getPercentageDiff(request, min, max, recommendation *resource.Quantity) float64 {
+func getPercentageDiff(request, recommendation *resource.Quantity) float64 {
 	if request == nil {
 		// resource requirement is not currently specified
 		// any recommendation for this resource we will treat as 100% change
@@ -128,39 +116,7 @@ func getPercentageDiff(request, min, max, recommendation *resource.Quantity) flo
 	if recommendation == nil || recommendation.IsZero() {
 		return 0
 	}
-	recommended := recommendation.MilliValue()
+	return float64(recommendation.MilliValue()-request.MilliValue()) / float64(request.MilliValue())
-	if min != nil && !min.IsZero() && recommendation.MilliValue() < min.MilliValue() {
-		glog.Warningf("recommendation outside of policy bounds : min value : %v recommended : %v",
-			min.MilliValue(), recommended)
-		recommended = min.MilliValue()
-	}
-	if max != nil && !max.IsZero() && recommendation.MilliValue() > max.MilliValue() {
-		glog.Warningf("recommendation outside of policy bounds : max value : %v recommended : %v",
-			max.MilliValue(), recommended)
-		recommended = max.MilliValue()
-	}
-	diff := recommended - request.MilliValue()
-	return float64(diff) / float64(request.MilliValue())
-}
-
-func getContainerPolicy(containerName string, policy *vpa_types.PodResourcePolicy) *vpa_types.ContainerResourcePolicy {
-	if policy != nil {
-		for _, container := range policy.ContainerPolicies {
-			if containerName == container.Name {
-				return &container
-			}
-		}
-	}
-	return nil
-}
-
-func getContainerRecommendation(containerName string, recommendation *vpa_types.RecommendedPodResources) *vpa_types.RecommendedContainerResources {
-	for _, container := range recommendation.ContainerRecommendations {
-		if containerName == container.Name {
-			return &container
-		}
-	}
-	return nil
 }
 
 type podPriority struct {

vertical-pod-autoscaler/updater/priority/update_priority_calculator_test.go

@@ -91,7 +91,7 @@ func TestSortPriorityMultiContainers(t *testing.T) {
 	assert.Exactly(t, []*apiv1.Pod{pod2, pod1}, result, "Wrong priority order")
 }
 
-func TestSortPriorityResorucesDecrease(t *testing.T) {
+func TestSortPriorityResourcesDecrease(t *testing.T) {
 	calculator := NewUpdatePriorityCalculator(nil, nil)
 
 	pod1 := test.BuildTestPod("POD1", containerName, "4", "", nil, nil)