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 #13449 Signed-off-by: Dirkjan Bussink <[email protected]> * muysqlctl: Ensure to implement server side API Signed-off-by: Dirkjan Bussink <[email protected]> * mysqlctld: Add endtoend test to verify the version string works Signed-off-by: Dirkjan Bussink <[email protected]> --------- Signed-off-by: Dirkjan Bussink <[email protected]>
vitessio · Jul 13, 2023 · 42585fd · 42585fd
1 parent 0babfe2
commit 42585fd
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 5 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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})