VPA: Configurable container limit scaling #3028

johanneswuerbach · 2020-04-06T22:23:52Z

Make the proportional limit scaling configurable.

I'm happy to update the generated files and add tests if the approach looks acceptable.

johanneswuerbach · 2020-04-13T10:56:18Z

bskiba

Apologies for the very long wait.
As I see demand for this feature, I am sold on the approach 👍I added some comments but nothing major.
Please do add tests and update the generated files (in a separate commit).

Ideally we would also have e2e test verifying turning limit scaling off works as expected. There are already tests for limit scaling.

vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go

...d-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go

johanneswuerbach · 2020-05-13T21:48:33Z

Thank you for your feedback, I addressed the first batch of review comments and will add tests tomorrow.

johanneswuerbach · 2020-05-16T22:57:47Z

@bskiba comments addressed and tests added :-)

johanneswuerbach · 2020-05-29T20:30:55Z

/assign @bskiba

bskiba · 2020-06-02T08:10:19Z

I'll take another look later today

bskiba · 2020-06-02T17:56:28Z

Apologies, didn't make it today 😓, will take a look tomorrow.

bskiba

Once again sorry for taking forever.

Would it be possible to split into multiple commits?

Added types
Autogen
Implementation
e2e tests
vendor and go.mod/go.sum changes

Also since there are changes in go.sum, can you run go mod vendor to see if vendor/ directory gets changed?

bskiba · 2020-06-03T15:15:36Z

...e2e/vendor/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go

+	// Whether autoscaler limit scaling is enabled for the container. The default is "Auto".
+	// Enabling this requires the autoscaler to be enabled for the container
+	// +optional
+	LimitMode *ContainerLimitScalingMode `json:"limitMode,omitempty" protobuf:"bytes,6,rep,name=limitMode"`


I've thought about this a bit more and I think it would be good to make the setting here more explicit and change this to sth like:
ControlledValues: RequestsOnly/RequestsAndLimits

with RequestsAndLimits being the default.

WDYT?

That reads better, updated :-)

bskiba · 2020-06-03T15:17:55Z

...e2e/vendor/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go

+	// Specifies the type of recommendations that will be computed
+	// (and possibly applied) by VPA.
+	// If not specified, the default of [ResourceCPU, ResourceMemory] will be used.
+	ControlledResources *[]v1.ResourceName `json:"controlledResources,omitempty" patchStrategy:"merge" protobuf:"bytes,5,rep,name=controlledResources"`


Any idea why ControlledResources shows up as diff here? I think it's already merged into master. Or are my eyes playing tricks on me?

The vendored e2e types don't include this in master https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/e2e/vendor/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go#L146-L149

johanneswuerbach · 2020-06-06T13:31:13Z

@bskiba rebased, implemented the requested changes and splitted the commits as requested :-)

bskiba

I think we're close \o/ Thanks for structuring this and for your patience :)

Left two small comments.
Question: Is it possible in the RequestsOnly mode that we will try to create a pod with request > limit?
We need to avoid that, since it causes the pod to be rejected by API server.

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go

...oscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go

bskiba · 2020-06-08T15:22:31Z

The travis fail seems unrelated (spellcheck failed in cluster autoscaler)

johanneswuerbach · 2020-06-12T22:14:33Z

@bskiba I added code & test to always stay below the limits for any recommendation provided. Is this what you had in mind?

johanneswuerbach · 2020-06-30T23:01:15Z

Ping @bskiba :-)

bskiba

Looks, good, I have one more request for the capping to limit code.
Thanks for the additional tests!

bskiba · 2020-07-02T08:38:30Z

...d-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go

@@ -70,6 +70,22 @@ func GetContainersResources(pod *core.Pod, vpaResourcePolicy *vpa_types.PodResou
 				}
 			}
 		}
+		// Ensure container requests always stay below container limits


Can you take a look at this: 8bc98b7

This is a PR that removed recommendation to limit capping when we started doing proportional limit. The capping is applied to all recommendation parts (Target, Upper and Lower limit). Could you reuse that solution? (sorry for only providing it now)

@bskiba updated. Does that look okay now?

johanneswuerbach · 2020-07-13T08:12:06Z

Ping @bskiba :-)

bskiba

Thanks for this change and thanks a lot for your patience with the reviewer (aka me)!
Looks good

/lgtm
/approve

k8s-ci-robot · 2020-07-15T07:56:27Z

[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:

~~vertical-pod-autoscaler/OWNERS~~ [bskiba]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 6, 2020

k8s-ci-robot requested review from jbartosik and krzysied April 6, 2020 22:24

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

k8s-ci-robot assigned jbartosik Apr 13, 2020

bskiba suggested changes May 7, 2020

View reviewed changes

bskiba mentioned this pull request May 7, 2020

General discussion: Limit scaling and performance throttling #3091

Closed

johanneswuerbach force-pushed the configurable-limit-scaling branch from 795f4aa to 2d63e89 Compare May 13, 2020 21:41

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2020

johanneswuerbach force-pushed the configurable-limit-scaling branch from 2d63e89 to 8fdc63d Compare May 13, 2020 21:41

johanneswuerbach force-pushed the configurable-limit-scaling branch 2 times, most recently from 30940f8 to 71ff275 Compare May 16, 2020 22:57

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 16, 2020

johanneswuerbach force-pushed the configurable-limit-scaling branch from 71ff275 to 3e664f1 Compare May 16, 2020 23:22

k8s-ci-robot assigned bskiba May 29, 2020

bskiba reviewed Jun 3, 2020

View reviewed changes

johanneswuerbach force-pushed the configurable-limit-scaling branch from 3e664f1 to 95b7d1e Compare June 6, 2020 13:28

bskiba reviewed Jun 8, 2020

View reviewed changes

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go Outdated Show resolved Hide resolved

...oscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go Show resolved Hide resolved

johanneswuerbach force-pushed the configurable-limit-scaling branch from 95b7d1e to 53e4758 Compare June 12, 2020 22:13

bskiba reviewed Jul 2, 2020

View reviewed changes

johanneswuerbach added 2 commits July 5, 2020 15:31

vpa: add container controlled values to types

c90abe6

vpa: update-codegen types

33eab5e

johanneswuerbach force-pushed the configurable-limit-scaling branch 2 times, most recently from 44fe28a to 86a3c30 Compare July 5, 2020 13:33

johanneswuerbach requested a review from bskiba July 5, 2020 13:33

johanneswuerbach added 3 commits July 5, 2020 15:36

vpa: container controlled values

4fdd927

vpa: e2e update vpa

f251c9c

vpa: container controlled values e2e

727dab6

johanneswuerbach force-pushed the configurable-limit-scaling branch from 86a3c30 to 727dab6 Compare July 5, 2020 13:36

bskiba approved these changes Jul 15, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2020

k8s-ci-robot merged commit 465d02a into kubernetes:master Jul 15, 2020

bskiba mentioned this pull request Jul 21, 2020

Only cap to limit in RequestOnly mode #3340

Merged

johanneswuerbach mentioned this pull request Jul 31, 2020

VPA Add controlledValues to CRD #3391

Merged

raywainman mentioned this pull request Sep 17, 2024

VPA Not honoring maxAllowed Memory Limit #6996

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPA: Configurable container limit scaling #3028

VPA: Configurable container limit scaling #3028

johanneswuerbach commented Apr 6, 2020

johanneswuerbach commented Apr 13, 2020

bskiba left a comment

johanneswuerbach commented May 13, 2020 •

edited

Loading

johanneswuerbach commented May 16, 2020

johanneswuerbach commented May 29, 2020

bskiba commented Jun 2, 2020

bskiba commented Jun 2, 2020

bskiba left a comment

bskiba Jun 3, 2020

johanneswuerbach Jun 6, 2020

bskiba Jun 3, 2020

johanneswuerbach Jun 6, 2020

johanneswuerbach commented Jun 6, 2020

bskiba left a comment

bskiba commented Jun 8, 2020

johanneswuerbach commented Jun 12, 2020

johanneswuerbach commented Jun 30, 2020

bskiba left a comment

bskiba Jul 2, 2020

johanneswuerbach Jul 5, 2020

johanneswuerbach commented Jul 13, 2020

bskiba left a comment

k8s-ci-robot commented Jul 15, 2020

VPA: Configurable container limit scaling #3028

VPA: Configurable container limit scaling #3028

Conversation

johanneswuerbach commented Apr 6, 2020

johanneswuerbach commented Apr 13, 2020

bskiba left a comment

Choose a reason for hiding this comment

johanneswuerbach commented May 13, 2020 • edited Loading

johanneswuerbach commented May 16, 2020

johanneswuerbach commented May 29, 2020

bskiba commented Jun 2, 2020

bskiba commented Jun 2, 2020

bskiba left a comment

Choose a reason for hiding this comment

bskiba Jun 3, 2020

Choose a reason for hiding this comment

johanneswuerbach Jun 6, 2020

Choose a reason for hiding this comment

bskiba Jun 3, 2020

Choose a reason for hiding this comment

johanneswuerbach Jun 6, 2020

Choose a reason for hiding this comment

johanneswuerbach commented Jun 6, 2020

bskiba left a comment

Choose a reason for hiding this comment

bskiba commented Jun 8, 2020

johanneswuerbach commented Jun 12, 2020

johanneswuerbach commented Jun 30, 2020

bskiba left a comment

Choose a reason for hiding this comment

bskiba Jul 2, 2020

Choose a reason for hiding this comment

johanneswuerbach Jul 5, 2020

Choose a reason for hiding this comment

johanneswuerbach commented Jul 13, 2020

bskiba left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 15, 2020

johanneswuerbach commented May 13, 2020 •

edited

Loading