CLI flags for VPA recommender aggregations config.

[amshuman-kr]

This PR exposes some of the aggregations configuration of VPA recommender as command-line flags in the VPA recommender. This can help change the default behaviour of the VPA recommender for different workload use-cases. The pre-existing constant values have been retained as the default values.

[k8s-ci-robot]

Welcome @amshuman-kr!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[amshuman-kr]

/assign @jbartosik

The failed test doesn't seem to have anything do with the change in this PR. It seems to be flakiness due to some randomness in the order of keys in the resources map. How should I proceed?

--- FAIL: TestGetPatchesForResourceRequest_TwoReplacementResources (0.00s)
    server_test.go:136: 
        	Error Trace:	server_test.go:136
        	            				server_test.go:410
        	Error:      	Should be true
        	Test:       	TestGetPatchesForResourceRequest_TwoReplacementResources
        	Messages:   	got {Op:add Path:/metadata/annotations/vpaUpdates Value:Pod resources updated by name: container 0: unobtanium request, cpu request}, want: {Op:add Path:/metadata/annotations/vpaUpdates Value:Pod resources updated by name: container 0: cpu request, unobtanium request}

[jbartosik]

The failure was caused by #2886. I think I have fixed the issue now.

/retest

[bskiba]

I've left a few comments. My main concern is that we are exposing as flags constants that are mutually dependent (one should be divisible by the other etc) which might not be obvious to the user.
Do you already have a use-case where you would like to modify those values? If yes can you share it? I'd like to understand what problems there are with the default implementation we propose.

[amshuman-kr]

Do you already have a use-case where you would like to modify those values? If yes can you share it? I'd like to understand what problems there are with the default implementation we propose

@bskiba, Thanks for the quick review!

We have clusters with non-batch workloads which still have load pattern cycles of less than 24h. Sometimes the workloads themselves might even last less than 24h (but still > 12h).

We are using VPA to scale these workloads vertically. This has worked well for scaling up. But not so much for scaling down. With the default values for aggregations config, scale down happens after 24h which might be too late or might not even happen as the next load cycle has started for the next day. We were thinking of tweaking these configurations to be less than 24h. Especially, the decay half-lives to something like 4h or 8h.

[amshuman-kr]

@bskiba I have addressed all of your review comments. Could you PTAL?

[k8s-ci-robot]

@amshuman-kr: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

[amshuman-kr]

cc @bskiba @jbartosik @krzysied

[bskiba]

Thanks a lot for reworking this.

I have small naming comments. I was thinking for a while if we can easily avoid having the global aggregations config variable but failed to find a way.

[amshuman-kr]

@bskiba Thanks for the review and I am sorry for the delay in response. I was not keeping well last week.

I have made the changes as you suggested. Can you PTAL?

[amshuman-kr]

@bskiba @jbartosik @krzysied. Could you PTAL?

[bskiba]

I'll take a look today, apolgies for the delay!

[bskiba]

/lgtm
/approve

[k8s-ci-robot]

[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

[amshuman-kr]

@bskiba Thanks a lot for your reviews, support and approval!