[release-18.0] Improve efficiency and accuracy of mysqld.GetVersionSt…

…ring (#15096) (#15111)

Signed-off-by: Matt Lord <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

go/vt/env/env.go

@@ -18,7 +18,6 @@ package env
 
 import (
 	"errors"
-	"fmt"
 	"os"
 	"os/exec"
 	"path"
@@ -30,9 +29,12 @@ const (
 	// DefaultVtDataRoot is the default value for VTROOT environment variable
 	DefaultVtDataRoot = "/vt"
 	// DefaultVtRoot is only required for hooks
-	DefaultVtRoot = "/usr/local/vitess"
+	DefaultVtRoot  = "/usr/local/vitess"
+	mysqldSbinPath = "/usr/sbin/mysqld"
 )
 
+var errMysqldNotFound = errors.New("VT_MYSQL_ROOT is not set and no mysqld could be found in your PATH")
+
 // VtRoot returns $VTROOT or tries to guess its value if it's not set.
 // This is the root for the 'vt' distribution, which contains bin/vttablet
 // for instance.
@@ -64,25 +66,30 @@ func VtDataRoot() string {
 }
 
 // VtMysqlRoot returns the root for the mysql distribution,
-// which contains bin/mysql CLI for instance.
-// If it is not set, look for mysqld in the path.
+// which contains the bin/mysql CLI for instance.
+// If $VT_MYSQL_ROOT is not set, look for mysqld in the $PATH.
 func VtMysqlRoot() (string, error) {
-	// if the environment variable is set, use that
+	// If the environment variable is set, use that.
 	if root := os.Getenv("VT_MYSQL_ROOT"); root != "" {
 		return root, nil
 	}
 
-	// otherwise let's look for mysqld in the PATH.
-	// ensure that /usr/sbin is included, as it might not be by default
-	// This is the default location for mysqld from packages.
-	newPath := fmt.Sprintf("/usr/sbin:%s", os.Getenv("PATH"))
-	os.Setenv("PATH", newPath)
-	path, err := exec.LookPath("mysqld")
+	getRoot := func(path string) string {
+		return filepath.Dir(filepath.Dir(path)) // Strip mysqld and [s]bin parts
+	}
+	binpath, err := exec.LookPath("mysqld")
 	if err != nil {
-		return "", errors.New("VT_MYSQL_ROOT is not set and no mysqld could be found in your PATH")
+		// First see if /usr/sbin/mysqld exists as it might not be in
+		// the PATH by default and this is often the default location
+		// used by mysqld OS system packages (apt, dnf, etc).
+		fi, err := os.Stat(mysqldSbinPath)
+		if err == nil /* file exists */ && fi.Mode().IsRegular() /* not a DIR or other special file */ &&
+			fi.Mode()&0111 != 0 /* executable by anyone */ {
+			return getRoot(mysqldSbinPath), nil
+		}
+		return "", errMysqldNotFound
 	}
-	path = filepath.Dir(filepath.Dir(path)) // strip mysqld, and the sbin
-	return path, nil
+	return getRoot(binpath), nil
 }
 
 // VtMysqlBaseDir returns the Mysql base directory, which

go/vt/env/env_test.go

@@ -18,7 +18,10 @@ package env
 
 import (
 	"os"
+	"path/filepath"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestVtDataRoot(t *testing.T) {
@@ -43,3 +46,82 @@ func TestVtDataRoot(t *testing.T) {
 		t.Errorf("The value of VtDataRoot should be %v, not %v.", passed, root)
 	}
 }
+
+func TestVtMysqlRoot(t *testing.T) {
+	envVar := "VT_MYSQL_ROOT"
+	originalMySQLRoot := os.Getenv(envVar)
+	defer os.Setenv(envVar, originalMySQLRoot)
+	originalPATH := os.Getenv("PATH")
+	defer os.Setenv("PATH", originalPATH)
+
+	// The test directory is used to create our fake mysqld binary.
+	testDir := t.TempDir() // This is automatically cleaned up
+	createExecutable := func(path string) error {
+		fullPath := testDir + path
+		err := os.MkdirAll(filepath.Dir(fullPath), 0755)
+		require.NoError(t, err)
+		return os.WriteFile(fullPath, []byte("test"), 0755)
+	}
+
+	type testcase struct {
+		name              string
+		preFunc           func() error
+		vtMysqlRootEnvVal string
+		pathEnvVal        string
+		expect            string // The return value we expect from VtMysqlRoot()
+		expectErr         string
+	}
+	testcases := []testcase{
+		{
+			name:              "VT_MYSQL_ROOT set",
+			vtMysqlRootEnvVal: "/home/mysql/binaries",
+		},
+		{
+			name: "VT_MYSQL_ROOT empty; PATH set without /usr/sbin",
+			pathEnvVal: testDir + filepath.Dir(mysqldSbinPath) +
+				":/usr/bin:/sbin:/bin:/usr/local/bin:/usr/local/sbin:/home/mysql/binaries",
+			preFunc: func() error {
+				return createExecutable(mysqldSbinPath)
+			},
+			expect: testDir + "/usr",
+		},
+	}
+
+	// If /usr/sbin/mysqld exists, confirm that we find it even
+	// when /usr/sbin is not in the PATH.
+	_, err := os.Stat(mysqldSbinPath)
+	if err == nil {
+		t.Logf("Found %s, confirming auto detection behavior", mysqldSbinPath)
+		testcases = append(testcases, testcase{
+			name:   "VT_MYSQL_ROOT empty; PATH empty; mysqld in /usr/sbin",
+			expect: "/usr",
+		})
+	} else {
+		testcases = append(testcases, testcase{ // Error expected
+			name:      "VT_MYSQL_ROOT empty; PATH empty; mysqld not in /usr/sbin",
+			expectErr: errMysqldNotFound.Error(),
+		})
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.preFunc != nil {
+				err := tc.preFunc()
+				require.NoError(t, err)
+			}
+			os.Setenv(envVar, tc.vtMysqlRootEnvVal)
+			os.Setenv("PATH", tc.pathEnvVal)
+			path, err := VtMysqlRoot()
+			if tc.expectErr != "" {
+				require.EqualError(t, err, tc.expectErr)
+			} else {
+				require.NoError(t, err)
+			}
+			if tc.vtMysqlRootEnvVal != "" {
+				// This should always be returned.
+				tc.expect = tc.vtMysqlRootEnvVal
+			}
+			require.Equal(t, tc.expect, path)
+		})
+	}
+}

go/vt/mysqlctl/mysqld.go

@@ -42,27 +42,32 @@ import (
 
 	"github.com/spf13/pflag"
 
-	"vitess.io/vitess/go/mysql/sqlerror"
-	"vitess.io/vitess/go/protoutil"
-
 	"vitess.io/vitess/config"
 	"vitess.io/vitess/go/mysql"
+	"vitess.io/vitess/go/mysql/sqlerror"
+	"vitess.io/vitess/go/protoutil"
 	"vitess.io/vitess/go/sqltypes"
 	"vitess.io/vitess/go/vt/dbconfigs"
 	"vitess.io/vitess/go/vt/dbconnpool"
+	vtenv "vitess.io/vitess/go/vt/env"
 	"vitess.io/vitess/go/vt/hook"
 	"vitess.io/vitess/go/vt/log"
 	"vitess.io/vitess/go/vt/mysqlctl/mysqlctlclient"
 	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/vterrors"
 
-	vtenv "vitess.io/vitess/go/vt/env"
 	mysqlctlpb "vitess.io/vitess/go/vt/proto/mysqlctl"
-	"vitess.io/vitess/go/vt/proto/vtrpc"
+	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 )
 
-var (
+// The string we expect before the MySQL version number
+// in strings containing MySQL version information.
+const versionStringPrefix = "Ver "
 
+// How many bytes from MySQL error log to sample for error messages
+const maxLogFileSampleSize = 4096
+
+var (
 	// DisableActiveReparents is a flag to disable active
 	// reparents for safety reasons. It is used in three places:
 	// 1. in this file to skip registering the commands.
@@ -86,15 +91,18 @@ var (
 
 	replicationConnectRetry = 10 * time.Second
 
-	versionRegex = regexp.MustCompile(`Ver ([0-9]+)\.([0-9]+)\.([0-9]+)`)
+	versionRegex = regexp.MustCompile(fmt.Sprintf(`%s([0-9]+)\.([0-9]+)\.([0-9]+)`, versionStringPrefix))
+	// versionSQLQuery will return a version string directly from
+	// a MySQL server that is compatible with what we expect from
+	// mysqld --version and matches the versionRegex. Example
+	// result: Ver 8.0.35 MySQL Community Server - GPL
+	versionSQLQuery = fmt.Sprintf("select concat('%s', @@global.version, ' ', @@global.version_comment) as version",
+		versionStringPrefix)
 
 	binlogEntryCommittedTimestampRegex = regexp.MustCompile("original_committed_timestamp=([0-9]+)")
 	binlogEntryTimestampGTIDRegexp     = regexp.MustCompile(`^#(.+) server id.*\bGTID\b`)
 )
 
-// How many bytes from MySQL error log to sample for error messages
-const maxLogFileSampleSize = 4096
-
 // Mysqld is the object that represents a mysqld daemon running on this server.
 type Mysqld struct {
 	dbcfgs  *dbconfigs.DBConfigs
@@ -1136,7 +1144,13 @@ func buildLdPaths() ([]string, error) {
 
 // GetVersionString is part of the MysqlExecutor interface.
 func (mysqld *Mysqld) GetVersionString(ctx context.Context) (string, error) {
-	// Execute as remote action on mysqlctld to ensure we get the actual running MySQL version.
+	// Try to query the mysqld instance directly.
+	qr, err := mysqld.FetchSuperQuery(ctx, versionSQLQuery)
+	if err == nil && len(qr.Rows) == 1 {
+		return qr.Rows[0][0].ToString(), nil
+	}
+	// Execute as remote action on mysqlctld to use the actual running MySQL
+	// version.
 	if socketFile != "" {
 		client, err := mysqlctlclient.New("unix", socketFile)
 		if err != nil {
@@ -1145,6 +1159,7 @@ func (mysqld *Mysqld) GetVersionString(ctx context.Context) (string, error) {
 		defer client.Close()
 		return client.VersionString(ctx)
 	}
+	// Fall back to the sys exec method using mysqld --version.
 	return GetVersionString()
 }
 
@@ -1379,7 +1394,7 @@ func (mysqld *Mysqld) scanBinlogTimestamp(
 // ReadBinlogFilesTimestamps reads all given binlog files via `mysqlbinlog` command and returns the first and last  found transaction timestamps
 func (mysqld *Mysqld) ReadBinlogFilesTimestamps(ctx context.Context, req *mysqlctlpb.ReadBinlogFilesTimestampsRequest) (*mysqlctlpb.ReadBinlogFilesTimestampsResponse, error) {
 	if len(req.BinlogFileNames) == 0 {
-		return nil, vterrors.Errorf(vtrpc.Code_INVALID_ARGUMENT, "empty binlog list in ReadBinlogFilesTimestampsRequest")
+		return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "empty binlog list in ReadBinlogFilesTimestampsRequest")
 	}
 	if socketFile != "" {
 		log.Infof("executing Mysqld.ReadBinlogFilesTimestamps() remotely via mysqlctld server: %v", socketFile)