introduce factory interface for karmadactl #2202

carlory · 2022-07-16T07:31:19Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

At present, there are many places in karmadactl, which are building clients and factories repeatedly, such as init, get, log, exec, etc.

The purpose of this PR is to provide a unified place to get clients accessing the cluster and factories needed to access sub-clusters.

With this change, after a little refactoring, the following files can be removed in the future:

An example of the alpha command is provided to make it easier for reviewers to understand the usage. It will be removed after feedback is collected.

(⎈ |karmada:default)➜  karmada git:(karmadactl-factory) go run cmd/karmadactl/karmadactl.go alpha
karmada: Kubernetes v1.21.7
ik8s: Kubernetes v1.21.5
ocp: Kubernetes v1.11.0+d4cacc0

(⎈ |karmada:default)➜  karmada git:(karmadactl-factory) go run cmd/karmadactl/karmadactl.go alpha -v6
I0716 15:29:22.371611   75493 loader.go:372] Config loaded from file:  /Users/kiki/.kube/config
I0716 15:29:22.414523   75493 round_trippers.go:553] GET https://10.29.0.221:5443/version 200 OK in 41 milliseconds
karmada: Kubernetes v1.21.7
I0716 15:29:22.440589   75493 round_trippers.go:553] GET https://10.29.0.221:5443/apis/cluster.karmada.io/v1alpha1/clusters 200 OK in 25 milliseconds
ik8s: I0716 15:29:22.798588   75493 round_trippers.go:553] GET https://10.29.0.221:5443/apis/cluster.karmada.io/v1alpha1/clusters/ik8s 200 OK in 24 milliseconds
I0716 15:29:22.828802   75493 loader.go:372] Config loaded from file:  /Users/kiki/.kube/config
I0716 15:29:22.887672   75493 round_trippers.go:553] GET https://10.29.0.221:5443/apis/cluster.karmada.io/v1alpha1/clusters/ik8s/proxy/version 200 OK in 57 milliseconds
Kubernetes v1.21.5
ocp: I0716 15:29:22.909929   75493 round_trippers.go:553] GET https://10.29.0.221:5443/apis/cluster.karmada.io/v1alpha1/clusters/ocp 200 OK in 21 milliseconds
I0716 15:29:22.935370   75493 loader.go:372] Config loaded from file:  /Users/kiki/.kube/config
I0716 15:29:23.024844   75493 round_trippers.go:553] GET https://10.29.0.221:5443/apis/cluster.karmada.io/v1alpha1/clusters/ocp/proxy/version 200 OK in 88 milliseconds
Kubernetes v1.11.0+d4cacc0

Does this PR introduce a user-facing change?:

NONE

carlory · 2022-07-16T07:33:10Z

/cc @lonelyCZ @RainbowMango @prodanlabs

lonelyCZ · 2022-07-16T12:08:11Z

Looks good, I will further look it.

lonelyCZ · 2022-07-16T12:55:29Z

/assign

carlory · 2022-07-22T01:33:13Z

Hi @lonelyCZ, any suggestions here?

carlory · 2022-07-27T06:08:52Z

/cc @lonelyCZ

lonelyCZ · 2022-07-27T10:08:04Z

Sorry for delay, I will back ASAP after previous prs.

carlory · 2022-08-01T00:08:31Z

Hi @lonelyCZ, any suggestions here?

lonelyCZ

Great thanks @carlory , I think we can push it forward now.

pkg/karmadactl/factory.go

carlory · 2022-08-04T06:47:33Z

/retitle introduce factory interface for karmadactl

carlory · 2022-08-04T06:49:11Z

@lonelyCZ rename FactoryExpansion to Factory and move it into util

lonelyCZ · 2022-08-07T12:25:18Z

rename FactoryExpansion to Factory and move it into util

I like it.

Could you please give a usage example for one subcommand that will be refactored by this interface?

carlory · 2022-08-09T07:28:35Z

@ lonelyCZ

karmadactl logs uses factory to access member cluster is an example.

(⎈ |karmada:default)➜  karmada git:(karmadactl-factory) go run cmd/karmadactl/karmadactl.go logs -C ocp master-etcd-nodedsp01.ocp.ats.io -n kube-system --tail 10
2022-08-09 15:21:51.800610 W | etcdserver: apply entries took too long [103.846094ms for 1 entries]
2022-08-09 15:21:51.800698 W | etcdserver: avoid queries with large range/delete range!
2022-08-09 15:24:04.745515 I | mvcc: store.index: compact 182209811
2022-08-09 15:24:04.877300 I | mvcc: finished scheduled compaction at 182209811 (took 125.525534ms)
2022-08-09 15:24:29.974311 W | etcdserver: apply entries took too long [120.414531ms for 1 entries]
2022-08-09 15:24:29.974353 W | etcdserver: avoid queries with large range/delete range!
2022-08-09 15:25:41.787238 W | wal: sync duration of 1.107194589s, expected less than 1s
2022-08-09 15:26:29.805921 W | wal: sync duration of 1.55465076s, expected less than 1s
2022-08-09 15:27:23.477009 W | etcdserver: apply entries took too long [118.29844ms for 1 entries]
2022-08-09 15:27:23.477058 W | etcdserver: avoid queries with large range/delete range!

(⎈ |karmada:default)➜  karmada git:(karmadactl-factory) go run cmd/karmadactl/karmadactl.go logs -C ocp router-1-hw9vw --tail 5
 - Health check ok : 0 retry attempt(s).
I0809 07:26:05.182271       1 router.go:481] Router reloaded:
 - Checking http://localhost:80 ...
 - Health check ok : 0 retry attempt(s).
W0809 07:27:12.762383       1 reflector.go:272] github.com/openshift/origin/pkg/router/controller/factory/factory.go:112: watch of *v1.Route ended with: The resourceVersion for the provided watch is too old.

carlory · 2022-08-11T05:37:06Z

/cc @lonelyCZ @prodanlabs

lonelyCZ · 2022-08-11T06:03:47Z

Looks good to me.

/cc @RainbowMango

carlory · 2022-08-15T02:55:41Z

Hi @RainbowMango , any suggestions here?

carlory · 2022-08-16T12:09:24Z

Please take a look @RainbowMango

RainbowMango · 2022-08-16T12:52:10Z

OK. I'll try to finish it by the end of this week.

RainbowMango · 2022-08-16T12:55:16Z

An example of the alpha command is provided to make it easier for reviewers to understand the usage. It will be removed after feedback is collected.

(⎈ |karmada:default)➜ karmada git:(karmadactl-factory) go run cmd/karmadactl/karmadactl.go alpha
karmada: Kubernetes v1.21.7
ik8s: Kubernetes v1.21.5
ocp: Kubernetes v1.11.0+d4cacc0

What's the alpha thing?

carlory · 2022-08-16T13:25:46Z

@RainbowMango alpha has been removed.

karmadactl logs uses factory to access member cluster is an example.

pkg/karmadactl/util/factory.go

RainbowMango · 2022-08-18T06:49:22Z

pkg/karmadactl/logs.go

-	karmadaRestConfig, err := karmadaConfig.GetRestConfig(o.KarmadaContext, o.KubeConfig)
-	if err != nil {
-		return fmt.Errorf("failed to get control plane rest config. context: %s, kube-config: %s, error: %v",
-			o.KarmadaContext, o.KubeConfig, err)
-	}
-	clusterInfo, err := getClusterInfo(karmadaRestConfig, o.Cluster, o.KubeConfig, o.KarmadaContext)
+	memberFactory, err := f.FactoryForMemberCluster(o.Cluster)
 	if err != nil {
 		return err
 	}
-	f := getFactory(o.Cluster, clusterInfo, o.Namespace)
-	return o.KubectlLogsOptions.Complete(f, cmd, args)


At present, there are many places in karmadactl, which are building clients and factories repeatedly, such as init, get, log, exec, etc.

Is it you mean building clients and factories repeatedly?

@RainbowMango

Currently, each subcommand has the logic to create client and cmdutil.Factory, for example:

https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/apply.go#L114

https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/cordon.go#L223

https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/deinit.go#L92

https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/describe.go#L105

https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/exec.go#L114

https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/get.go#L294

https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/join.go#L161

https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/join.go#L242

https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/promote.go#L207

I expect each sub command take factory as input parameter. factory provides functions to create kube client, karmada client and cmdutil.Factory for member cluster.

groups := templates.CommandGroups{ { Message: "Basic Commands:", Commands: []*cobra.Command{ NewCmdGet(factory, parentCommand, ioStreams), }, }, { Message: "Cluster Registeration Commands:", Commands: []*cobra.Command{ cmdinit.NewCmdInit(factory, parentCommand), NewCmdDeInit(factory, parentCommand), addons.NewCommandAddons(factory, parentCommand), NewCmdJoin(factory, parentCommand), NewCmdUnjoin(factory, parentCommand), }, }, { Message: "Cluster Management Commands:", Commands: []*cobra.Command{ NewCmdCordon(factory, parentCommand), NewCmdUncordon(factory, parentCommand), NewCmdTaint(factory, parentCommand), }, }, { Message: "Troubleshooting and Debugging Commands:", Commands: []*cobra.Command{ NewCmdLogs(factory, parentCommand, ioStreams), NewCmdExec(factory, parentCommand, ioStreams), NewCmdDescribe(factory, parentCommand, ioStreams), }, }, { Message: "Advanced Commands:", Commands: []*cobra.Command{ NewCmdApply(factory, parentCommand, ioStreams), NewCmdPromote(factory, parentCommand), }, }, }

There seems not all subcommands needing karmadaclientset, should we need to provide a common kubeclientset in my Factory?

Factory extends cmdutil.Factory, so kubeclient can be created by this factory.

https://github.com/karmada-io/karmada/pull/2202/files#diff-8aa0689c77923337c7ccd21b21fec3fd92f16fee1c417149d6fd89d4b2af45d0R33

Factory extends cmdutil.Factory, so kubeclient can be created by this factory.

How to do it for deinit subcommand.

In future, the adm and control commands possibly will be seprated, like kubectl and kubeadm

In future, the adm and control commands possibly will be seprated, like kubectl and kubeadm

I like it.

How to directly use KubernetesClientSet(kubeconfig, context string)?

But I am worried about that we init the unuseful KarmadaClientSet for init, deinit commends. Perhaps, we can don't care these cmds firstly, not need to pass f to them.

It makes sense for me.

@lonelyCZ Does this PR need to be changed?

It should be no problem, but there seem a lot of works at #2349 , I'm afraid I won't have enough time to review it, because I need to develop #2282 for coming new version these days.

Since there are several new commands to be added, could we postpone this cleanup after the new release? @carlory

@lonelyCZ ok

carlory · 2022-08-19T06:04:51Z

@RainbowMango this issue #2349 list things we need to do

RainbowMango · 2022-08-19T06:12:05Z

I'm good with it. But I'll leave approval to the owner @lonelyCZ .

Signed-off-by: carlory <baofa.fan@daocloud.io>

lonelyCZ · 2022-09-01T02:26:00Z

Hi @carlory, sorry for delaying too long, let's back.

lonelyCZ · 2022-09-01T03:39:30Z

pkg/karmadactl/logs.go

@@ -50,18 +50,20 @@ var (
 )

 // NewCmdLogs new logs command.
-func NewCmdLogs(karmadaConfig KarmadaConfig, parentCommand string, streams genericclioptions.IOStreams) *cobra.Command {
+// TODO(@carlory): take a factory as an argument when we resolve conflicts on flags.


Could we directly resolve this TODO in this PR? Move f := util.NewFactory(defaultConfigFlags) to pkg/karmadactl?

@lonelyCZ This requires changing all subcommands, I filled #2349 to track this task.

(⎈ |karmada:default)➜ karmada git:(karmadactl-factory) ✗ go run cmd/karmadactl/karmadactl.go panic: karmadactl flag redefined: kubeconfig goroutine 1 [running]: github.com/spf13/pflag.(*FlagSet).AddFlag(0x140000de600, 0x14000924b40) /Users/kiki/workspace/golang/src/github.com/karmada-io/karmada/vendor/github.com/spf13/pflag/flag.go:848 +0x548 github.com/spf13/pflag.(*FlagSet).VarPF(0xa?, {0x106849978, 0x140003df460}, {0x106049248, 0xa}, {0x0, 0x0}, {0x10609b245, 0x34}) /Users/kiki/workspace/golang/src/github.com/karmada-io/karmada/vendor/github.com/spf13/pflag/flag.go:831 +0x140 github.com/spf13/pflag.(*FlagSet).VarP(...) /Users/kiki/workspace/golang/src/github.com/karmada-io/karmada/vendor/github.com/spf13/pflag/flag.go:837 github.com/spf13/pflag.(*FlagSet).StringVar(0x3?, 0x1020100?, {0x106049248?, 0xffffffffffffffff?}, {0x0?, 0x10602b450?}, {0x10609b245?, 0x10602b448?}) /Users/kiki/workspace/golang/src/github.com/karmada-io/karmada/vendor/github.com/spf13/pflag/string.go:37 +0x88 k8s.io/cli-runtime/pkg/genericclioptions.(*ConfigFlags).AddFlags(0x140002c9200, 0x0?) /Users/kiki/workspace/golang/src/github.com/karmada-io/karmada/vendor/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go:327 +0x58 github.com/karmada-io/karmada/pkg/karmadactl.NewKarmadaCtlCommand({0x106049234, 0xa}, {0x106049234, 0xa}) /Users/kiki/workspace/golang/src/github.com/karmada-io/karmada/pkg/karmadactl/karmadactl.go:55 +0x23c main.main() /Users/kiki/workspace/golang/src/github.com/karmada-io/karmada/cmd/karmadactl/karmadactl.go:11 +0x34 exit status 2

I just tested it that seems no problems. How do you move it?

My test is shown below

func NewKarmadaCtlCommand(cmdUse, parentCommand string) *cobra.Command { .... f := util.NewFactory(defaultConfigFlags) .... Commands: []*cobra.Command{ NewCmdLogs(f, parentCommand, ioStreams), NewCmdExec(karmadaConfig, parentCommand, ioStreams), NewCmdDescribe(karmadaConfig, parentCommand, ioStreams), },

func NewCmdLogs(f util.Factory, parentCommand string, streams genericclioptions.IOStreams) *cobra.Command {

I just tested it that seems no problems. How do you move it?

I make the defaultConfigFlags as global command-line options in the karmadactl.go.

pkg/karmadactl/karmadactl.go

Signed-off-by: carlory <baofa.fan@daocloud.io> Co-authored-by: Hongcai Ren <renhongcai@huawei.com>

carlory · 2022-09-01T14:55:14Z

@lonelyCZ updated.

lonelyCZ · 2022-09-02T02:53:18Z

Thanks @carlory , I just tested it that worked fine.

/lgtm

PTAL
/assign @RainbowMango

RainbowMango

/approve

karmada-bot · 2022-09-02T07:20:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~pkg/karmadactl/OWNERS~~ [RainbowMango]

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

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 16, 2022

karmada-bot requested review from lonelyCZ and RainbowMango July 16, 2022 07:31

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 16, 2022

carlory force-pushed the karmadactl-factory branch from b7f099e to 25f749b Compare July 16, 2022 07:31

karmada-bot requested a review from prodanlabs July 16, 2022 07:33

carlory force-pushed the karmadactl-factory branch 2 times, most recently from a7dfd63 to eae85e6 Compare July 16, 2022 07:51

karmada-bot assigned lonelyCZ Jul 16, 2022

carlory force-pushed the karmadactl-factory branch from eae85e6 to b1839aa Compare July 26, 2022 17:03

lonelyCZ reviewed Aug 2, 2022

View reviewed changes

pkg/karmadactl/factory.go Outdated Show resolved Hide resolved

pkg/karmadactl/factory.go Outdated Show resolved Hide resolved

carlory force-pushed the karmadactl-factory branch 2 times, most recently from 7a8118c to 65836e5 Compare August 4, 2022 06:39

karmada-bot changed the title ~~introduce factoryexpansion interface for karmadactl~~ introduce factory interface for karmadactl Aug 4, 2022

carlory force-pushed the karmadactl-factory branch from 65836e5 to 2c3c7ea Compare August 9, 2022 07:24

carlory mentioned this pull request Aug 9, 2022

[Umbrella] karmadactl uses factory instead of karmadaConfig #2349

Open

8 tasks

karmada-bot requested a review from lonelyCZ August 11, 2022 05:37

RainbowMango reviewed Aug 18, 2022

View reviewed changes

carlory force-pushed the karmadactl-factory branch from 230ada6 to e2fb179 Compare August 19, 2022 05:44

introduce factoryexpansion interface for karmadactl

44e3c5e

Signed-off-by: carlory <baofa.fan@daocloud.io>

carlory force-pushed the karmadactl-factory branch from e2fb179 to 8798f33 Compare September 1, 2022 01:34

lonelyCZ reviewed Sep 1, 2022

View reviewed changes

pkg/karmadactl/karmadactl.go Show resolved Hide resolved

karmadactl logs uses factory to access member cluster

eb31ff4

Signed-off-by: carlory <baofa.fan@daocloud.io> Co-authored-by: Hongcai Ren <renhongcai@huawei.com>

carlory force-pushed the karmadactl-factory branch from 8798f33 to eb31ff4 Compare September 1, 2022 14:42

karmada-bot assigned RainbowMango Sep 2, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2022

RainbowMango approved these changes Sep 2, 2022

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2022

karmada-bot merged commit fd1c0b2 into karmada-io:master Sep 2, 2022

carlory deleted the karmadactl-factory branch September 2, 2022 08:20

carlory mentioned this pull request Sep 19, 2022

make the logic for getting the kubernetes config path a separate func… #2528

Closed

introduce factory interface for karmadactl #2202

introduce factory interface for karmadactl #2202

Conversation

carlory commented Jul 16, 2022

carlory commented Jul 16, 2022

lonelyCZ commented Jul 16, 2022

lonelyCZ commented Jul 16, 2022

carlory commented Jul 22, 2022

carlory commented Jul 27, 2022

lonelyCZ commented Jul 27, 2022

carlory commented Aug 1, 2022

lonelyCZ left a comment

Choose a reason for hiding this comment

carlory commented Aug 4, 2022

carlory commented Aug 4, 2022

lonelyCZ commented Aug 7, 2022

carlory commented Aug 9, 2022 • edited

carlory commented Aug 11, 2022

lonelyCZ commented Aug 11, 2022

carlory commented Aug 15, 2022

carlory commented Aug 16, 2022

RainbowMango commented Aug 16, 2022

RainbowMango commented Aug 16, 2022

carlory commented Aug 16, 2022

Choose a reason for hiding this comment

carlory Aug 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlory commented Aug 19, 2022

RainbowMango commented Aug 19, 2022

lonelyCZ commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlory commented Sep 1, 2022

lonelyCZ commented Sep 2, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Sep 2, 2022

carlory commented Aug 9, 2022 •

edited

carlory Aug 19, 2022 •

edited