Add enhancement proposal for feature request #3913

[wangchen615]

Added the enhancement proposal for the support of a customized recommender in Vertical Pod Autoscaler.

@bskiba

[k8s-ci-robot]

Welcome @wangchen615!

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. 😃

[bskiba]

One concern that I did not think of before and @kgolab brought it to my attention:

What do we do with VPA objects that specify a recommender that does not exist? In the current proposal they would get no recommendations, without any way to signal that to the user. Maybe we should actually produce default recommendations for them? Can you check how this is done for the custom scheduling feature? It seems they would have the same problem.

[bskiba]

@jbartosik FYI

[wangchen615]

@bskiba @kgolab could you review again?

[kgolab]

@wangchen615 , I reviewed once more. I'm fine with the latest changes - no new comments there. However my old comments still stand:

• one is purely editorial: I think it would be useful to state how we expect the additional recommenders to be deployed
• the other was about not closing the way towards multiple recommenders. I understand you want to keep multiple recommenders but let's not close the possibility with this change. For example: what if recommenderName was not a string but an array? We could still say that for now only one recommender is supported (e.g. we don't change status) but if we ever come to multiple recommenders, the field won't need to be changed (or rather accompanied by a new one).

[wangchen615]

@wangchen615 , I reviewed once more. I'm fine with the latest changes - no new comments there. However my old comments still stand:

• one is purely editorial: I think it would be useful to state how we expect the additional recommenders to be deployed
• the other was about not closing the way towards multiple recommenders. I understand you want to keep multiple recommenders but let's not close the possibility with this change. For example: what if recommenderName was not a string but an array? We could still say that for now only one recommender is supported (e.g. we don't change status) but if we ever come to multiple recommenders, the field won't need to be changed (or rather accompanied by a new one).

@kgolab , I added a section called deployment details according to your comments. https://github.com/wangchen615/autoscaler/blob/customizable_recommender_kep/vertical-pod-autoscaler/enhancements/3919-customized-recommender-vpa/README.md#deployment-details

Could you help take a final review?

[kgolab]

[...]

@kgolab , I added a section called deployment details according to your comments. https://github.com/wangchen615/autoscaler/blob/customizable_recommender_kep/vertical-pod-autoscaler/enhancements/3919-customized-recommender-vpa/README.md#deployment-details

Could you help take a final review?

Thank you, this diagram and its description clarifies a lot.

[wangchen615]

@kgolab @bskiba Thanks a lot for all your comments. Is this KEP ready to be merged?

[kgolab]

@kgolab @bskiba Thanks a lot for all your comments. Is this KEP ready to be merged?

It looks fine from my perspective.

I'd still slightly prefer to make recommenderName an array already (and just use the first item for now) but since you marked similar comment as resolved, I guess your preference is to stay with a string field and solve the API change separately, if/when required.

Leaving to @bskiba for final approval.

[wangchen615]

/assign @bskiba

[bskiba]

Looks good, thanks a lot.

Last comment before LGTM is that I would like to once more to consider making recommender an array (the change Karol suggested), for which we will support only first element as part of this KEP. Otherwise, we will not be able to change that field later due to backwards incompatibility.

I can support you with adding validation for now that the array contains one item only.

One more nit: can you add white background to the image? I'm viewing github in dark mode and the image text is not visible in that setting.

[bskiba]

/lgtm
/approve

Thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba, wangchen615

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