doc: proposal custom cloud provider over gRPC

[hectorj2f]

As discussed over Slack with @MaciekPytel, I share a proposal to make CA more plugable and allow calling custom cloud providers without having to fork the CA.

[hectorj2f]

/assign @MaciekPytel

[MaciekPytel]

I think the idea is sound, but this design is missing a lot of details (most importantly all the stuff related to NodeGroups).

[elmiko]

overall i think it's a nice idea, i added a few of my nits.

i also agree with @MaciekPytel's comments.

[bhargav-naik]

@hectorj2f

• I was working on similar usecase(Proposal for decoupling ClusterAutoScaler from CloudProvider implemen… #3127) where the aim was to decouple cloudProvider specific implementation from core and being able to plugin new cloudProvider and processors at boot-up time.

I went through your proposal and had a query:

• How can ExternalCloudProvider plugin nodeInfoComparator? (a very common requirement for cloudProvider implementation)

[bhargav-naik]

Some more points:

• Is it a design goal to give flexibility to let cloudProvider to use any language/framework of choice for their implementation?
• If someone wants to override the autoscaling_processor, they will still have to maintain their fork and add override the processor.
• Why do we want to go for a solve which will increase runtime complexity (need of separate process for CloudProvider and have a gRPC communication between CA and CloudProvider process) instead of making changes in compile time workflow ?

[hardikdr]

We would also need to define the contract between gRPC client and server, basically the messages, error-codes, and so on. Wondering if that should be part of this proposal as well?

• Also, there could be a scope of learning a little from here the csi-model, around using error-schemes, secret management, and so on, only if relatable.

[hectorj2f]

@bhargav-naik

Is it a design goal to give flexibility to let cloudProvider to use any language/framework of choice for their implementation?

I think a bit of common sense would help here, golang is the language commonly used in Kubernetes. If users want to code on a different language that would be up to them. Besides of that, this is not a requirement neither a motivation for this proposal.

If someone wants to override the autoscaling_processor, they will still have to maintain their fork and add override the processor.

This external provider could be extended to satisfy that purpose. It should not be considered as the initial requirement in my opinion, (solving a problem at the time). We could iterate over it and leave the design open to expose the processors per provider in the future.

Why do we want to go for a solve which will increase runtime complexity (need of separate process for CloudProvider and have a gRPC communication between CA and CloudProvider process) instead of making changes in compile time workflow ?

I have to argue about this. The runtime complexity would depend on where this external provider would seat. I believe the purpose is being flexible and dynamic here. gRPC doesn't offer a bit penalty in terms of computation.

Regarding #3127, I think the development and maintenance cost of moving out all the cloud providers is really high. This proposal tries to add a new external provider without re-designing the whole CA. The impact is minimal and everyone could benefit from it.

[bhargav-naik]

Inline replies:
I think a bit of common sense would help here, golang is the language commonly used in Kubernetes. If users want to code on a different language that would be up to them. Besides of that, this is not a requirement neither a motivation for this proposal.

• In absence of this requirement, what is the motivation to decouple CloudProvider using gRPC.

This external provider could be extended to satisfy that purpose. It should not be considered as the initial requirement in my opinion, (solving a problem at the time). We could iterate over it and leave the design open to expose the processors per provider in the future.

• I called out this as it was called out as a requirement in Proposal for decoupling ClusterAutoScaler from CloudProvider implemen… #3127. Also it came out as requirement primarily for nodeInfoComparator.

I have to argue about this. The runtime complexity would depend on where this external provider would seat. I believe the purpose is being flexible and dynamic here. gRPC doesn't offer a bit penalty in terms of computation.

• Now user will have to solve on how to run 1 extra process and deal with issues/debugging of distributed process. If the cloudProvider is able to plugin it's implementation in the same runtime wouldn't it be simpler.

[hectorj2f]

I called out this as it was called out as a requirement in #3127. Also it came out as requirement primarily for nodeInfoComparator.

Does #3127 cover this scenario ?

Now user will have to solve on how to run 1 extra process and deal with issues/debugging of distributed process. If the cloudProvider is able to plugin it's implementation in the same runtime wouldn't it be simpler.

You are making deployment assumptions that will depend on how and where the grpc server leaves. I suggest you have a look at grafana go plugins.

If the cloudProvider is able to plugin it's implementation in the same runtime wouldn't it be simpler.

Honestly, I didn't like your initial approach because was unrealistic and hard to maintain in a longer term. However you just made some changes that might ease this integration, that makes me like it more now. There are many questions to be answered there but there is some progress.

[bhargav-naik]

Inline replies:
Does #3127 cover this scenario ?

• I have updated the current proposal to enable injection of autoscaling_processor in a similar manner handled in the original proposal. Please have a look at it and provide inputs.

Honestly, I didn't like your initial approach because was unrealistic and hard to maintain in a longer term.

• The motivation for earlier approach was to decouple the Cloudprovider completely from the core (not having client libraries even in a vendored form). Though I understand the apprehension on such a big change as it radically changes the dev/build/release cycles. I leave that call with the project maintainers. Though I believe that in the long term it will be benificial as core, cloudprovider modules can have their own independent release per CloudProvider. End user can just use the required go module (based on their CP) and not have to maitain their separate builds. They will not get any unnecessary dependencies (even in vendored form).

[mwielgus]

What is the status of this PR? Did you address the comments?

[hectorj2f]

@mwielgus I didn't address them yet. I'd try to address them during this week.

[Bessonov]

@hectorj2f great stuff, keep going! It can resolve a bunch of use cases: #3644

[hectorj2f]

@MaciekPytel @Bessonov @hardikdr @elmiko I finally managed to get some time to invest on this KEP to address the last comments. Let me know if you want me to specify the error codes as well.

[hectorj2f]

It seems i hit the docker request limit in my travis build, toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading.

[ricoleabricot]

Hi @hectorj2f 👋
I really like the idea, I just implemented a cloudprovider for OVHcloud so I know the pain behind copy-pasting the go sdk for building the provider.
Also, I currently work on gRPC eco-system so, if you need any help, we should definitively check this out together !

[hectorj2f]

@MaciekPytel I addressed the last minor comments.

[MaciekPytel]

/lgtm
/approve

Left some minor comments, but I don't think those are blocking for merging this.

[MaciekPytel]

nit: To clarify - results of other calls (such as NodeGroups(), NodeGroup.MinSize(), etc) must not change unless Refresh is called. We should document that explicitly.

[MaciekPytel]

I'm not sure if we should actually include min, max and debug. min/max can change (after Refresh() call), while id is the consistent group identifier. For something like NodeGroupForNode it seems more consistent to return permanent id and not some settings that can change later on.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko, hectorj2f, Kafei59, MaciekPytel

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:

• cluster-autoscaler/OWNERS [MaciekPytel]

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

[MaciekPytel]

One more comment - in the meantime #3789 extended the cloudprovider API with another optional call. It should trivially convert into grpc (struct with a field for each option in NodeGroupAutoscalingOptions) or alternatively it can be skipped in initial version.

[hectorj2f]

@MaciekPytel Good. I'll update the proposal to include that new api call, and address the minor comments.

[migueleliasweb]

Hi all, is there any work being done to implement this provider? If not, would a PR be welcomed?

[hectorj2f]

Hi @migueleliasweb,
Unfortunately there is not any on-going work. Our company switched gears to focus on the CAPI autoscaler as a solution.