Merge pull request #2073 from bskiba/min-limit

Support multiple limitrange items

vertical-pod-autoscaler/pkg/utils/limitrange/limit_range_calculator.go

@@ -19,7 +19,8 @@ package limitrange
 import (
 	"fmt"
 
-	"k8s.io/api/core/v1"
+	core "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/client-go/informers"
 	listers "k8s.io/client-go/listers/core/v1"
@@ -28,12 +29,12 @@ import (
 // LimitRangeCalculator calculates limit range items that has the same effect as all limit range items present in the cluster.
 type LimitRangeCalculator interface {
 	// GetContainerLimitRangeItem returns LimitRangeItem that describes limitation on container limits in the given namespace.
-	GetContainerLimitRangeItem(namespace string) (*v1.LimitRangeItem, error)
+	GetContainerLimitRangeItem(namespace string) (*core.LimitRangeItem, error)
 }
 
 type noopLimitsRangeCalculator struct{}
 
-func (lc *noopLimitsRangeCalculator) GetContainerLimitRangeItem(namespace string) (*v1.LimitRangeItem, error) {
+func (lc *noopLimitsRangeCalculator) GetContainerLimitRangeItem(namespace string) (*core.LimitRangeItem, error) {
 	return nil, nil
 }
 
@@ -64,19 +65,59 @@ func NewNoopLimitsCalculator() *noopLimitsRangeCalculator {
 	return &noopLimitsRangeCalculator{}
 }
 
-func (lc *limitsChecker) GetContainerLimitRangeItem(namespace string) (*v1.LimitRangeItem, error) {
+func (lc *limitsChecker) GetContainerLimitRangeItem(namespace string) (*core.LimitRangeItem, error) {
 	limitRanges, err := lc.limitRangeLister.LimitRanges(namespace).List(labels.Everything())
 	if err != nil {
 		return nil, fmt.Errorf("error loading limit ranges: %s", err)
 	}
 
+	updatedResult := func(result core.ResourceList, lrItem core.ResourceList,
+		resourceName core.ResourceName, picker func(q1, q2 resource.Quantity) resource.Quantity) core.ResourceList {
+		if lrItem == nil {
+			return result
+		}
+		if result == nil {
+			return lrItem.DeepCopy()
+		}
+		if lrResource, lrHas := lrItem[resourceName]; lrHas {
+			resultResource, resultHas := result[resourceName]
+			if !resultHas {
+				result[resourceName] = lrResource.DeepCopy()
+			} else {
+				result[resourceName] = picker(resultResource, lrResource)
+			}
+		}
+		return result
+	}
+	pickLowerMax := func(q1, q2 resource.Quantity) resource.Quantity {
+		if q1.Cmp(q2) < 0 {
+			return q1
+		}
+		return q2
+	}
+	chooseHigherMin := func(q1, q2 resource.Quantity) resource.Quantity {
+		if q1.Cmp(q2) > 0 {
+			return q1
+		}
+		return q2
+	}
+
+	result := &core.LimitRangeItem{Type: core.LimitTypeContainer}
 	for _, lr := range limitRanges {
 		for _, lri := range lr.Spec.Limits {
-			if lri.Type == v1.LimitTypeContainer && (lri.Max != nil || lri.Default != nil) {
+			if lri.Type == core.LimitTypeContainer && (lri.Max != nil || lri.Default != nil || lri.Min != nil) {
-				// TODO: handle multiple limit ranges matching a pod.
+				if lri.Default != nil {
-				return &lri, nil
+					result.Default = lri.Default
+				}
+				result.Max = updatedResult(result.Max, lri.Max, core.ResourceCPU, pickLowerMax)
+				result.Max = updatedResult(result.Max, lri.Max, core.ResourceMemory, pickLowerMax)
+				result.Min = updatedResult(result.Min, lri.Min, core.ResourceCPU, chooseHigherMin)
+				result.Min = updatedResult(result.Min, lri.Min, core.ResourceMemory, chooseHigherMin)
 			}
 		}
 	}
+	if result.Min != nil || result.Max != nil || result.Default != nil {
+		return result, nil
+	}
 	return nil, nil
 }

vertical-pod-autoscaler/pkg/utils/limitrange/limit_range_calculator_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	apiv1 "k8s.io/api/core/v1"
+	core "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test"
 	"k8s.io/client-go/informers"
@@ -28,11 +29,11 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-const defaultNamespace = "default"
+const testNamespace = "test-namespace"
 
 func TestNewNoopLimitsChecker(t *testing.T) {
 	nlc := NewNoopLimitsCalculator()
-	limitRange, err := nlc.GetContainerLimitRangeItem(defaultNamespace)
+	limitRange, err := nlc.GetContainerLimitRangeItem(testNamespace)
 	assert.NoError(t, err)
 	assert.Nil(t, limitRange)
 }
@@ -43,15 +44,17 @@ func TestNoLimitRange(t *testing.T) {
 	lc, err := NewLimitsRangeCalculator(factory)
 
 	if assert.NoError(t, err) {
-		limitRange, err := lc.GetContainerLimitRangeItem(defaultNamespace)
+		limitRange, err := lc.GetContainerLimitRangeItem(testNamespace)
 		assert.NoError(t, err)
 		assert.Nil(t, limitRange)
 	}
 }
 
 func TestGetContainerLimitRangeItem(t *testing.T) {
-	containerLimitRangeWithMax := test.LimitRange().WithName("default").WithNamespace(defaultNamespace).WithType(apiv1.LimitTypeContainer).WithMax(test.Resources("2", "2")).Get()
+	baseContainerLimitRange := test.LimitRange().WithName("test-lr").WithNamespace(testNamespace).WithType(apiv1.LimitTypeContainer)
-	containerLimitRangeWithDefault := test.LimitRange().WithName("default").WithNamespace(defaultNamespace).WithType(apiv1.LimitTypeContainer).WithDefault(test.Resources("2", "2")).Get()
+	containerLimitRangeWithMax := baseContainerLimitRange.WithMax(test.Resources("2", "2")).Get()
+	containerLimitRangeWithDefault := baseContainerLimitRange.WithDefault(test.Resources("2", "2")).Get()
+	containerLimitRangeWithMin := baseContainerLimitRange.WithMin(test.Resources("2", "2")).Get()
 	testCases := []struct {
 		name           string
 		limitRanges    []runtime.Object
@@ -62,7 +65,7 @@ func TestGetContainerLimitRangeItem(t *testing.T) {
 			name: "no matching limit ranges",
 			limitRanges: []runtime.Object{
 				test.LimitRange().WithName("different-namespace").WithNamespace("different").WithType(apiv1.LimitTypeContainer).WithMax(test.Resources("2", "2")).Get(),
-				test.LimitRange().WithName("differen-type").WithNamespace(defaultNamespace).WithType(apiv1.LimitTypePersistentVolumeClaim).WithMax(test.Resources("2", "2")).Get(),
+				test.LimitRange().WithName("different-type").WithNamespace(testNamespace).WithType(apiv1.LimitTypePersistentVolumeClaim).WithMax(test.Resources("2", "2")).Get(),
 			},
 			expectedErr:    nil,
 			expectedLimits: nil,
@@ -83,6 +86,50 @@ func TestGetContainerLimitRangeItem(t *testing.T) {
 			expectedErr:    nil,
 			expectedLimits: &containerLimitRangeWithDefault.Spec.Limits[0],
 		},
+		{
+			name: "respects min",
+			limitRanges: []runtime.Object{
+				containerLimitRangeWithMin,
+			},
+			expectedErr:    nil,
+			expectedLimits: &containerLimitRangeWithMin.Spec.Limits[0],
+		},
+		{
+			name: "multiple items",
+			limitRanges: []runtime.Object{
+				baseContainerLimitRange.WithMax(test.Resources("2", "2")).WithDefault(test.Resources("1.5", "1.5")).
+					WithMin(test.Resources("1", "1")).Get(),
+			},
+			expectedErr: nil,
+			expectedLimits: &core.LimitRangeItem{
+				Type:    core.LimitTypeContainer,
+				Min:     test.Resources("1", "1"),
+				Max:     test.Resources("2", "2"),
+				Default: test.Resources("1.5", "1.5"),
+			},
+		},
+		{
+			name: "takes lowest max",
+			limitRanges: []runtime.Object{
+				baseContainerLimitRange.WithMax(test.Resources("1.5", "1.5")).WithMax(test.Resources("2.", "2.")).Get(),
+			},
+			expectedErr: nil,
+			expectedLimits: &core.LimitRangeItem{
+				Type: core.LimitTypeContainer,
+				Max:  test.Resources("1.5", "1.5"),
+			},
+		},
+		{
+			name: "takes highest min",
+			limitRanges: []runtime.Object{
+				baseContainerLimitRange.WithMin(test.Resources("1.5", "1.5")).WithMin(test.Resources("1.", "1.")).Get(),
+			},
+			expectedErr: nil,
+			expectedLimits: &core.LimitRangeItem{
+				Type: core.LimitTypeContainer,
+				Min:  test.Resources("1.5", "1.5"),
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -91,7 +138,7 @@ func TestGetContainerLimitRangeItem(t *testing.T) {
 			factory := informers.NewSharedInformerFactory(cs, 0)
 			lc, err := NewLimitsRangeCalculator(factory)
 			if assert.NoError(t, err) {
-				limitRange, err := lc.GetContainerLimitRangeItem(defaultNamespace)
+				limitRange, err := lc.GetContainerLimitRangeItem(testNamespace)
 				if tc.expectedErr == nil {
 					assert.NoError(t, err)
 				} else {

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

@@ -17,8 +17,8 @@ limitations under the License.
 package test
 
 import (
-	"k8s.io/api/core/v1"
+	core "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	meta "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // LimitRange returns an object that helps build a LimitRangeItem object for tests.
@@ -29,9 +29,10 @@ func LimitRange() *limitRangeBuilder {
 type limitRangeBuilder struct {
 	namespace     string
 	name          string
-	rangeType     v1.LimitType
+	rangeType     core.LimitType
-	defaultValues *v1.ResourceList
+	defaultValues []*core.ResourceList
-	max           *v1.ResourceList
+	maxValues     []*core.ResourceList
+	minValues     []*core.ResourceList
 }
 
 func (lrb *limitRangeBuilder) WithName(name string) *limitRangeBuilder {
@@ -46,46 +47,58 @@ func (lrb *limitRangeBuilder) WithNamespace(namespace string) *limitRangeBuilder
 	return &result
 }
 
-func (lrb *limitRangeBuilder) WithType(rangeType v1.LimitType) *limitRangeBuilder {
+func (lrb *limitRangeBuilder) WithType(rangeType core.LimitType) *limitRangeBuilder {
 	result := *lrb
 	result.rangeType = rangeType
 	return &result
 }
 
-func (lrb *limitRangeBuilder) WithDefault(defaultValues v1.ResourceList) *limitRangeBuilder {
+func (lrb *limitRangeBuilder) WithDefault(defaultValues core.ResourceList) *limitRangeBuilder {
 	result := *lrb
-	result.defaultValues = &defaultValues
+	result.defaultValues = append(result.defaultValues, &defaultValues)
 	return &result
 }
 
-func (lrb *limitRangeBuilder) WithMax(max v1.ResourceList) *limitRangeBuilder {
+func (lrb *limitRangeBuilder) WithMax(max core.ResourceList) *limitRangeBuilder {
 	result := *lrb
-	result.max = &max
+	result.maxValues = append(result.maxValues, &max)
 	return &result
 }
 
-func (lrb *limitRangeBuilder) Get() *v1.LimitRange {
+func (lrb *limitRangeBuilder) WithMin(min core.ResourceList) *limitRangeBuilder {
-	result := v1.LimitRange{
+	result := *lrb
-		ObjectMeta: metav1.ObjectMeta{
+	result.minValues = append(result.minValues, &min)
+	return &result
+}
+
+func (lrb *limitRangeBuilder) Get() *core.LimitRange {
+	result := core.LimitRange{
+		ObjectMeta: meta.ObjectMeta{
 			Namespace: lrb.namespace,
 			Name:      lrb.name,
 		},
 	}
-	if lrb.defaultValues != nil || lrb.max != nil {
+	if len(lrb.defaultValues) > 0 || len(lrb.maxValues) > 0 || len(lrb.minValues) > 0 {
-		result.Spec = v1.LimitRangeSpec{
+		result.Spec = core.LimitRangeSpec{
-			Limits: []v1.LimitRangeItem{},
+			Limits: []core.LimitRangeItem{},
 		}
 	}
-	if lrb.defaultValues != nil {
+	for _, v := range lrb.defaultValues {
-		result.Spec.Limits = append(result.Spec.Limits, v1.LimitRangeItem{
+		result.Spec.Limits = append(result.Spec.Limits, core.LimitRangeItem{
 			Type:    lrb.rangeType,
-			Default: *lrb.defaultValues,
+			Default: *v,
 		})
 	}
-	if lrb.max != nil {
+	for _, v := range lrb.maxValues {
-		result.Spec.Limits = append(result.Spec.Limits, v1.LimitRangeItem{
+		result.Spec.Limits = append(result.Spec.Limits, core.LimitRangeItem{
 			Type: lrb.rangeType,
-			Max:  *lrb.max,
+			Max:  *v,
+		})
+	}
+	for _, v := range lrb.minValues {
+		result.Spec.Limits = append(result.Spec.Limits, core.LimitRangeItem{
+			Type: lrb.rangeType,
+			Min:  *v,
 		})
 	}
 	return &result