Skip to content

Commit

Permalink
Address comments from original PR review
Browse files Browse the repository at this point in the history 
I'm keeping original CL from kubernetes#1813
and applying changes requested in the review in a separate CL to keep
autoship information clean.

Conflicts because master has VPA preprocessor, resolved manually:
	vertical-pod-autoscaler/pkg/admission-controller/logic/server_test.go
	vertical-pod-autoscaler/pkg/admission-controller/main.go
  • Loading branch information
@jbartosik
jbartosik committed Jun 11, 2019
1 parent 7ae2626 commit f121ef5
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 100 deletions.
78 changes: 38 additions & 40 deletions vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -27,41 +28,34 @@ 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
}

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

Expand Down Expand Up @@ -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{}
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -245,5 +243,5 @@ func (lc *limitsChecker) NeedsLimits(pod *v1.Pod, containersResources []Containe
if !needsLimits {
lrh = nil
}
return LimitsHints(lrh)
return LimitsHints(lrh), nil
}
107 changes: 56 additions & 51 deletions vertical-pod-autoscaler/pkg/admission-controller/logic/limitrange_checker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
})

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

}
Expand Down
12 changes: 7 additions & 5 deletions vertical-pod-autoscaler/pkg/admission-controller/logic/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,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{}
Expand Down Expand Up @@ -136,14 +139,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)
Expand Down
4 changes: 2 additions & 2 deletions vertical-pod-autoscaler/pkg/admission-controller/logic/server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ func TestGetPatchesForResourceRequest(t *testing.T) {
t.Run(fmt.Sprintf("test case: %s", tc.name), func(t *testing.T) {
fppp := fakePodPreProcessor{e: tc.preProcessorError}
frp := fakeRecommendationProvider{tc.recommendResources, tc.recommendAnnotations, tc.recommendName, tc.recommendError}
lc := NewLimitsChecker(nil)
lc := NewNoopLimitsChecker()
s := NewAdmissionServer(&frp, &fppp, lc)
patches, err := s.getPatchesForPodResourceRequest(tc.podJson, tc.namespace)
if tc.expectError == nil {
Expand Down Expand Up @@ -359,7 +359,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, lc)
patches, err := s.getPatchesForPodResourceRequest(podJson, "default")
assert.NoError(t, err)
Expand Down
10 changes: 8 additions & 2 deletions vertical-pod-autoscaler/pkg/admission-controller/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,15 @@ func main() {
)
recommendationProvider := logic.NewRecommendationProvider(vpaLister, vpa_api_util.NewCappingRecommendationProcessor(), targetSelectorFetcher)
podPreprocessor := logic.NewDefaultPodPreProcessor()
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, limitsChecker)
http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
Expand Down

0 comments on commit f121ef5

Please sign in to comment.