Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(*): move getKubeClient to utils/kubernetes #6180

Closed
wants to merge 1 commit into from
Closed

refactor(*): move getKubeClient to utils/kubernetes #6180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cluster-autoscaler/config/autoscaling_options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,12 @@ type AutoscalingOptions struct {
AWSUseStaticInstanceList bool
// GCEOptions contain autoscaling options specific to GCE cloud provider.
GCEOptions GCEOptions
// 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
Expand Down
35 changes: 5 additions & 30 deletions cluster-autoscaler/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"flag"
"fmt"
"net/http"
"net/url"
"os"
"os/signal"
"strconv"
Expand Down Expand Up @@ -63,7 +62,6 @@ import (
"k8s.io/client-go/informers"
kube_client "k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/tools/leaderelection"
"k8s.io/client-go/tools/leaderelection/resourcelock"
kube_flag "k8s.io/component-base/cli/flag"
Expand Down Expand Up @@ -353,7 +351,9 @@ func createAutoscalingOptions() config.AutoscalingOptions {
StatusTaints: *statusTaintsFlag,
BalancingExtraIgnoredLabels: *balancingIgnoreLabelsFlag,
BalancingLabels: *balancingLabelsFlag,
Kubernetes: *kubernetes,
KubeConfigPath: *kubeConfigFile,
KubeAPIContentType: *kubeAPIContentType,
KubeClientBurst: *kubeClientBurst,
KubeClientQPS: *kubeClientQPS,
NodeDeletionDelayTimeout: *nodeDeletionDelayTimeout,
Expand Down Expand Up @@ -393,31 +393,6 @@ func createAutoscalingOptions() config.AutoscalingOptions {
}
}

func getKubeConfig() *rest.Config {
if *kubeConfigFile != "" {
klog.V(1).Infof("Using kubeconfig file: %s", *kubeConfigFile)
// use the current context in kubeconfig
config, err := clientcmd.BuildConfigFromFlags("", *kubeConfigFile)
if err != nil {
klog.Fatalf("Failed to build config: %v", err)
}
return config
}
url, err := url.Parse(*kubernetes)
if err != nil {
klog.Fatalf("Failed to parse Kubernetes url: %v", err)
}

kubeConfig, err := config.GetKubeClientConfig(url)
if err != nil {
klog.Fatalf("Failed to build Kubernetes client configuration: %v", err)
}

kubeConfig.ContentType = *kubeAPIContentType

return kubeConfig
}

func createKubeClient(kubeConfig *rest.Config) kube_client.Interface {
return kube_client.NewForConfigOrDie(kubeConfig)
}
Expand All @@ -441,7 +416,7 @@ func buildAutoscaler(debuggingSnapshotter debuggingsnapshot.DebuggingSnapshotter
// Create basic config from flags.
autoscalingOptions := createAutoscalingOptions()

kubeClientConfig := getKubeConfig()
kubeClientConfig := kube_util.GetKubeConfig(autoscalingOptions)
kubeClientConfig.Burst = autoscalingOptions.KubeClientBurst
kubeClientConfig.QPS = float32(autoscalingOptions.KubeClientQPS)
kubeClient := createKubeClient(kubeClientConfig)
Expand All @@ -455,7 +430,7 @@ func buildAutoscaler(debuggingSnapshotter debuggingsnapshot.DebuggingSnapshotter
}
informerFactory := informers.NewSharedInformerFactoryWithOptions(kubeClient, 0, informers.WithTransform(trim))

eventsKubeClient := createKubeClient(getKubeConfig())
eventsKubeClient := createKubeClient(kube_util.GetKubeConfig(autoscalingOptions))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@qianlei90 qianlei90 Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


predicateChecker, err := predicatechecker.NewSchedulerBasedPredicateChecker(informerFactory, autoscalingOptions.SchedulerConfig)
if err != nil {
Expand Down Expand Up @@ -624,7 +599,7 @@ func main() {
klog.Fatalf("Unable to get hostname: %v", err)
}

kubeClient := createKubeClient(getKubeConfig())
kubeClient := createKubeClient(kube_util.GetKubeConfig(createAutoscalingOptions()))

// Validate that the client is ok.
_, err = kubeClient.CoreV1().Nodes().List(ctx.TODO(), metav1.ListOptions{})
Expand Down
56 changes: 56 additions & 0 deletions cluster-autoscaler/utils/kubernetes/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
Copyright 2023 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package kubernetes

import (
"net/url"

"k8s.io/autoscaler/cluster-autoscaler/config"

"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/klog/v2"
)

// GetKubeConfig returns the rest config from AutoscalingOptions.
func GetKubeConfig(opts config.AutoscalingOptions) *rest.Config {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@qianlei90 qianlei90 Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

var kubeConfig *rest.Config
var err error

if opts.KubeConfigPath != "" {
klog.V(1).Infof("Using kubeconfig file: %s", opts.KubeConfigPath)
// use the current context in kubeconfig
kubeConfig, err = clientcmd.BuildConfigFromFlags("", opts.KubeConfigPath)
if err != nil {
klog.Fatalf("Failed to build config: %v", err)
}
} else {
url, err := url.Parse(opts.Kubernetes)
if err != nil {
klog.Fatalf("Failed to parse Kubernetes url: %v", err)
}

kubeConfig, err = config.GetKubeClientConfig(url)
if err != nil {
klog.Fatalf("Failed to build Kubernetes client configuration: %v", err)
}
}

kubeConfig.ContentType = opts.KubeAPIContentType

return kubeConfig
}
Loading