Ac take care of limit range #1813

safanaj · 2019-03-22T09:31:08Z

k8s-ci-robot · 2019-03-22T09:31:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: bskiba

If they are not already assigned, you can assign the PR to them by writing /assign @bskiba in a comment when ready.

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

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

bskiba · 2019-03-29T11:58:13Z

Thank you for this PR. There is good work here. We will review early next week, still discussing some points.
@kgolab

bskiba · 2019-04-03T10:02:25Z

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

@@ -0,0 +1,251 @@
+/*
+Copyright 2018 The Kubernetes Authors.


bskiba · 2019-04-03T10:03:19Z

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

+}
+
+type interestingData struct {
+	// Min                  v1.ResourceList


Should this be part of this PR?

bskiba · 2019-04-03T10:07:15Z

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

+
+var _ LimitsChecker = &limitsChecker{}
+
+// NewLimitsChecker create a LimitsChecker


nit: creates

bskiba · 2019-04-03T10:09:19Z

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

+var _ LimitsChecker = &neverNeedsLimitsChecker{}
+
+func (lc *neverNeedsLimitsChecker) NeedsLimits(pod *v1.Pod, containersResources []ContainerResources) LimitsHints {
+	return LimitsHints((*LimitRangeHints)(nil))


It might be simpler if we returned an object with two empty lists instead of nil.

bskiba · 2019-04-03T11:47:35Z

vertical-pod-autoscaler/pkg/recommender/model/vpa.go

@@ -121,7 +121,7 @@ func (vpa *Vpa) UseAggregationIfMatching(aggregationKey AggregateStateKey, aggre
 	}
 }

-// UsesAggregation returns true iff an aggregation with the given key contributes to the VPA.
+// UsesAggregation returns true if an aggregation with the given key contributes to the VPA.


This is not a typo - iff means if and only if

bskiba · 2019-04-03T11:48:39Z

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

+var _ LimitsChecker = &limitsChecker{}
+
+// NewLimitsChecker create a LimitsChecker
+func NewLimitsChecker(i interface{}) LimitsChecker {


Can we have a SharedInformerFactory passed here?

bskiba · 2019-04-03T11:50:03Z

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

@@ -75,11 +78,14 @@ func main() {
 	vpaLister := vpa_api_util.NewAllVpasLister(vpaClient, make(chan struct{}))
 	kubeClient := kube_client.NewForConfigOrDie(config)
 	factory := informers.NewSharedInformerFactory(kubeClient, defaultResyncPeriod)
+	if *allowToAdjustLimits {
+		factoryForLimitsChecker = factory


I'd rather explicitly pass the no-op limitChecker when *allowToAdjustLimits = false

bskiba · 2019-04-03T11:51:28Z

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

+
+			// Set limit if needed
+			if limitsHints.RequestsExceedsRatio(i, resource) {
+				// we need just to take care of max ratio


bskiba · 2019-04-03T11:57:08Z

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

+
+// LimitRangeHints implements LimitsHints interface
+type LimitRangeHints struct {
+	requestsExceedsRatio  []map[v1.ResourceName]bool


since v1.ResourceList is already a map, can we store proposedLimits map[container_name]proposed_limit_changes
Then:

requestExceedsRatio(container_name, resourceName) bool {
_, found := proposedLimits[container_name][resourceName]
return found
}

bskiba · 2019-04-03T12:05:33Z

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

+	limitranges, err := lc.limitrangeLister.
+		LimitRanges(pod.GetNamespace()).
+		List(labels.Everything())
+	if err == nil {


I'd rather fail fast:
if err != nil {
return nil
}

Also, it would be good to log the error.

bskiba · 2019-04-03T13:56:07Z

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

+					continue
+				}
+				foundInterstingData = true
+				id.parse(&lri)


Is it possible to have multiple items that have non nil MaxLimitRequestRatio per namespace?

bskiba · 2019-04-03T13:59:01Z

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

@@ -0,0 +1,236 @@
+/*
+Copyright 2018 The Kubernetes Authors.


bskiba · 2019-04-03T14:00:24Z

Thank you very much for this PR, it is needed functionality for VPA.

I left some initial comments. My main concern is why do we only care about MaxRatio? I assume we can also easily violate the MinRatio constraint.

safanaj · 2019-04-08T06:55:24Z

Thanks for the comments, I will take these as soon as I have time

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`

safanaj · 2019-05-10T14:41:33Z

Hi @bskiba I am sorry for the delay but was a very busy month for me.
I re-pushed the branch for the PR rebased on master. I addressed several of you comments. For other I am not sure.

My main concern is why do we only care about MaxRatio? I assume we can also easily violate the MinRatio constraint.

The min ratio is 1:1 and this would make limits equals to requests would make a pod in Guaranteed (QoS). In LimitRange API there is no MinRatio property.

Is it possible to have multiple items that have non nil MaxLimitRequestRatio per namespace?

Yes it is possible and in this case all ratio have to be respected so I get the minimum one. For the Default, to consider the limit quantity in case on the container there is not an explicit limit, is not so clear (and I am not sure if my code is correct) because looks like that the LimitRanger set the default limit from the first LimitRange it gets the problem here is that the query using the client-go could return a different order compared to what LimitRanger get on the server-side . I didn't verify this, btw I think that is quite uncommon to have multiple LimitRange for the same resources in a namespace.

bskiba · 2019-05-14T09:44:51Z

Thanks for the changes and the explanation. It was my oversight that I din't check properly that there is no corresponding minLimit, apologies.
I need some more time to review.

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

bskiba · 2019-05-30T10:41:29Z

@safanaj I believe the other PRs (based on this one) fix the original issue. I'm closing this, let me know if there is something still to be adressed.
/close

k8s-ci-robot · 2019-05-30T10:41:31Z

@bskiba: Closed this PR.

In response to this:

@safanaj I believe the other PRs (based on this one) fix the original issue. I'm closing this, let me know if there is something still to be adressed.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

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 the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 22, 2019

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 22, 2019

k8s-ci-robot requested review from bskiba and mwielgus March 22, 2019 09:31

safanaj force-pushed the ac-take-care-of-limit-range branch from 2403a97 to 98d2a05 Compare March 25, 2019 08:38

bskiba reviewed Apr 3, 2019

View reviewed changes

bskiba added the area/vertical-pod-autoscaler label Apr 25, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2019

safanaj force-pushed the ac-take-care-of-limit-range branch from 98d2a05 to 5304a4b Compare April 29, 2019 08:11

safanaj changed the base branch from master to vpa-release-0.5 April 29, 2019 08:33

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 Apr 29, 2019

safanaj changed the base branch from vpa-release-0.5 to master April 29, 2019 08:33

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 29, 2019

safanaj force-pushed the ac-take-care-of-limit-range branch from 49725d3 to cb63756 Compare May 10, 2019 13:00

k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels May 10, 2019

Added LimitsChecker that provides hints for limits

8b813d5

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`

safanaj force-pushed the ac-take-care-of-limit-range branch from cb63756 to 8b813d5 Compare May 10, 2019 13:09

jbartosik added a commit to jbartosik/autoscaler that referenced this pull request May 20, 2019

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.

jbartosik mentioned this pull request May 20, 2019

Limit range limit support #2049

Merged

k8s-ci-robot closed this May 30, 2019

tim-smart pushed a commit to arisechurch/autoscaler that referenced this pull request Nov 22, 2022

Address comments from original PR review

e7c03f1

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ac take care of limit range #1813

Ac take care of limit range #1813

safanaj commented Mar 22, 2019

k8s-ci-robot commented Mar 22, 2019

bskiba commented Mar 29, 2019 •

edited

Loading

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019

bskiba Apr 3, 2019 •

edited

Loading

bskiba Apr 3, 2019

bskiba commented Apr 3, 2019

safanaj commented Apr 8, 2019

safanaj commented May 10, 2019

bskiba commented May 14, 2019

bskiba commented May 30, 2019

k8s-ci-robot commented May 30, 2019


		var _ LimitsChecker = &limitsChecker{}

		// NewLimitsChecker create a LimitsChecker

Ac take care of limit range #1813

Ac take care of limit range #1813

Conversation

safanaj commented Mar 22, 2019

k8s-ci-robot commented Mar 22, 2019

bskiba commented Mar 29, 2019 • edited Loading

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 Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bskiba commented Apr 3, 2019

safanaj commented Apr 8, 2019

safanaj commented May 10, 2019

bskiba commented May 14, 2019

bskiba commented May 30, 2019

k8s-ci-robot commented May 30, 2019

bskiba commented Mar 29, 2019 •

edited

Loading

bskiba Apr 3, 2019 •

edited

Loading