Limit range limit support #2049

jbartosik · 2019-05-20T16:12:49Z

This is built on #1813. I follow it with a change addressing comments from review of the original PR. On top of that I make changes to logic of handling limit range.

VPA now preserves limit/request ratio configured by user. VPA will now preserve the ratio if limit comes from default in limit range. It will also preserve the ratio if it must cap limit to keep it below max defined in the limit range.

This uses limit range that appears first in the list and applies to containers. It should be able to handle multiple limit ranges. It should also handle limit ranges on pods.

To allow admission controller to set limits on containers if needed because LimitRange in namespace with default and max ratio. This feature have to be explicitly enabled by passing the flag `--allow-to-adjust-limits`

I'm keeping original CL from kubernetes#1813 and applying changes requested in the review in a separate CL to keep autoship information clean.

jbartosik · 2019-05-20T16:15:53Z

@safanaj I'd like support for limit range to VPA. I would appreciate if you find time to check if changes I'm applying on top of your PR make sense.

jbartosik · 2019-05-20T16:16:14Z

Issue: #1812

jbartosik · 2019-05-20T16:17:19Z

Marking as WIP - I think this is ready for review but I didn't perform any E2E tests yet.

jbartosik · 2019-05-20T16:17:50Z

/assign @bskiba

jbartosik · 2019-05-20T18:33:15Z

@kgolab PTAL if you have time.

vertical-pod-autoscaler/pkg/admission-controller/main.go

bskiba · 2019-05-22T09:21:16Z

@jbartosik There seems to still be a lot of logic for checking the request/limit ratio. Since your changes for VPA to keep the request to limit ratio constant, I think we no longer need it. The only thing we need to observe is the appropriate maxRatio.

safanaj · 2019-05-23T12:02:45Z

@jbartosik Thanks, I think you addressed what I missed in my PR review cycle and does make sense for me.

safanaj · 2019-05-27T07:12:39Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator.go

+
+	for _, lr := range limitRanges {
+		for _, lri := range lr.Spec.Limits {
+			if lri.Type == v1.LimitTypeContainer || lri.Type == v1.LimitTypePod && (lri.MaxLimitRequestRatio == nil || lri.Default == nil) {


Hi @jbartosik , I don't get this. In case a LimitRange has MaxRatio and Default both not nil we don't choose that Limit?
Also, in server code I understand that any limit range have to be respected so the more restrictive (minumum max ratio) have to be respected. In this code I understand that we are not checking for the more restrictive limits max ratio.

My mistake. We care about limit range if it has at least one of Max / Default set.

but what about the MaxRatio? the purpose of my initial PR was exactly just to shrink limits in case a max ratio was present in LimitRange definition to prevent that after the vpa-updater kill a pod the pod couldn't be recreated anymore because the vpa-admission-controller is setting the requests too low and the LimitRanger admission controller prevent the pod to be created.
I am sorry but I am a bit lost on this rework. this PR like it is now doesn't address anymore the issue #1812

@safanaj since your PR we have changed the logic of VPA to keep the ratio between request and limit constant. This means that VPA will never break things - since we keep the original request to limit ratio that was specified for the deployment, we rely on the user to specify it in a way that respects the MaxRatio.

Keep limit/request user set (even if the limit comes from limit range). Cap limits to Max configured in the limit range.

bskiba

Main problem - we are changing the recommendation for request as seen by admission controller (if we proportionally cap to maxlimit) but we do not do this in updater.
We can end up with an infinite updating loop since updater has a different view of recommendation than admission controller.

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator.go

bskiba · 2019-05-28T12:08:22Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator.go

+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/client-go/informers"
+
+	v1_listers "k8s.io/client-go/listers/core/v1"


nit: just listers?

bskiba · 2019-05-28T12:08:52Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator.go

+)
+
+// LimitsRangeCalculator checks for LimitRange and if container needs limits to be set
+type LimitsRangeCalculator interface {


This is actually a LimitRangeItemLister, with a filter applied. So maybe LimitRangeFetcher would be a more appropriate name.

It will aggregate all matching limit ranges into a single limit range having the same effect. For example if one limit range specifies default, and two other limit ranges specify two different maxes it would return limit range with default from the first limit range and lower max.

I will send that change for review this week so I thought I'd avoid renaming the class.

Ok, then since it's LimitRange not LimitsRange, lets call it LimitRangeCalculator

bskiba · 2019-05-28T12:10:54Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator.go

+		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 &v1.LimitRangeItem{


Why do we make a copy?

Not necessary, done.

bskiba · 2019-05-28T12:11:35Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator_test.go

+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/informers"
+
+	//"fmt"


nit: please organise import and remove comments

bskiba · 2019-05-28T12:21:40Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator_test.go

+		pod          *apiv1.Pod
+		limitRanges  []runtime.Object
+		expectErr    error
+		expectLimits *apiv1.LimitRangeItem


nit: expectedLimits

bskiba · 2019-05-28T12:22:08Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator_test.go

+	}
+}
+
+func TestUpdateResourceLimits(t *testing.T) {


nit: TestGetContainerLimitRangeItem

bskiba · 2019-05-28T12:23:56Z

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

-		return resource.NewMilliQuantity(recommendedMilliLimit.Int64(), recommendedRequest.Format), false
-	}
-	return resource.NewMilliQuantity(math.MaxInt64, recommendedRequest.Format), true
+	return scaleQuantityProportionally( /*scaledQuantity=*/ originalLimit /*scaleBase=*/, originalRequest /*scaleResult=*/, recommendedRequest)


The commas seem to be in weird places.

bskiba · 2019-05-28T12:32:46Z

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

+	return scaledRequest, maxLimit
+}
+
+func proportionallyCapLimitsToMax(recommendedRequests v1.ResourceList, cpuLimit, memLimit, maxCpuLimit, maxMemLimit *resource.Quantity) ContainerResources {


I'd call it proportionallyCapResourcesToMaxLimit, since it also caps request (I was suprised)

This logic actually changes the recommended request, but this is not included in the updater - updater and admission controller will disagree about the request - we can get into an infinite updating loop.

bskiba · 2019-05-28T12:33:46Z

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

+	scaledCpuRequest, scaledCpuLimit := proportionallyCapLimitToMax(recommendedRequests.Cpu(), cpuLimit, maxCpuLimit)
+	scaledMemRequest, scaledMemLimit := proportionallyCapLimitToMax(recommendedRequests.Memory(), memLimit, maxMemLimit)
+	result := newContainerResources()
+	if scaledCpuRequest != nil {


When will those be nil?

Limits might be nil (if they're not in the original request and there is no default in the limit range).

kgolab · 2019-05-28T13:02:01Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator.go

+	v1_listers "k8s.io/client-go/listers/core/v1"
+)
+
+// LimitsRangeCalculator checks for LimitRange and if container needs limits to be set


It's probably nitpicking but I don't see why this interface is called Calculator, Finder would be more appropriate.
I also don't get the second part of the comment.

Reworded comment (to describe what I intend the class to do). For now this returns first matching limit range item in the list. In future PR I will improve the logic.

kgolab · 2019-05-28T13:05:13Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator.go

+	f.Start(stopCh)
+	for _, ok := range f.WaitForCacheSync(stopCh) {
+		if !ok {
+			if f.Core().V1().LimitRanges().Informer().HasSynced() {


Shouldn't this be ! HasSynced() ?

kgolab · 2019-05-28T13:10:10Z

vertical-pod-autoscaler/pkg/admission-controller/logic/limit_range_calculator_test.go

+			name: "no matching limit ranges",
+			pod:  getPod(),
+			limitRanges: []runtime.Object{
+				test.LimitRange().WithName("different-namespace").WithNamespace("different").WithType(apiv1.LimitTypePod).WithMax(test.Resources("2", "2")).Get(),


nit: since it's different namespace already maybe it's not worth changing type too?

The point is to test that neither limit range matches. One has correct type (now, it was a pod, I think this was you requested) but wrong namespace. Other has good namespace but wrong type.

jbartosik · 2019-05-29T13:45:44Z

@bskiba PTAL

bskiba · 2019-05-29T13:58:13Z

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

@@ -30,7 +30,7 @@ type RecommendationProcessor interface {
 	// VPA policy and possibly other internal RecommendationProcessor context.
 	// Must return a non-nil pointer to RecommendedPodResources or error.
 	Apply(podRecommendation *vpa_types.RecommendedPodResources,
-		policy *vpa_types.PodResourcePolicy,
-		conditions []vpa_types.VerticalPodAutoscalerCondition,
+		_ *vpa_types.PodResourcePolicy,


Was in hurry, made a mistake.

bskiba · 2019-05-29T13:58:42Z

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

@@ -84,6 +91,36 @@ func (c *cappingRecommendationProcessor) Apply(
 	return &vpa_types.RecommendedPodResources{ContainerRecommendations: updatedRecommendations}, containerToAnnotationsMap, nil
 }

+func (c *cappingRecommendationProcessor) scaleInProportionWithLimit(podRecommendation *vpa_types.RecommendedPodResources, pod *apiv1.Pod) ([]vpa_types.RecommendedContainerResources, error) {


This is actually capProportionallyToMaxLimit

bskiba · 2019-05-29T13:59:05Z

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

+		}
+		cpuLimit, _ := GetProportionalLimit(container.Resources.Limits.Cpu(), container.Resources.Requests.Cpu(), recommendation.Target.Cpu(), defaultCpu)
+		memLimit, _ := GetProportionalLimit(container.Resources.Limits.Memory(), container.Resources.Requests.Memory(), recommendation.Target.Memory(), defaultMem)
+		capped := ProportionallyCapResourcesToMaxLimit(recommendation.Target, cpuLimit, memLimit, maxCpuLimit, maxMemLimit)


If capping has been done, this definitely deserves an annotation.

bskiba · 2019-05-29T14:14:05Z

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

+				ContainerName: "container",
+				Target: apiv1.ResourceList{
+					apiv1.ResourceCPU:    resource.MustParse("1000m"),
+					apiv1.ResourceMemory: resource.MustParse("1000000000000m"),


(I'm not sure if my comment was lost) Why millivalue here? It's not too readable

I want to use assert.equal to keep test easy to read. The way assert.equal works representation of quantities must be the same, not just their actual values.

bskiba · 2019-05-29T14:56:23Z

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

+			containerToAnnotationsMap[container.Name] = append(containerToAnnotationsMap[container.Name], "changed CPU limit to fit within limit range")
+		}
+		if capped.Requests.Memory().Value() != recommendation.Target.Memory().Value() {
+			containerToAnnotationsMap[container.Name] = append(containerToAnnotationsMap[container.Name], "changed memoery limit to fit within limit range")


nit s/memoery/memory/

bskiba · 2019-05-29T15:04:02Z

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

@@ -62,7 +65,11 @@ func (c *cappingRecommendationProcessor) Apply(
 	}
 	updatedRecommendations := []vpa_types.RecommendedContainerResources{}
 	containerToAnnotationsMap := ContainerToAnnotationsMap{}
-	for _, containerRecommendation := range podRecommendation.ContainerRecommendations {
+	limitAdjustedRecommendation, err := c.capProportionallyToMaxLimit(podRecommendation, pod, containerToAnnotationsMap)


This doesn't work. We need to apply capping also to upper and lower bound. What we need is to integrate capping to max/min limitrange into getCappedRecommendationForContainer function.
Just as we have ApplyVpaPolicy, we need to have ApplyMaxRange here:

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

Line 146 in 5094dc0

process := func(recommendation apiv1.ResourceList, genAnnotations bool) {

Apologies for not spotting it earlier.

Changed to adjust not only target but also bounds. I think this will work correctly (if limit range, limit/request ratio and policy allow assigning a recommendation).

To be squashed before submit

bskiba · 2019-05-30T07:22:20Z

/lgtm
/approve

please remove WIP from the title

k8s-ci-robot · 2019-05-30T07:22:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bskiba]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

safanaj and others added 2 commits May 20, 2019 15:47

Added LimitsChecker that provides hints for limits

a619223

To allow admission controller to set limits on containers if needed because LimitRange in namespace with default and max ratio. This feature have to be explicitly enabled by passing the flag `--allow-to-adjust-limits`

Address comments from original PR review

0d2afe5

I'm keeping original CL from kubernetes#1813 and applying changes requested in the review in a separate CL to keep autoship information clean.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 20, 2019

k8s-ci-robot requested review from bskiba and MaciekPytel May 20, 2019 16:13

jbartosik changed the title ~~Limit range limit support~~ [WIP] Limit range limit support May 20, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 20, 2019

k8s-ci-robot assigned bskiba May 20, 2019

safanaj reviewed May 22, 2019

View reviewed changes

vertical-pod-autoscaler/pkg/admission-controller/main.go Outdated Show resolved Hide resolved

jbartosik force-pushed the limit-range-limit-support branch 2 times, most recently from 7686ed2 to 29a6991 Compare May 24, 2019 15:56

safanaj reviewed May 27, 2019

View reviewed changes

jbartosik force-pushed the limit-range-limit-support branch 3 times, most recently from 4a5d4d1 to 87937e5 Compare May 27, 2019 12:32

jbartosik added 2 commits May 27, 2019 14:43

Change logic for supporting limits.

33928d9

Keep limit/request user set (even if the limit comes from limit range). Cap limits to Max configured in the limit range.

Rename limit_range_checker files to limit_range_calculator

f1a537a

jbartosik force-pushed the limit-range-limit-support branch from 87937e5 to f1a537a Compare May 27, 2019 12:44

bskiba reviewed May 28, 2019

View reviewed changes

kgolab reviewed May 28, 2019

View reviewed changes

jbartosik force-pushed the limit-range-limit-support branch from 3b70e32 to 3aa21fe Compare May 29, 2019 13:42

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 29, 2019

jbartosik force-pushed the limit-range-limit-support branch from 3aa21fe to e4a8efa Compare May 29, 2019 13:45

jbartosik force-pushed the limit-range-limit-support branch from e4a8efa to 2437763 Compare May 29, 2019 13:53

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 29, 2019

jbartosik force-pushed the limit-range-limit-support branch from 77d7762 to 5f9eb0e Compare May 29, 2019 14:05

bskiba reviewed May 29, 2019

View reviewed changes

jbartosik force-pushed the limit-range-limit-support branch 3 times, most recently from 9a98d9d to 5094dc0 Compare May 29, 2019 14:55

bskiba reviewed May 29, 2019

View reviewed changes

Support scaling request to maintain ratio with limit in updater

1b0165c

To be squashed before submit

jbartosik force-pushed the limit-range-limit-support branch from 5094dc0 to 1b0165c Compare May 29, 2019 16:09

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2019

bskiba changed the title ~~[WIP] Limit range limit support~~ Limit range limit support May 30, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 30, 2019

k8s-ci-robot merged commit 63039fb into kubernetes:master May 30, 2019

jbartosik deleted the limit-range-limit-support branch May 30, 2019 07:49

This was referenced Jun 7, 2019

Cherry limit 05 #2105

Merged

Cherry pick limit support #2106

Merged

Cherry pick limit support #2117

Merged

johanneswuerbach mentioned this pull request Sep 18, 2019

VPA: Protect against runaway pods #2359

Closed

danielmellado mentioned this pull request Oct 24, 2019

Rebase to upstream/cluster-autoscaler-release-1.16 openshift/kubernetes-autoscaler#119

Closed

Limit range limit support #2049

Limit range limit support #2049

Conversation

jbartosik commented May 20, 2019 • edited Loading

jbartosik commented May 20, 2019

jbartosik commented May 20, 2019

jbartosik commented May 20, 2019

jbartosik commented May 20, 2019

jbartosik commented May 20, 2019

bskiba commented May 22, 2019

safanaj commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bskiba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbartosik commented May 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bskiba commented May 30, 2019

k8s-ci-robot commented May 30, 2019

jbartosik commented May 20, 2019 •

edited

Loading