PR feedback

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

docker/lite/Dockerfile

@@ -42,7 +42,7 @@ RUN /vt/dist/install_dependencies.sh mysql80
 
 # Set up Vitess user and directory tree.
 RUN groupadd -r vitess && useradd -r -g vitess vitess
-RUN mkdir -p /vt/vtdataroot && chown -R vitess:vitess /vt
+RUN mkdir -p /vt/vtdataroot /home/vitess && chown -R vitess:vitess /vt /home/vitess
 
 # Set up Vitess environment (just enough to run pre-built Go binaries)
 ENV VTROOT /vt/src/vitess.io/vitess

docker/utils/install_dependencies.sh

@@ -86,6 +86,7 @@ mysql57)
         /tmp/mysql-client_${VERSION}-1debian10_amd64.deb
         /tmp/mysql-community-server_${VERSION}-1debian10_amd64.deb
         /tmp/mysql-server_${VERSION}-1debian10_amd64.deb
+        mysql-shell
         percona-xtrabackup-24
     )
     ;;
@@ -112,6 +113,7 @@ mysql80)
         /tmp/mysql-community-server-core_${VERSION}-1debian11_amd64.deb
         /tmp/mysql-community-server_${VERSION}-1debian11_amd64.deb
         /tmp/mysql-server_${VERSION}-1debian11_amd64.deb
+        mysql-shell
         percona-xtrabackup-80
     )
     ;;

go/flags/endtoend/vtbackup.txt

@@ -178,12 +178,12 @@ Flags:
       --mysql-shutdown-timeout duration                             how long to wait for mysqld shutdown (default 5m0s)
       --mysql_port int                                              mysql port (default 3306)
       --mysql_server_version string                                 MySQL server version to advertise. (default "8.0.30-Vitess")
-      --mysql_shell_backup_location string                          location where the backup will be stored
-      --mysql_shell_dump_flags string                               flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST (default "{\"threads\": 2}")
-      --mysql_shell_flags string                                    execution flags to pass to mysqlsh binary to be used during dump/load (default "--defaults-file=/dev/null --js -h localhost")
-      --mysql_shell_load_flags string                               flags to pass to mysql shell load utility. This should be a JSON string (default "{\"threads\": 4, \"updateGtidSet\": \"replace\", \"skipBinlog\": true, \"progressFile\": \"\"}")
-      --mysql_shell_should_drain                                    decide if we should drain while taking a backup or continue to serving traffic
-      --mysql_shell_speedup_restore                                 speed up restore by disabling redo logging and double write buffer during the restore process
+      --mysql-shell-backup-location string                          location where the backup will be stored
+      --mysql-shell-dump-flags string                               flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST (default "{\"threads\": 2}")
+      --mysql-shell-flags string                                    execution flags to pass to mysqlsh binary to be used during dump/load (default "--defaults-file=/dev/null --js -h localhost")
+      --mysql-shell-load-flags string                               flags to pass to mysql shell load utility. This should be a JSON string (default "{\"threads\": 4, \"updateGtidSet\": \"replace\", \"skipBinlog\": true, \"progressFile\": \"\"}")
+      --mysql-shell-should-drain                                    decide if we should drain while taking a backup or continue to serving traffic
+      --mysql-shell-speedup-restore                                 speed up restore by disabling redo logging and double write buffer during the restore process
       --mysql_socket string                                         path to the mysql socket
       --mysql_timeout duration                                      how long to wait for mysqld startup (default 5m0s)
       --opentsdb_uri string                                         URI of opentsdb /api/put method

go/flags/endtoend/vtcombo.txt

@@ -245,12 +245,12 @@ Flags:
       --mysql_server_tls_min_version string                              Configures the minimal TLS version negotiated when SSL is enabled. Defaults to TLSv1.2. Options: TLSv1.0, TLSv1.1, TLSv1.2, TLSv1.3.
       --mysql_server_version string                                      MySQL server version to advertise. (default "8.0.30-Vitess")
       --mysql_server_write_timeout duration                              connection write timeout
-      --mysql_shell_backup_location string                               location where the backup will be stored
-      --mysql_shell_dump_flags string                                    flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST (default "{\"threads\": 2}")
-      --mysql_shell_flags string                                         execution flags to pass to mysqlsh binary to be used during dump/load (default "--defaults-file=/dev/null --js -h localhost")
-      --mysql_shell_load_flags string                                    flags to pass to mysql shell load utility. This should be a JSON string (default "{\"threads\": 4, \"updateGtidSet\": \"replace\", \"skipBinlog\": true, \"progressFile\": \"\"}")
-      --mysql_shell_should_drain                                         decide if we should drain while taking a backup or continue to serving traffic
-      --mysql_shell_speedup_restore                                      speed up restore by disabling redo logging and double write buffer during the restore process
+      --mysql-shell-backup-location string                               location where the backup will be stored
+      --mysql-shell-dump-flags string                                    flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST (default "{\"threads\": 2}")
+      --mysql-shell-flags string                                         execution flags to pass to mysqlsh binary to be used during dump/load (default "--defaults-file=/dev/null --js -h localhost")
+      --mysql-shell-load-flags string                                    flags to pass to mysql shell load utility. This should be a JSON string (default "{\"threads\": 4, \"updateGtidSet\": \"replace\", \"skipBinlog\": true, \"progressFile\": \"\"}")
+      --mysql-shell-should-drain                                         decide if we should drain while taking a backup or continue to serving traffic
+      --mysql-shell-speedup-restore                                      speed up restore by disabling redo logging and double write buffer during the restore process
       --mysql_slow_connect_warn_threshold duration                       Warn if it takes more than the given threshold for a mysql connection to establish
       --mysql_tcp_version string                                         Select tcp, tcp4, or tcp6 to control the socket type. (default "tcp")
       --mysqlctl_mycnf_template string                                   template file to use for generating the my.cnf file during server init

go/flags/endtoend/vttablet.txt

@@ -245,12 +245,12 @@ Flags:
       --mycnf_tmp_dir string                                             mysql tmp directory
       --mysql-shutdown-timeout duration                                  timeout to use when MySQL is being shut down. (default 5m0s)
       --mysql_server_version string                                      MySQL server version to advertise. (default "8.0.30-Vitess")
-      --mysql_shell_backup_location string                               location where the backup will be stored
-      --mysql_shell_dump_flags string                                    flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST (default "{\"threads\": 2}")
-      --mysql_shell_flags string                                         execution flags to pass to mysqlsh binary to be used during dump/load (default "--defaults-file=/dev/null --js -h localhost")
-      --mysql_shell_load_flags string                                    flags to pass to mysql shell load utility. This should be a JSON string (default "{\"threads\": 4, \"updateGtidSet\": \"replace\", \"skipBinlog\": true, \"progressFile\": \"\"}")
-      --mysql_shell_should_drain                                         decide if we should drain while taking a backup or continue to serving traffic
-      --mysql_shell_speedup_restore                                      speed up restore by disabling redo logging and double write buffer during the restore process
+      --mysql-shell-backup-location string                               location where the backup will be stored
+      --mysql-shell-dump-flags string                                    flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST (default "{\"threads\": 2}")
+      --mysql-shell-flags string                                         execution flags to pass to mysqlsh binary to be used during dump/load (default "--defaults-file=/dev/null --js -h localhost")
+      --mysql-shell-load-flags string                                    flags to pass to mysql shell load utility. This should be a JSON string (default "{\"threads\": 4, \"updateGtidSet\": \"replace\", \"skipBinlog\": true, \"progressFile\": \"\"}")
+      --mysql-shell-should-drain                                         decide if we should drain while taking a backup or continue to serving traffic
+      --mysql-shell-speedup-restore                                      speed up restore by disabling redo logging and double write buffer during the restore process
       --mysqlctl_mycnf_template string                                   template file to use for generating the my.cnf file during server init
       --mysqlctl_socket string                                           socket file to use for remote mysqlctl actions (empty for local actions)
       --onclose_timeout duration                                         wait no more than this for OnClose handlers before stopping (default 10s)

go/flags/endtoend/vttestserver.txt

@@ -90,12 +90,12 @@ Flags:
       --mysql_bind_host string                                           which host to bind vtgate mysql listener to (default "localhost")
       --mysql_only                                                       If this flag is set only mysql is initialized. The rest of the vitess components are not started. Also, the output specifies the mysql unix socket instead of the vtgate port.
       --mysql_server_version string                                      MySQL server version to advertise. (default "8.0.30-Vitess")
-      --mysql_shell_backup_location string                               location where the backup will be stored
-      --mysql_shell_dump_flags string                                    flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST (default "{\"threads\": 2}")
-      --mysql_shell_flags string                                         execution flags to pass to mysqlsh binary to be used during dump/load (default "--defaults-file=/dev/null --js -h localhost")
-      --mysql_shell_load_flags string                                    flags to pass to mysql shell load utility. This should be a JSON string (default "{\"threads\": 4, \"updateGtidSet\": \"replace\", \"skipBinlog\": true, \"progressFile\": \"\"}")
-      --mysql_shell_should_drain                                         decide if we should drain while taking a backup or continue to serving traffic
-      --mysql_shell_speedup_restore                                      speed up restore by disabling redo logging and double write buffer during the restore process
+      --mysql-shell-backup-location string                               location where the backup will be stored
+      --mysql-shell-dump-flags string                                    flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST (default "{\"threads\": 2}")
+      --mysql-shell-flags string                                         execution flags to pass to mysqlsh binary to be used during dump/load (default "--defaults-file=/dev/null --js -h localhost")
+      --mysql-shell-load-flags string                                    flags to pass to mysql shell load utility. This should be a JSON string (default "{\"threads\": 4, \"updateGtidSet\": \"replace\", \"skipBinlog\": true, \"progressFile\": \"\"}")
+      --mysql-shell-should-drain                                         decide if we should drain while taking a backup or continue to serving traffic
+      --mysql-shell-speedup-restore                                      speed up restore by disabling redo logging and double write buffer during the restore process
       --mysqlctl_mycnf_template string                                   template file to use for generating the my.cnf file during server init
       --mysqlctl_socket string                                           socket file to use for remote mysqlctl actions (empty for local actions)
       --no_scatter                                                       when set to true, the planner will fail instead of producing a plan that includes scatter queries

go/vt/mysqlctl/mysqlshellbackupengine.go

@@ -13,6 +13,8 @@ import (
 
 	"github.com/spf13/pflag"
 
+	"vitess.io/vitess/go/mysql"
+	"vitess.io/vitess/go/mysql/capabilities"
 	"vitess.io/vitess/go/mysql/replication"
 	"vitess.io/vitess/go/vt/mysqlctl/backupstorage"
 	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
@@ -57,12 +59,12 @@ func init() {
 }
 
 func registerMysqlShellBackupEngineFlags(fs *pflag.FlagSet) {
-	fs.StringVar(&mysqlShellBackupLocation, "mysql_shell_backup_location", mysqlShellBackupLocation, "location where the backup will be stored")
-	fs.StringVar(&mysqlShellFlags, "mysql_shell_flags", mysqlShellFlags, "execution flags to pass to mysqlsh binary to be used during dump/load")
-	fs.StringVar(&mysqlShellDumpFlags, "mysql_shell_dump_flags", mysqlShellDumpFlags, "flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST")
-	fs.StringVar(&mysqlShellLoadFlags, "mysql_shell_load_flags", mysqlShellLoadFlags, "flags to pass to mysql shell load utility. This should be a JSON string")
-	fs.BoolVar(&mysqlShellBackupShouldDrain, "mysql_shell_should_drain", mysqlShellBackupShouldDrain, "decide if we should drain while taking a backup or continue to serving traffic")
-	fs.BoolVar(&mysqlShellSpeedUpRestore, "mysql_shell_speedup_restore", mysqlShellSpeedUpRestore, "speed up restore by disabling redo logging and double write buffer during the restore process")
+	fs.StringVar(&mysqlShellBackupLocation, "mysql-shell-backup_location", mysqlShellBackupLocation, "location where the backup will be stored")
+	fs.StringVar(&mysqlShellFlags, "mysql-shell-flags", mysqlShellFlags, "execution flags to pass to mysqlsh binary to be used during dump/load")
+	fs.StringVar(&mysqlShellDumpFlags, "mysql-shell-dump-flags", mysqlShellDumpFlags, "flags to pass to mysql shell dump utility. This should be a JSON string and will be saved in the MANIFEST")
+	fs.StringVar(&mysqlShellLoadFlags, "mysql-shell-load-flags", mysqlShellLoadFlags, "flags to pass to mysql shell load utility. This should be a JSON string")
+	fs.BoolVar(&mysqlShellBackupShouldDrain, "mysql-shell-should-drain", mysqlShellBackupShouldDrain, "decide if we should drain while taking a backup or continue to serving traffic")
+	fs.BoolVar(&mysqlShellSpeedUpRestore, "mysql-shell-speedup-restore", mysqlShellSpeedUpRestore, "speed up restore by disabling redo logging and double write buffer during the restore process")
 }
 
 // MySQLShellBackupEngine encapsulates the logic to implement the restoration
@@ -84,7 +86,7 @@ func (be *MySQLShellBackupEngine) ExecuteBackup(ctx context.Context, params Back
 	}
 
 	start := time.Now().UTC()
-	location := path.Join(mysqlShellBackupLocation, params.Keyspace, params.Shard, start.Format("2006-01-02_15-04-05"))
+	location := path.Join(mysqlShellBackupLocation, params.Keyspace, params.Shard, start.Format(BackupTimestampFormat))
 
 	args := []string{}
 
@@ -166,7 +168,7 @@ func (be *MySQLShellBackupEngine) ExecuteBackup(ctx context.Context, params Back
 func (be *MySQLShellBackupEngine) ExecuteRestore(ctx context.Context, params RestoreParams, bh backupstorage.BackupHandle) (*BackupManifest, error) {
 	params.Logger.Infof("Calling ExecuteRestore for %s (DeleteBeforeRestore: %v)", params.DbName, params.DeleteBeforeRestore)
 
-	err := be.restorePreCheck()
+	err := be.restorePreCheck(ctx, params)
 	if err != nil {
 		return nil, vterrors.Wrap(err, "failed restore precheck")
 	}
@@ -313,7 +315,7 @@ func (be *MySQLShellBackupEngine) backupPreCheck() error {
 	return nil
 }
 
-func (be *MySQLShellBackupEngine) restorePreCheck() error {
+func (be *MySQLShellBackupEngine) restorePreCheck(ctx context.Context, params RestoreParams) error {
 	if mysqlShellFlags == "" {
 		return fmt.Errorf("%w: at least the --js flag is required", MySQLShellPreCheckError)
 	}
@@ -332,6 +334,23 @@ func (be *MySQLShellBackupEngine) restorePreCheck() error {
 		return fmt.Errorf("%w: \"progressFile\" needs to be empty as vitess always starts a restore from scratch", MySQLShellPreCheckError)
 	}
 
+	if mysqlShellSpeedUpRestore {
+		version, err := params.Mysqld.GetVersionString(ctx)
+		if err != nil {
+			return fmt.Errorf("%w: failed to fetch MySQL version: %v", MySQLShellPreCheckError, err)
+		}
+
+		capableOf := mysql.ServerVersionCapableOf(version)
+		capable, err := capableOf(capabilities.DisableRedoLogFlavorCapability)
+		if err != nil {
+			return fmt.Errorf("%w: error checking if server supports disabling redo log: %v", MySQLShellPreCheckError, err)
+		}
+
+		if !capable {
+			return fmt.Errorf("%w: MySQL version doesn't support disabling the redo log (must be >=8.0.21)", MySQLShellPreCheckError)
+		}
+	}
+
 	return nil
 }
 

go/vt/mysqlctl/mysqlshellbackupengine_test.go

@@ -1,10 +1,13 @@
 package mysqlctl
 
 import (
+	"context"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
+	"vitess.io/vitess/go/mysql/fakesqldb"
 	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 )
 
@@ -106,12 +109,35 @@ func TestMySQLShellBackupRestorePreCheck(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			mysqlShellLoadFlags = tt.flags
-			assert.ErrorIs(t, engine.restorePreCheck(), tt.err)
+			assert.ErrorIs(t, engine.restorePreCheck(context.Background(), RestoreParams{}), tt.err)
 		})
 	}
 
 }
 
+func TestMySQLShellBackupRestorePreCheckDisableRedolog(t *testing.T) {
+	original := mysqlShellSpeedUpRestore
+	defer func() { mysqlShellSpeedUpRestore = original }()
+
+	mysqlShellSpeedUpRestore = true
+	engine := MySQLShellBackupEngine{}
+
+	fakeMysqld := NewFakeMysqlDaemon(fakesqldb.New(t)) // defaults to 8.0.32
+	params := RestoreParams{
+		Mysqld: fakeMysqld,
+	}
+
+	// this should work as it is supported since 8.0.21
+	require.NoError(t, engine.restorePreCheck(context.Background(), params))
+
+	// it should error out if we change to an older version
+	fakeMysqld.Version = "8.0.20"
+
+	err := engine.restorePreCheck(context.Background(), params)
+	require.ErrorIs(t, err, MySQLShellPreCheckError)
+	require.ErrorContains(t, err, "doesn't support disabling the redo log")
+}
+
 func TestShouldDrainForBackupMySQLShell(t *testing.T) {
 	original := mysqlShellBackupShouldDrain
 	defer func() { mysqlShellBackupShouldDrain = original }()