refactor(*): move getKubeClient to utils/kubernetes

[qianlei90]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

move getKubeClient to utils/kubernetes and make this function public.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qianlei90
Once this PR has been reviewed and has the lgtm label, please assign feiskyer for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• cluster-autoscaler/OWNERS

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

[x13n]

/assign

Thanks for the change! Left some comments, let me know what you think.

[x13n]

Passing all AutoscalingOptions here seems like overkill for this: it only requires a few values. WDYT about moving all kube-client related fields to a dedicated struct and only passing that struct here? If it is necessary in AutoscalingOptions, it can still be there.

[qianlei90]

type AutoscalingOptions struct {
    	...

    	// Kubernetes master location.
	Kubernetes string
	// Path to kube configuration if available
	KubeConfigPath string
	// Content type of requests sent to APIServer.
	KubeAPIContentType string
	// Burst setting for kubernetes client
	KubeClientBurst int
	// QPS setting for kubernetes client
	KubeClientQPS float64

    	...
}

Yeah I think it's very reasonable. How about moving these fields to a dedicated struct KubeClientOptions in utils/kubernetes/client.go？

I want to create a kube client in cloudprovider(see #5820 (comment)) but buildCloudProvider only accept AutoscalingOptions:

autoscaler/cluster-autoscaler/cloudprovider/builder/builder_all.go

Line 90 in e7bf3ec

func buildCloudProvider(opts config.AutoscalingOptions, do cloudprovider.NodeGroupDiscoveryOptions, rl *cloudprovider.ResourceLimiter) cloudprovider.CloudProvider {

If we move kube-client releated fields and functions to utils/kubernetes/client.go, I'm wondering how to create kube client in cloudprovider.

[x13n]

I think the struct should still live next to AutoscalingOptions - this package has almost no dependencies and I'd like to keep it that way. Otherwise it would depend on the new file. (And transitively depend on anything else the new file imports.)

What I'm proposing is:

type AutoscalingOptions struct {
	...
	KubeClient KubeClientOptions
	...
}

type KubeClientOptions struct {
 	// Kubernetes master location.
	Kubernetes string
 	// Path to kube configuration if available
 	KubeConfigPath string
 	// Content type of requests sent to APIServer.
 	ContentType string
 	// Burst setting for kubernetes client
 	BurstLimit int
 	// QPS setting for kubernetes client
 	QPSLimit float64
}

And then in the new package:

package client

...

func New(opts config.KubeClientOptions) kube_client.Interface {
	...
}

With that, given AutoscalingOptions, you can still use the new utils to create kube client. Does that make sense?

[x13n]

Is KubeConfig abstraction needed at all in main? Can the entire client creation be moved to a dedicated package?

[qianlei90]

Is KubeConfig abstraction needed at all in main?

Yes. Releated fields and getKubeConfig、createKubeClient now are only used in main.go. I do this refactor because I want to reuse these functions in cloudprovider.

Can the entire client creation be moved to a dedicated package?

I think Yes.

[x13n]

Ack, in this case let's move both kube config and client creation to the util package, so main will be able to create client directly and kube config you'll be able to reuse in cloudprovider.

[vadasambar]

@qianlei90 any updates on this? I wanted to include changes in this PR in #5820. Since this is related to the kwok provider PR, I can help with the changes if you want. Other options I am thinking of:

1. Make code changes in feat: implement kwok cloud provider #5820 to match this PR
2. Pull this PR into feat: implement kwok cloud provider #5820 after it is approved and merged

Let me know what you think.

[vadasambar]

@qianlei90 wanted to remind you about #6180 (comment)

[k8s-ci-robot]

PR needs rebase.

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.

[vadasambar]

Seems like @qianlei90 might not be available to work on this.
@x13n do you mind if I create a separate PR based off this. We can close this one and the review can be continued in the new PR. @qianlei90 generously created this PR in response to the PR I am working on: #5820 (comment)
I would like to close the loop.

[x13n]

Yup, makes sense. If you are going to follow up separately, I'll just close this one. Please assign me to the follow-up PR once it exists.

/close

[k8s-ci-robot]

@x13n: Closed this PR.

In response to this:

Yup, makes sense. If you are going to follow up separately, I'll just close this one. Please assign me to the follow-up PR once it exists.

/close

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.