mysqlctl: Remove usage of MYSQL_FLAVOR (#13135)

* mysqlctl: Remove usage of MYSQL_FLAVOR

After the changes in #13123, I
realized that there's no cases left where we actually use or depend on
`MYSQL_FLAVOR`.

This means we can actually remove usages of `MYSQL_FLAVOR`. It doesn't
yet remove it from the artifacts we build, because we shouldn't clean
that up until v18 then because users might mix versions during the
upgrade and we don't want to have old code that still depends on this
fail then.

Signed-off-by: Dirkjan Bussink <[email protected]>

* Add release note item for removed `MYSQL_FLAVOR`.

Signed-off-by: Dirkjan Bussink <[email protected]>

---------

Signed-off-by: Dirkjan Bussink <[email protected]>

changelog/17.0/17.0.0/summary.md

@@ -284,6 +284,7 @@ vttablet_v_replication_stream_state{counts="1",state="Running",workflow="commerc
 * Auto-population of DDL revert actions and tables at execution-time has been removed. This is now handled entirely at enqueue-time.
 * Backwards-compatibility for failed migrations without a `completed_timestamp` has been removed (see https://github.com/vitessio/vitess/issues/8499).
 * The deprecated `Key`, `Name`, `Up`, and `TabletExternallyReparentedTimestamp` fields were removed from the JSON representation of `TabletHealth` structures.
+* The `MYSQL_FLAVOR` environment variable is no longer used.
 
 ### <a id="deprecated-flags"/>Deprecated Command Line Flags
 

go/mysql/endtoend/main_test.go

@@ -66,27 +66,13 @@ func runMysql(t *testing.T, params *mysql.ConnParams, command string) (string, b
 	// In particular, it has the message:
 	// Query OK, 1 row affected (0.00 sec)
 
-	version, getErr := mysqlctl.GetVersionString()
+	version, err := mysqlctl.GetVersionString()
+	if err != nil {
+		failVersionDetection(err)
+	}
 	f, v, err := mysqlctl.ParseVersionString(version)
-
-	if getErr != nil || err != nil {
-		f, v, err = mysqlctl.GetVersionFromEnv()
-		if err != nil {
-			vtenvMysqlRoot, _ := vtenv.VtMysqlRoot()
-			message := fmt.Sprintf(`could not auto-detect MySQL version. You may need to set your PATH so a mysqld binary can be found, or set the environment variable MYSQL_FLAVOR if mysqld is not available locally:
-	PATH: %s
-	VT_MYSQL_ROOT: %s
-	VTROOT: %s
-	vtenv.VtMysqlRoot(): %s
-	MYSQL_FLAVOR: %s
-	`,
-				os.Getenv("PATH"),
-				os.Getenv("VT_MYSQL_ROOT"),
-				os.Getenv("VTROOT"),
-				vtenvMysqlRoot,
-				os.Getenv("MYSQL_FLAVOR"))
-			panic(message)
-		}
+	if err != nil {
+		failVersionDetection(err)
 	}
 
 	t.Logf("Using flavor: %v, version: %v", f, v)
@@ -237,3 +223,20 @@ ssl-key=%v/server-key.pem
 	}()
 	os.Exit(exitCode)
 }
+
+func failVersionDetection(err error) {
+	vtenvMysqlRoot, _ := vtenv.VtMysqlRoot()
+	message := fmt.Sprintf(`could not auto-detect MySQL version: %v
+You may need to set your PATH so a mysqld binary can be found:
+	PATH: %s
+	VT_MYSQL_ROOT: %s
+	VTROOT: %s
+	vtenv.VtMysqlRoot(): %s
+	`,
+		err,
+		os.Getenv("PATH"),
+		os.Getenv("VT_MYSQL_ROOT"),
+		os.Getenv("VTROOT"),
+		vtenvMysqlRoot)
+	panic(message)
+}

go/test/endtoend/mysqlctl/mysqlctl_test.go

@@ -150,10 +150,6 @@ func TestRestart(t *testing.T) {
 func TestAutoDetect(t *testing.T) {
 	defer cluster.PanicHandler(t)
 
-	// Start up tablets with an empty MYSQL_FLAVOR, which means auto-detect
-	sqlFlavor := os.Getenv("MYSQL_FLAVOR")
-	os.Setenv("MYSQL_FLAVOR", "")
-
 	err := clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].VttabletProcess.Setup()
 	require.Nil(t, err, "error should be nil")
 	err = clusterInstance.Keyspaces[0].Shards[0].Vttablets[1].VttabletProcess.Setup()
@@ -162,8 +158,4 @@ func TestAutoDetect(t *testing.T) {
 	// Reparent tablets, which requires flavor detection
 	err = clusterInstance.VtctlclientProcess.InitializeShard(keyspaceName, shardName, cell, primaryTablet.TabletUID)
 	require.Nil(t, err, "error should be nil")
-
-	//Reset flavor
-	os.Setenv("MYSQL_FLAVOR", sqlFlavor)
-
 }

go/test/endtoend/mysqlctld/mysqlctld_test.go

@@ -147,10 +147,6 @@ func TestRestart(t *testing.T) {
 func TestAutoDetect(t *testing.T) {
 	defer cluster.PanicHandler(t)
 
-	// Start up tablets with an empty MYSQL_FLAVOR, which means auto-detect
-	sqlFlavor := os.Getenv("MYSQL_FLAVOR")
-	os.Setenv("MYSQL_FLAVOR", "")
-
 	err := clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].VttabletProcess.Setup()
 	require.Nil(t, err, "error should be nil")
 	err = clusterInstance.Keyspaces[0].Shards[0].Vttablets[1].VttabletProcess.Setup()
@@ -159,8 +155,4 @@ func TestAutoDetect(t *testing.T) {
 	// Reparent tablets, which requires flavor detection
 	err = clusterInstance.VtctlclientProcess.InitializeShard(keyspaceName, shardName, cell, primaryTablet.TabletUID)
 	require.Nil(t, err, "error should be nil")
-
-	//Reset flavor
-	os.Setenv("MYSQL_FLAVOR", sqlFlavor)
-
 }

go/test/endtoend/vreplication/cluster_test.go

@@ -178,37 +178,10 @@ func setVtMySQLRoot(mysqlRoot string) error {
 	return nil
 }
 
-// setDBFlavor sets the MYSQL_FLAVOR OS env var.
-// You should call this after calling setVtMySQLRoot() to ensure that the
-// correct flavor is used by mysqlctl based on the current mysqld version
-// in the path. If you don't do this then mysqlctl will use the incorrect
-// config/mycnf/<flavor>.cnf file and mysqld may fail to start.
-func setDBFlavor() error {
-	versionStr, err := mysqlctl.GetVersionString()
-	if err != nil {
-		return err
-	}
-	f, v, err := mysqlctl.ParseVersionString(versionStr)
-	if err != nil {
-		return err
-	}
-	flavor := fmt.Sprintf("%s%d%d", f, v.Major, v.Minor)
-	err = os.Setenv("MYSQL_FLAVOR", string(flavor))
-	if err != nil {
-		return err
-	}
-	fmt.Printf("MYSQL_FLAVOR is %s\n", string(flavor))
-	return nil
-}
-
 func unsetVtMySQLRoot() {
 	_ = os.Unsetenv("VT_MYSQL_ROOT")
 }
 
-func unsetDBFlavor() {
-	_ = os.Unsetenv("MYSQL_FLAVOR")
-}
-
 // getDBTypeVersionInUse checks the major DB version of the mysqld binary
 // that mysqlctl would currently use, e.g. 5.7 or 8.0 (in semantic versioning
 // this would be major.minor but in MySQL it's effectively the major version).
@@ -781,12 +754,7 @@ func setupDBTypeVersion(t *testing.T, value string) func() {
 	if err := downloadDBTypeVersion(dbType, majorVersion, path); err != nil {
 		t.Fatalf("Could not download %s, error: %v", majorVersion, err)
 	}
-	// Set the MYSQL_FLAVOR OS ENV var for mysqlctl to use the correct config file
-	if err := setDBFlavor(); err != nil {
-		t.Fatalf("Could not set MYSQL_FLAVOR: %v", err)
-	}
 	return func() {
-		unsetDBFlavor()
 		unsetVtMySQLRoot()
 	}
 }

go/vt/mysqlctl/mysqld.go

@@ -145,93 +145,39 @@ func NewMysqld(dbcfgs *dbconfigs.DBConfigs) *Mysqld {
 	result.appPool.Open(dbcfgs.AppWithDB())
 
 	/*
-	 Unmanaged tablets are special because the MYSQL_FLAVOR detection
-	 will not be accurate because the mysqld might not be the same
-	 one as the server started.
-
-	 This skips the panic that checks that we can detect a server,
-	 but also relies on none of the flavor detection features being
-	 used at runtime. Currently this assumption is guaranteed true.
+	 If we have an external unmanaged tablet, we can't do the flavor
+	 detection here. We also won't need it, since mysqlctl itself is the only
+	 one that needs capabilities and the flavor.
 	*/
 	if dbconfigs.GlobalDBConfigs.HasGlobalSettings() {
 		log.Info("mysqld is unmanaged or remote. Skipping flavor detection")
 		return result
 	}
 
+	/*
+	 If we have a socketFile here, it means we're not running inside mysqlctl.
+	 This means we don't need the flavor and capability detection, since mysqlctl
+	 itself is the only one that needs this.
+	*/
 	if socketFile != "" {
 		log.Info("mysqld is remote. Skipping flavor detection")
 		return result
 	}
 
-	version, getErr := GetVersionString()
+	version, err := GetVersionString()
+	if err != nil {
+		failVersionDetection(err)
+	}
 	f, v, err := ParseVersionString(version)
-
-	/*
-	 By default Vitess searches in vtenv.VtMysqlRoot() for a mysqld binary.
-	 This is historically the VT_MYSQL_ROOT env, but if it is unset or empty,
-	 Vitess will search the PATH. See go/vt/env/env.go.
-
-	 A number of subdirs inside vtenv.VtMysqlRoot() will be searched, see
-	 func binaryPath() for context. If no mysqld binary is found (possibly
-	 because it is in a container or both VT_MYSQL_ROOT and VTROOT are set
-	 incorrectly), there will be a fallback to using the MYSQL_FLAVOR env
-	 variable.
-
-	 If MYSQL_FLAVOR is not defined, there will be a panic.
-
-	 Note: relying on MySQL_FLAVOR is not recommended, since for historical
-	 purposes "MySQL56" actually means MySQL 5.7, which is a very strange
-	 behavior.
-	*/
-
-	if getErr != nil || err != nil {
-		f, v, err = GetVersionFromEnv()
-		if err != nil {
-			vtenvMysqlRoot, _ := vtenv.VtMysqlRoot()
-			message := fmt.Sprintf(`could not auto-detect MySQL version. You may need to set your PATH so a mysqld binary can be found, or set the environment variable MYSQL_FLAVOR if mysqld is not available locally:
-	PATH: %s
-	VT_MYSQL_ROOT: %s
-	VTROOT: %s
-	vtenv.VtMysqlRoot(): %s
-	MYSQL_FLAVOR: %s
-	`,
-				os.Getenv("PATH"),
-				os.Getenv("VT_MYSQL_ROOT"),
-				os.Getenv("VTROOT"),
-				vtenvMysqlRoot,
-				os.Getenv("MYSQL_FLAVOR"))
-			panic(message)
-		}
+	if err != nil {
+		failVersionDetection(err)
 	}
 
 	log.Infof("Using flavor: %v, version: %v", f, v)
 	result.capabilities = newCapabilitySet(f, v)
 	return result
 }
 
-/*
-GetVersionFromEnv returns the flavor and an assumed version based on the legacy
-MYSQL_FLAVOR environment variable.
-
-The assumed version may not be accurate since the legacy variable only specifies
-broad families of compatible versions. However, the differences between those
-versions should only matter if Vitess is managing the lifecycle of mysqld, in which
-case we should have a local copy of the mysqld binary from which we can fetch
-the accurate version instead of falling back to this function (see GetVersionString).
-*/
-func GetVersionFromEnv() (flavor MySQLFlavor, ver ServerVersion, err error) {
-	env := os.Getenv("MYSQL_FLAVOR")
-	switch env {
-	case "MariaDB":
-		return FlavorMariaDB, ServerVersion{10, 6, 11}, nil
-	case "MySQL80":
-		return FlavorMySQL, ServerVersion{8, 0, 11}, nil
-	case "MySQL56":
-		return FlavorMySQL, ServerVersion{5, 7, 10}, nil
-	}
-	return flavor, ver, fmt.Errorf("could not determine version from MYSQL_FLAVOR: %s", env)
-}
-
 // GetVersionString runs mysqld --version and returns its output as a string
 func GetVersionString() (string, error) {
 	noSocketFile()
@@ -1332,3 +1278,20 @@ func noSocketFile() {
 		panic("Running remotely through mysqlctl, socketFile must not be set")
 	}
 }
+
+func failVersionDetection(err error) {
+	vtenvMysqlRoot, _ := vtenv.VtMysqlRoot()
+	message := fmt.Sprintf(`could not auto-detect MySQL version: %v
+You may need to set your PATH so a mysqld binary can be found:
+	PATH: %s
+	VT_MYSQL_ROOT: %s
+	VTROOT: %s
+	vtenv.VtMysqlRoot(): %s
+	`,
+		err,
+		os.Getenv("PATH"),
+		os.Getenv("VT_MYSQL_ROOT"),
+		os.Getenv("VTROOT"),
+		vtenvMysqlRoot)
+	panic(message)
+}

go/vt/mysqlctl/mysqld_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package mysqlctl
 
 import (
-	"os"
 	"testing"
 )
 
@@ -106,36 +105,3 @@ func TestParseVersionString(t *testing.T) {
 	}
 
 }
-
-func TestAssumeVersionString(t *testing.T) {
-
-	// In these cases, the versionstring is nonsensical or unspecified.
-	// MYSQL_FLAVOR is used instead.
-
-	var testcases = []testcase{
-		{
-			versionString: "MySQL80",
-			version:       ServerVersion{8, 0, 11},
-			flavor:        FlavorMySQL,
-		},
-		{
-			versionString: "MySQL56",
-			version:       ServerVersion{5, 7, 10}, // Yes, this has to lie!
-			flavor:        FlavorMySQL,             // There was no MySQL57 option
-		},
-		{
-			versionString: "MariaDB",
-			version:       ServerVersion{10, 6, 11},
-			flavor:        FlavorMariaDB,
-		},
-	}
-
-	for _, testcase := range testcases {
-		os.Setenv("MYSQL_FLAVOR", testcase.versionString)
-		f, v, err := GetVersionFromEnv()
-		if v != testcase.version || f != testcase.flavor || err != nil {
-			t.Errorf("GetVersionFromEnv() failed for: %#v, Got: %#v, %#v Expected: %#v, %#v", testcase.versionString, v, f, testcase.version, testcase.flavor)
-		}
-	}
-
-}

go/vt/vttest/environment.go

@@ -103,21 +103,16 @@ type LocalTestEnv struct {
 	Env          []string
 }
 
-// DefaultMySQLFlavor is the MySQL flavor used by vttest when MYSQL_FLAVOR is not
-// set in the environment
+// DefaultMySQLFlavor is the MySQL flavor used by vttest when no explicit
+// flavor is given.
 const DefaultMySQLFlavor = "MySQL56"
 
 // GetMySQLOptions returns the default option set for the given MySQL
-// flavor. If flavor is not set, the value from the `MYSQL_FLAVOR` env
-// variable is used, and if this is not set, DefaultMySQLFlavor will
+// flavor. If flavor is not set, DefaultMySQLFlavor will
 // be used.
 // Returns the name of the MySQL flavor being used, the set of MySQL CNF
 // files specific to this flavor, and any errors.
 func GetMySQLOptions(flavor string) (string, []string, error) {
-	if flavor == "" {
-		flavor = os.Getenv("MYSQL_FLAVOR")
-	}
-
 	if flavor == "" {
 		flavor = DefaultMySQLFlavor
 	}
@@ -237,8 +232,7 @@ func randomPort() int {
 // up when closing the Environment.
 // - LogDirectory() is the `logs` subdir inside Directory()
 // - The MySQL flavor is set to `flavor`. If the argument is not set, it will
-// default to the value of MYSQL_FLAVOR, and if this variable is not set, to
-// DefaultMySQLFlavor
+// default DefaultMySQLFlavor
 // - PortForProtocol() will return ports based off the given basePort. If basePort
 // is zero, a random port between 10000 and 20000 will be chosen.
 // - DefaultProtocol() is always "grpc"