karmada icon indicating copy to clipboard operation
karmada copied to clipboard

introduce factory interface for karmadactl

Open carlory opened this issue 2 years ago • 14 comments

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:

  • https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/get_flags.go
  • https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/config.go
  • https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/options/global.go

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 avatar Jul 16 '22 07:07 carlory

/cc @lonelyCZ @RainbowMango @prodanlabs

carlory avatar Jul 16 '22 07:07 carlory

Looks good, I will further look it.

lonelyCZ avatar Jul 16 '22 12:07 lonelyCZ

/assign

lonelyCZ avatar Jul 16 '22 12:07 lonelyCZ

Hi @lonelyCZ, any suggestions here?

carlory avatar Jul 22 '22 01:07 carlory

/cc @lonelyCZ

carlory avatar Jul 27 '22 06:07 carlory

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

lonelyCZ avatar Jul 27 '22 10:07 lonelyCZ

Hi @lonelyCZ, any suggestions here?

carlory avatar Aug 01 '22 00:08 carlory

/retitle introduce factory interface for karmadactl

carlory avatar Aug 04 '22 06:08 carlory

@lonelyCZ rename FactoryExpansion to Factory and move it into util

carlory avatar Aug 04 '22 06:08 carlory

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?

lonelyCZ avatar Aug 07 '22 12:08 lonelyCZ

@ 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 avatar Aug 09 '22 07:08 carlory

/cc @lonelyCZ @prodanlabs

carlory avatar Aug 11 '22 05:08 carlory

Looks good to me.

/cc @RainbowMango

lonelyCZ avatar Aug 11 '22 06:08 lonelyCZ

Hi @RainbowMango , any suggestions here?

carlory avatar Aug 15 '22 02:08 carlory

Please take a look @RainbowMango

carlory avatar Aug 16 '22 12:08 carlory

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

RainbowMango avatar Aug 16 '22 12:08 RainbowMango

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?

RainbowMango avatar Aug 16 '22 12:08 RainbowMango

@RainbowMango alpha has been removed.

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

carlory avatar Aug 16 '22 13:08 carlory

@RainbowMango this issue https://github.com/karmada-io/karmada/issues/2349 list things we need to do

carlory avatar Aug 19 '22 06:08 carlory

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

RainbowMango avatar Aug 19 '22 06:08 RainbowMango

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

lonelyCZ avatar Sep 01 '22 02:09 lonelyCZ

@lonelyCZ updated.

carlory avatar Sep 01 '22 14:09 carlory

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

/lgtm

PTAL /assign @RainbowMango

lonelyCZ avatar Sep 02 '22 02:09 lonelyCZ

[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:

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

karmada-bot avatar Sep 02 '22 07:09 karmada-bot