From 42585fd40c6161618468147bc00209cd263924ee Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Thu, 13 Jul 2023 10:03:33 +0200 Subject: [PATCH] Fix remote VersionString API (#13484) * Fix potential nil crash If this errors, we can't derefence v and read this value. This was broken in https://github.com/vitessio/vitess/pull/13449 Signed-off-by: Dirkjan Bussink * muysqlctl: Ensure to implement server side API Signed-off-by: Dirkjan Bussink * mysqlctld: Add endtoend test to verify the version string works Signed-off-by: Dirkjan Bussink --------- Signed-off-by: Dirkjan Bussink --- go/test/endtoend/cluster/mysqlctld_process.go | 21 +++++++++++++++---- go/test/endtoend/mysqlctld/mysqlctld_test.go | 13 ++++++++++++ go/vt/mysqlctl/grpcmysqlctlclient/client.go | 5 ++++- go/vt/mysqlctl/grpcmysqlctlserver/server.go | 9 ++++++++ 4 files changed, 43 insertions(+), 5 deletions(-) diff --git a/go/test/endtoend/cluster/mysqlctld_process.go b/go/test/endtoend/cluster/mysqlctld_process.go index 8aa3204b6eb..4f51e8ca888 100644 --- a/go/test/endtoend/cluster/mysqlctld_process.go +++ b/go/test/endtoend/cluster/mysqlctld_process.go @@ -43,17 +43,24 @@ type MysqlctldProcess struct { process *exec.Cmd exit chan error InitMysql bool + SocketFile string exitSignalReceived bool } // InitDb executes mysqlctld command to add cell info func (mysqlctld *MysqlctldProcess) InitDb() (err error) { - tmpProcess := exec.Command( - mysqlctld.Binary, + args := []string{ "--log_dir", mysqlctld.LogDirectory, "--tablet_uid", fmt.Sprintf("%d", mysqlctld.TabletUID), "--mysql_port", fmt.Sprintf("%d", mysqlctld.MySQLPort), "--init_db_sql_file", mysqlctld.InitDBFile, + } + if mysqlctld.SocketFile != "" { + args = append(args, "--socket_file", mysqlctld.SocketFile) + } + tmpProcess := exec.Command( + mysqlctld.Binary, + args..., ) return tmpProcess.Run() } @@ -64,11 +71,17 @@ func (mysqlctld *MysqlctldProcess) Start() error { return fmt.Errorf("process is already running") } _ = createDirectory(mysqlctld.LogDirectory, 0700) - tempProcess := exec.Command( - mysqlctld.Binary, + args := []string{ "--log_dir", mysqlctld.LogDirectory, "--tablet_uid", fmt.Sprintf("%d", mysqlctld.TabletUID), "--mysql_port", fmt.Sprintf("%d", mysqlctld.MySQLPort), + } + if mysqlctld.SocketFile != "" { + args = append(args, "--socket_file", mysqlctld.SocketFile) + } + tempProcess := exec.Command( + mysqlctld.Binary, + args..., ) tempProcess.Args = append(tempProcess.Args, mysqlctld.ExtraArgs...) diff --git a/go/test/endtoend/mysqlctld/mysqlctld_test.go b/go/test/endtoend/mysqlctld/mysqlctld_test.go index b73efccdba8..a8da1ccb948 100644 --- a/go/test/endtoend/mysqlctld/mysqlctld_test.go +++ b/go/test/endtoend/mysqlctld/mysqlctld_test.go @@ -17,13 +17,17 @@ limitations under the License. package mysqlctld import ( + "context" "flag" "fmt" "os" + "path" "testing" "github.com/stretchr/testify/require" + "vitess.io/vitess/go/vt/mysqlctl/mysqlctlclient" + "vitess.io/vitess/go/test/endtoend/cluster" "vitess.io/vitess/go/vt/sidecardb" ) @@ -101,6 +105,7 @@ func initCluster(shardNames []string, totalTabletsRequired int) error { if err != nil { return err } + mysqlctldProcess.SocketFile = path.Join(clusterInstance.TmpDirectory, fmt.Sprintf("mysqlctld_%d.sock", tablet.TabletUID)) tablet.MysqlctldProcess = *mysqlctldProcess err = tablet.MysqlctldProcess.Start() if err != nil { @@ -156,3 +161,11 @@ func TestAutoDetect(t *testing.T) { err = clusterInstance.VtctlclientProcess.InitializeShard(keyspaceName, shardName, cell, primaryTablet.TabletUID) require.Nil(t, err, "error should be nil") } + +func TestVersionString(t *testing.T) { + client, err := mysqlctlclient.New("unix", primaryTablet.MysqlctldProcess.SocketFile) + require.NoError(t, err) + version, err := client.VersionString(context.Background()) + require.NoError(t, err) + require.NotEmpty(t, version) +} diff --git a/go/vt/mysqlctl/grpcmysqlctlclient/client.go b/go/vt/mysqlctl/grpcmysqlctlclient/client.go index 449e2e8ceef..26fd2f2aeba 100644 --- a/go/vt/mysqlctl/grpcmysqlctlclient/client.go +++ b/go/vt/mysqlctl/grpcmysqlctlclient/client.go @@ -116,8 +116,11 @@ func (c *client) VersionString(ctx context.Context) (string, error) { var version string err := c.withRetry(ctx, func() error { r, err := c.c.VersionString(ctx, &mysqlctlpb.VersionStringRequest{}) + if err != nil { + return err + } version = r.Version - return err + return nil }) return version, err } diff --git a/go/vt/mysqlctl/grpcmysqlctlserver/server.go b/go/vt/mysqlctl/grpcmysqlctlserver/server.go index 898d06f1a96..0f98a1d72f6 100644 --- a/go/vt/mysqlctl/grpcmysqlctlserver/server.go +++ b/go/vt/mysqlctl/grpcmysqlctlserver/server.go @@ -71,6 +71,15 @@ func (s *server) RefreshConfig(ctx context.Context, request *mysqlctlpb.RefreshC return &mysqlctlpb.RefreshConfigResponse{}, s.mysqld.RefreshConfig(ctx, s.cnf) } +// VersionString registers the Server for RPCs. +func (s *server) VersionString(ctx context.Context, request *mysqlctlpb.VersionStringRequest) (*mysqlctlpb.VersionStringResponse, error) { + version, err := s.mysqld.GetVersionString(ctx) + if err != nil { + return nil, err + } + return &mysqlctlpb.VersionStringResponse{Version: version}, nil +} + // StartServer registers the Server for RPCs. func StartServer(s *grpc.Server, cnf *mysqlctl.Mycnf, mysqld *mysqlctl.Mysqld) { mysqlctlpb.RegisterMysqlCtlServer(s, &server{cnf: cnf, mysqld: mysqld})