Antctl for multicluster #3287

bangqipropel · 2022-02-09T03:43:54Z

antctl multicluster command for clusterset & clustersets, resourceexport

For command lines for ClusterSets, we have

Gel all ClusterSets in default Namesapce
$ antctl mc get clusterset
Get all ClusterSets in all Namespaces
$ antctl mc get clusterset -A
Get all ClusterSets in the specified Namespace
$ antctl mc get clusterset -n <NAMESPACE>
Get all ClusterSets and print them in JSON format
$ antctl mc get clusterset -o json
Get the specified ClusterSet
$ antctl mc get clusterset <CLUSTERSETID>

For command lines for ResourceExport, we have

Gel all ResourceExports of ClusterSet in default Namesapce
$ antctl mc get resourceexport
Get all ResourceExports of ClusterSet in all Namespaces
$ antctl mc get resourceexport -A
Get all ResourceExports in the specified Namespace
$ antctl mc get resourceexport -n <NAMESPACE>
Get all ResourceExports and print them in JSON format
$ antctl mc get resourceexport -o json
Get the specified ResourceExport
$ antctl mc get resourceexport <RESOURCEEXPORT> -n <NAMESPACE>

luolanzone · 2022-02-09T03:55:08Z

@bangqipropel please checkout latest code from main and make sure you have correct sign-off info in your commits.

bangqipropel · 2022-02-09T03:56:08Z

@bangqipropel please checkout latest code from main.

@luolanzone Doing it now, VM stuck suddenly, will push the new one after this

codecov-commenter · 2022-02-09T05:06:51Z

Codecov Report

Merging #3287 (bae9741) into main (b04af1b) will decrease coverage by 10.53%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #3287       +/-   ##
===========================================
- Coverage   65.57%   55.03%   -10.54%     
===========================================
  Files         268      374      +106     
  Lines       26780    41323    +14543     
===========================================
+ Hits        17560    22743     +5183     
- Misses       7315    16189     +8874     
- Partials     1905     2391      +486

Flag	Coverage Δ
integration-tests	`35.86% <ø> (?)`
kind-e2e-tests	`55.65% <ø> (-0.15%)`	⬇️
unit-tests	`42.58% <ø> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/controller/networkpolicy/reject.go	`78.22% <0.00%> (-9.68%)`	⬇️
pkg/apiserver/certificate/certificate.go	`69.44% <0.00%> (-6.95%)`	⬇️
pkg/controller/grouping/controller.go	`64.65% <0.00%> (-2.59%)`	⬇️
pkg/agent/openflow/pipeline.go	`78.13% <0.00%> (-1.33%)`	⬇️
pkg/ovs/openflow/ofctrl_bridge.go	`55.28% <0.00%> (-1.21%)`	⬇️
pkg/agent/openflow/client.go	`62.39% <0.00%> (-0.82%)`	⬇️
pkg/agent/agent.go	`53.11% <0.00%> (-0.54%)`	⬇️
antrea/pkg/agent/proxy/types/types.go	`10.52% <0.00%> (ø)`
antrea/pkg/ovs/ovsconfig/ovs_client.go	`55.72% <0.00%> (ø)`
...rea/pkg/agent/cniserver/pod_configuration_linux.go	`26.31% <0.00%> (ø)`
... and 107 more

pkg/antctl/raw/resourceexport/command.go

luolanzone · 2022-02-09T05:24:22Z

pkg/antctl/raw/clusterset/command.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+	mcsscheme "sigs.k8s.io/mcs-api/pkg/client/clientset/versioned/scheme"
+
+	multiclusterv1alpha1 "antrea.io/antrea/multicluster/apis/multicluster/v1alpha1"


please reorganize the import section, all Antrea package should be placed in last section, you can refer to other files. the CI build also failed due to this. https://github.com/antrea-io/antrea/runs/5120537819?check_suite_focus=true, you can run make golangci-lint to double check your fix.

luolanzone · 2022-02-09T05:26:29Z

pkg/antctl/raw/clusterset/command.go

+			}
+		},
+	}
+	Command.Flags().StringVarP(&option.namespace, "namespace", "n", "", "source of the Traceflow: Namespace/Pod, Pod, or IP")


the comment of this option is incorrect.

luolanzone · 2022-02-09T05:30:44Z

pkg/antctl/raw/resourceexport/command.go

+	formatter := "%-50s%-50s%-50s\n"
+	output.Write([]byte(fmt.Sprintf(formatter, "NAME", "NAMESPACE", "ClusterID")))
+	for _, r := range resExports {
+		fmt.Fprintf(&output, formatter, r.Name, r.Namespace, r.Labels["sourceClusterID"])


better to place Namespace before Name like kubectl do. is there is way to guarantee the order of output? I mean it's better to put ClusterID in first column and list all ResourceExport together from one cluster.

luolanzone · 2022-02-09T05:34:33Z

pkg/antctl/raw/clusterset/command.go

+func output(clusterSets []multiclusterv1alpha1.ClusterSet) string {
+	var output strings.Builder
+	formatter := "%-15s%-15s%-18s%-10s%-30s%-50s%-10s\n"
+	output.Write([]byte(fmt.Sprintf(formatter, "ClusterID", "NAMESPACE", "Type", "Status", "LastTransitionTime", "Message", "Reason")))


please make sure all column name are upper case.

I tried locally, the format doesn't look good:

ClusterID NAMESPACE Type Status LastTransitionTime Message Reason test-cluster-westdocs Ready True 2022-02-07 18:05:49 +0800 CST Connected test-cluster-westdocs ImportsResources True 2022-02-07 18:05:49 +0800 CST Local cluster is the elected leader of member: test-cluster-westElectedLeader test-cluster-northdocs Ready False 2022-02-09 12:36:37 +0800 CST No MemberClusterAnnounce update after 2022-02-09 04:36:18.960999439 +0000 UTC m=+153058.719069367Disconnected test-cluster-northdocs ImportsResources False 2022-02-09 12:36:37 +0800 CST No MemberClusterAnnounce update after 2022-02-09 04:36:18.960999439 +0000 UTC m=+153058.719069367Disconnected test-cluster-eastdocs Ready False 2022-02-09 12:36:37 +0800 CST No MemberClusterAnnounce update after 2022-02-09 04:36:18.960981033 +0000 UTC m=+153058.719050945Disconnected test-cluster-eastdocs ImportsResources False 2022-02-09 12:36:37 +0800 CST No MemberClusterAnnounce update after 2022-02-09 04:36:18.960981033 +0000 UTC m=+153058.719050945Disconnected test-cluster-northkube-system Ready True 2022-01-21 15:02:02 +0800 CST test-cluster-northkube-system IsElectedLeader True 2022-01-21 15:02:04 +0800 CST This leader cluster is an elected leader for local cluster

@bangqipropel I think you can remove message column.

luolanzone · 2022-02-11T08:22:57Z

pkg/antctl/raw/clusterset/command.go

+	Command = &cobra.Command{
+		Use:     "clusterset",
+		Aliases: []string{"clustersets"},
+		Short:   "Print multicluster clustersets",


s/multicluster/Multi-cluster

luolanzone · 2022-02-11T08:23:26Z

pkg/antctl/raw/clusterset/command.go

+				err = runE(cmd, "")
+			}
+			if err != nil {
+				fmt.Println(fmt.Errorf("please input the correct numbers of argumrnts here"))


why you need fmt.Errorf here?

s/argumrnts/arguments/
s/please/Please/
Please fix all same issue in other file.

luolanzone · 2022-02-11T08:27:13Z

pkg/antctl/raw/clusterset/command.go

+			}
+		},
+	}
+	Command.Flags().StringVarP(&option.namespace, "namespace", "n", "", "namespace of the clusterset")


s/namespace of/Namespace of/

luolanzone · 2022-02-11T08:43:01Z

pkg/antctl/raw/clusterset/command.go

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


I think you can wrap this as a method considering it's duplicated in each file.

luolanzone · 2022-02-11T08:43:55Z

pkg/antctl/raw/clusterset/command.go

+		err = k8sClient.List(context.Background(), clusterSetList, &client.ListOptions{Namespace: option.namespace})
+	}
+	if err != nil {
+		return fmt.Errorf("failed to list Clustersets: %w", err)


s/Clustersets/ClusterSets/

luolanzone · 2022-02-11T09:27:20Z

pkg/antctl/raw/clusterset/command.go

+	output.Write([]byte(fmt.Sprintf(formatter, "CLUSTERID", "NAMESPACE", "TYPE", "STATUS", "LASTTRANSACTIONTIME", "MESSAGE", "REASON")))
+	for _, clusterSet := range clusterSets {
+		for _, r := range clusterSet.Status.ClusterStatuses {
+			for _, condition := range r.Conditions {


what's the basic information in ClusterSet spec?

Can you add a column named 'ClUSTERSETID', otherwise the output only shows the overall status and user is unable to tell which ClusterSet it belongs to:

CLUSTERID NAMESPACE TYPE STATUS REASON test-cluster-east docs Ready Unknown NeverConnected test-cluster-east docs ImportsResources False NeverConnected test-cluster-west docs Ready True Connected test-cluster-west docs ImportsResources True ElectedLeader test-cluster-north docs Ready Unknown NeverConnected test-cluster-north docs ImportsResources False NeverConnected

@aravindakidambi any suggestion here?

Could we do some thing like this:
(1) Command to show summary of the ClusterSet
(2) Command to show details of a ClusterSet, customer needs to provide a cluster-set-id to see the details
The above command seems more like (2) for me

For (1), we should just have these columns

CLUSTERSETID TOTAL_CLUSTERS NUM_READY_CLUSTERS OVERALL_CONDITION REASON test-clusterset-1 4 3 Ready=True test-cluster-2 2 0 Ready=Unknown NoReadyCluster

luolanzone · 2022-02-11T09:28:05Z

pkg/antctl/raw/resourceexport/command.go

+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"


please fix import issue.

luolanzone · 2022-02-11T09:28:20Z

pkg/antctl/raw/resourceexport/command.go

+				err = runE(cmd, "")
+			}
+			if err != nil {
+				fmt.Println(fmt.Errorf("please input the correct numbers of argumrnts here"))


luolanzone · 2022-02-11T09:32:16Z

pkg/antctl/raw/resourceexport/command.go

+	Command = &cobra.Command{
+		Use:     "resourceexport",
+		Aliases: []string{"resourceexports"},
+		Short:   "Print multicluster resourceexports",


luolanzone · 2022-02-11T09:37:54Z

pkg/antctl/raw/resourceexport/command.go

+	formatter := "%-50s%-50s%-50s\n"
+	output.Write([]byte(fmt.Sprintf(formatter, "NAME", "NAMESPACE", "CLUSTERID")))
+	for _, r := range resExports {
+		fmt.Fprintf(&output, formatter, r.Namespace, r.Name, r.Labels["sourceClusterID"])


as we discussed offline, it's better to move ClusterID as first column.

bangqipropel · 2022-02-15T06:57:18Z

#3287 (comment) @luolanzone May I know what this means? We want the information from clusterset.spec? I thought we have already can see if it is leader or member from condition.Type?

luolanzone · 2022-02-15T09:43:01Z

pkg/antctl/raw/clusterset/command.go

+	"github.com/spf13/cobra"
+	"gopkg.in/yaml.v2"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"


you still have the import issue, please merge these two sections as below:

"github.com/spf13/cobra" "gopkg.in/yaml.v2" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

luolanzone · 2022-02-15T09:43:37Z

pkg/antctl/raw/clusterset/command.go

+		},
+	}
+	Command.Flags().StringVarP(&option.namespace, "namespace", "n", "", "Namespace of the clusterset")
+	Command.Flags().StringVarP(&option.outputFormat, "output", "o", "", "Output format. Supported formats: yaml")


according to following code, json is also supported. this should be 'json|yaml'

luolanzone · 2022-02-15T09:44:19Z

pkg/antctl/raw/clusterset/command.go

+			}
+		},
+	}
+	Command.Flags().StringVarP(&option.namespace, "namespace", "n", "", "Namespace of the clusterset")


s/clusterset/ClusterSet/

luolanzone · 2022-02-15T09:44:54Z

pkg/antctl/raw/clusterset/command.go

+	Command = &cobra.Command{
+		Use:     "clusterset",
+		Aliases: []string{"clustersets"},
+		Short:   "Print Multi-cluster clustersets",


s/clustersets/ClusterSets/
all CRDs name should be started with upper case: eg: resourceexports -> ResourceExports, please update corresponding name in all other files.

luolanzone · 2022-02-15T09:48:24Z

pkg/antctl/raw/clusterset/command.go

+	if option.outputFormat == "" {
+		fmt.Println(output(clusterSets))
+	} else if option.outputFormat == "json" {
+		bytesJSON, _ := json.Marshal(clusterSets)
+		var prettyJSON bytes.Buffer
+		err = json.Indent(&prettyJSON, bytesJSON, "", " ")
+		if err != nil {
+			return err
+		}
+		fmt.Println(prettyJSON.String())
+	} else if option.outputFormat == "yaml" {
+		yamlOutput, _ := yaml.Marshal(clusterSets)
+		fmt.Println(string(yamlOutput))
+	}


better to use 'switch' to organize the code.

luolanzone · 2022-02-15T10:19:48Z

pkg/antctl/raw/clusterset/command.go

+	output.Write([]byte(fmt.Sprintf(formatter, "CLUSTERID", "NAMESPACE", "TYPE", "STATUS", "LASTTRANSACTIONTIME", "MESSAGE", "REASON")))
+	for _, clusterSet := range clusterSets {
+		for _, r := range clusterSet.Status.ClusterStatuses {
+			for _, condition := range r.Conditions {


Can you add a column named 'ClUSTERSETID', otherwise the output only shows the overall status and user is unable to tell which ClusterSet it belongs to:

CLUSTERID NAMESPACE TYPE STATUS REASON test-cluster-east docs Ready Unknown NeverConnected test-cluster-east docs ImportsResources False NeverConnected test-cluster-west docs Ready True Connected test-cluster-west docs ImportsResources True ElectedLeader test-cluster-north docs Ready Unknown NeverConnected test-cluster-north docs ImportsResources False NeverConnected

@aravindakidambi any suggestion here?

luolanzone · 2022-02-15T10:21:45Z

pkg/antctl/raw/resourceexport/command.go

+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"


luolanzone · 2022-02-15T10:22:57Z

pkg/antctl/raw/resourceexport/command.go

+func output(resExports []multiclusterv1alpha1.ResourceExport) string {
+	var output strings.Builder
+	formatter := "%-50s%-50s%-50s\n"
+	output.Write([]byte(fmt.Sprintf(formatter, "CLUSTERID", "NAME", "NAMESPACE")))


the sequence is wrong, it's inconsistent with output, it should be "NAMESPACE", "NAME".

I would suggest to support -o for all commands.

bangqipropel · 2022-02-15T22:27:23Z

#3287 (comment) I think for this table output itself is good, but do we still need the yaml/json output for the non-specify Cluster-id command? It seems too big difference between the table output and the json/yaml output for the non-specify Cluster-ID command. @luolanzone @aravindakidambi

luolanzone · 2022-02-16T02:15:51Z

pkg/antctl/antctl.go

+			supportAgent:      true,
+			supportController: true,


I think we only support to run it out-of-cluster, right? if so, both of them should be false.

hjiajing · 2022-02-16T08:11:42Z

pkg/antctl/raw/clusterset/command.go

+			} else {
+				_ = runE(cmd, "")
+			}
+		},


Maybe it is better to put clusterID argument in options struct and use flags to get it .

hjiajing · 2022-02-16T08:28:15Z

pkg/antctl/raw/serviceimport/command.go

+func output(resImports []k8smcsv1alpha1.ServiceImport) string {
+	var output strings.Builder
+	formatter := "%-50s%-50s%-50s\n"
+	output.Write([]byte(fmt.Sprintf(formatter, "NAMESPACE", "NAME", "ClusterID")))


CLUSTERID maybe better.

lgtm-com · 2022-02-23T01:17:05Z

This pull request introduces 1 alert when merging c933fcc into 3f30d84 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

luolanzone · 2022-02-24T09:13:53Z

@bangqipropel please check and fix build error: https://github.com/antrea-io/antrea/runs/5297062280?check_suite_focus=true

luolanzone · 2022-02-24T09:53:12Z

pkg/antctl/raw/clusterset/command.go

+		Args:    cobra.MaximumNArgs(1),
+		Run: func(cmd *cobra.Command, args []string) {
+			if len(args) != 0 {
+				_ = runE(cmd, args[0])


you need to handle returned error, otherwise, it will do nothing when there is an error returned by runE and user will be unable to know what's going on. please fix it in all new added commands

luolanzone · 2022-02-24T09:54:42Z

pkg/antctl/raw/serviceexport/command.go

+	var seviceExportMap map[string][]k8smcsv1alpha1.ServiceExport
+	seviceExportMap = make(map[string][]k8smcsv1alpha1.ServiceExport)


seviceExportMap := make(map[string][]k8smcsv1alpha1.ServiceExport)

s/seviceExportMap/serviceExportMap/
please fix all for this typo.

luolanzone · 2022-02-24T09:55:05Z

pkg/antctl/raw/serviceexport/command.go

+
+		clusterClaimList := &multiclusterv1alpha1.ClusterClaimList{}
+
+		err = k8sClient.List(context.Background(), clusterClaimList, &client.ListOptions{Namespace: metav1.NamespaceAll})


err is not handled.

luolanzone · 2022-02-24T10:11:00Z

pkg/antctl/raw/serviceexport/command.go

+
+		err = k8sClient.List(context.Background(), clusterClaimList, &client.ListOptions{Namespace: metav1.NamespaceAll})
+
+		clusterID := clusterClaimList.Items[0].Value


it's better to check the length of clusterClaimList first, there might be no ClusterClaim at all or two ClusterClaims, one is for ClusterSet, another is for local cluster, please check the guide doc https://github.com/antrea-io/antrea/blob/main/docs/multicluster/getting-started.md.
better to wrap the logic into a function to do following check:

when there is only no ClusterClaim, skip the cluster.

when there are two clusterclaims, make sure you get the value from the one with name id.k8s.io, otherwise, you are getting ClusterSet ID.

when there are more than two ClusterClaims, skip it.

luolanzone · 2022-02-24T10:11:59Z

pkg/antctl/raw/serviceexport/command.go

+		}
+
+		results := res.Items
+		if ClusterID != "" {


it's quite confusing to use two variables with similar names. you can change the parameter var as 'targetClusterID'.

luolanzone · 2022-02-24T10:13:34Z

pkg/antctl/raw/serviceexport/command.go

+		results := res.Items
+		if ClusterID != "" {
+			if clusterID == ClusterID {
+				resExports := results


What's your purpose to add a new var? I suppose you can use results directly.
btw, you are handling ServiceExport,not ResourceExport, you can name it as svcExports if needed.

luolanzone · 2022-02-24T10:15:31Z

pkg/antctl/raw/serviceimport/command.go

+		Args:    cobra.MaximumNArgs(1),
+		Run: func(cmd *cobra.Command, args []string) {
+			if len(args) != 0 {
+				_ = runE(cmd, args[0])


luolanzone · 2022-02-24T10:16:13Z

pkg/antctl/raw/serviceimport/command.go

+	res := &k8smcsv1alpha1.ServiceImportList{}
+
+	err = k8sClient.List(context.TODO(), res, &client.ListOptions{Namespace: metav1.NamespaceAll})
+


remove the blank line here.

bangqipropel · 2022-02-28T06:12:28Z

@luolanzone @hjiajing @aravindakidambi this can be reivewed, thanks!

tnqn · 2022-03-16T02:29:09Z

cmd/antctl/main.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/component-base/logs"

 	"antrea.io/antrea/pkg/antctl"
+	_ "antrea.io/antrea/pkg/antctl/raw/multicluster/get"


Instead of leveraging the obscure side effect of importing and adding the package of one subcommand here while other subcommands are initialized in another fashion, it would be better to make the multicluster.GetCmd initialized in its own package and the initialization should be done automatically when importing the command. Otherwise it would lead to an unmanageable maintenance effort to its consumer: "If you want to use A, you should also import B, C, etc.

I would suggest to move init function to pkg/antctl/raw/multicluster/commands.go:

var GetCmd = &cobra.Command{ Use: "get", Short: "Display one or many resources in a ClusterSet", } func init() { GetCmd.AddCommand(get.NewClusterSetCommand()) GetCmd.AddCommand(get.NewResourceExportCommand()) GetCmd.AddCommand(get.NewResourceImportCommand()) }

tnqn

Could you please summarize the added commands and their examples in the PR description?

tnqn · 2022-03-16T03:06:24Z

pkg/antctl/raw/helper.go

+
+func AddSchemeToClientCfg(kubeconfig *rest.Config) (client.Client, error) {
+	var err error
+	scheme := k8sruntime.NewScheme()


I think it's more common to declare a global schema and initialize it in init function, then all consumers can share the schema.

@tnqn editing on other comments, but for this one, I know the meaning, but init() cannot throw out an error, at first we wanted to throw out error on this, so do we think it is not that important to throw out an error?

Initializing a schema is a static routine which has no external dependency, it either throws error because of a programming error which should be identified during development or never throw error. In many code it's wrapped with utilruntime.Must() to ensure it never fails.

antrea/pkg/client/clientset/versioned/fake/register.go

Line 65 in 1d27432

utilruntime.Must(AddToScheme(scheme))

tnqn · 2022-03-16T03:22:41Z

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

+
+	if len(clusterSets) == 0 {
+		if optionsClusterSet.namespace != "" {
+			fmt.Printf("No resource found in Namespace %s\n", optionsClusterSet.namespace)


I think this information normally goes to stderr to make user able to distinguish it from valid output easily.

fmt.Fprintln(cmd.ErrOrStderr(), "No resources found")

I think you mean for the allNamespace? if yes, changed

I mean both cases, the point is outputing non-data message to stderr, not mixed to stdout.

tnqn · 2022-03-16T03:28:58Z

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

+	default:
+		clusterSetsNum := len(clusterSets)
+		for i, singleclusterset := range clusterSets {
+			err := output(singleclusterset, true, optionsResourceExport.outputFormat, clusterset.Transform)


Isn't the above two conditions duplicated with what output has done?

tnqn · 2022-03-16T03:30:06Z

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

+Get all ClusterSets and print them in JSON format
+$ antctl mc get clusterset -o json
+Get the specified ClusterSet
+$ antctl mc get clusterset <clusterSetID>


Use uppercase for all placeholders consistently

tnqn · 2022-03-16T03:32:53Z

pkg/antctl/raw/multicluster/get/resourceexport.go

+	cmdResourceExport.Flags().StringVarP(&o.namespace, "namespace", "n", "", "Namespace of ResourceExport")
+	cmdResourceExport.Flags().StringVarP(&o.outputFormat, "output", "o", "", "Output format. Supported formats: json|yaml")
+	cmdResourceExport.Flags().BoolVarP(&o.allNamespaces, "all-namespaces", "A", false, "If present, list ResourceExport across all namespaces")
+	cmdResourceExport.Flags().StringVarP(&o.clusterID, "clusterid", "", "", "List of the ResourceExport of specific clusterID")


Suggested change

cmdResourceExport.Flags().StringVarP(&o.clusterID, "clusterid", "", "", "List of the ResourceExport of specific clusterID")

cmdResourceExport.Flags().StringVarP(&o.clusterID, "cluster-id", "", "", "List of the ResourceExport of specific clusterID")

To follow same convention that multi words are connected with "-" to make it easier to remember the writing

tnqn · 2022-03-16T03:49:46Z

pkg/antctl/raw/multicluster/get/resourceexport.go

+
+	if len(resExports) == 0 {
+		if optionsResourceExport.namespace != "" {
+			fmt.Printf("No resource found in Namespace %s\n", optionsResourceExport.namespace)


tnqn · 2022-03-16T03:51:59Z

pkg/antctl/transform/clusterset/transform.go

+)
+
+type Response struct {
+	ClusterID    string `json:"clusterid" yaml:"clusterid"`


I think the convention is using lowerCamelCase for field names in json and yaml output

tnqn · 2022-03-16T03:54:43Z

pkg/antctl/transform/clusterset/transform.go

+	return result, nil
+}
+
+func objectTransform(o interface{}, r interface{}, c interface{}) (interface{}, error) {


Why it uses interface{} as the types and do another conversion?

tnqn · 2022-03-16T03:58:37Z

pkg/antctl/transform/clusterset/transform.go

+var _ common.TableOutput = new(Response)
+
+func (r Response) GetTableHeader() []string {
+	return []string{"CLUSTERID", "NAMESPACE", "CLUSTERSETID", "TYPE", "STATUS", "REASON"}


The convention is "CLUSTER-ID", "CLUSTER-SET-ID"

tnqn · 2022-03-16T03:59:14Z

pkg/antctl/transform/resourceexport/transform.go

+)
+
+type Response struct {
+	ClusterID string `json:"clusterid" yaml:"clusterid"`


bangqipropel · 2022-03-18T05:57:13Z

@tnqn can this be reviewed again? thanks!

tnqn

LGTM overall, some nits

tnqn · 2022-03-18T10:41:40Z

pkg/antctl/raw/helper.go

@@ -146,3 +148,12 @@ func CreateControllerClientCfg(k8sClientset kubernetes.Interface, antreaClientse
 	cfg.Host = fmt.Sprintf("https://%s", net.JoinHostPort(nodeIP, fmt.Sprint(controllerInfo.APIPort)))
 	return cfg, nil
 }
+
+func AddSchemeToClientCfg(kubeconfig *rest.Config) (client.Client, error) {


It seems this is function is not very necessary now as it's actually oneline function after reducing the redundance, and AddSchemeToClientCfg sounds it will only mutate the client config but it actually creates a client:

func AddSchemeToClientCfg(kubeconfig *rest.Config) (client.Client, error) { return client.New(kubeconfig, client.Options{Scheme: multiclusterscheme.Scheme}) }

Maybe we could just call client.New(kubeconfig, client.Options{Scheme: multiclusterscheme.Scheme}) in its caller?

is it done?

tnqn · 2022-03-18T12:10:20Z

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

+		return nil
+	}
+
+	switch optionsClusterSet.outputFormat {


Maybe it could reuse what output function has done and move some logic to clusterset.Transform so this file doesn't have format related code, e.g.

return output(clusterSets, singleResource, optionsResourceExport.outputFormat, resourceexport.Transform)

func Transform(r interface{}, single bool) (interface{}, error) { if single { return objectTransform(r) } return listTransform(r) } func listTransform(l interface{}) (interface{}, error) { clusterSets := l.([]multiclusterv1alpha1.ClusterSet) ... } func objectTransform(o interface{}) (interface{}, error) { clusterSet := o.(multiclusterv1alpha1.ClusterSet) ... } func clusterTransform(clusterSet multiclusterv1alpha1.ClusterSet, status multiclusterv1alpha1.ClusterStatus, condition multiclusterv1alpha1.ClusterCondition) (interface{}, error) { ... }

@tnqn Hi Quan, I tried some ways, but the blockers for me is here: we don't want to change the function TableOutputForGetCommands, but we want to output the table output result one by one, so the "tableHeader" will always output once, but Jianjun wanted to output them multiple times here: #3287 (comment)
So the only left way seems to be also ouput the yaml and json format one-by-one, but this seems a little weird for me, because if we do this, the json and yaml output for clusterset will be totally different to other kind of resources,

tnqn · 2022-03-18T12:14:19Z

pkg/antctl/raw/multicluster/get/resourceexport.go

+var optionsResourceExport *resourceExportOptions
+
+var resourceExportExamples = strings.Trim(`
+Gel all ResourceExports of ClusterSet in default Namesapce


tnqn · 2022-03-18T12:20:09Z

pkg/antctl/transform/resourceexport/transform.go

+}
+
+func (r Response) GetTableRow(maxColumnLength int) []string {
+	return []string{r.ClusterID, r.Namespace, r.Name}


Should it include kind? otherwise it would not straightforward to know what's this resource?

tnqn

LGTM except one nit, and PR needs rebasing

tnqn · 2022-03-21T17:04:08Z

pkg/antctl/raw/helper.go

@@ -146,3 +148,12 @@ func CreateControllerClientCfg(k8sClientset kubernetes.Interface, antreaClientse
 	cfg.Host = fmt.Sprintf("https://%s", net.JoinHostPort(nodeIP, fmt.Sprint(controllerInfo.APIPort)))
 	return cfg, nil
 }
+
+func AddSchemeToClientCfg(kubeconfig *rest.Config) (client.Client, error) {


is it done?

tnqn · 2022-03-21T17:06:18Z

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

+		return nil
+	}
+
+	switch optionsClusterSet.outputFormat {


tnqn · 2022-03-22T02:21:34Z

@bangqipropel could you remove the first commit that has been merged via #3341 from this one and make the commit message more specific like #3341 (comment).

antctl: add multicluster subcommands for clusterset and resourceexport

Example:

# antctl mc get clusterset -A
...

# antctl mc get resourceexport -A
...

tnqn · 2022-03-22T04:07:35Z

@bangqipropel

The updated commit message title is exactly same as 9879f4c, which will look confusing from git log (someone would think it's a duplicate commit). I suggested one title in Antctl for multicluster #3287 (comment), feel free to use it or change to one that makes more sense to you, but please be more specific as this is already the second commit that adds multicluster subcommand.
Please add the command that gets the example output as well.
Please add an example for resourceexport.

Add subcommands to get two more antrea multicluster resources, ClusterSet and ResourceExport. We could list or get mc resource by antctl mc get <Resource>. Example: ➜ antctl mc get resourceexport -A CLUSTER-ID NAMESPACE NAME KIND cluster-a leader-ns cluster-a-testns-stale-busybus-service Service cluster-a leader-ns cluster-a-testns-stale-nginx-service Service cluster-a1 leader-nsforholdingalotof cluster-b-testns-stale-nginx-service Service ➜ antctl mc get clusterset -A CLUSTER-ID NAMESPACE CLUSTER-SET-ID TYPE STATUS REASON cluster-a leader-ns test-clusterset IsElectedLeader True <NONE> cluster-a leader-ns test-clusterset Ready True <NONE> CLUSTER-ID NAMESPACE CLUSTER-SET-ID TYPE STATUS REASON cluster-a1 leader-nsforholdingalotof test-clusterset1 IsElectedLeader True <NONE> cluster-a1 leader-nsforholdingalotof test-clusterset1 Ready True <NONE> In the future, other resources like serviceimport or serviceexport could be added to the sub-command if necessary. Signed-off-by: zbangqi <zbangqi@vmware.com>

tnqn · 2022-03-22T04:44:22Z

/skip-all
/test-multicluster-e2e

tnqn · 2022-03-22T04:48:07Z

I will edit the commit message when merging it, please format the body (wrap the body at 72 characters) in the future. For more details, check this guide and links it refer to: https://github.com/antrea-io/antrea/blob/main/CONTRIBUTING.md#getting-reviewers

bangqipropel · 2022-03-22T04:55:00Z

@tnqn Thanks, I am checking this file now

bangqipropel force-pushed the antctl_for_multicluster branch from e769498 to 062627d Compare February 9, 2022 04:59

bangqipropel force-pushed the antctl_for_multicluster branch 2 times, most recently from 10378d7 to 6d79091 Compare February 9, 2022 05:40

luolanzone reviewed Feb 9, 2022

View reviewed changes

bangqipropel force-pushed the antctl_for_multicluster branch 4 times, most recently from 8f7f77b to 3035a9b Compare February 9, 2022 23:29

luolanzone reviewed Feb 11, 2022

View reviewed changes

bangqipropel force-pushed the antctl_for_multicluster branch from 3035a9b to bb6581d Compare February 15, 2022 00:07

luolanzone requested a review from hjiajing February 15, 2022 02:27

bangqipropel force-pushed the antctl_for_multicluster branch from bb6581d to 302a72d Compare February 15, 2022 06:58

luolanzone reviewed Feb 15, 2022

View reviewed changes

bangqipropel force-pushed the antctl_for_multicluster branch from 302a72d to c9b6fd7 Compare February 15, 2022 22:52

luolanzone reviewed Feb 16, 2022

View reviewed changes

hjiajing reviewed Feb 16, 2022

View reviewed changes

bangqipropel force-pushed the antctl_for_multicluster branch 2 times, most recently from 8d268ea to c933fcc Compare February 23, 2022 00:46

luolanzone reviewed Feb 24, 2022

View reviewed changes

bangqipropel force-pushed the antctl_for_multicluster branch 2 times, most recently from 278bd14 to ad0e219 Compare February 28, 2022 05:55

tnqn reviewed Mar 16, 2022

View reviewed changes

luolanzone added this to the Antrea v1.6 release milestone Mar 16, 2022

bangqipropel dismissed jianjuns’s stale review via f67ad13 March 16, 2022 22:14

bangqipropel force-pushed the antctl_for_multicluster branch 8 times, most recently from ed591c2 to ab786bc Compare March 18, 2022 05:55

tnqn reviewed Mar 18, 2022

View reviewed changes

bangqipropel force-pushed the antctl_for_multicluster branch from ab786bc to d65aa10 Compare March 21, 2022 08:10

tnqn previously approved these changes Mar 21, 2022

View reviewed changes

bangqipropel dismissed tnqn’s stale review via 8805d50 March 21, 2022 19:07

bangqipropel force-pushed the antctl_for_multicluster branch from d65aa10 to 8805d50 Compare March 21, 2022 19:07

bangqipropel force-pushed the antctl_for_multicluster branch from 8805d50 to 6511a6a Compare March 22, 2022 03:58

bangqipropel force-pushed the antctl_for_multicluster branch from 6511a6a to bae9741 Compare March 22, 2022 04:33

tnqn approved these changes Mar 22, 2022

View reviewed changes

hjiajing mentioned this pull request Mar 22, 2022

Multi-cluster bootstrap in antctl #3474

Merged

tnqn merged commit 7764255 into antrea-io:main Mar 22, 2022

luolanzone mentioned this pull request Oct 12, 2022

Multicluster support in antctl #3107

Closed

		var seviceExportMap map[string][]k8smcsv1alpha1.ServiceExport
		seviceExportMap = make(map[string][]k8smcsv1alpha1.ServiceExport)


		clusterClaimList := &multiclusterv1alpha1.ClusterClaimList{}

		err = k8sClient.List(context.Background(), clusterClaimList, &client.ListOptions{Namespace: metav1.NamespaceAll})


		err = k8sClient.List(context.Background(), clusterClaimList, &client.ListOptions{Namespace: metav1.NamespaceAll})

		clusterID := clusterClaimList.Items[0].Value

		res := &k8smcsv1alpha1.ServiceImportList{}

		err = k8sClient.List(context.TODO(), res, &client.ListOptions{Namespace: metav1.NamespaceAll})

	cmdResourceExport.Flags().StringVarP(&o.clusterID, "clusterid", "", "", "List of the ResourceExport of specific clusterID")
	cmdResourceExport.Flags().StringVarP(&o.clusterID, "cluster-id", "", "", "List of the ResourceExport of specific clusterID")

Antctl for multicluster #3287

Antctl for multicluster #3287

Conversation

bangqipropel commented Feb 9, 2022 • edited

luolanzone commented Feb 9, 2022 • edited

bangqipropel commented Feb 9, 2022

codecov-commenter commented Feb 9, 2022 • edited

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

aravindakidambi Feb 15, 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

bangqipropel commented Feb 15, 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

bangqipropel commented Feb 15, 2022 • edited

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 Feb 23, 2022

luolanzone commented Feb 24, 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

bangqipropel commented Feb 28, 2022

Choose a reason for hiding this comment

tnqn 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

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

bangqipropel commented Mar 18, 2022

tnqn left a comment

Choose a reason for hiding this comment

bangqipropel commented Feb 9, 2022 •

edited

luolanzone commented Feb 9, 2022 •

edited

codecov-commenter commented Feb 9, 2022 •

edited

aravindakidambi Feb 15, 2022 •

edited

bangqipropel commented Feb 15, 2022 •

edited