Changes from self review

Signed-off-by: Matt Lord <mattalord@gmail.com>

go/cmd/vtctldclient/command/permissions.go

@@ -36,16 +36,20 @@ var (
 		Args:                  cobra.ExactArgs(1),
 		RunE:                  commandGetPermissions,
 	}
+	// ValidatePermissionsShard makes a ValidatePermissionsKeyspace gRPC call to a
+	// vtctld with the specified shard to examine in the keyspace.
 	ValidatePermissionsShard = &cobra.Command{
 		Use:                   "ValidatePermissionsShard <keyspace/shard>",
-		Short:                 "Validates that the permissions on primary match all the replicas.",
+		Short:                 "Validates that the permissions on the primary match all of the replicas.",
 		DisableFlagsInUseLine: true,
 		Args:                  cobra.ExactArgs(1),
 		RunE:                  commandValidatePermissionsShard,
 	}
+	// ValidatePermissionsKeyspace makes a ValidatePermissionsKeyspace gRPC call to a
+	// vtctld.
 	ValidatePermissionsKeyspace = &cobra.Command{
 		Use:                   "ValidatePermissionsKeyspace <keyspace name>",
-		Short:                 "Validates that the permissions on primary of the first shard match those of all of the other tablets in the keyspace.",
+		Short:                 "Validates that the permissions on the primary of the first shard match those of all of the other tablets in the keyspace.",
 		DisableFlagsInUseLine: true,
 		Args:                  cobra.ExactArgs(1),
 		RunE:                  commandValidatePermissionsKeyspace,

go/cmd/vtctldclient/command/schema.go

@@ -107,8 +107,8 @@ For --sql, semi-colons and repeated values may be mixed, for example:
 		Args:                  cobra.ExactArgs(1),
 		RunE:                  commandValidateSchemaKeyspace,
 	}
-	// ValidateSchemaShard makes a ValidateSchemaKeyspace gRPC call to a vtctld WITH
-	// 1 specific shard to examine in the keyspace.
+	// ValidateSchemaShard makes a ValidateSchemaKeyspace gRPC call to a vtctld with
+	// the specified shard to examine in the keyspace.
 	ValidateSchemaShard = &cobra.Command{
 		Use:                   "ValidateSchemaShard [--exclude-tables=<exclude_tables>] [--include-views] [--skip-no-primary] [--include-vschema] <keyspace/shard>",
 		Short:                 "Validates that the schema on the primary tablet for the specified shard matches the schema on all other tablets in that shard.",
@@ -190,13 +190,13 @@ var copySchemaShardOptions = struct {
 }{}
 
 func commandCopySchemaShard(cmd *cobra.Command, args []string) error {
-	cli.FinishedParsing(cmd)
-
 	destKeyspace, destShard, err := topoproto.ParseKeyspaceShard(cmd.Flags().Arg(1))
 	if err != nil {
 		return err
 	}
 
+	cli.FinishedParsing(cmd)
+
 	var sourceTabletAlias *topodatapb.TabletAlias
 	sourceKeyspace, sourceShard, err := topoproto.ParseKeyspaceShard(cmd.Flags().Arg(0))
 	if err == nil {
@@ -377,9 +377,9 @@ var validateSchemaKeyspaceOptions = struct {
 func commandValidateSchemaKeyspace(cmd *cobra.Command, args []string) error {
 	cli.FinishedParsing(cmd)
 
-	ks := cmd.Flags().Arg(0)
+	keyspace := cmd.Flags().Arg(0)
 	resp, err := client.ValidateSchemaKeyspace(commandCtx, &vtctldatapb.ValidateSchemaKeyspaceRequest{
-		Keyspace:       ks,
+		Keyspace:       keyspace,
 		ExcludeTables:  validateSchemaKeyspaceOptions.ExcludeTables,
 		IncludeVschema: validateSchemaKeyspaceOptions.IncludeVSchema,
 		SkipNoPrimary:  validateSchemaKeyspaceOptions.SkipNoPrimary,

go/cmd/vtctldclient/command/topology.go

@@ -116,7 +116,7 @@ func commandWriteTopologyPath(cmd *cobra.Command, args []string) error {
 	}
 	data, err := os.ReadFile(file)
 	if err != nil {
-		return fmt.Errorf("failed to read %s: %v", file, err)
+		return fmt.Errorf("failed to read file %s: %v", file, err)
 	}
 	_, err = conn.Update(cmd.Context(), path, data, nil)
 	if err != nil {

go/cmd/vtctldclient/plugin_grpctmclient.go

@@ -16,7 +16,11 @@ limitations under the License.
 
 package main
 
-// Imports and register the gRPC tabletmanager client
+// Imports and registers the gRPC tabletmanager client.
+// This is needed when --server=internal as the vtctldclient
+// binary will then not only need to talk to the topo server
+// directly but it will also need to talk to tablets directly
+// via tmclient.
 
 import (
 	_ "vitess.io/vitess/go/vt/vttablet/grpctmclient"

go/flags/endtoend/vtctldclient.txt

@@ -105,8 +105,8 @@ Available Commands:
   VDiff                       Perform commands related to diffing tables involved in a VReplication workflow between the source and target.
   Validate                    Validates that all nodes reachable from the global replication graph, as well as all tablets in discoverable cells, are consistent.
   ValidateKeyspace            Validates that all nodes reachable from the specified keyspace are consistent.
-  ValidatePermissionsKeyspace Validates that the permissions on primary of the first shard match those of all of the other tablets in the keyspace.
-  ValidatePermissionsShard    Validates that the permissions on primary match all the replicas.
+  ValidatePermissionsKeyspace Validates that the permissions on the primary of the first shard match those of all of the other tablets in the keyspace.
+  ValidatePermissionsShard    Validates that the permissions on the primary match all of the replicas.
   ValidateSchemaKeyspace      Validates that the schema on the primary tablet for the first shard matches the schema on all other tablets in the keyspace.
   ValidateSchemaShard         Validates that the schema on the primary tablet for the specified shard matches the schema on all other tablets in that shard.
   ValidateShard               Validates that all nodes reachable from the specified shard are consistent.

go/test/endtoend/cluster/cluster_process.go

@@ -1292,25 +1292,25 @@ func (cluster *LocalProcessCluster) NewVttabletInstance(tabletType string, UID i
 func (cluster *LocalProcessCluster) NewVTOrcProcess(config VTOrcConfiguration) *VTOrcProcess {
 	base := VtProcessInstance("vtorc", "vtorc", cluster.TopoProcess.Port, cluster.Hostname)
 	return &VTOrcProcess{
-		VtProcess: *base,
+		VtProcess: base,
 		LogDir:    cluster.TmpDirectory,
 		Config:    config,
 		Port:      cluster.GetAndReservePort(),
 	}
 }
 
-// VtctldClientProcessInstance returns a VtctldProcess handle for vtctldclient process
-// configured with the given Config.
+// VtctldClientProcessInstance returns a VtctldProcess handle for a
+// vtctldclient process configured with the given Config.
 func (cluster *LocalProcessCluster) NewVtctldClientProcessInstance(hostname string, grpcPort int, tmpDirectory string) *VtctldClientProcess {
 	version, err := GetMajorVersion("vtctldclient")
 	if err != nil {
-		log.Warningf("failed to get major vtctldclient version; interop with CLI changes for VEP-4 may not work: %s", err)
+		log.Warningf("failed to get major vtctldclient version; interop with CLI changes for VEP-4 may not work: %v", err)
 	}
 
 	base := VtProcessInstance("vtctldclient", "vtctldclient", cluster.TopoProcess.Port, cluster.Hostname)
 
 	vtctldclient := &VtctldClientProcess{
-		VtProcess:                *base,
+		VtProcess:                base,
 		Server:                   fmt.Sprintf("%s:%d", hostname, grpcPort),
 		TempDirectory:            tmpDirectory,
 		VtctldClientMajorVersion: version,

go/test/endtoend/cluster/vt_process.go

@@ -34,7 +34,7 @@ type VtProcess struct {
 
 // VtProcessInstance returns a VtProcess handle configured with the given Config.
 // The process must be manually started by calling setup()
-func VtProcessInstance(name, binary string, topoPort int, hostname string) *VtProcess {
+func VtProcessInstance(name, binary string, topoPort int, hostname string) VtProcess {
 	// Default values for etcd2 topo server.
 	topoImplementation := "etcd2"
 	topoRootPath := "/"
@@ -49,7 +49,7 @@ func VtProcessInstance(name, binary string, topoPort int, hostname string) *VtPr
 		topoRootPath = ""
 	}
 
-	vt := &VtProcess{
+	vt := VtProcess{
 		Name:               name,
 		Binary:             binary,
 		TopoImplementation: topoImplementation,

go/test/endtoend/cluster/vtbackup_process.go

@@ -132,7 +132,7 @@ func VtbackupProcessInstance(tabletUID int, mysqlPort int, newInitDBFile string,
 	cell string, hostname string, tmpDirectory string, topoPort int, initialBackup bool) *VtbackupProcess {
 	base := VtProcessInstance("vtbackup", "vtbackup", topoPort, hostname)
 	vtbackup := &VtbackupProcess{
-		VtProcess:                   *base,
+		VtProcess:                   base,
 		LogDir:                      tmpDirectory,
 		Directory:                   os.Getenv("VTDATAROOT"),
 		BackupStorageImplementation: "file",

go/test/endtoend/cluster/vtctld_process.go

@@ -168,7 +168,7 @@ func (vtctld *VtctldProcess) TearDown() error {
 func VtctldProcessInstance(httpPort int, grpcPort int, topoPort int, hostname string, tmpDirectory string) *VtctldProcess {
 	base := VtProcessInstance("vtctld", "vtctld", topoPort, hostname)
 	vtctld := &VtctldProcess{
-		VtProcess:                   *base,
+		VtProcess:                   base,
 		ServiceMap:                  "grpc-vtctl,grpc-vtctld",
 		BackupStorageImplementation: "file",
 		FileBackupStorageRoot:       path.Join(os.Getenv("VTDATAROOT"), "/backups"),

go/test/endtoend/cluster/vtctldclient_process.go

@@ -55,7 +55,7 @@ func VtctldClientProcessInstance(grpcPort int, topoPort int, hostname string, tm
 
 	base := VtProcessInstance("vtctldclient", "vtctldclient", topoPort, hostname)
 	vtctldclient := &VtctldClientProcess{
-		VtProcess:                *base,
+		VtProcess:                base,
 		Server:                   fmt.Sprintf("%s:%d", hostname, grpcPort),
 		TempDirectory:            tmpDirectory,
 		VtctldClientMajorVersion: version,
@@ -116,6 +116,8 @@ func (vtctldclient *VtctldClientProcess) ExecuteCommandWithOutput(args ...string
 }
 
 // AddCellInfo executes the vtctldclient command to add cell info.
+// It uses --server=internal as there may not yet be a vtctld running
+// as we need to create a cell for vtctld to use first.
 func (vtctldclient *VtctldClientProcess) AddCellInfo(Cell string) error {
 	args := []string{
 		"--server", "internal",
@@ -340,8 +342,9 @@ func (vtctldclient *VtctldClientProcess) OnlineDDLShow(keyspace, workflow string
 	)
 }
 
-// shouldRetry tells us if the command should be retried based on the results/output -- meaning that it
-// is likely an ephemeral or recoverable issue that is likely to succeed when retried.
+// shouldRetry tells us if the command should be retried based on the results/output
+// -- meaning that it is likely an ephemeral or recoverable issue that is likely to
+// succeed when retried.
 func shouldRetry(cmdResults string) bool {
 	return strings.Contains(cmdResults, "Deadlock found when trying to get lock; try restarting transaction")
 }

go/test/endtoend/cluster/vtgate_process.go

@@ -370,7 +370,7 @@ func VtgateProcessInstance(
 ) *VtgateProcess {
 	base := VtProcessInstance("vtgate", "vtgate", topoPort, hostname)
 	vtgate := &VtgateProcess{
-		VtProcess:             *base,
+		VtProcess:             base,
 		FileToLogQueries:      path.Join(tmpDirectory, "/vtgate_querylog.txt"),
 		ConfigFile:            path.Join(tmpDirectory, fmt.Sprintf("vtgate-config-%d.json", port)),
 		Directory:             os.Getenv("VTDATAROOT"),

go/test/endtoend/cluster/vtorc_process.go

@@ -116,7 +116,7 @@ func (orc *VTOrcProcess) Setup() (err error) {
 	--config config/vtorc/default.json --alsologtostderr
 	*/
 	orc.proc = exec.Command(
-		"vtorc",
+		orc.Binary,
 		"--topo_implementation", orc.TopoImplementation,
 		"--topo_global_server_address", orc.TopoGlobalAddress,
 		"--topo_global_root", orc.TopoGlobalRoot,

go/test/endtoend/cluster/vttablet_process.go

@@ -717,7 +717,7 @@ func (vttablet *VttabletProcess) IsShutdown() bool {
 func VttabletProcessInstance(port, grpcPort, tabletUID int, cell, shard, keyspace string, vtctldPort int, tabletType string, topoPort int, hostname, tmpDirectory string, extraArgs []string, charset string) *VttabletProcess {
 	base := VtProcessInstance("vttablet", "vttablet", topoPort, hostname)
 	vttablet := &VttabletProcess{
-		VtProcess:                   *base,
+		VtProcess:                   base,
 		FileToLogQueries:            path.Join(tmpDirectory, fmt.Sprintf("/vt_%010d_querylog.txt", tabletUID)),
 		Directory:                   path.Join(os.Getenv("VTDATAROOT"), fmt.Sprintf("/vt_%010d", tabletUID)),
 		Cell:                        cell,

go/test/endtoend/reparent/utils/utils.go

@@ -406,7 +406,7 @@ func ErsIgnoreTablet(clusterInstance *cluster.LocalProcessCluster, tab *cluster.
 	return clusterInstance.VtctldClientProcess.ExecuteCommandWithOutput(args...)
 }
 
-// ErsWithVtctldClient runs ERS via vtctldclient binary
+// ErsWithVtctldClient runs ERS via a vtctldclient binary.
 func ErsWithVtctldClient(clusterInstance *cluster.LocalProcessCluster) (string, error) {
 	args := []string{"EmergencyReparentShard", fmt.Sprintf("%s/%s", KeyspaceName, ShardName)}
 	return clusterInstance.VtctldClientProcess.ExecuteCommandWithOutput(args...)

go/test/endtoend/vreplication/cluster_test.go

@@ -166,7 +166,7 @@ func (vc *VitessCluster) StartVTOrc() error {
 	}
 	base := cluster.VtProcessInstance("vtorc", "vtorc", vc.ClusterConfig.topoPort, vc.ClusterConfig.hostname)
 	vtorcProcess := &cluster.VTOrcProcess{
-		VtProcess: *base,
+		VtProcess: base,
 		LogDir:    vc.ClusterConfig.tmpDir,
 		Config:    cluster.VTOrcConfiguration{},
 		Port:      vc.ClusterConfig.vtorcPort,

go/test/endtoend/vreplication/migrate_test.go

@@ -41,12 +41,14 @@ func insertInitialDataIntoExternalCluster(t *testing.T, conn *mysql.Conn) {
 	})
 }
 
-// TestMigrateUnsharded runs an e2e test for importing from an external cluster using the vtctldclient Mount and Migrate commands.
-// We have an anti-pattern in Vitess: vt executables look for an environment variable VTDATAROOT for certain cluster parameters
-// like the log directory when they are created. Until this test we just needed a single cluster for e2e tests.
-// However now we need to create an external Vitess cluster. For this we need a different VTDATAROOT and
-// hence the VTDATAROOT env variable gets overwritten.
-// Each time we need to create vt processes in the "other" cluster we need to set the appropriate VTDATAROOT
+// TestMigrateUnsharded runs an e2e test for importing from an external cluster using the
+// vtctldclient Mount and Migrate commands.We have an anti-pattern in Vitess: vt executables
+// look for an environment variable VTDATAROOT for certain cluster parameters like the log
+// directory when they are created. Until this test we just needed a single cluster for e2e
+// tests. However now we need to create an external Vitess cluster. For this we need a
+// different VTDATAROOT and hence the VTDATAROOT env variable gets overwritten. Each time
+// we need to create vt processes in the "other" cluster we need to set the appropriate
+// VTDATAROOT.
 func TestMigrateUnsharded(t *testing.T) {
 	vc = NewVitessCluster(t, nil)
 	defer vc.TearDown()
@@ -188,9 +190,9 @@ func TestMigrateUnsharded(t *testing.T) {
 	})
 }
 
-// TestMigrateSharded adds a test for a sharded cluster to validate a fix for a bug where the target keyspace name
-// doesn't match that of the source cluster. The test migrates from a cluster with keyspace customer to an "external"
-// cluster with keyspace rating.
+// TestMigrateSharded adds a test for a sharded cluster to validate a fix for a bug where
+// the target keyspace name doesn't match that of the source cluster. The test migrates
+// from a cluster with keyspace customer to an "external" cluster with keyspace rating.
 func TestMigrateSharded(t *testing.T) {
 	setSidecarDBName("_vt")
 	currentWorkflowType = binlogdatapb.VReplicationWorkflowType_MoveTables

go/vt/vtctl/grpcvtctldserver/server.go

@@ -886,9 +886,10 @@ func (s *VtctldServer) CopySchemaShard(ctx context.Context, req *vtctldatapb.Cop
 		return nil, err
 	}
 
-	return &vtctldatapb.CopySchemaShardResponse{}, s.ws.CopySchemaShard(ctx,
-		req.SourceTabletAlias, req.Tables, req.ExcludeTables, req.IncludeViews, req.DestinationKeyspace, req.DestinationShard,
-		waitReplicasTimeout, req.SkipVerify)
+	err = s.ws.CopySchemaShard(ctx, req.SourceTabletAlias, req.Tables, req.ExcludeTables, req.IncludeViews,
+		req.DestinationKeyspace, req.DestinationShard, waitReplicasTimeout, req.SkipVerify)
+
+	return &vtctldatapb.CopySchemaShardResponse{}, err
 }
 
 // CreateKeyspace is part of the vtctlservicepb.VtctldServer interface.
@@ -4736,7 +4737,7 @@ func (s *VtctldServer) ValidatePermissionsKeyspace(ctx context.Context, req *vtc
 	}
 
 	if len(shards) == 0 {
-		return nil, fmt.Errorf("no shards in keyspace %v", req.Keyspace)
+		return nil, fmt.Errorf("no shards found in keyspace %s", req.Keyspace)
 	}
 	sort.Strings(shards)
 
@@ -4758,7 +4759,7 @@ func (s *VtctldServer) ValidatePermissionsKeyspace(ctx context.Context, req *vtc
 	}
 	referencePermissions := pres.Permissions
 
-	// Then diff the first tablet with all others.
+	// Then diff the first/reference tablet with all the others.
 	eg, egctx := errgroup.WithContext(ctx)
 	for _, shard := range shards {
 		eg.Go(func() error {
@@ -4770,15 +4771,16 @@ func (s *VtctldServer) ValidatePermissionsKeyspace(ctx context.Context, req *vtc
 				if topoproto.TabletAliasEqual(alias, si.PrimaryAlias) {
 					continue
 				}
-				log.Infof("Gathering permissions for %v", topoproto.TabletAliasString(alias))
+				log.Infof("Gathering permissions for %s", topoproto.TabletAliasString(alias))
 				presp, err := s.GetPermissions(ctx, &vtctldatapb.GetPermissionsRequest{
 					TabletAlias: alias,
 				})
 				if err != nil {
 					return err
 				}
 
-				log.Infof("Diffing permissions for %s", topoproto.TabletAliasString(alias))
+				log.Infof("Diffing permissions between %s and %s", topoproto.TabletAliasString(referenceAlias),
+					topoproto.TabletAliasString(alias))
 				er := &concurrency.AllErrorRecorder{}
 				tmutils.DiffPermissions(topoproto.TabletAliasString(referenceAlias), referencePermissions,
 					topoproto.TabletAliasString(alias), presp.Permissions, er)
@@ -4792,11 +4794,13 @@ func (s *VtctldServer) ValidatePermissionsKeyspace(ctx context.Context, req *vtc
 	if err := eg.Wait(); err != nil {
 		return nil, fmt.Errorf("permissions diffs: %v", err)
 	}
+
 	return &vtctldatapb.ValidatePermissionsKeyspaceResponse{}, nil
 }
 
 // ValidateSchemaKeyspace is a part of the vtctlservicepb.VtctldServer interface.
-// It will diff the schema from all the tablets in the keyspace.
+// It will diff the schema between the tablets in all shards -- or a subset if
+// any specific shards are specified -- within the keyspace.
 func (s *VtctldServer) ValidateSchemaKeyspace(ctx context.Context, req *vtctldatapb.ValidateSchemaKeyspaceRequest) (resp *vtctldatapb.ValidateSchemaKeyspaceResponse, err error) {
 	span, ctx := trace.NewSpan(ctx, "VtctldServer.ValidateSchemaKeyspace")
 	defer span.Finish()
@@ -4819,7 +4823,7 @@ func (s *VtctldServer) ValidateSchemaKeyspace(ctx context.Context, req *vtctldat
 		// Otherwise we look at all the shards in the keyspace.
 		shards, err = s.ts.GetShardNames(ctx, keyspace)
 		if err != nil {
-			resp.Results = append(resp.Results, fmt.Sprintf("TopologyServer.GetShardNames(%v) failed: %v", req.Keyspace, err))
+			resp.Results = append(resp.Results, fmt.Sprintf("TopologyServer.GetShardNames(%s) failed: %v", req.Keyspace, err))
 			err = nil
 			return resp, err
 		}