Rename yurtctl init/join/reset to yurtadm init/join/reset #819

lonelyCZ · 2022-05-04T15:35:08Z

Signed-off-by: lonelyCZ [email protected]

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage

/kind feature

What this PR does / why we need it:

yurtctl init/join/reset are not suitable as name for creating OpenYurt cluster and join nodes.
so rename to yurtadm init/join/reset for conforming to kubeadm init/join/reset so reducing confusion for end users.

[root@master68 bin]# ./yurtadm -h
yurtadm administers the yurt cluster

Usage:
  yurtadm [command]

Available Commands:
  help        Help about any command
  init        Run this command in order to set up the OpenYurt control plane
  join        Run this on any machine you wish to join an existing cluster
  reset       Performs a best effort revert of changes made to this host by 'yurtadm join'

[root@master68 bin]# ./yurtctl -h
yurtctl controls the yurt cluster

Usage:
  yurtctl [command]

Available Commands:
  cluster-info   list cloud nodes and edge nodes in cluster
  convert        Converts the kubernetes cluster to a yurt cluster
  help           Help about any command
  markautonomous mark the nodes as autonomous
  revert         Reverts the yurt cluster back to a Kubernetes cluster
  test           Tools for developers to test the OpenYurt Cluster

Which issue(s) this PR fixes:

Fixes #737

Special notes for your reviewer:

Since it is uncertain whether yurtctl will be retained, in order to reduce the workload, I don't move the dependency for now.

/assign @rambohe-ch @Congrool

Does this PR introduce a user-facing change?

rename yurtctl init/join/reset to yurtadm init/join/reset

other Note

openyurt-bot · 2022-05-04T15:35:13Z

@lonelyCZ: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Signed-off-by: lonelyCZ [email protected]

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage

/kind feature

What this PR does / why we need it:

yurtctl init/join/reset are not suitable as name for creating OpenYurt cluster and join nodes.
so rename to yurtadm init/join/reset for conforming to kubeadm init/join/reset so reducing confusion for end users.
[root@master68 bin]# ./yurtadm -h
yurtadm administers the yurt cluster

Usage:
 yurtadm [command]

Available Commands:
 help        Help about any command
 init        Run this command in order to set up the OpenYurt control plane
 join        Run this on any machine you wish to join an existing cluster
 reset       Performs a best effort revert of changes made to this host by 'yurtadm join'
[root@master68 bin]# ./yurtctl -h
yurtctl controls the yurt cluster

Usage:
 yurtctl [command]

Available Commands:
 cluster-info   list cloud nodes and edge nodes in cluster
 convert        Converts the kubernetes cluster to a yurt cluster
 help           Help about any command
 markautonomous mark the nodes as autonomous
 revert         Reverts the yurt cluster back to a Kubernetes cluster
 test           Tools for developers to test the OpenYurt Cluster
Which issue(s) this PR fixes:

Fixes #737

Special notes for your reviewer:

Since it is uncertain whether yurtctl will be retained, in order to reduce the workload, I don't move the dependency for now.

/assign @rambohe-ch @Congrool

Does this PR introduce a user-facing change?
rename yurtctl init/join/reset to yurtadm init/join/reset
other Note

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.

rambohe-ch · 2022-05-05T02:40:24Z

pkg/yurtadm/cmd/cmd.go

+	"github.com/openyurtio/openyurt/pkg/projectinfo"
+	"github.com/openyurtio/openyurt/pkg/yurtctl/cmd/join"
+	"github.com/openyurtio/openyurt/pkg/yurtctl/cmd/reset"
+	"github.com/openyurtio/openyurt/pkg/yurtctl/cmd/yurtinit"


please do not import yurtctl/cmd/xxx, and rename to yurtadm/cmd/xxx

lonelyCZ · 2022-05-05T14:04:12Z

Since some of the functionality in yurtctl will be removed, it is possible to temporarily make yurtctl dependent on yurtadm to reduce the amount of renamed work and subsequent removal efforts.

Congrool · 2022-05-07T07:00:34Z

pkg/yurtctl/cmd/clusterinfo/clusterinfo.go

@@ -30,7 +30,7 @@ import (
 	"k8s.io/klog/v2"

 	"github.com/openyurtio/openyurt/pkg/projectinfo"
-	kubeutil "github.com/openyurtio/openyurt/pkg/yurtctl/util/kubernetes"
+	kubeutil "github.com/openyurtio/openyurt/pkg/yurtadm/util/kubernetes"


It looks strange that yurtctl import package of yurtadm.
Is it possible to move pkg/yurtadm/util/kubernetes to pkg/util/kubernetes

It can be, but pkg/util/kubernetes also depend on pkg/yurtadm/kubernetes.

What do you mean? If we move pkg/yurtadm/util/kubernetes entirely to pkg/util/kubernetes, there's no dependence between them.

There's a lot of subtle dependencies between them. For example, pkg/yurtadm/kubernetes/kubeadm/app/discovery/token depend on pkg/yurtadm/cmd/join/joindata. And pkg/yurtadm/kubernetes and pkg/yurtadm/util/kubernetes are different files. :(

Well, that's a problem. I find that yurtctl depends on the yurtadm/kubernetes mostly beacuse of convert/revert and test cmd. And test relies on yurtadm/kubernetes beacuse it reuses the code of convert. It's a solution that move test into yurtadm and remove convert/revert subcommand(which is under work in pr #748).

For dependence between pkg/yurtadm/kubernetes/kubeadm/app/discovery/token and pkg/yurtadm/cmd/join/joindata, can we move the joindata into yurtadm/kubernetes? It seems that yurtadm/kubernetes only depends on joindata pkg.

And what do you think @rambohe-ch ?

@Congrool @lonelyCZ The original idea of yurtadm/kubernetes or yurtctl/kubernetes is that codes comes from kubernetes project in order to avoid to import k8s.io/kubernetes. so move joindata into yurtadm/kubernetes maybe change the original idea.

how about deprecate yurtctl convert/revert at first? and now we have no response form writer of #748, so maybe we can post a new pull request to solve this case. @lonelyCZ would you like to take over this case?

Congrool · 2022-05-07T07:10:28Z

pkg/yurtctl/cmd/markautonomous/markautonomous.go

@@ -29,9 +29,9 @@ import (
 	"k8s.io/klog/v2"

 	"github.com/openyurtio/openyurt/pkg/projectinfo"
-	"github.com/openyurtio/openyurt/pkg/yurtctl/constants"
+	"github.com/openyurtio/openyurt/pkg/yurtadm/constants"


For constants, how about retaining the constants pkg both in yurtctl and yurtadm?
We can extract constants what yurtadm and yurtctl need respectively in pkg/yurtadm/constants and pkg/yurtctl/constants.
And each cmd only needs to import the constants pkg under its folder.

Many constants is common, may also need to put the part in pkg/util.

You can have a try, I don't find so many common constants. If there're only several common constants used by both yurtctl and yurtadm, the duplication is bearable.

We only use the constants.AnnotationAutonomy in yurtctl, please move it from pkg/yurtadm/constants to pkg/yurtctl/constants and import pkg/yurtctl/constants instead.

rambohe-ch · 2022-05-16T06:42:15Z

@Congrool PTAL

Congrool · 2022-05-16T12:48:17Z

pkg/yurtctl/cmd/markautonomous/markautonomous.go

@@ -29,9 +29,9 @@ import (
 	"k8s.io/klog/v2"

 	"github.com/openyurtio/openyurt/pkg/projectinfo"
-	"github.com/openyurtio/openyurt/pkg/yurtctl/constants"
+	"github.com/openyurtio/openyurt/pkg/yurtadm/constants"


We only use the constants.AnnotationAutonomy in yurtctl, please move it from pkg/yurtadm/constants to pkg/yurtctl/constants and import pkg/yurtctl/constants instead.

Congrool · 2022-05-16T12:52:53Z

pkg/yurtctl/cmd/yurttest/kindinit/init.go

-	kubeutil "github.com/openyurtio/openyurt/pkg/yurtctl/util/kubernetes"
-	strutil "github.com/openyurtio/openyurt/pkg/yurtctl/util/strings"
-	tmplutil "github.com/openyurtio/openyurt/pkg/yurtctl/util/templates"
+	"github.com/openyurtio/openyurt/pkg/yurtadm/constants"


We should import pkg/yurtctl/constants instead of pkg/yurtadm/constants. Any constants used in this file should be in the pkg/yurtctl/constants, even there's duplication. In this way, we can decouple yurtctl with yurtadm.

Signed-off-by: lonelyCZ <[email protected]>

lonelyCZ · 2022-05-17T11:07:10Z

/cc @Congrool
PTAL

Congrool · 2022-05-17T12:44:56Z

It's not an easy job to entirely decouple yurtadm with yurtctl, more work need to be put on it. In terms of this pr, it looks good.
/lgtm

Congrool · 2022-05-17T12:46:52Z

/lgtm

openyurt-bot · 2022-05-18T01:19:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lonelyCZ, rambohe-ch

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:

~~OWNERS~~ [rambohe-ch]

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

openyurt-bot assigned Congrool and rambohe-ch May 4, 2022

openyurt-bot added the kind/feature kind/feature label May 4, 2022

openyurt-bot requested review from charleszheng44 and yixingjia May 4, 2022 15:35

openyurt-bot added the size/L size/L: 100-499 label May 4, 2022

rambohe-ch reviewed May 5, 2022

View reviewed changes

lonelyCZ force-pushed the pr-rename-yurtadm branch 2 times, most recently from 9792232 to 8a3bfdd Compare May 5, 2022 14:01

Congrool reviewed May 7, 2022

View reviewed changes

lonelyCZ force-pushed the pr-rename-yurtadm branch from 8a3bfdd to a70f11d Compare May 16, 2022 05:51

openyurt-bot added size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels May 16, 2022

lonelyCZ force-pushed the pr-rename-yurtadm branch 2 times, most recently from 9f5883e to 8b19ad1 Compare May 16, 2022 06:10

Congrool reviewed May 16, 2022

View reviewed changes

lonelyCZ force-pushed the pr-rename-yurtadm branch 2 times, most recently from abbe2d2 to de13611 Compare May 16, 2022 14:10

Rename yurtctl init/join/reset to yurtadm init/join/reset

71f0a48

Signed-off-by: lonelyCZ <[email protected]>

lonelyCZ force-pushed the pr-rename-yurtadm branch from de13611 to 71f0a48 Compare May 17, 2022 10:51

openyurt-bot requested a review from Congrool May 17, 2022 11:07

openyurt-bot added the lgtm lgtm label May 17, 2022

rambohe-ch approved these changes May 18, 2022

View reviewed changes

openyurt-bot added the approved approved label May 18, 2022

openyurt-bot merged commit 5ac7aa8 into openyurtio:master May 18, 2022

lonelyCZ mentioned this pull request May 20, 2022

[REQUEST Membership]: New membership for lonelyCZ openyurtio/community#39

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename yurtctl init/join/reset to yurtadm init/join/reset #819

Rename yurtctl init/join/reset to yurtadm init/join/reset #819

lonelyCZ commented May 4, 2022

openyurt-bot commented May 4, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

rambohe-ch May 5, 2022

lonelyCZ commented May 5, 2022

Congrool May 7, 2022

lonelyCZ May 7, 2022

Congrool May 7, 2022

lonelyCZ May 7, 2022

Congrool May 7, 2022

rambohe-ch May 10, 2022

lonelyCZ May 10, 2022

Congrool May 7, 2022

lonelyCZ May 7, 2022

Congrool May 7, 2022

Congrool May 16, 2022

rambohe-ch commented May 16, 2022

Congrool May 16, 2022

Congrool May 16, 2022

lonelyCZ commented May 17, 2022

Congrool commented May 17, 2022

Congrool commented May 17, 2022

openyurt-bot commented May 18, 2022

Rename yurtctl init/join/reset to yurtadm init/join/reset #819

Rename yurtctl init/join/reset to yurtadm init/join/reset #819

Conversation

lonelyCZ commented May 4, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented May 4, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

Choose a reason for hiding this comment

lonelyCZ commented May 5, 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

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

rambohe-ch commented May 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lonelyCZ commented May 17, 2022

Congrool commented May 17, 2022

Congrool commented May 17, 2022

openyurt-bot commented May 18, 2022