From c158a65798d8d550f6dec024ad48037fa303fc48 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 3 Jan 2025 13:15:20 -0500 Subject: [PATCH] Changes from self review Signed-off-by: Matt Lord --- go/cmd/vtctldclient/command/permissions.go | 8 +++++-- go/cmd/vtctldclient/command/schema.go | 12 +++++----- go/cmd/vtctldclient/command/topology.go | 2 +- go/cmd/vtctldclient/plugin_grpctmclient.go | 6 ++++- go/test/endtoend/cluster/cluster_process.go | 10 ++++----- go/test/endtoend/cluster/vt_process.go | 4 ++-- go/test/endtoend/cluster/vtbackup_process.go | 2 +- go/test/endtoend/cluster/vtctld_process.go | 2 +- .../endtoend/cluster/vtctldclient_process.go | 9 +++++--- go/test/endtoend/cluster/vtgate_process.go | 2 +- go/test/endtoend/cluster/vtorc_process.go | 2 +- go/test/endtoend/cluster/vttablet_process.go | 2 +- go/test/endtoend/reparent/utils/utils.go | 2 +- go/test/endtoend/vreplication/cluster_test.go | 2 +- go/test/endtoend/vreplication/migrate_test.go | 20 +++++++++-------- go/vt/vtctl/grpcvtctldserver/server.go | 22 +++++++++++-------- 16 files changed, 62 insertions(+), 45 deletions(-) diff --git a/go/cmd/vtctldclient/command/permissions.go b/go/cmd/vtctldclient/command/permissions.go index 53036307927..35efbf82ae5 100644 --- a/go/cmd/vtctldclient/command/permissions.go +++ b/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 ", - 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 ", - 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, diff --git a/go/cmd/vtctldclient/command/schema.go b/go/cmd/vtctldclient/command/schema.go index 59cbe39276f..21a995f30a1 100644 --- a/go/cmd/vtctldclient/command/schema.go +++ b/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=] [--include-views] [--skip-no-primary] [--include-vschema] ", 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, diff --git a/go/cmd/vtctldclient/command/topology.go b/go/cmd/vtctldclient/command/topology.go index 834976ad7fb..a03ce403eb8 100644 --- a/go/cmd/vtctldclient/command/topology.go +++ b/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 { diff --git a/go/cmd/vtctldclient/plugin_grpctmclient.go b/go/cmd/vtctldclient/plugin_grpctmclient.go index fafba4ebc7e..74e1891edb5 100644 --- a/go/cmd/vtctldclient/plugin_grpctmclient.go +++ b/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" diff --git a/go/test/endtoend/cluster/cluster_process.go b/go/test/endtoend/cluster/cluster_process.go index bd3262d6155..ed61f7fd0c9 100644 --- a/go/test/endtoend/cluster/cluster_process.go +++ b/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, diff --git a/go/test/endtoend/cluster/vt_process.go b/go/test/endtoend/cluster/vt_process.go index e20d240a8bc..334a413f4e6 100644 --- a/go/test/endtoend/cluster/vt_process.go +++ b/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, diff --git a/go/test/endtoend/cluster/vtbackup_process.go b/go/test/endtoend/cluster/vtbackup_process.go index 866edb2577d..ea12c8200d4 100644 --- a/go/test/endtoend/cluster/vtbackup_process.go +++ b/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", diff --git a/go/test/endtoend/cluster/vtctld_process.go b/go/test/endtoend/cluster/vtctld_process.go index 077e8ee3f41..831f5a23af7 100644 --- a/go/test/endtoend/cluster/vtctld_process.go +++ b/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"), diff --git a/go/test/endtoend/cluster/vtctldclient_process.go b/go/test/endtoend/cluster/vtctldclient_process.go index fd8f627f7e5..3dc1a811930 100644 --- a/go/test/endtoend/cluster/vtctldclient_process.go +++ b/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") } diff --git a/go/test/endtoend/cluster/vtgate_process.go b/go/test/endtoend/cluster/vtgate_process.go index 30823a77a71..834bb0c9bce 100644 --- a/go/test/endtoend/cluster/vtgate_process.go +++ b/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"), diff --git a/go/test/endtoend/cluster/vtorc_process.go b/go/test/endtoend/cluster/vtorc_process.go index 9bc14916446..4d726241756 100644 --- a/go/test/endtoend/cluster/vtorc_process.go +++ b/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, diff --git a/go/test/endtoend/cluster/vttablet_process.go b/go/test/endtoend/cluster/vttablet_process.go index d3c69016ec4..65c6fbeec26 100644 --- a/go/test/endtoend/cluster/vttablet_process.go +++ b/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, diff --git a/go/test/endtoend/reparent/utils/utils.go b/go/test/endtoend/reparent/utils/utils.go index e84248df9f0..91eb3c3b9cc 100644 --- a/go/test/endtoend/reparent/utils/utils.go +++ b/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...) diff --git a/go/test/endtoend/vreplication/cluster_test.go b/go/test/endtoend/vreplication/cluster_test.go index a2f32fcebb8..bcf50d43702 100644 --- a/go/test/endtoend/vreplication/cluster_test.go +++ b/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, diff --git a/go/test/endtoend/vreplication/migrate_test.go b/go/test/endtoend/vreplication/migrate_test.go index d7d5339db2b..ef20d953781 100644 --- a/go/test/endtoend/vreplication/migrate_test.go +++ b/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 diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index d4987200af9..494614f2ce1 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/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,7 +4771,7 @@ 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, }) @@ -4778,7 +4779,8 @@ func (s *VtctldServer) ValidatePermissionsKeyspace(ctx context.Context, req *vtc 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 }