From 7b89672f2817530365e137e6911dd12ed54885c4 Mon Sep 17 00:00:00 2001 From: Beata Skiba Date: Thu, 30 May 2019 09:46:02 +0200 Subject: [PATCH] Support multiple limitrange items --- .../limitrange/limit_range_calculator.go | 55 ++++++++++++++--- .../limitrange/limit_range_calculator_test.go | 61 ++++++++++++++++--- .../pkg/utils/test/test_limit_range.go | 57 ++++++++++------- 3 files changed, 137 insertions(+), 36 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/utils/limitrange/limit_range_calculator.go b/vertical-pod-autoscaler/pkg/utils/limitrange/limit_range_calculator.go index cbc184f879..7b666c83e7 100644 --- a/vertical-pod-autoscaler/pkg/utils/limitrange/limit_range_calculator.go +++ b/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) { - // TODO: handle multiple limit ranges matching a pod. - return &lri, nil + if lri.Type == core.LimitTypeContainer && (lri.Max != nil || lri.Default != nil || lri.Min != nil) { + if lri.Default != 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 } diff --git a/vertical-pod-autoscaler/pkg/utils/limitrange/limit_range_calculator_test.go b/vertical-pod-autoscaler/pkg/utils/limitrange/limit_range_calculator_test.go index d7e65e4d0f..9e06f5c999 100644 --- a/vertical-pod-autoscaler/pkg/utils/limitrange/limit_range_calculator_test.go +++ b/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() - containerLimitRangeWithDefault := test.LimitRange().WithName("default").WithNamespace(defaultNamespace).WithType(apiv1.LimitTypeContainer).WithDefault(test.Resources("2", "2")).Get() + baseContainerLimitRange := test.LimitRange().WithName("test-lr").WithNamespace(testNamespace).WithType(apiv1.LimitTypeContainer) + 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 { diff --git a/vertical-pod-autoscaler/pkg/utils/test/test_limit_range.go b/vertical-pod-autoscaler/pkg/utils/test/test_limit_range.go index e62efe059b..52cbc0e271 100644 --- a/vertical-pod-autoscaler/pkg/utils/test/test_limit_range.go +++ b/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" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + core "k8s.io/api/core/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 - defaultValues *v1.ResourceList - max *v1.ResourceList + rangeType core.LimitType + defaultValues []*core.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 { - result := v1.LimitRange{ - ObjectMeta: metav1.ObjectMeta{ +func (lrb *limitRangeBuilder) WithMin(min core.ResourceList) *limitRangeBuilder { + result := *lrb + 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 { - result.Spec = v1.LimitRangeSpec{ - Limits: []v1.LimitRangeItem{}, + if len(lrb.defaultValues) > 0 || len(lrb.maxValues) > 0 || len(lrb.minValues) > 0 { + result.Spec = core.LimitRangeSpec{ + Limits: []core.LimitRangeItem{}, } } - if lrb.defaultValues != nil { - result.Spec.Limits = append(result.Spec.Limits, v1.LimitRangeItem{ + for _, v := range lrb.defaultValues { + result.Spec.Limits = append(result.Spec.Limits, core.LimitRangeItem{ Type: lrb.rangeType, - Default: *lrb.defaultValues, + Default: *v, }) } - if lrb.max != nil { - result.Spec.Limits = append(result.Spec.Limits, v1.LimitRangeItem{ + for _, v := range lrb.maxValues { + 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