From 9243f2256943db39c2428144503504a1220e36e5 Mon Sep 17 00:00:00 2001 From: Renan Rangel Date: Wed, 18 Sep 2024 05:22:03 -0700 Subject: [PATCH] add ShouldStartMySQLAfterRestore() Signed-off-by: Renan Rangel --- go/vt/mysqlctl/backup.go | 24 +++++++++++++----------- go/vt/mysqlctl/backupengine.go | 1 + go/vt/mysqlctl/builtinbackupengine.go | 5 +++++ go/vt/mysqlctl/fakebackupengine.go | 4 ++++ go/vt/mysqlctl/mysqlshellbackupengine.go | 6 ++++++ go/vt/mysqlctl/xtrabackupengine.go | 5 +++++ 6 files changed, 34 insertions(+), 11 deletions(-) diff --git a/go/vt/mysqlctl/backup.go b/go/vt/mysqlctl/backup.go index fd067b02887..4a89add9c9e 100644 --- a/go/vt/mysqlctl/backup.go +++ b/go/vt/mysqlctl/backup.go @@ -447,17 +447,19 @@ func Restore(ctx context.Context, params RestoreParams) (*BackupManifest, error) return nil, err } - // mysqld needs to be running in order for mysql_upgrade to work. - // If we've just restored from a backup from previous MySQL version then mysqld - // may fail to start due to a different structure of mysql.* tables. The flag - // --skip-grant-tables ensures that these tables are not read until mysql_upgrade - // is executed. And since with --skip-grant-tables anyone can connect to MySQL - // without password, we are passing --skip-networking to greatly reduce the set - // of those who can connect. - params.Logger.Infof("Restore: starting mysqld for mysql_upgrade") - // Note Start will use dba user for waiting, this is fine, it will be allowed. - if err := params.Mysqld.Start(context.Background(), params.Cnf, "--skip-grant-tables", "--skip-networking"); err != nil { - return nil, err + if re.ShouldStartMySQLAfterRestore() { // all engines except mysqlshell since MySQL is always running there + // mysqld needs to be running in order for mysql_upgrade to work. + // If we've just restored from a backup from previous MySQL version then mysqld + // may fail to start due to a different structure of mysql.* tables. The flag + // --skip-grant-tables ensures that these tables are not read until mysql_upgrade + // is executed. And since with --skip-grant-tables anyone can connect to MySQL + // without password, we are passing --skip-networking to greatly reduce the set + // of those who can connect. + params.Logger.Infof("Restore: starting mysqld for mysql_upgrade") + // Note Start will use dba user for waiting, this is fine, it will be allowed. + if err := params.Mysqld.Start(context.Background(), params.Cnf, "--skip-grant-tables", "--skip-networking"); err != nil { + return nil, err + } } params.Logger.Infof("Restore: running mysql_upgrade") diff --git a/go/vt/mysqlctl/backupengine.go b/go/vt/mysqlctl/backupengine.go index c483aff3d78..1401c7e2f84 100644 --- a/go/vt/mysqlctl/backupengine.go +++ b/go/vt/mysqlctl/backupengine.go @@ -179,6 +179,7 @@ func (p *RestoreParams) IsIncrementalRecovery() bool { // Returns the manifest of a backup if successful, otherwise returns an error type RestoreEngine interface { ExecuteRestore(ctx context.Context, params RestoreParams, bh backupstorage.BackupHandle) (*BackupManifest, error) + ShouldStartMySQLAfterRestore() bool } // BackupRestoreEngine is a combination of BackupEngine and RestoreEngine. diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index 494d765f2a9..9876de36098 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -1185,6 +1185,11 @@ func (be *BuiltinBackupEngine) ShouldDrainForBackup(req *tabletmanagerdatapb.Bac return true } +// ShouldStartMySQLAfterRestore signifies if this backup engine needs to restart MySQL once the restore is completed. +func (be *BuiltinBackupEngine) ShouldStartMySQLAfterRestore() bool { + return true +} + func getPrimaryPosition(ctx context.Context, tmc tmclient.TabletManagerClient, ts *topo.Server, keyspace, shard string) (replication.Position, error) { si, err := ts.GetShard(ctx, keyspace, shard) if err != nil { diff --git a/go/vt/mysqlctl/fakebackupengine.go b/go/vt/mysqlctl/fakebackupengine.go index d78282e6aff..180922347e1 100644 --- a/go/vt/mysqlctl/fakebackupengine.go +++ b/go/vt/mysqlctl/fakebackupengine.go @@ -91,3 +91,7 @@ func (be *FakeBackupEngine) ShouldDrainForBackup(req *tabletmanagerdatapb.Backup be.ShouldDrainForBackupCalls = be.ShouldDrainForBackupCalls + 1 return be.ShouldDrainForBackupReturn } + +func (be *FakeBackupEngine) ShouldStartMySQLAfterRestore() bool { + return true +} diff --git a/go/vt/mysqlctl/mysqlshellbackupengine.go b/go/vt/mysqlctl/mysqlshellbackupengine.go index 98f1230c9eb..a4210518ac8 100644 --- a/go/vt/mysqlctl/mysqlshellbackupengine.go +++ b/go/vt/mysqlctl/mysqlshellbackupengine.go @@ -381,6 +381,12 @@ func (be *MySQLShellBackupEngine) ShouldDrainForBackup(req *tabletmanagerdatapb. return mysqlShellBackupShouldDrain } +// ShouldStartMySQLAfterRestore signifies if this backup engine needs to restart MySQL once the restore is completed. +// Since MySQL Shell operates on a live MySQL instance, there is no need to start it once the restore is completed +func (be *MySQLShellBackupEngine) ShouldStartMySQLAfterRestore() bool { + return false +} + func (be *MySQLShellBackupEngine) backupPreCheck(location string) error { if mysqlShellBackupLocation == "" { return fmt.Errorf("%w: no backup location set via --mysql-shell-backup-location", MySQLShellPreCheckError) diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index 5dcca37c984..9e4917abb0f 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -942,6 +942,11 @@ func (be *XtrabackupEngine) ShouldDrainForBackup(req *tabletmanagerdatapb.Backup return false } +// ShouldStartMySQLAfterRestore signifies if this backup engine needs to restart MySQL once the restore is completed. +func (be *XtrabackupEngine) ShouldStartMySQLAfterRestore() bool { + return true +} + func init() { BackupRestoreEngineMap[xtrabackupEngineName] = &XtrabackupEngine{} }