Move targetCPUPercentile into a flag #4799

matthyx · 2022-04-07T14:12:14Z

Which component this PR applies to?

vertical-pod-autoscaler

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #4786

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add flags to specify recommender name and targetCPUPercentile via command line arguments.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

matthyx · 2022-04-07T14:17:07Z

@jbartosik something like that?
Let me know and I'll add some test coverage and docs.

matthyx · 2022-04-07T14:35:43Z

/test

k8s-ci-robot · 2022-04-07T14:39:23Z

@matthyx: No presubmit jobs available for kubernetes/autoscaler@master

In response to this:

/test

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.

jbartosik

Thank you, some comments

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go

vertical-pod-autoscaler/pkg/recommender/logic/recommender.go

vertical-pod-autoscaler/pkg/recommender/main.go

k8s-triage-robot · 2022-07-13T15:31:50Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

matthyx · 2022-08-04T11:16:54Z

/remove-lifecycle stale
@jbartosik can check and tell me what's missing, please?

jbartosik

Thanks. Mostly looking good, I made a few, hopefully small suggestions.

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder_test.go

vertical-pod-autoscaler/pkg/recommender/main.go

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go

vertical-pod-autoscaler/pkg/recommender/logic/recommender.go

vertical-pod-autoscaler/pkg/recommender/main.go

jbartosik · 2022-08-24T12:07:09Z

Please ping me when I should take another look

matthyx · 2022-08-25T14:36:47Z

Thanks @jbartosik I've missed that... does my change suit you?

jbartosik

I think we're getting close. I left a few comments but I saw just some small stuff to change.

jbartosik · 2022-08-26T14:19:00Z

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder_test.go


- targetSelectorFetcher.EXPECT().Fetch(vpa).Return(tc.selector, tc.fetchSelectorError)
+ if !tc.expectNoVpaFetch {


I'd strongly prefer to avoid having variables with negations in names. So expectVpaFetch instead of expectNoVpaFetch. It's easier to read & reason about variables that say something positive than about negated ones. I know it's some more writing for this commit but I think making code easier to maintain is worth it.

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder_test.go

vertical-pod-autoscaler/pkg/recommender/main.go

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go

matthyx · 2022-08-26T20:53:54Z

@jbartosik thanks for your help, I think we're good to go :)

jbartosik

Looks good to me. But I don't want to merge it for a while more. I want to finish releasing VPA 0.12.0 (I think code is good but e2e tests are timing out) before merging any changes to VPA that are not necessary for the release.

So

/hold

for now. I plan to unhold and merge after I publish the release

k8s-ci-robot · 2022-09-02T13:11:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbartosik, matthyx

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~~ [jbartosik]

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

jbartosik · 2022-09-20T11:20:31Z

/hold cancel

matthyx · 2022-09-21T08:35:53Z

@jbartosik can you also change the "Draft" status of the PR? I cannot do it, strangely :(

jbartosik · 2022-09-26T13:20:21Z

/lgtm

jbartosik · 2022-09-26T13:53:00Z

@matthyx can you add some documentation on running multiple recommenders.

I think it can be quite simple, something like:

a yaml with recommender with different cpu percentile and name in flag
a short .md file pointing to the yaml and highlighting the flags and what they do.

Move targetCPUPercentile into a flag

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 7, 2022

k8s-ci-robot requested review from jbartosik and krzysied April 7, 2022 14:12

matthyx force-pushed the flags branch from 963008f to 76c3f6a Compare April 7, 2022 14:32

matthyx mentioned this pull request Apr 9, 2022

add TargetCPUPercentile to VerticalPodAutoscalerSpec #4574

Closed

jbartosik added the area/vertical-pod-autoscaler label Apr 14, 2022

jbartosik requested changes Apr 14, 2022

View reviewed changes

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go Outdated Show resolved Hide resolved

vertical-pod-autoscaler/pkg/recommender/logic/recommender.go Outdated Show resolved Hide resolved

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

matthyx force-pushed the flags branch 2 times, most recently from 339909d to 5412b00 Compare April 14, 2022 13:51

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 14, 2022

matthyx force-pushed the flags branch 2 times, most recently from 06770e6 to ab7ddd8 Compare April 14, 2022 14:31

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 13, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 4, 2022

matthyx changed the title ~~WIP move targetCPUPercentile into a flag~~ Move targetCPUPercentile into a flag Aug 4, 2022

jbartosik requested changes Aug 17, 2022

View reviewed changes

vertical-pod-autoscaler/pkg/recommender/logic/recommender.go Outdated Show resolved Hide resolved

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

matthyx force-pushed the flags branch from ab7ddd8 to 3aedbed Compare August 23, 2022 14:48

matthyx force-pushed the flags branch from 3aedbed to c95cb58 Compare August 24, 2022 13:46

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 Aug 24, 2022

matthyx force-pushed the flags branch from b89d5d2 to fd100e1 Compare August 25, 2022 14:35

jbartosik requested changes Aug 26, 2022

View reviewed changes

move targetCPUPercentile into a flag

690dcd1

matthyx force-pushed the flags branch from fd100e1 to 690dcd1 Compare August 26, 2022 20:51

jbartosik approved these changes Sep 2, 2022

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2022

jbartosik marked this pull request as ready for review September 26, 2022 11:37

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 Sep 26, 2022

k8s-ci-robot requested a review from jbartosik September 26, 2022 11:37

k8s-ci-robot assigned jbartosik Sep 26, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2022

k8s-ci-robot merged commit 15a2afa into kubernetes:master Sep 26, 2022

matthyx deleted the flags branch October 3, 2022 06:50

This was referenced Oct 3, 2022

add example for multiple recommenders #5231

Merged

Make the target configurable #4293

Closed

navinjoy pushed a commit to navinjoy/autoscaler that referenced this pull request Oct 26, 2022

Merge pull request kubernetes#4799 from matthyx/flags

51beced

Move targetCPUPercentile into a flag

voelzmo mentioned this pull request Dec 12, 2022

VPA: make parameters oomBumpUpRatio and oomMinBumpUp configurable #5275

Merged

voelzmo mentioned this pull request Jan 30, 2023

Documented but unknown flags "--name" and "--target-cpu-percentile" for recommender #5451

Closed

This was referenced Feb 3, 2023

Bump default VPA version to 0.13.0 #5462

Merged

Bump default VPA version to 0.13.0 #5460

Merged

mKeRix mentioned this pull request Apr 10, 2024

Make VPA recommender target memory percentile configurable #6700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move targetCPUPercentile into a flag #4799

Move targetCPUPercentile into a flag #4799

matthyx commented Apr 7, 2022 •

edited

Loading

matthyx commented Apr 7, 2022

matthyx commented Apr 7, 2022

k8s-ci-robot commented Apr 7, 2022

jbartosik left a comment

k8s-triage-robot commented Jul 13, 2022

matthyx commented Aug 4, 2022

jbartosik left a comment

jbartosik commented Aug 24, 2022

matthyx commented Aug 25, 2022

jbartosik left a comment

jbartosik Aug 26, 2022

matthyx commented Aug 26, 2022

jbartosik left a comment

k8s-ci-robot commented Sep 2, 2022

jbartosik commented Sep 20, 2022

matthyx commented Sep 21, 2022

jbartosik commented Sep 26, 2022

jbartosik commented Sep 26, 2022


		targetSelectorFetcher.EXPECT().Fetch(vpa).Return(tc.selector, tc.fetchSelectorError)
		if !tc.expectNoVpaFetch {

Move targetCPUPercentile into a flag #4799

Move targetCPUPercentile into a flag #4799

Conversation

matthyx commented Apr 7, 2022 • edited Loading

Which component this PR applies to?

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

matthyx commented Apr 7, 2022

matthyx commented Apr 7, 2022

k8s-ci-robot commented Apr 7, 2022

jbartosik left a comment

Choose a reason for hiding this comment

k8s-triage-robot commented Jul 13, 2022

matthyx commented Aug 4, 2022

jbartosik left a comment

Choose a reason for hiding this comment

jbartosik commented Aug 24, 2022

matthyx commented Aug 25, 2022

jbartosik left a comment

Choose a reason for hiding this comment

jbartosik Aug 26, 2022

Choose a reason for hiding this comment

matthyx commented Aug 26, 2022

jbartosik left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 2, 2022

jbartosik commented Sep 20, 2022

matthyx commented Sep 21, 2022

jbartosik commented Sep 26, 2022

jbartosik commented Sep 26, 2022

matthyx commented Apr 7, 2022 •

edited

Loading