From 0d2afe5e7b2efc075b405cdd09e5ba7e2379a0c1 Mon Sep 17 00:00:00 2001 From: Joachim Bartosik Date: Mon, 20 May 2019 17:29:13 +0200 Subject: [PATCH] Address comments from original PR review I'm keeping original CL from https://github.com/kubernetes/autoscaler/pull/1813 and applying changes requested in the review in a separate CL to keep autoship information clean. --- .../logic/limitrange_checker.go | 78 +++++++------ .../logic/limitrange_checker_test.go | 107 +++++++++--------- .../pkg/admission-controller/logic/server.go | 12 +- .../admission-controller/logic/server_test.go | 4 +- .../pkg/admission-controller/main.go | 11 +- 5 files changed, 112 insertions(+), 100 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker.go b/vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker.go index c2996b2495cf..88bc1097a9c1 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker.go @@ -17,6 +17,7 @@ limitations under the License. package logic import ( + "fmt" "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/labels" @@ -27,14 +28,12 @@ import ( // LimitsHints provides hinted limits that respect limit range max ratio type LimitsHints interface { - IsNil() bool RequestsExceedsRatio(indexOfContainer int, resourceName v1.ResourceName) bool HintedLimit(indexOfContainer int, resourceName v1.ResourceName) resource.Quantity } // LimitRangeHints implements LimitsHints interface type LimitRangeHints struct { - requestsExceedsRatio []map[v1.ResourceName]bool limitsRespectingRatio []v1.ResourceList } @@ -42,26 +41,21 @@ var _ LimitsHints = &LimitRangeHints{} // LimitsChecker checks for LimitRange and if container needs limits to be set type LimitsChecker interface { - NeedsLimits(*v1.Pod, []ContainerResources) LimitsHints -} - -// IsNil return true if there is no hints to set limits -func (lrh *LimitRangeHints) IsNil() bool { - return lrh == (*LimitRangeHints)(nil) + NeedsLimits(*v1.Pod, []ContainerResources) (LimitsHints, error) } // RequestsExceedsRatio return true if limits have to be set to respect limit range max ratio func (lrh *LimitRangeHints) RequestsExceedsRatio(indexOfContainer int, resourceName v1.ResourceName) bool { - if !lrh.IsNil() { - yes, ok := lrh.requestsExceedsRatio[indexOfContainer][resourceName] - return ok && yes + if lrh != nil && indexOfContainer < len(lrh.limitsRespectingRatio) { + _, present := lrh.limitsRespectingRatio[indexOfContainer][resourceName] + return present } return false } // HintedLimit return the limit Quantity that respect the limit range max ration func (lrh *LimitRangeHints) HintedLimit(indexOfContainer int, resourceName v1.ResourceName) resource.Quantity { - if !lrh.IsNil() { + if lrh != nil && indexOfContainer < len(lrh.limitsRespectingRatio) { limit, ok := lrh.limitsRespectingRatio[indexOfContainer][resourceName] if ok { return limit @@ -75,8 +69,8 @@ type neverNeedsLimitsChecker struct{} var _ LimitsChecker = &neverNeedsLimitsChecker{} -func (lc *neverNeedsLimitsChecker) NeedsLimits(pod *v1.Pod, containersResources []ContainerResources) LimitsHints { - return LimitsHints((*LimitRangeHints)(nil)) +func (lc *neverNeedsLimitsChecker) NeedsLimits(pod *v1.Pod, containersResources []ContainerResources) (LimitsHints, error) { + return LimitsHints((*LimitRangeHints)(nil)), nil } type limitsChecker struct { @@ -85,21 +79,26 @@ type limitsChecker struct { var _ LimitsChecker = &limitsChecker{} -// NewLimitsChecker creates a LimitsChecker -func NewLimitsChecker(f informers.SharedInformerFactory) LimitsChecker { - if f != nil { - limitrangeLister := f.Core().V1().LimitRanges().Lister() - stopCh := make(chan struct{}) - f.Start(stopCh) - for _, ok := range f.WaitForCacheSync(stopCh) { - if !ok { - if ok := f.Core().V1().LimitRanges().Informer().HasSynced(); !ok { - return &neverNeedsLimitsChecker{} - } +// NewLimitsChecker returns a limitsChecker or an error it encountered when attempting to create it. +func NewLimitsChecker(f informers.SharedInformerFactory) (*limitsChecker, error) { + if f == nil { + return nil, fmt.Errorf("NewLimitsChecker requires a SharedInformerFactory but got nil") + } + limitrangeLister := f.Core().V1().LimitRanges().Lister() + stopCh := make(chan struct{}) + f.Start(stopCh) + for _, ok := range f.WaitForCacheSync(stopCh) { + if !ok { + if f.Core().V1().LimitRanges().Informer().HasSynced() { + return nil, fmt.Errorf("Informer did not sync") } } - return &limitsChecker{limitrangeLister} } + return &limitsChecker{limitrangeLister}, nil +} + +// NewNoopLimitsChecker returns a limit checker that +func NewNoopLimitsChecker() *neverNeedsLimitsChecker { return &neverNeedsLimitsChecker{} } @@ -143,14 +142,13 @@ func (id *interestingData) parse(lri *v1.LimitRangeItem) { } } -func (lc *limitsChecker) getLimitRangeItem(pod *v1.Pod) (ret *v1.LimitRangeItem) { - ret = nil +func (lc *limitsChecker) getLimitRangeItem(pod *v1.Pod) (*v1.LimitRangeItem, error) { limitranges, err := lc.limitrangeLister. LimitRanges(pod.GetNamespace()). List(labels.Everything()) if err != nil { - return ret + return nil, fmt.Errorf("error loading limit ranges: %s", err) } id := &interestingData{} @@ -164,35 +162,36 @@ func (lc *limitsChecker) getLimitRangeItem(pod *v1.Pod) (ret *v1.LimitRangeItem) lri.Default == nil { continue } + // TODO: handle multiple limit ranges matching a pod. foundInterstingData = true id.parse(&lri) } } if foundInterstingData { - ret = &v1.LimitRangeItem{ + return &v1.LimitRangeItem{ MaxLimitRequestRatio: id.MaxLimitRequestRatio, Default: id.Default, - } + }, nil } - - return ret + return nil, nil } -func (lc *limitsChecker) NeedsLimits(pod *v1.Pod, containersResources []ContainerResources) LimitsHints { - lri := lc.getLimitRangeItem(pod) +func (lc *limitsChecker) NeedsLimits(pod *v1.Pod, containersResources []ContainerResources) (LimitsHints, error) { + lri, err := lc.getLimitRangeItem(pod) + if err != nil { + return nil, fmt.Errorf("error getting limit range for pod: %s", err) + } if lri == (*v1.LimitRangeItem)(nil) { - return LimitsHints((*LimitRangeHints)(nil)) + return &LimitRangeHints{}, nil } lrh := &LimitRangeHints{ - requestsExceedsRatio: make([]map[v1.ResourceName]bool, len(containersResources)), limitsRespectingRatio: make([]v1.ResourceList, len(containersResources)), } needsLimits := false for i, cr := range containersResources { - lrh.requestsExceedsRatio[i] = make(map[v1.ResourceName]bool) lrh.limitsRespectingRatio[i] = make(v1.ResourceList) for name, value := range cr.Requests { var ctrLimit *resource.Quantity @@ -227,7 +226,6 @@ func (lc *limitsChecker) NeedsLimits(pod *v1.Pod, containersResources []Containe if futureRatio > maxRatio { needsLimits = true - lrh.requestsExceedsRatio[i][name] = true l := int64(float64(vv) * maxRatio) if useMilli { if l > resource.MaxMilliValue { @@ -245,5 +243,5 @@ func (lc *limitsChecker) NeedsLimits(pod *v1.Pod, containersResources []Containe if !needsLimits { lrh = nil } - return LimitsHints(lrh) + return LimitsHints(lrh), nil } diff --git a/vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker_test.go b/vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker_test.go index 95298778c1a2..4b1519720e69 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker_test.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker_test.go @@ -164,42 +164,43 @@ func TestUpdateResourceLimits(t *testing.T) { // - no needed limits // - RequestsExceedsRatio always return false t.Run("test case for neverNeedsLimitsChecker", func(t *testing.T) { - nlc := NewLimitsChecker(nil) - hints := nlc.NeedsLimits(uninitialized, vpaContainersResources) - hintsPtr, _ := hints.(*LimitRangeHints) - if hintsPtr != nil { - t.Errorf("%v NeedsLimits didn't not return nil: %v", nlc, hints) - } - if !hints.IsNil() { - t.Errorf("%v NeedsLimits returned a LimitsHints not nil: %v", nlc, hints) - } - if hints.RequestsExceedsRatio(0, apiv1.ResourceMemory) != false { - t.Errorf("%v RequestsExceedsRatio didn't not return false", hints) - } - hinted := hints.HintedLimit(0, apiv1.ResourceMemory) - if !(&hinted).IsZero() { - t.Errorf("%v RequestsExceedsRatio didn't not return zero quantity", hints) + nlc := NewNoopLimitsChecker() + hints, err := nlc.NeedsLimits(uninitialized, vpaContainersResources) + if assert.NoError(t, err) { + hintsPtr, _ := hints.(*LimitRangeHints) + if hintsPtr != nil { + t.Errorf("%v NeedsLimits didn't not return nil: %v", nlc, hints) + } + assert.Nil(t, hints) + if hints.RequestsExceedsRatio(0, apiv1.ResourceMemory) != false { + t.Errorf("%v RequestsExceedsRatio didn't not return false", hints) + } + hinted := hints.HintedLimit(0, apiv1.ResourceMemory) + if !(&hinted).IsZero() { + t.Errorf("%v RequestsExceedsRatio didn't not return zero quantity", hints) + } } }) t.Run("test case for no Limit Range", func(t *testing.T) { cs := fake.NewSimpleClientset() factory := informers.NewSharedInformerFactory(cs, 0) - lc := NewLimitsChecker(factory) - hints := lc.NeedsLimits(uninitialized, vpaContainersResources) - hintsPtr, _ := hints.(*LimitRangeHints) - if hintsPtr != nil { - t.Errorf("%v NeedsLimits didn't not return nil: %v", lc, hints) - } - if !hints.IsNil() { - t.Errorf("%v NeedsLimits returned a LimitsHints not nil: %v", lc, hints) - } - if hints.RequestsExceedsRatio(0, apiv1.ResourceMemory) != false { - t.Errorf("%v RequestsExceedsRatio didn't not return false", hints) - } - hinted := hints.HintedLimit(0, apiv1.ResourceMemory) - if !(&hinted).IsZero() { - t.Errorf("%v RequestsExceedsRatio didn't not return zero quantity", hints) + lc, err := NewLimitsChecker(factory) + if assert.NoError(t, err) { + hints, err := lc.NeedsLimits(uninitialized, vpaContainersResources) + if assert.NoError(t, err) { + hintsPtr, _ := hints.(*LimitRangeHints) + if assert.NotNil(t, hintsPtr) { + assert.Empty(t, hintsPtr.limitsRespectingRatio) + } + if hints.RequestsExceedsRatio(0, apiv1.ResourceMemory) != false { + t.Errorf("%v RequestsExceedsRatio didn't not return false", hints) + } + hinted := hints.HintedLimit(0, apiv1.ResourceMemory) + if !(&hinted).IsZero() { + t.Errorf("%v RequestsExceedsRatio didn't not return zero quantity", hints) + } + } } }) @@ -208,28 +209,32 @@ func TestUpdateResourceLimits(t *testing.T) { t.Run(fmt.Sprintf("test case number: %d", i), func(t *testing.T) { cs := fake.NewSimpleClientset(tc.limitRanges...) factory := informers.NewSharedInformerFactory(cs, 0) - lc := NewLimitsChecker(factory) - resources := tc.containerResources - - hints := lc.NeedsLimits(tc.pod, resources) - assert.NotNil(t, hints, fmt.Sprintf("hints is: %+v", hints)) - - if tc.requestsExceedsRatioCPU { - assert.True(t, hints.RequestsExceedsRatio(0, apiv1.ResourceCPU)) - } else { - assert.False(t, hints.RequestsExceedsRatio(0, apiv1.ResourceCPU)) + lc, err := NewLimitsChecker(factory) + if assert.NoError(t, err) { + resources := tc.containerResources + + hints, err := lc.NeedsLimits(tc.pod, resources) + if assert.NoError(t, err) { + assert.NotNil(t, hints, fmt.Sprintf("hints is: %+v", hints)) + + if tc.requestsExceedsRatioCPU { + assert.True(t, hints.RequestsExceedsRatio(0, apiv1.ResourceCPU)) + } else { + assert.False(t, hints.RequestsExceedsRatio(0, apiv1.ResourceCPU)) + } + + if tc.requestsExceedsRatioMemory { + assert.True(t, hints.RequestsExceedsRatio(0, apiv1.ResourceMemory)) + } else { + assert.False(t, hints.RequestsExceedsRatio(0, apiv1.ResourceMemory)) + } + + hintedCPULimits := hints.HintedLimit(0, apiv1.ResourceCPU) + hintedMemoryLimits := hints.HintedLimit(0, apiv1.ResourceMemory) + assert.EqualValues(t, tc.limitsRespectingRatioCPU.Value(), hintedCPULimits.Value(), fmt.Sprintf("cpu limits doesn't match: %v != %v\n", tc.limitsRespectingRatioCPU.Value(), hintedCPULimits.Value())) + assert.EqualValues(t, tc.limitsRespectingRatioMemory.Value(), hintedMemoryLimits.Value(), fmt.Sprintf("memory limits doesn't match: %v != %v\n", tc.limitsRespectingRatioMemory.Value(), hintedMemoryLimits.Value())) + } } - - if tc.requestsExceedsRatioMemory { - assert.True(t, hints.RequestsExceedsRatio(0, apiv1.ResourceMemory)) - } else { - assert.False(t, hints.RequestsExceedsRatio(0, apiv1.ResourceMemory)) - } - - hintedCPULimits := hints.HintedLimit(0, apiv1.ResourceCPU) - hintedMemoryLimits := hints.HintedLimit(0, apiv1.ResourceMemory) - assert.EqualValues(t, tc.limitsRespectingRatioCPU.Value(), hintedCPULimits.Value(), fmt.Sprintf("cpu limits doesn't match: %v != %v\n", tc.limitsRespectingRatioCPU.Value(), hintedCPULimits.Value())) - assert.EqualValues(t, tc.limitsRespectingRatioMemory.Value(), hintedMemoryLimits.Value(), fmt.Sprintf("memory limits doesn't match: %v != %v\n", tc.limitsRespectingRatioMemory.Value(), hintedMemoryLimits.Value())) }) } diff --git a/vertical-pod-autoscaler/pkg/admission-controller/logic/server.go b/vertical-pod-autoscaler/pkg/admission-controller/logic/server.go index f5e732eb6ee5..9c7a9ec61f28 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/logic/server.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/logic/server.go @@ -75,7 +75,10 @@ func (s *AdmissionServer) getPatchesForPodResourceRequest(raw []byte, namespace annotationsPerContainer = vpa_api_util.ContainerToAnnotationsMap{} } - limitsHints := s.limitsChecker.NeedsLimits(&pod, containersResources) + limitsHints, err := s.limitsChecker.NeedsLimits(&pod, containersResources) + if err != nil { + return nil, err + } patches := []patchRecord{} updatesAnnotation := []string{} @@ -137,14 +140,13 @@ func (s *AdmissionServer) getContainerPatch(pod v1.Pod, i int, annotationsPerCon annotations = make([]string, 0) } - if !limitsHints.IsNil() { + if limitsHints != nil { var resources v1.ResourceList resourceNames := []v1.ResourceName{"cpu", "memory"} for _, resource := range resourceNames { if limitsHints.RequestsExceedsRatio(i, resource) { - // we need just to take care of max ratio - // setting limits to request*maxRatio, - // It's needed when we are lowering requests too much + // LimitRange cannot specify min ratio: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.12/#limitrangeitem-v1-core + // If we exceed max ratio cap limit to request*maxRatio. limit := limitsHints.HintedLimit(i, resource) if resources == nil { resources = make(v1.ResourceList) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/logic/server_test.go b/vertical-pod-autoscaler/pkg/admission-controller/logic/server_test.go index 83ad62dce2eb..864f1a479d7e 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/logic/server_test.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/logic/server_test.go @@ -320,7 +320,7 @@ func TestGetPatchesForResourceRequest(t *testing.T) { fppp := fakePodPreProcessor{e: tc.podPreProcessorError} fvpp := fakeVpaPreProcessor{} frp := fakeRecommendationProvider{tc.recommendResources, tc.recommendAnnotations, tc.recommendName, tc.recommendError} - lc := NewLimitsChecker(nil) + lc := NewNoopLimitsChecker() s := NewAdmissionServer(&frp, &fppp, &fvpp, lc) patches, err := s.getPatchesForPodResourceRequest(tc.podJson, tc.namespace) if tc.expectError == nil { @@ -368,7 +368,7 @@ func TestGetPatchesForResourceRequest_TwoReplacementResources(t *testing.T) { }`) recommendAnnotations := vpa_api_util.ContainerToAnnotationsMap{} frp := fakeRecommendationProvider{recommendResources, recommendAnnotations, "name", nil} - lc := NewLimitsChecker(nil) + lc := NewNoopLimitsChecker() s := NewAdmissionServer(&frp, &fppp, &fvpp, lc) patches, err := s.getPatchesForPodResourceRequest(podJson, "default") assert.NoError(t, err) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/main.go b/vertical-pod-autoscaler/pkg/admission-controller/main.go index 3d6313b05754..e2d22be0f3c5 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/main.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/main.go @@ -86,10 +86,17 @@ func main() { recommendationProvider := logic.NewRecommendationProvider(vpaLister, vpa_api_util.NewCappingRecommendationProcessor(), targetSelectorFetcher) podPreprocessor := logic.NewDefaultPodPreProcessor() vpaPreprocessor := logic.NewDefaultVpaPreProcessor() - limitsChecker := logic.NewLimitsChecker(nil) + var limitsChecker logic.LimitsChecker if *allowToAdjustLimits { - limitsChecker = logic.NewLimitsChecker(factory) + limitsChecker, err = logic.NewLimitsChecker(factory) + if err != nil { + klog.Errorf("Failed to create limitsChecker, falling back to not checking limits. Error message: %s", err) + limitsChecker = logic.NewNoopLimitsChecker() + } + } else { + limitsChecker = logic.NewNoopLimitsChecker() } + as := logic.NewAdmissionServer(recommendationProvider, podPreprocessor, vpaPreprocessor, limitsChecker) http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { as.Serve(w, r)