Multi-cluster bootstrap in antctl #3474

hjiajing · 2022-03-18T02:01:31Z

Deploy multi-cluster ClusterSet in antctl command.
The users could just input antctl mc create or antctl mc add to create a new ClusterSet or add a member cluster to a existing ClusterSet rather than edit YAML files.

There are three verb in antctl mc command, add, create and delete. Use for creating ClusterSet, updating ClusterSet and deleting ClusterSet.
Signed-off-by: hjiajing hjiajing@vmware.com

hjiajing · 2022-03-18T02:01:53Z

@luolanzone could please help to review? Thanks

codecov-commenter · 2022-03-18T02:08:01Z

Codecov Report

Merging #3474 (c3c595e) into main (87918f6) will decrease coverage by 2.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3474      +/-   ##
==========================================
- Coverage   64.33%   62.12%   -2.22%     
==========================================
  Files         290      288       -2     
  Lines       41225    41252      +27     
==========================================
- Hits        26524    25626     -898     
- Misses      12578    13579    +1001     
+ Partials     2123     2047      -76

Flag	Coverage Δ
kind-e2e-tests	`48.24% <ø> (-3.42%)`	⬇️
unit-tests	`44.34% <ø> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/antctl/antctl.go	`50.00% <ø> (ø)`
...g/agent/apiserver/handlers/featuregates/handler.go	`4.54% <0.00%> (-77.28%)`	⬇️
pkg/controller/egress/store/egressgroup.go	`1.72% <0.00%> (-54.32%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`16.90% <0.00%> (-52.82%)`	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	`22.17% <0.00%> (-50.44%)`	⬇️
pkg/agent/flowexporter/exporter/certificate.go	`0.00% <0.00%> (-50.00%)`	⬇️
pkg/support/dump.go	`7.90% <0.00%> (-49.16%)`	⬇️
pkg/support/dump_others.go	`0.00% <0.00%> (-44.74%)`	⬇️
...g/agent/apiserver/handlers/addressgroup/handler.go	`5.00% <0.00%> (-35.00%)`	⬇️
...agent/apiserver/handlers/appliedtogroup/handler.go	`5.00% <0.00%> (-35.00%)`	⬇️
... and 45 more

pkg/antctl/raw/multicluster/add/member_cluster.go

pkg/antctl/raw/multicluster/commands.go

pkg/antctl/raw/multicluster/add/member_cluster.go

luolanzone · 2022-03-18T02:36:25Z

pkg/antctl/raw/multicluster/add/member_cluster.go

+	scheme := k8sruntime.NewScheme()
+	if err = mcsscheme.AddToScheme(scheme); err != nil {
+		return err
+	}
+	if err = antreamcscheme.AddToScheme(scheme); err != nil {
+		return err
+	}
+	if err = k8sscheme.AddToScheme(scheme); err != nil {
+		return err
+	}


I suppose this will be done in a shared place for all mc commands?

This section is implemented in PR #3287 , so I will do rebase later and update it. Thanks

pkg/antctl/raw/multicluster/delete/clusterset.go

luolanzone · 2022-03-18T03:19:22Z

pkg/antctl/raw/multicluster/delete/member_cluster.go

+
+func (o *memberClusterOptions) validateAndComplete() error {
+	if o.namespace == "" {
+		o.namespace = metav1.NamespaceDefault


luolanzone · 2022-03-18T03:20:03Z

pkg/antctl/raw/multicluster/delete/member_cluster.go

+		return err
+	}
+
+	fmt.Fprintf(cmd.OutOrStdout(), "member-cluster %s deleted", memberClusterID)


s/member-cluster/member cluster/
s/deleted/is deleted/

luolanzone · 2022-03-18T03:22:26Z

pkg/antctl/raw/multicluster/delete/member_cluster.go

+		}
+	}
+	if len(memberClusters) == len(clusterSet.Spec.Members) {
+		return fmt.Errorf("member-cluster %s not found in ClusterSet %s", memberClusterID, memberClusterOpts.clusterSet)


pkg/antctl/raw/multicluster/delete/member_cluster.go

pkg/antctl/raw/multicluster/add/member_cluster.go

pkg/antctl/raw/multicluster/commands.go

luolanzone · 2022-03-22T09:37:30Z

pkg/antctl/raw/multicluster/commands.go

+
+var DeleteCmd = &cobra.Command{
+	Use:   "delete",
+	Short: "Delete Resources in a ClusterSet",


s/Resources/resources/

pkg/antctl/raw/multicluster/create/clusterclaim.go

pkg/antctl/raw/multicluster/create/clusterset.go

luolanzone · 2022-03-22T09:52:56Z

pkg/antctl/raw/multicluster/create/clusterset.go

+	command.Flags().StringSliceVarP(&o.serviceAccount, "service-account", "", nil, "ServiceAccounts of the member clusters")
+	command.Flags().StringVarP(&o.secret, "secret", "", "", "Secret to access the leader cluster")


serviceAccount is required only when creating ClusterSet in leader and secret is required only when it's in member cluster. maybe add some hints about this.

luolanzone · 2022-03-22T09:56:26Z

pkg/antctl/raw/multicluster/create/clusterset.go

+	command.Flags().StringVarP(&o.leaderClusterServer, "leader-server", "", "", "Leader cluster server address of the ClusterSet")
+	command.Flags().StringVarP(&o.leaderClusterPort, "leader-port", "", "6443", "Leader cluster port. If not set, use 6443 as default")


I suggest to combine these two options as one like option in kubectl
"leader-server'' The address and port of the leader cluster's Kubernetes API server

luolanzone · 2022-03-22T10:00:23Z

pkg/antctl/raw/multicluster/delete/member_cluster.go

+
+var memberClusterExamples = strings.Trim(`
+Delete a member cluster in a ClusterSet
+$ antctl mc delete member-cluster <MEMBER_CLUSTER> -n <NAMESPACE> --cluster-set <CLUSTER_SET>


s/MEMBER_CLUSTER/MEMBER_CLUSTER_ID

luolanzone · 2022-03-22T10:02:45Z

pkg/antctl/raw/multicluster/delete/member_cluster.go

+		return err
+	}
+
+	fmt.Fprintf(cmd.OutOrStdout(), "Member Cluster %s is deleted", memberClusterID)


s/Cluster/cluster/

lgtm-com · 2022-04-05T20:02:35Z

This pull request introduces 1 alert when merging f8b36f8 into 7be763e - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2022-04-07T15:37:13Z

This pull request introduces 1 alert when merging 1cfcb80 into 03b3f2b - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

luolanzone · 2022-04-08T06:46:15Z

pkg/antctl/raw/multicluster/add/member_cluster.go

+		return err
+	}
+
+	fmt.Fprintf(cmd.OutOrStdout(), "member cluster %s is added to ClusterSet %s successfully", memberClusterID, memberClusterOpt.clusterSet)


s/member cluster/the member cluster/
s/ClusterSet/the ClusterSet/
make sure your error and output message format consistent.

pkg/antctl/raw/multicluster/create/access_token.go

pkg/antctl/raw/multicluster/deploy/deploy_helper.go

pkg/antctl/raw/multicluster/create/access_token.go

lgtm-com · 2022-04-10T11:15:57Z

This pull request introduces 1 alert when merging 5f69be5 into 7fea8d3 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

luolanzone · 2022-04-14T09:45:53Z

pkg/antctl/raw/multicluster/create/clusterset.go

+	if o.leaderCluster == "" {
+		return fmt.Errorf("the leader-cluster-id cannot be empty")
+	}
+	if o.secret == "" {


the serviceAccount field is only required when it's a ClustserSet in a leader cluster, and secret is only required when it's ClusterSet in a member cluster, Could you help to refine this kind of check? it's probably hard to check if it's member or leader, but maybe we can refine the validation here similar as below:

if o.secret == "" && o.memberClusters == nil { return fmt.Errorf("the service accounts list is required in leader cluster, the secret is required in member cluster") }

luolanzone · 2022-04-14T10:00:14Z

pkg/antctl/raw/multicluster/create/clusterset.go

+	command.Flags().StringVarP(&o.leaderCluster, "leader-cluster", "", "", "Leader cluster of the ClusterSet")
+	command.Flags().StringVarP(&o.leaderClusterServer, "leader-server", "", "", "Leader cluster server address of the ClusterSet")
+	command.Flags().StringVarP(&o.leaderClusterNamespace, "leader-namespace", "", "", "Leader cluster Namespace")
+	command.Flags().StringToStringVarP(&o.memberClusters, "member-clusters", "", nil, "clusterID-ServiceAccount of the member clusters, it is required only when creating ClusterSet in leader cluster")


maybe change clusterID-ServiceAccount of the member clusters to clusterID and ServiceAccount group of the member clusters(e.g. --member-clusters member1=sa1,member2=sa2)

pkg/antctl/raw/multicluster/create/clusterset.go

pkg/antctl/raw/multicluster/delete/member_cluster.go

luolanzone · 2022-04-26T09:53:09Z

pkg/antctl/raw/multicluster/delete/member_cluster.go

+		return err
+	}
+	if len(args) != 1 {
+		return fmt.Errorf("exactly one NAME is required, got %d", len(args))


Suggested change

return fmt.Errorf("exactly one NAME is required, got %d", len(args))

return fmt.Errorf("exactly one ClusterID is required, got %d", len(args))

luolanzone · 2022-04-26T09:54:23Z

pkg/antctl/raw/multicluster/delete/member_cluster.go

+		}
+	}
+	if len(memberClusters) == len(clusterSet.Spec.Members) {
+		return fmt.Errorf("member-cluster %s not found in ClusterSet %s", memberClusterID, memberClusterOpts.clusterSet)


Suggested change

return fmt.Errorf("member-cluster %s not found in ClusterSet %s", memberClusterID, memberClusterOpts.clusterSet)

return fmt.Errorf("member cluster %s not found in ClusterSet %s", memberClusterID, memberClusterOpts.clusterSet)

pkg/antctl/raw/multicluster/deploy/deploy_helper.go

luolanzone · 2022-04-26T10:04:37Z

pkg/antctl/raw/multicluster/deploy/member_cluster.go

+var memberClusterOpts *memberClusterOptions
+
+var memberClusterExamples = strings.Trim(`
+Create all the CRDs and resources of member cluster in namespace antrea-mcs-ns


Suggested change

Create all the CRDs and resources of member cluster in namespace antrea-mcs-ns

Create all the CRDs and resources of member cluster in Namespace antrea-mcs-ns

Reminder for Namespace

pkg/antctl/raw/multicluster/deploy/member_cluster.go

pkg/antctl/raw/multicluster/deploy/leader_cluster.go

luolanzone · 2022-04-26T12:59:01Z

pkg/antctl/raw/multicluster/deploy/leader_cluster.go

+var leaderClusterOpts *leaderClusterOptions
+
+var leaderClusterExamples = strings.Trim(`
+Create all the CRDs and resources of leader cluster in namespace antrea-mcs-ns


Suggested change

Create all the CRDs and resources of leader cluster in namespace antrea-mcs-ns

Create all the CRDs and resources of leader cluster in Namespace antrea-mcs-ns

pkg/antctl/raw/multicluster/deploy/deploy_helper.go

jianjuns

Could you add a link to #3315? And is it the design description in the issue updated? If not, could you do so, so reviewers can understand?

jianjuns · 2022-04-28T18:32:25Z

pkg/antctl/raw/multicluster/deploy/member_cluster.go

+var memberClusterOpts *memberClusterOptions
+
+var memberClusterExamples = strings.Trim(`
+Create all the CRDs and resources of member cluster in namespace antrea-mcs-ns


Reminder for Namespace

jianjuns · 2022-04-28T18:32:56Z

pkg/antctl/raw/multicluster/deploy/leader_cluster.go

+		return fmt.Errorf("the namespace cannot be empty")
+	}
+	if o.antreaVersion == "" {
+		return fmt.Errorf("the antrea-version cannot be empty")


Why this is a must? Could we detect it from Antrea? We cannot default to antctl version? @luolanzone

Hi Jianjun. I think we could get the controller version by use antctl version code. I'm concerned about that only release version available and users may not use the antrea release verion.

Sorry, what is the issue if users are not using a release version? antctl can return dev version as well:

$ antctl version antctlVersion: v1.7.0-dev-ac49f476.dirty controllerVersion: v1.6.0-dev-f619dbd9.dirty

jianjuns · 2022-04-28T18:33:50Z

pkg/antctl/raw/multicluster/deploy/leader_cluster.go

+var leaderClusterOpts *leaderClusterOptions
+
+var leaderClusterExamples = strings.Trim(`
+Create all the CRDs and resources of leader cluster in Namespace antrea-mcs-ns


Sorry, what exact this command does? What CRDs and resources are created? Why it is called "deploy" if it is not about deployment?

Hi Jianjun. This command will create some CRDs and resources such as ServiceAccount and Role/RoleBinding of the multi-cluster controller, the deploy the Deployment "antrea-mc-controller". And I add more comments to the command.

jianjuns · 2022-04-28T18:34:25Z

pkg/antctl/raw/multicluster/delete/member_cluster.go

+
+func (o *memberClusterOptions) validateAndComplete() error {
+	if o.namespace == "" {
+		return fmt.Errorf("the namespace cannot be empty")


pkg/antctl/raw/multicluster/delete/clusterset.go

pkg/antctl/raw/multicluster/create/clusterset.go

jianjuns · 2022-05-06T17:57:35Z

pkg/antctl/raw/multicluster/deploy/leader_cluster.go

+		return fmt.Errorf("the namespace cannot be empty")
+	}
+	if o.antreaVersion == "" {
+		return fmt.Errorf("the antrea-version cannot be empty")


Sorry, what is the issue if users are not using a release version? antctl can return dev version as well:

$ antctl version antctlVersion: v1.7.0-dev-ac49f476.dirty controllerVersion: v1.6.0-dev-f619dbd9.dirty

pkg/antctl/raw/multicluster/create/access_token.go

pkg/antctl/raw/multicluster/deploy/leader_cluster.go

pkg/antctl/raw/multicluster/create/clusterclaim.go

pkg/antctl/raw/multicluster/delete/clusterclaim.go

hjiajing · 2022-06-07T05:07:12Z

/test-multicluster-e2e

hjiajing · 2022-06-07T05:45:40Z

/test-multicluster-e2e

hjiajing · 2022-06-07T06:19:48Z

/test-multicluster-e2e

luolanzone

LGTM, One nit.

docs/multicluster/antctl.md

hjiajing · 2022-06-07T08:30:25Z

/test-multicluster-e2e

docs/multicluster/antctl.md

pkg/antctl/raw/multicluster/create/access_token.go

pkg/antctl/raw/multicluster/create/clusterset.go

hjiajing · 2022-06-07T14:17:10Z

/test-multicluster-e2e

hjiajing · 2022-06-08T00:40:35Z

/test-multicluster-e2e

jianjuns · 2022-06-08T00:42:26Z

Please fix the "Go / Verify docs and spelling" test failure. @hjiajing

hjiajing · 2022-06-08T00:54:36Z

@jianjuns Thanks for reminding. The Docs spelling failed because I used dollar as an example without output. Then I delete the dollar it passed now.

docs/multicluster/antctl.md

Add new subcommands to Create or Delete multi-cluster Resources. Signed-off-by: hjiajing <hjiajing@vmware.com>

jianjuns · 2022-06-08T16:30:42Z

/test-multicluster-e2e

luolanzone reviewed Mar 18, 2022

View reviewed changes

luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Mar 18, 2022

luolanzone reviewed Mar 21, 2022

View reviewed changes

pkg/antctl/raw/multicluster/add/member_cluster.go Outdated Show resolved Hide resolved

hjiajing force-pushed the bootstrap branch from abbee46 to 3f0c050 Compare March 22, 2022 05:56

luolanzone reviewed Mar 22, 2022

View reviewed changes

hjiajing force-pushed the bootstrap branch from 3f0c050 to f8b36f8 Compare April 5, 2022 19:30

hjiajing force-pushed the bootstrap branch from f8b36f8 to 1cfcb80 Compare April 7, 2022 15:02

luolanzone reviewed Apr 8, 2022

View reviewed changes

hjiajing force-pushed the bootstrap branch from 1cfcb80 to 5f69be5 Compare April 10, 2022 10:45

hjiajing force-pushed the bootstrap branch 2 times, most recently from 53d0c1b to cde6995 Compare April 11, 2022 02:10

luolanzone reviewed Apr 14, 2022

View reviewed changes

luolanzone reviewed Apr 26, 2022

View reviewed changes

hjiajing force-pushed the bootstrap branch from cde6995 to 83ae1bf Compare April 26, 2022 16:50

luolanzone requested a review from jianjuns April 28, 2022 02:45

luolanzone added this to the Antrea v1.7 release milestone Apr 28, 2022

jianjuns reviewed Apr 28, 2022

View reviewed changes

hjiajing force-pushed the bootstrap branch 2 times, most recently from 2a432f5 to 3a081ab Compare May 6, 2022 07:47

hjiajing mentioned this pull request May 6, 2022

Multicluster bootstrap in antctl #3315

Closed

jianjuns reviewed May 6, 2022

View reviewed changes

hjiajing force-pushed the bootstrap branch 4 times, most recently from 066af81 to b7be588 Compare May 16, 2022 11:04

hjiajing force-pushed the bootstrap branch from ec1d120 to 7d7a9f5 Compare June 7, 2022 03:03

jianjuns reviewed Jun 7, 2022

View reviewed changes

pkg/antctl/raw/multicluster/create/clusterclaim.go Show resolved Hide resolved

pkg/antctl/raw/multicluster/delete/clusterclaim.go Show resolved Hide resolved

hjiajing force-pushed the bootstrap branch from 7d7a9f5 to 647dbf7 Compare June 7, 2022 05:03

jianjuns previously approved these changes Jun 7, 2022

View reviewed changes

luolanzone previously approved these changes Jun 7, 2022

View reviewed changes

docs/multicluster/antctl.md Outdated Show resolved Hide resolved

hjiajing dismissed stale reviews from luolanzone and jianjuns via 9967584 June 7, 2022 08:29

hjiajing force-pushed the bootstrap branch from 647dbf7 to 9967584 Compare June 7, 2022 08:29

tnqn reviewed Jun 7, 2022

View reviewed changes

docs/multicluster/antctl.md Outdated Show resolved Hide resolved

pkg/antctl/raw/multicluster/create/access_token.go Outdated Show resolved Hide resolved

pkg/antctl/raw/multicluster/create/clusterset.go Outdated Show resolved Hide resolved

hjiajing force-pushed the bootstrap branch from 9967584 to eff59f9 Compare June 7, 2022 14:12

hjiajing force-pushed the bootstrap branch from eff59f9 to 74feff8 Compare June 8, 2022 00:40

hjiajing force-pushed the bootstrap branch from 74feff8 to 6cc21e4 Compare June 8, 2022 00:50

jianjuns reviewed Jun 8, 2022

View reviewed changes

docs/multicluster/antctl.md Outdated Show resolved Hide resolved

hjiajing force-pushed the bootstrap branch from 6cc21e4 to 2c8ee41 Compare June 8, 2022 01:47

jianjuns reviewed Jun 8, 2022

View reviewed changes

docs/multicluster/antctl.md Outdated Show resolved Hide resolved

multi-cluster bootstrap in antctl

c3c595e

Add new subcommands to Create or Delete multi-cluster Resources. Signed-off-by: hjiajing <hjiajing@vmware.com>

hjiajing force-pushed the bootstrap branch from 2c8ee41 to c3c595e Compare June 8, 2022 03:32

jianjuns approved these changes Jun 8, 2022

View reviewed changes

jianjuns merged commit 3e1b254 into antrea-io:main Jun 8, 2022

jianjuns added the action/release-note Indicates a PR that should be included in release notes. label Jun 8, 2022

luolanzone mentioned this pull request Oct 12, 2022

Multicluster support in antctl #3107

Closed

		command.Flags().StringSliceVarP(&o.serviceAccount, "service-account", "", nil, "ServiceAccounts of the member clusters")
		command.Flags().StringVarP(&o.secret, "secret", "", "", "Secret to access the leader cluster")

		command.Flags().StringVarP(&o.leaderClusterServer, "leader-server", "", "", "Leader cluster server address of the ClusterSet")
		command.Flags().StringVarP(&o.leaderClusterPort, "leader-port", "", "6443", "Leader cluster port. If not set, use 6443 as default")

	return fmt.Errorf("exactly one NAME is required, got %d", len(args))
	return fmt.Errorf("exactly one ClusterID is required, got %d", len(args))

	return fmt.Errorf("member-cluster %s not found in ClusterSet %s", memberClusterID, memberClusterOpts.clusterSet)
	return fmt.Errorf("member cluster %s not found in ClusterSet %s", memberClusterID, memberClusterOpts.clusterSet)

	Create all the CRDs and resources of member cluster in namespace antrea-mcs-ns
	Create all the CRDs and resources of member cluster in Namespace antrea-mcs-ns

	Create all the CRDs and resources of leader cluster in namespace antrea-mcs-ns
	Create all the CRDs and resources of leader cluster in Namespace antrea-mcs-ns

Multi-cluster bootstrap in antctl #3474

Multi-cluster bootstrap in antctl #3474

Conversation

hjiajing commented Mar 18, 2022

hjiajing commented Mar 18, 2022

codecov-commenter commented Mar 18, 2022 • edited Loading

Codecov Report

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

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

lgtm-com bot commented Apr 5, 2022

lgtm-com bot commented Apr 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 10, 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

Choose a reason for hiding this comment

jianjuns left a 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

hjiajing commented Jun 7, 2022

hjiajing commented Jun 7, 2022

hjiajing commented Jun 7, 2022

luolanzone left a comment

Choose a reason for hiding this comment

hjiajing commented Jun 7, 2022

hjiajing commented Jun 7, 2022

hjiajing commented Jun 8, 2022

jianjuns commented Jun 8, 2022

hjiajing commented Jun 8, 2022 • edited Loading

jianjuns commented Jun 8, 2022

codecov-commenter commented Mar 18, 2022 •

edited

Loading

hjiajing commented Jun 8, 2022 •

edited

Loading