From ceadf8ee3fb60b754dded203c98297ef9923d0f1 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Thu, 19 Oct 2023 14:16:42 -0400 Subject: [PATCH 1/8] Support cluster bootstrapping in vtctldclient Signed-off-by: Matt Lord --- examples/common/scripts/etcd-up.sh | 10 +-- go/cmd/vtctldclient/command/cells.go | 91 +++++++++++++++++++++++++--- go/cmd/vtctldclient/command/root.go | 7 +++ 3 files changed, 94 insertions(+), 14 deletions(-) diff --git a/examples/common/scripts/etcd-up.sh b/examples/common/scripts/etcd-up.sh index ac81c1fbd28..b0c0aba65a4 100755 --- a/examples/common/scripts/etcd-up.sh +++ b/examples/common/scripts/etcd-up.sh @@ -32,13 +32,13 @@ sleep 5 # And also add the CellInfo description for the cell. # If the node already exists, it's fine, means we used existing data. -echo "add $cell CellInfo" +echo "add ${cell} CellInfo" set +e -# shellcheck disable=SC2086 -vtctl $TOPOLOGY_FLAGS VtctldCommand AddCellInfo \ - --root /vitess/$cell \ +command vtctldclient --server bundled AddCellInfo \ + --bootstrap --topology-servers "${ETCD_SERVER}" \ + --root "/vitess/${cell}" \ --server-address "${ETCD_SERVER}" \ - $cell + "${cell}" set -e echo "etcd is running!" diff --git a/go/cmd/vtctldclient/command/cells.go b/go/cmd/vtctldclient/command/cells.go index 0a1e7ec727d..812f505a39a 100644 --- a/go/cmd/vtctldclient/command/cells.go +++ b/go/cmd/vtctldclient/command/cells.go @@ -17,12 +17,23 @@ limitations under the License. package command import ( + "context" "fmt" + "net" "strings" "github.com/spf13/cobra" "vitess.io/vitess/go/cmd/vtctldclient/cli" + "vitess.io/vitess/go/vt/topo" + "vitess.io/vitess/go/vt/vtctl/grpcvtctldserver" + "vitess.io/vitess/go/vt/vtctl/localvtctldclient" + "vitess.io/vitess/go/vt/vtctl/vtctldclient" + "vitess.io/vitess/go/vt/vttablet/tmclient" + + _ "vitess.io/vitess/go/vt/topo/consultopo" + _ "vitess.io/vitess/go/vt/topo/etcd2topo" + _ "vitess.io/vitess/go/vt/topo/zk2topo" topodatapb "vitess.io/vitess/go/vt/proto/topodata" vtctldatapb "vitess.io/vitess/go/vt/proto/vtctldata" @@ -33,13 +44,20 @@ var ( AddCellInfo = &cobra.Command{ Use: "AddCellInfo --root [--server-address ] ", Short: "Registers a local topology service in a new cell by creating the CellInfo.", - Long: `Registers a local topology service in a new cell by creating the CellInfo + Long: fmt.Sprintf(`Registers a local topology service in a new cell by creating the CellInfo with the provided parameters. The address will be used to connect to the topology service, and Vitess data will -be stored starting at the provided root.`, +be stored starting at the provided root. + +If the --boostrap flag is specified then you must specify a value of '%s' for --server +and also provide at least one topology server endpoint using --topology-servers so that +we do not attempt to connect to a remote vtctld server and vtctldclient can instead +connect directly to the topology server in order to create the cell that you can then +start vtctld in.`, useBundledVtctld), DisableFlagsInUseLine: true, Args: cobra.ExactArgs(1), + PreRunE: validateAddCellInfoFlags, RunE: commandAddCellInfo, } // AddCellsAlias makes an AddCellsAlias gRPC call to a vtctld. @@ -119,21 +137,72 @@ If a value is empty, it is ignored.`, } ) -var addCellInfoOptions topodatapb.CellInfo +var addCellInfoOptions = struct { + cellInfo topodatapb.CellInfo + bootstrap bool + topoImpl string + topoRoot string + topoServers []string +}{} + +func validateAddCellInfoFlags(cmd *cobra.Command, args []string) error { + if addCellInfoOptions.bootstrap { + // You must at least specify one topology server to bootstrap the cluster. + if len(addCellInfoOptions.topoServers) == 0 { + return fmt.Errorf("you must specify at least one topology server to bootstrap the cluster") + } + for _, topoServer := range addCellInfoOptions.topoServers { + if _, _, err := net.SplitHostPort(topoServer); err != nil { + return fmt.Errorf("invalid topology server address (%s): %v", topoServer, err) + } + } + } + + return nil +} func commandAddCellInfo(cmd *cobra.Command, args []string) error { cli.FinishedParsing(cmd) - cell := cmd.Flags().Arg(0) - _, err := client.AddCellInfo(commandCtx, &vtctldatapb.AddCellInfoRequest{ + var ( + cell = cmd.Flags().Arg(0) + ctx = commandCtx + cancel context.CancelFunc + ) + + if addCellInfoOptions.bootstrap { + ts, err := topo.OpenServer(addCellInfoOptions.topoImpl, strings.Join(addCellInfoOptions.topoServers, ","), addCellInfoOptions.topoRoot) + if err != nil { + return fmt.Errorf("failed to connect to the topology server: %v", err) + } + defer ts.Close() + + // Use internal vtcltd server implementation. + // Register a nil grpc handler -- we will not use tmclient at all. + tmclient.RegisterTabletManagerClientFactory("grpc", func() tmclient.TabletManagerClient { + return nil + }) + vtctld := grpcvtctldserver.NewVtctldServer(ts) + localvtctldclient.SetServer(vtctld) + VtctldClientProtocol = "local" + client, err = vtctldclient.New(VtctldClientProtocol, "") + if err != nil { + return fmt.Errorf("failed to setup internal vtctld server: %v", err) + } + + ctx, cancel = context.WithTimeout(commandCtx, topo.RemoteOperationTimeout) + defer cancel() + } + + _, err := client.AddCellInfo(ctx, &vtctldatapb.AddCellInfoRequest{ Name: cell, - CellInfo: &addCellInfoOptions, + CellInfo: &addCellInfoOptions.cellInfo, }) if err != nil { return err } - fmt.Printf("Created cell: %s\n", cell) + return nil } @@ -288,8 +357,12 @@ func commandUpdateCellsAlias(cmd *cobra.Command, args []string) error { } func init() { - AddCellInfo.Flags().StringVarP(&addCellInfoOptions.ServerAddress, "server-address", "a", "", "The address the topology server will connect to for this cell.") - AddCellInfo.Flags().StringVarP(&addCellInfoOptions.Root, "root", "r", "", "The root path the topology server will use for this cell.") + AddCellInfo.Flags().StringVarP(&addCellInfoOptions.cellInfo.ServerAddress, "server-address", "a", "", "The address the topology server will connect to for this cell.") + AddCellInfo.Flags().StringVarP(&addCellInfoOptions.cellInfo.Root, "root", "r", "", "The root path the topology server will use for this cell.") + AddCellInfo.Flags().BoolVar(&addCellInfoOptions.bootstrap, "bootstrap", false, fmt.Sprintf("Should we create the cell directly in the topology server(s) to bootstrap the cluster so that we can then start a vtctld process in this cell. Note: you will also need to specify a value of '%s' for --server when using this flag.", useBundledVtctld)) + AddCellInfo.Flags().StringVar(&addCellInfoOptions.topoImpl, "topology-implementation", "etcd2", "The topology server implementation used.") + AddCellInfo.Flags().StringSliceVar(&addCellInfoOptions.topoServers, "topology-servers", nil, "The endpoints (host and port) to use when connecting directly to the topology server(s) when boostrapping a cluster.") + AddCellInfo.Flags().StringVar(&addCellInfoOptions.topoRoot, "topology-global-root", "/vitess/global", "The topology server root path to use.") AddCellInfo.MarkFlagRequired("root") Root.AddCommand(AddCellInfo) diff --git a/go/cmd/vtctldclient/command/root.go b/go/cmd/vtctldclient/command/root.go index 1194b49ec8f..8f28fba0911 100644 --- a/go/cmd/vtctldclient/command/root.go +++ b/go/cmd/vtctldclient/command/root.go @@ -44,6 +44,10 @@ import ( _ "vitess.io/vitess/go/cmd/vtctldclient/command/vreplication/workflow" ) +// The --server value if you want to use a "local" +// vtctld server. +const useBundledVtctld = "bundled" + var ( // VtctldClientProtocol is the protocol to use when creating the vtctldclient.VtctldClient. VtctldClientProtocol = "grpc" @@ -130,6 +134,9 @@ const skipClientCreationKey = "skip_client_creation" // getClientForCommand returns a vtctldclient.VtctldClient for a given command. // It validates that --server was passed to the CLI for commands that need it. func getClientForCommand(cmd *cobra.Command) (vtctldclient.VtctldClient, error) { + if server == useBundledVtctld { + return nil, nil // The command will need to later setup a local vtctld server and client. + } if skipStr, ok := cmd.Annotations[skipClientCreationKey]; ok { skipClientCreation, err := strconv.ParseBool(skipStr) if err != nil { From 7b37ec8d1902066316ecca84a7d37cb647e96def Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 20 Oct 2023 13:09:03 -0400 Subject: [PATCH 2/8] Support all commands w/o a vtctld server Signed-off-by: Matt Lord --- examples/common/scripts/etcd-up.sh | 1 - go/cmd/vtctldclient/command/cells.go | 91 +++------------------------- go/cmd/vtctldclient/command/root.go | 55 ++++++++++++++++- go/flags/endtoend/vtctldclient.txt | 8 ++- 4 files changed, 68 insertions(+), 87 deletions(-) diff --git a/examples/common/scripts/etcd-up.sh b/examples/common/scripts/etcd-up.sh index b0c0aba65a4..6204c8600ec 100755 --- a/examples/common/scripts/etcd-up.sh +++ b/examples/common/scripts/etcd-up.sh @@ -35,7 +35,6 @@ sleep 5 echo "add ${cell} CellInfo" set +e command vtctldclient --server bundled AddCellInfo \ - --bootstrap --topology-servers "${ETCD_SERVER}" \ --root "/vitess/${cell}" \ --server-address "${ETCD_SERVER}" \ "${cell}" diff --git a/go/cmd/vtctldclient/command/cells.go b/go/cmd/vtctldclient/command/cells.go index 812f505a39a..0a1e7ec727d 100644 --- a/go/cmd/vtctldclient/command/cells.go +++ b/go/cmd/vtctldclient/command/cells.go @@ -17,23 +17,12 @@ limitations under the License. package command import ( - "context" "fmt" - "net" "strings" "github.com/spf13/cobra" "vitess.io/vitess/go/cmd/vtctldclient/cli" - "vitess.io/vitess/go/vt/topo" - "vitess.io/vitess/go/vt/vtctl/grpcvtctldserver" - "vitess.io/vitess/go/vt/vtctl/localvtctldclient" - "vitess.io/vitess/go/vt/vtctl/vtctldclient" - "vitess.io/vitess/go/vt/vttablet/tmclient" - - _ "vitess.io/vitess/go/vt/topo/consultopo" - _ "vitess.io/vitess/go/vt/topo/etcd2topo" - _ "vitess.io/vitess/go/vt/topo/zk2topo" topodatapb "vitess.io/vitess/go/vt/proto/topodata" vtctldatapb "vitess.io/vitess/go/vt/proto/vtctldata" @@ -44,20 +33,13 @@ var ( AddCellInfo = &cobra.Command{ Use: "AddCellInfo --root [--server-address ] ", Short: "Registers a local topology service in a new cell by creating the CellInfo.", - Long: fmt.Sprintf(`Registers a local topology service in a new cell by creating the CellInfo + Long: `Registers a local topology service in a new cell by creating the CellInfo with the provided parameters. The address will be used to connect to the topology service, and Vitess data will -be stored starting at the provided root. - -If the --boostrap flag is specified then you must specify a value of '%s' for --server -and also provide at least one topology server endpoint using --topology-servers so that -we do not attempt to connect to a remote vtctld server and vtctldclient can instead -connect directly to the topology server in order to create the cell that you can then -start vtctld in.`, useBundledVtctld), +be stored starting at the provided root.`, DisableFlagsInUseLine: true, Args: cobra.ExactArgs(1), - PreRunE: validateAddCellInfoFlags, RunE: commandAddCellInfo, } // AddCellsAlias makes an AddCellsAlias gRPC call to a vtctld. @@ -137,72 +119,21 @@ If a value is empty, it is ignored.`, } ) -var addCellInfoOptions = struct { - cellInfo topodatapb.CellInfo - bootstrap bool - topoImpl string - topoRoot string - topoServers []string -}{} - -func validateAddCellInfoFlags(cmd *cobra.Command, args []string) error { - if addCellInfoOptions.bootstrap { - // You must at least specify one topology server to bootstrap the cluster. - if len(addCellInfoOptions.topoServers) == 0 { - return fmt.Errorf("you must specify at least one topology server to bootstrap the cluster") - } - for _, topoServer := range addCellInfoOptions.topoServers { - if _, _, err := net.SplitHostPort(topoServer); err != nil { - return fmt.Errorf("invalid topology server address (%s): %v", topoServer, err) - } - } - } - - return nil -} +var addCellInfoOptions topodatapb.CellInfo func commandAddCellInfo(cmd *cobra.Command, args []string) error { cli.FinishedParsing(cmd) - var ( - cell = cmd.Flags().Arg(0) - ctx = commandCtx - cancel context.CancelFunc - ) - - if addCellInfoOptions.bootstrap { - ts, err := topo.OpenServer(addCellInfoOptions.topoImpl, strings.Join(addCellInfoOptions.topoServers, ","), addCellInfoOptions.topoRoot) - if err != nil { - return fmt.Errorf("failed to connect to the topology server: %v", err) - } - defer ts.Close() - - // Use internal vtcltd server implementation. - // Register a nil grpc handler -- we will not use tmclient at all. - tmclient.RegisterTabletManagerClientFactory("grpc", func() tmclient.TabletManagerClient { - return nil - }) - vtctld := grpcvtctldserver.NewVtctldServer(ts) - localvtctldclient.SetServer(vtctld) - VtctldClientProtocol = "local" - client, err = vtctldclient.New(VtctldClientProtocol, "") - if err != nil { - return fmt.Errorf("failed to setup internal vtctld server: %v", err) - } - - ctx, cancel = context.WithTimeout(commandCtx, topo.RemoteOperationTimeout) - defer cancel() - } - - _, err := client.AddCellInfo(ctx, &vtctldatapb.AddCellInfoRequest{ + cell := cmd.Flags().Arg(0) + _, err := client.AddCellInfo(commandCtx, &vtctldatapb.AddCellInfoRequest{ Name: cell, - CellInfo: &addCellInfoOptions.cellInfo, + CellInfo: &addCellInfoOptions, }) if err != nil { return err } - fmt.Printf("Created cell: %s\n", cell) + fmt.Printf("Created cell: %s\n", cell) return nil } @@ -357,12 +288,8 @@ func commandUpdateCellsAlias(cmd *cobra.Command, args []string) error { } func init() { - AddCellInfo.Flags().StringVarP(&addCellInfoOptions.cellInfo.ServerAddress, "server-address", "a", "", "The address the topology server will connect to for this cell.") - AddCellInfo.Flags().StringVarP(&addCellInfoOptions.cellInfo.Root, "root", "r", "", "The root path the topology server will use for this cell.") - AddCellInfo.Flags().BoolVar(&addCellInfoOptions.bootstrap, "bootstrap", false, fmt.Sprintf("Should we create the cell directly in the topology server(s) to bootstrap the cluster so that we can then start a vtctld process in this cell. Note: you will also need to specify a value of '%s' for --server when using this flag.", useBundledVtctld)) - AddCellInfo.Flags().StringVar(&addCellInfoOptions.topoImpl, "topology-implementation", "etcd2", "The topology server implementation used.") - AddCellInfo.Flags().StringSliceVar(&addCellInfoOptions.topoServers, "topology-servers", nil, "The endpoints (host and port) to use when connecting directly to the topology server(s) when boostrapping a cluster.") - AddCellInfo.Flags().StringVar(&addCellInfoOptions.topoRoot, "topology-global-root", "/vitess/global", "The topology server root path to use.") + AddCellInfo.Flags().StringVarP(&addCellInfoOptions.ServerAddress, "server-address", "a", "", "The address the topology server will connect to for this cell.") + AddCellInfo.Flags().StringVarP(&addCellInfoOptions.Root, "root", "r", "", "The root path the topology server will use for this cell.") AddCellInfo.MarkFlagRequired("root") Root.AddCommand(AddCellInfo) diff --git a/go/cmd/vtctldclient/command/root.go b/go/cmd/vtctldclient/command/root.go index 8f28fba0911..0868a23bf1e 100644 --- a/go/cmd/vtctldclient/command/root.go +++ b/go/cmd/vtctldclient/command/root.go @@ -22,6 +22,7 @@ import ( "fmt" "io" "strconv" + "strings" "time" "github.com/spf13/cobra" @@ -29,7 +30,11 @@ import ( "vitess.io/vitess/go/trace" "vitess.io/vitess/go/vt/logutil" "vitess.io/vitess/go/vt/servenv" + "vitess.io/vitess/go/vt/topo" + "vitess.io/vitess/go/vt/vtctl/grpcvtctldserver" + "vitess.io/vitess/go/vt/vtctl/localvtctldclient" "vitess.io/vitess/go/vt/vtctl/vtctldclient" + "vitess.io/vitess/go/vt/vttablet/tmclient" // These imports ensure init()s within them get called and they register their commands/subcommands. "vitess.io/vitess/go/cmd/vtctldclient/cli" @@ -42,6 +47,11 @@ import ( _ "vitess.io/vitess/go/cmd/vtctldclient/command/vreplication/reshard" _ "vitess.io/vitess/go/cmd/vtctldclient/command/vreplication/vdiff" _ "vitess.io/vitess/go/cmd/vtctldclient/command/vreplication/workflow" + + // These imports register the topo factories to use when --server=bundled. + _ "vitess.io/vitess/go/vt/topo/consultopo" + _ "vitess.io/vitess/go/vt/topo/etcd2topo" + _ "vitess.io/vitess/go/vt/topo/zk2topo" ) // The --server value if you want to use a "local" @@ -58,14 +68,31 @@ var ( commandCtx context.Context commandCancel func() + // Register functions to be called when the command completes. + onTerm = []func(){} + server string actionTimeout time.Duration compactOutput bool + topoOptions = struct { + implementation string + globalServerAddresses []string + globalRoot string + }{ // Set defaults + implementation: "etcd2", + globalServerAddresses: []string{"localhost:2379"}, + globalRoot: "/vitess/global", + } + // Root is the main entrypoint to the vtctldclient CLI. Root = &cobra.Command{ Use: "vtctldclient", Short: "Executes a cluster management command on the remote vtctld server.", + Long: fmt.Sprintf(`If there are no running vtctld servers -- for example when boostrapping +a new Vitess cluster -- you can specify a --server value of '%s'. +When doing so, you would use the --topo* flags so that the client can +connect directly to the topo server(s).`, useBundledVtctld), // We use PersistentPreRun to set up the tracer, grpc client, and // command context for every command. PersistentPreRunE: func(cmd *cobra.Command, args []string) (err error) { @@ -91,6 +118,10 @@ var ( if client != nil { err = client.Close() } + // Execute any registered onTerm functions. + for _, f := range onTerm { + f() + } trace.LogErrorsWhenClosing(traceCloser) return err }, @@ -134,9 +165,6 @@ const skipClientCreationKey = "skip_client_creation" // getClientForCommand returns a vtctldclient.VtctldClient for a given command. // It validates that --server was passed to the CLI for commands that need it. func getClientForCommand(cmd *cobra.Command) (vtctldclient.VtctldClient, error) { - if server == useBundledVtctld { - return nil, nil // The command will need to later setup a local vtctld server and client. - } if skipStr, ok := cmd.Annotations[skipClientCreationKey]; ok { skipClientCreation, err := strconv.ParseBool(skipStr) if err != nil { @@ -159,6 +187,24 @@ func getClientForCommand(cmd *cobra.Command) (vtctldclient.VtctldClient, error) return nil, errNoServer } + if server == useBundledVtctld { + ts, err := topo.OpenServer(topoOptions.implementation, strings.Join(topoOptions.globalServerAddresses, ","), topoOptions.globalRoot) + if err != nil { + return nil, fmt.Errorf("failed to connect to the topology server: %v", err) + } + onTerm = append(onTerm, ts.Close) + + // Use internal vtcltd server implementation. + // Register a nil grpc handler -- we will not use tmclient at all. + tmclient.RegisterTabletManagerClientFactory("grpc", func() tmclient.TabletManagerClient { + return nil + }) + vtctld := grpcvtctldserver.NewVtctldServer(ts) + localvtctldclient.SetServer(vtctld) + VtctldClientProtocol = "local" + server = "" + } + return vtctldclient.New(VtctldClientProtocol, server) } @@ -166,5 +212,8 @@ func init() { Root.PersistentFlags().StringVar(&server, "server", "", "server to use for the connection (required)") Root.PersistentFlags().DurationVar(&actionTimeout, "action_timeout", time.Hour, "timeout to use for the command") Root.PersistentFlags().BoolVar(&compactOutput, "compact", false, "use compact format for otherwise verbose outputs") + Root.PersistentFlags().StringVar(&topoOptions.implementation, "topo-implementation", topoOptions.implementation, "the topology implementation to use") + Root.PersistentFlags().StringSliceVar(&topoOptions.globalServerAddresses, "topo-global-server-address", topoOptions.globalServerAddresses, "the address of the global topology server(s)") + Root.PersistentFlags().StringVar(&topoOptions.globalRoot, "topo-global-root", topoOptions.globalRoot, "the path of the global topology data in the global topology server") vreplcommon.RegisterCommands(Root) } diff --git a/go/flags/endtoend/vtctldclient.txt b/go/flags/endtoend/vtctldclient.txt index 7fddb7eebfe..3871135378f 100644 --- a/go/flags/endtoend/vtctldclient.txt +++ b/go/flags/endtoend/vtctldclient.txt @@ -1,4 +1,7 @@ -Executes a cluster management command on the remote vtctld server. +If there are no running vtctld servers -- for example when boostrapping +a new Vitess cluster -- you can specify a --server value of 'bundled'. +When doing so, you would use the --topo* flags so that the client can +connect directly to the topo server(s). Usage: vtctldclient [flags] @@ -124,6 +127,9 @@ Flags: --security_policy string the name of a registered security policy to use for controlling access to URLs - empty means allow all for anyone (built-in policies: deny-all, read-only) --server string server to use for the connection (required) --stderrthreshold severity logs at or above this threshold go to stderr (default 1) + --topo-global-root string the path of the global topology data in the global topology server (default "/vitess/global") + --topo-global-server-address strings the address of the global topology server(s) (default [localhost:2379]) + --topo-implementation string the topology implementation to use (default "etcd2") -v, --v Level log level for V logs --version version for vtctldclient --vmodule moduleSpec comma-separated list of pattern=N settings for file-filtered logging From fc385fe48e53321d9d347551912e2f72ff728e79 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 20 Oct 2023 13:20:23 -0400 Subject: [PATCH 3/8] Use internal rather than bundled Signed-off-by: Matt Lord --- examples/common/scripts/etcd-up.sh | 2 +- go/cmd/vtctldclient/command/root.go | 11 +++++------ go/flags/endtoend/vtctldclient.txt | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/examples/common/scripts/etcd-up.sh b/examples/common/scripts/etcd-up.sh index 6204c8600ec..1ed22ffce2e 100755 --- a/examples/common/scripts/etcd-up.sh +++ b/examples/common/scripts/etcd-up.sh @@ -34,7 +34,7 @@ sleep 5 # If the node already exists, it's fine, means we used existing data. echo "add ${cell} CellInfo" set +e -command vtctldclient --server bundled AddCellInfo \ +command vtctldclient --server internal AddCellInfo \ --root "/vitess/${cell}" \ --server-address "${ETCD_SERVER}" \ "${cell}" diff --git a/go/cmd/vtctldclient/command/root.go b/go/cmd/vtctldclient/command/root.go index 0868a23bf1e..8a03eadc764 100644 --- a/go/cmd/vtctldclient/command/root.go +++ b/go/cmd/vtctldclient/command/root.go @@ -48,15 +48,14 @@ import ( _ "vitess.io/vitess/go/cmd/vtctldclient/command/vreplication/vdiff" _ "vitess.io/vitess/go/cmd/vtctldclient/command/vreplication/workflow" - // These imports register the topo factories to use when --server=bundled. + // These imports register the topo factories to use when --server=internal. _ "vitess.io/vitess/go/vt/topo/consultopo" _ "vitess.io/vitess/go/vt/topo/etcd2topo" _ "vitess.io/vitess/go/vt/topo/zk2topo" ) -// The --server value if you want to use a "local" -// vtctld server. -const useBundledVtctld = "bundled" +// The --server value if you want to use a "local" vtctld server. +const useInternalVtctld = "internal" var ( // VtctldClientProtocol is the protocol to use when creating the vtctldclient.VtctldClient. @@ -92,7 +91,7 @@ var ( Long: fmt.Sprintf(`If there are no running vtctld servers -- for example when boostrapping a new Vitess cluster -- you can specify a --server value of '%s'. When doing so, you would use the --topo* flags so that the client can -connect directly to the topo server(s).`, useBundledVtctld), +connect directly to the topo server(s).`, useInternalVtctld), // We use PersistentPreRun to set up the tracer, grpc client, and // command context for every command. PersistentPreRunE: func(cmd *cobra.Command, args []string) (err error) { @@ -187,7 +186,7 @@ func getClientForCommand(cmd *cobra.Command) (vtctldclient.VtctldClient, error) return nil, errNoServer } - if server == useBundledVtctld { + if server == useInternalVtctld { ts, err := topo.OpenServer(topoOptions.implementation, strings.Join(topoOptions.globalServerAddresses, ","), topoOptions.globalRoot) if err != nil { return nil, fmt.Errorf("failed to connect to the topology server: %v", err) diff --git a/go/flags/endtoend/vtctldclient.txt b/go/flags/endtoend/vtctldclient.txt index 3871135378f..7070a123c99 100644 --- a/go/flags/endtoend/vtctldclient.txt +++ b/go/flags/endtoend/vtctldclient.txt @@ -1,5 +1,5 @@ If there are no running vtctld servers -- for example when boostrapping -a new Vitess cluster -- you can specify a --server value of 'bundled'. +a new Vitess cluster -- you can specify a --server value of 'internal'. When doing so, you would use the --topo* flags so that the client can connect directly to the topo server(s). From 2afb16099f3ba533844b6137c7e9fa5b33b9d4be Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 20 Oct 2023 14:21:15 -0400 Subject: [PATCH 4/8] Add unit test Signed-off-by: Matt Lord --- go/cmd/vtctldclient/command/root.go | 15 ++++-- go/cmd/vtctldclient/command/root_test.go | 67 ++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/go/cmd/vtctldclient/command/root.go b/go/cmd/vtctldclient/command/root.go index 8a03eadc764..742944315f1 100644 --- a/go/cmd/vtctldclient/command/root.go +++ b/go/cmd/vtctldclient/command/root.go @@ -23,6 +23,7 @@ import ( "io" "strconv" "strings" + "sync" "time" "github.com/spf13/cobra" @@ -70,6 +71,11 @@ var ( // Register functions to be called when the command completes. onTerm = []func(){} + // Register our nil tmclient grpc handler only one time. + // This is primarily for tests where we execute the root + // command multiple times. + once = sync.Once{} + server string actionTimeout time.Duration compactOutput bool @@ -194,9 +200,12 @@ func getClientForCommand(cmd *cobra.Command) (vtctldclient.VtctldClient, error) onTerm = append(onTerm, ts.Close) // Use internal vtcltd server implementation. - // Register a nil grpc handler -- we will not use tmclient at all. - tmclient.RegisterTabletManagerClientFactory("grpc", func() tmclient.TabletManagerClient { - return nil + // Register a nil grpc handler -- we will not use tmclient at all but + // a factory still needs to be registered. + once.Do(func() { + tmclient.RegisterTabletManagerClientFactory("grpc", func() tmclient.TabletManagerClient { + return nil + }) }) vtctld := grpcvtctldserver.NewVtctldServer(ts) localvtctldclient.SetServer(vtctld) diff --git a/go/cmd/vtctldclient/command/root_test.go b/go/cmd/vtctldclient/command/root_test.go index 155fac78705..5efe844e1a1 100644 --- a/go/cmd/vtctldclient/command/root_test.go +++ b/go/cmd/vtctldclient/command/root_test.go @@ -17,13 +17,19 @@ limitations under the License. package command_test import ( + "context" + "fmt" "os" + "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "vitess.io/vitess/go/cmd/vtctldclient/command" + "vitess.io/vitess/go/vt/topo" + "vitess.io/vitess/go/vt/topo/memorytopo" "vitess.io/vitess/go/vt/vtctl/localvtctldclient" vtctlservicepb "vitess.io/vitess/go/vt/proto/vtctlservice" @@ -52,3 +58,64 @@ func TestRoot(t *testing.T) { assert.Contains(t, err.Error(), "unknown command") }) } + +// TestRootWithInternalVtctld tests that the internal VtctldServer +// implementation -- used with --server=internal -- works for +// commands as expected. +func TestRootWithInternalVtctld(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cell := "zone1" + ts, factory := memorytopo.NewServerAndFactory(ctx, cell) + topo.RegisterFactory("test", factory) + command.VtctldClientProtocol = "local" + baseArgs := []string{"vtctldclient", "--server", "internal", "--topo-implementation", "test"} + + args := append([]string{}, os.Args...) + protocol := command.VtctldClientProtocol + t.Cleanup(func() { + ts.Close() + os.Args = append([]string{}, args...) + command.VtctldClientProtocol = protocol + }) + + testCases := []struct { + command string + args []string + expectErr string + }{ + { + command: "AddCellInfo", + args: []string{"--root", fmt.Sprintf("/vitess/%s", cell), "--server-address", "", cell}, + expectErr: "node already exists", // Cell already exists + }, + { + command: "GetTablets", + }, + { + command: "NoCommandDrJones", + expectErr: "unknown command", // Invalid command + }, + } + + for _, tc := range testCases { + t.Run(tc.command, func(t *testing.T) { + defer func() { + // Reset the OS args. + os.Args = append([]string{}, args...) + }() + + os.Args = append(baseArgs, tc.command) + os.Args = append(os.Args, tc.args...) + + err := command.Root.Execute() + if tc.expectErr != "" { + if !strings.Contains(err.Error(), tc.expectErr) { + t.Errorf(fmt.Sprintf("%s error = %v, expectErr = %v", tc.command, err, tc.expectErr)) + } + } else { + require.NoError(t, err, "unexpected error: %v", err) + } + }) + } +} From d0e62d47720a2b3e05f5377616546a05b5dbdf70 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 20 Oct 2023 17:01:11 -0400 Subject: [PATCH 5/8] Replace vtctl usage in consul and zk examples as well. Signed-off-by: Matt Lord --- examples/common/scripts/consul-up.sh | 8 ++++---- examples/common/scripts/zk-up.sh | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/examples/common/scripts/consul-up.sh b/examples/common/scripts/consul-up.sh index 584a25f437a..fb75495b278 100755 --- a/examples/common/scripts/consul-up.sh +++ b/examples/common/scripts/consul-up.sh @@ -40,13 +40,13 @@ sleep 5 # Add the CellInfo description for the cell. # If the node already exists, it's fine, means we used existing data. -echo "add $cell CellInfo" +echo "add ${cell} CellInfo" set +e # shellcheck disable=SC2086 -vtctl $TOPOLOGY_FLAGS VtctldCommand AddCellInfo \ - --root "vitess/$cell" \ +command vtctldclient --server internal --topo-implementation consul --topo-global-server "${CONSUL_SERVER}:${consul_http_port}" AddCellInfo \ + --root "/vitess/${cell}" \ --server-address "${CONSUL_SERVER}:${consul_http_port}" \ - "$cell" + "${cell}" set -e echo "consul start done..." diff --git a/examples/common/scripts/zk-up.sh b/examples/common/scripts/zk-up.sh index 519d5305b25..94ff1b85b93 100755 --- a/examples/common/scripts/zk-up.sh +++ b/examples/common/scripts/zk-up.sh @@ -53,10 +53,10 @@ echo "Started zk servers." # If the node already exists, it's fine, means we used existing data. set +e # shellcheck disable=SC2086 -vtctl $TOPOLOGY_FLAGS VtctldCommand AddCellInfo \ - --root /vitess/$cell \ - --server-address $ZK_SERVER \ - $cell +command vtctldclient --server internal --topo-implementation zk2 --topo-global-server "${ZK_SERVER}" AddCellInfo \ + --root "/vitess/${cell}" \ + --server-address "${ZK_SERVER}" \ + "${cell}" set -e echo "Configured zk servers." From 91fb502b99d770da9d76fe58de44fcad2257cead Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 20 Oct 2023 17:29:26 -0400 Subject: [PATCH 6/8] Fix typo Signed-off-by: Matt Lord --- go/cmd/vtctldclient/command/root.go | 4 ++-- go/flags/endtoend/vtctldclient.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go/cmd/vtctldclient/command/root.go b/go/cmd/vtctldclient/command/root.go index 742944315f1..2802b1ba822 100644 --- a/go/cmd/vtctldclient/command/root.go +++ b/go/cmd/vtctldclient/command/root.go @@ -94,7 +94,7 @@ var ( Root = &cobra.Command{ Use: "vtctldclient", Short: "Executes a cluster management command on the remote vtctld server.", - Long: fmt.Sprintf(`If there are no running vtctld servers -- for example when boostrapping + Long: fmt.Sprintf(`If there are no running vtctld servers -- for example when bootstrapping a new Vitess cluster -- you can specify a --server value of '%s'. When doing so, you would use the --topo* flags so that the client can connect directly to the topo server(s).`, useInternalVtctld), @@ -199,7 +199,7 @@ func getClientForCommand(cmd *cobra.Command) (vtctldclient.VtctldClient, error) } onTerm = append(onTerm, ts.Close) - // Use internal vtcltd server implementation. + // Use internal vtctld server implementation. // Register a nil grpc handler -- we will not use tmclient at all but // a factory still needs to be registered. once.Do(func() { diff --git a/go/flags/endtoend/vtctldclient.txt b/go/flags/endtoend/vtctldclient.txt index 7070a123c99..93d7d8af422 100644 --- a/go/flags/endtoend/vtctldclient.txt +++ b/go/flags/endtoend/vtctldclient.txt @@ -1,4 +1,4 @@ -If there are no running vtctld servers -- for example when boostrapping +If there are no running vtctld servers -- for example when bootstrapping a new Vitess cluster -- you can specify a --server value of 'internal'. When doing so, you would use the --topo* flags so that the client can connect directly to the topo server(s). From 52645bbca436edf7cf6d18caf1cb0d5bb6af1004 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Tue, 24 Oct 2023 17:49:40 -0400 Subject: [PATCH 7/8] Correct help output Signed-off-by: Matt Lord --- go/cmd/vtctldclient/command/root.go | 3 ++- go/flags/endtoend/vtctldclient.txt | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go/cmd/vtctldclient/command/root.go b/go/cmd/vtctldclient/command/root.go index 2802b1ba822..a5848a7b42a 100644 --- a/go/cmd/vtctldclient/command/root.go +++ b/go/cmd/vtctldclient/command/root.go @@ -94,7 +94,8 @@ var ( Root = &cobra.Command{ Use: "vtctldclient", Short: "Executes a cluster management command on the remote vtctld server.", - Long: fmt.Sprintf(`If there are no running vtctld servers -- for example when bootstrapping + Long: fmt.Sprintf(`Executes a cluster management command on the remote vtctld server. +If there are no running vtctld servers -- for example when bootstrapping a new Vitess cluster -- you can specify a --server value of '%s'. When doing so, you would use the --topo* flags so that the client can connect directly to the topo server(s).`, useInternalVtctld), diff --git a/go/flags/endtoend/vtctldclient.txt b/go/flags/endtoend/vtctldclient.txt index 93d7d8af422..78167b742c6 100644 --- a/go/flags/endtoend/vtctldclient.txt +++ b/go/flags/endtoend/vtctldclient.txt @@ -1,3 +1,4 @@ +Executes a cluster management command on the remote vtctld server. If there are no running vtctld servers -- for example when bootstrapping a new Vitess cluster -- you can specify a --server value of 'internal'. When doing so, you would use the --topo* flags so that the client can From 459a27b7d6b5313521bac8dad4f2564ca527ef34 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Thu, 16 Nov 2023 10:00:18 +0100 Subject: [PATCH 8/8] Sneak in two non-functional changes from discussions Signed-off-by: Matt Lord --- go/vt/vtctl/grpcvtctldserver/server.go | 4 ++-- go/vt/vttablet/tabletmanager/vreplication/vcopier_atomic.go | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index 25b711e1019..21c4dc272f6 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -2018,7 +2018,7 @@ func (s *VtctldServer) GetTablets(ctx context.Context, req *vtctldatapb.GetTable tablets := make([]*topodatapb.Tablet, 0, len(tabletMap)) for _, ti := range tabletMap { - if req.TabletType != 0 && ti.Type != req.TabletType { + if req.TabletType != topodatapb.TabletType_UNKNOWN && ti.Type != req.TabletType { continue } adjustTypeForStalePrimary(ti, truePrimaryTimestamp) @@ -2086,7 +2086,7 @@ func (s *VtctldServer) GetTablets(ctx context.Context, req *vtctldatapb.GetTable if req.Keyspace != "" && tablet.Keyspace != req.Keyspace { continue } - if req.TabletType != 0 && tablet.Type != req.TabletType { + if req.TabletType != topodatapb.TabletType_UNKNOWN && tablet.Type != req.TabletType { continue } diff --git a/go/vt/vttablet/tabletmanager/vreplication/vcopier_atomic.go b/go/vt/vttablet/tabletmanager/vreplication/vcopier_atomic.go index 4da072e3955..fe92f284ce8 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/vcopier_atomic.go +++ b/go/vt/vttablet/tabletmanager/vreplication/vcopier_atomic.go @@ -303,7 +303,6 @@ func (vc *vcopier) copyAll(ctx context.Context, settings binlogplayer.VRSettings // deleteCopyState deletes the copy state entry for a table, signifying that the copy phase is complete for that table. func (vc *vcopier) deleteCopyState(tableName string) error { log.Infof("Deleting copy state for table %s", tableName) - //FIXME get sidecar db name delQuery := fmt.Sprintf("delete from _vt.copy_state where table_name=%s and vrepl_id = %d", encodeString(tableName), vc.vr.id) if _, err := vc.vr.dbClient.Execute(delQuery); err != nil { return err