backup: Allow for upgrade safe backups (vitessio#13449)

* backup: Allow for upgrade safe backups This adds functionality so that it's possible to use `Backup` and `BackupShard` and enforce that the backup created is safe to be used for upgrading MySQL. Signed-off-by: Dirkjan Bussink <[email protected]> * Fix test output Signed-off-by: Dirkjan Bussink <[email protected]> * backup: Add version number to manifest This adds the version number and upgrade safe state to the manifest. If these values are set, they are also validated on restore. For backwards compability we always allow still empty version numbers. Signed-off-by: Dirkjan Bussink <[email protected]> * Move restart logic into xtrabackupengine This already happens correctly with the shutdown for the builtin engine, where we're shutdown twice for `vtbackup` then. With this move we also properly honor the `UpgradeSafe` flag if the path is called through `Backup` or `BackupShard` vtctld commands. Signed-off-by: Dirkjan Bussink <[email protected]> * backup: Rework version detection It's not guaranteed that MySQL will always be up when doing a restore, this is shown in some of the failing tests with the changes so far. This means we need a way to get the MySQL version without having it running. We can though rely in `mysqlctld` being up. So this adds a new RPC to retrieve the version number through invoking the binary. This ensures we still get the right version also if we're running from the tablet as we'd execute the RPC. We also have less of a versioning issue here, as the vttablet + mysql are always upgraded together and the new mysqlctl is injected into the mysql container if needed so we don't have a version mismatch. So it's safe to do this in one step. Also decided to make the upgrade safe an explicit flag for `vtbackup` as well since overloading the existing flag was wonky and not really what we'd want. This makes it more explicit and safe. Signed-off-by: Dirkjan Bussink <[email protected]> * Rename function Signed-off-by: Dirkjan Bussink <[email protected]> --------- Signed-off-by: Dirkjan Bussink <[email protected]>
slackhq · Jul 12, 2023 · 40979b6 · 40979b6
1 parent e6ab2da
commit 40979b6
Show file tree

Hide file tree

Showing 30 changed files with 2,892 additions and 1,315 deletions.
diff --git a/go/cmd/vtbackup/vtbackup.go b/go/cmd/vtbackup/vtbackup.go
@@ -107,6 +107,7 @@ var (
 	initialBackup       bool
 	allowFirstBackup    bool
 	restartBeforeBackup bool
+	upgradeSafe         bool
 	// vttablet-like flags
 	initDbNameOverride string
 	initKeyspace       string
@@ -135,6 +136,7 @@ func registerFlags(fs *pflag.FlagSet) {
 	fs.BoolVar(&initialBackup, "initial_backup", initialBackup, "Instead of restoring from backup, initialize an empty database with the provided init_db_sql_file and upload a backup of that for the shard, if the shard has no backups yet. This can be used to seed a brand new shard with an initial, empty backup. If any backups already exist for the shard, this will be considered a successful no-op. This can only be done before the shard exists in topology (i.e. before any tablets are deployed).")
 	fs.BoolVar(&allowFirstBackup, "allow_first_backup", allowFirstBackup, "Allow this job to take the first backup of an existing shard.")
 	fs.BoolVar(&restartBeforeBackup, "restart_before_backup", restartBeforeBackup, "Perform a mysqld clean/full restart after applying binlogs, but before taking the backup. Only makes sense to work around xtrabackup bugs.")
+	fs.BoolVar(&upgradeSafe, "upgrade_safe", upgradeSafe, "Whether to use innodb_fast_shutdown=0 for the backup so it is safe to use for MySQL upgrades.")
 	// vttablet-like flags
 	fs.StringVar(&initDbNameOverride, "init_db_name_override", initDbNameOverride, "(init parameter) override the name of the db used by vttablet")
 	fs.StringVar(&initKeyspace, "init_keyspace", initKeyspace, "(init parameter) keyspace to use for this tablet")
@@ -301,6 +303,7 @@ func takeBackup(ctx context.Context, topoServer *topo.Server, backupStorage back
 		Shard:              initShard,
 		TabletAlias:        topoproto.TabletAliasString(tabletAlias),
 		Stats:              backupstats.BackupStats(),
+		UpgradeSafe:        upgradeSafe,
 	}
 	// In initial_backup mode, just take a backup of this empty database.
 	if initialBackup {

diff --git a/go/flags/endtoend/vtbackup.txt b/go/flags/endtoend/vtbackup.txt
@@ -183,6 +183,7 @@ Usage of vtbackup:
       --topo_zk_tls_ca string                                       the server ca to use to validate servers when connecting to the zk topo server
       --topo_zk_tls_cert string                                     the cert to use to connect to the zk topo server, requires topo_zk_tls_key, enables TLS
       --topo_zk_tls_key string                                      the key to use to connect to the zk topo server, enables TLS
+      --upgrade_safe                                                Whether to use innodb_fast_shutdown=0 for the backup so it is safe to use for MySQL upgrades.
       --v Level                                                     log level for V logs
   -v, --version                                                     print binary version
       --vmodule moduleSpec                                          comma-separated list of pattern=N settings for file-filtered logging

diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go
@@ -810,6 +810,7 @@ func terminatedRestore(t *testing.T) {
 }
 
 func checkTabletType(t *testing.T, alias string, tabletType topodata.TabletType) {
+	t.Helper()
 	// for loop for 15 seconds to check if tablet type is correct
 	for i := 0; i < 15; i++ {
 		output, err := localCluster.VtctldClientProcess.ExecuteCommandWithOutput("GetTablet", alias)
@@ -1060,8 +1061,8 @@ func terminateRestore(t *testing.T) {
 				assert.Fail(t, "restore in progress file missing")
 			}
 			tmpProcess.Process.Signal(syscall.SIGTERM)
-			found = true //nolint
-			return
+			found = true
+			break
 		}
 	}
 	assert.True(t, found, "Restore message not found")

diff --git a/go/vt/mysqlctl/backup_test.go b/go/vt/mysqlctl/backup_test.go
@@ -20,6 +20,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"os"
 	"path"
@@ -419,6 +420,134 @@ func TestRestoreTriesToParameterizeBackupStorage(t *testing.T) {
 	require.NotNil(t, scopedStats)
 }
 
+// TestRestoreManifestMySQLVersionValidation tests that Restore tries to validate
+// the MySQL version and safe upgrade attribute.
+func TestRestoreManifestMySQLVersionValidation(t *testing.T) {
+	testCases := []struct {
+		fromVersion, toVersion string
+		upgradeSafe            bool
+		wantErr                bool
+	}{
+		{
+			fromVersion: "mysqld  Ver 5.6.42",
+			toVersion:   "mysqld  Ver 5.7.40",
+			upgradeSafe: false,
+			wantErr:     true,
+		},
+		{
+			fromVersion: "mysqld  Ver 5.6.42",
+			toVersion:   "mysqld  Ver 5.7.40",
+			upgradeSafe: true,
+			wantErr:     false,
+		},
+		{
+			fromVersion: "mysqld  Ver 5.7.42",
+			toVersion:   "mysqld  Ver 8.0.32",
+			upgradeSafe: true,
+			wantErr:     false,
+		},
+		{
+			fromVersion: "mysqld  Ver 5.7.42",
+			toVersion:   "mysqld  Ver 8.0.32",
+			upgradeSafe: false,
+			wantErr:     true,
+		},
+		{
+			fromVersion: "mysqld  Ver 5.7.42",
+			toVersion:   "mysqld  Ver 8.0.32",
+			upgradeSafe: true,
+			wantErr:     false,
+		},
+		{
+			fromVersion: "mysqld  Ver 8.0.32",
+			toVersion:   "mysqld  Ver 8.0.32",
+			upgradeSafe: false,
+			wantErr:     false,
+		},
+		{
+			fromVersion: "mysqld  Ver 8.0.32",
+			toVersion:   "mysqld  Ver 8.0.32",
+			upgradeSafe: true,
+			wantErr:     false,
+		},
+		{
+			fromVersion: "mysqld  Ver 8.0.32",
+			toVersion:   "mysqld  Ver 8.0.31",
+			upgradeSafe: false,
+			wantErr:     true,
+		},
+		{
+			fromVersion: "mysqld  Ver 8.0.32",
+			toVersion:   "mysqld  Ver 8.0.31",
+			upgradeSafe: true,
+			wantErr:     true,
+		},
+		{
+			fromVersion: "mysqld  Ver 8.0.32",
+			toVersion:   "mysqld  Ver 8.0.33",
+			upgradeSafe: false,
+			wantErr:     true,
+		},
+		{
+			fromVersion: "mysqld  Ver 8.0.32",
+			toVersion:   "mysqld  Ver 8.0.33",
+			upgradeSafe: true,
+			wantErr:     false,
+		},
+		{
+			fromVersion: "",
+			toVersion:   "mysqld  Ver 8.0.33",
+			upgradeSafe: false,
+			wantErr:     false,
+		},
+		{
+			fromVersion: "",
+			toVersion:   "mysqld  Ver 8.0.33",
+			upgradeSafe: true,
+			wantErr:     false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(fmt.Sprintf("%s->%s upgradeSafe=%t", tc.fromVersion, tc.toVersion, tc.upgradeSafe), func(t *testing.T) {
+			env, closer := createFakeBackupRestoreEnv(t)
+			defer closer()
+			env.mysqld.Version = tc.toVersion
+
+			manifest := BackupManifest{
+				BackupTime:   time.Now().Add(-1 * time.Hour).Format(time.RFC3339),
+				BackupMethod: "fake",
+				Keyspace:     "test",
+				Shard:        "-",
+				MySQLVersion: tc.fromVersion,
+				UpgradeSafe:  tc.upgradeSafe,
+			}
+
+			manifestBytes, err := json.Marshal(manifest)
+			require.Nil(t, err)
+
+			env.backupEngine.ExecuteRestoreReturn = FakeBackupEngineExecuteRestoreReturn{&manifest, nil}
+			env.backupStorage.ListBackupsReturn = FakeBackupStorageListBackupsReturn{
+				BackupHandles: []backupstorage.BackupHandle{
+					&FakeBackupHandle{
+						ReadFileReturnF: func(context.Context, string) (io.ReadCloser, error) {
+							return io.NopCloser(bytes.NewBuffer(manifestBytes)), nil
+						},
+					},
+				},
+			}
+
+			_, err = Restore(env.ctx, env.restoreParams)
+			if tc.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+
+}
+
 type forTest []FileEntry
 
 func (f forTest) Len() int           { return len(f) }
@@ -490,6 +619,7 @@ func createFakeBackupRestoreEnv(t *testing.T) (*fakeBackupRestoreEnv, func()) {
 		BackupMethod: "fake",
 		Keyspace:     "test",
 		Shard:        "-",
+		MySQLVersion: "8.0.32",
 	}
 
 	manifestBytes, err := json.Marshal(manifest)

diff --git a/go/vt/mysqlctl/backupengine.go b/go/vt/mysqlctl/backupengine.go
@@ -73,22 +73,25 @@ type BackupParams struct {
 	IncrementalFromPos string
 	// Stats let's backup engines report detailed backup timings.
 	Stats backupstats.Stats
+	// UpgradeSafe indicates whether the backup is safe for upgrade and created with innodb_fast_shutdown=0
+	UpgradeSafe bool
 }
 
-func (b BackupParams) Copy() BackupParams {
+func (b *BackupParams) Copy() BackupParams {
 	return BackupParams{
-		b.Cnf,
-		b.Mysqld,
-		b.Logger,
-		b.Concurrency,
-		b.HookExtraEnv,
-		b.TopoServer,
-		b.Keyspace,
-		b.Shard,
-		b.TabletAlias,
-		b.BackupTime,
-		b.IncrementalFromPos,
-		b.Stats,
+		Cnf:                b.Cnf,
+		Mysqld:             b.Mysqld,
+		Logger:             b.Logger,
+		Concurrency:        b.Concurrency,
+		HookExtraEnv:       b.HookExtraEnv,
+		TopoServer:         b.TopoServer,
+		Keyspace:           b.Keyspace,
+		Shard:              b.Shard,
+		TabletAlias:        b.TabletAlias,
+		BackupTime:         b.BackupTime,
+		IncrementalFromPos: b.IncrementalFromPos,
+		Stats:              b.Stats,
+		UpgradeSafe:        b.UpgradeSafe,
 	}
 }
 
@@ -123,21 +126,21 @@ type RestoreParams struct {
 	Stats backupstats.Stats
 }
 
-func (p RestoreParams) Copy() RestoreParams {
+func (p *RestoreParams) Copy() RestoreParams {
 	return RestoreParams{
-		p.Cnf,
-		p.Mysqld,
-		p.Logger,
-		p.Concurrency,
-		p.HookExtraEnv,
-		p.DeleteBeforeRestore,
-		p.DbName,
-		p.Keyspace,
-		p.Shard,
-		p.StartTime,
-		p.RestoreToPos,
-		p.DryRun,
-		p.Stats,
+		Cnf:                 p.Cnf,
+		Mysqld:              p.Mysqld,
+		Logger:              p.Logger,
+		Concurrency:         p.Concurrency,
+		HookExtraEnv:        p.HookExtraEnv,
+		DeleteBeforeRestore: p.DeleteBeforeRestore,
+		DbName:              p.DbName,
+		Keyspace:            p.Keyspace,
+		Shard:               p.Shard,
+		StartTime:           p.StartTime,
+		RestoreToPos:        p.RestoreToPos,
+		DryRun:              p.DryRun,
+		Stats:               p.Stats,
 	}
 }
 
@@ -273,6 +276,12 @@ type BackupManifest struct {
 	Keyspace string
 
 	Shard string
+
+	// MySQLversion is the version of MySQL when the backup was taken.
+	MySQLVersion string
+
+	// UpgradeSafe indicates whether the backup is safe to use for an upgrade to a newer MySQL version
+	UpgradeSafe bool
 }
 
 func (m *BackupManifest) HashKey() string {
@@ -392,6 +401,11 @@ func FindBackupToRestore(ctx context.Context, params RestoreParams, bhs []backup
 	manifests := make([]*BackupManifest, len(bhs))
 	manifestHandleMap := NewManifestHandleMap()
 
+	mysqlVersion, err := params.Mysqld.GetVersionString(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	fullBackupIndex := func() int {
 		for index := len(bhs) - 1; index >= 0; index-- {
 			bh := bhs[index]
@@ -409,6 +423,14 @@ func FindBackupToRestore(ctx context.Context, params RestoreParams, bhs []backup
 				continue
 			}
 
+			// check if the backup can be used with this MySQL version.
+			if bm.MySQLVersion != "" {
+				if err := validateMySQLVersionUpgradeCompatible(mysqlVersion, bm.MySQLVersion, bm.UpgradeSafe); err != nil {
+					params.Logger.Warningf("Skipping backup %v/%v with incompatible MySQL version %v (upgrade safe: %v): %v", backupDir, bh.Name(), bm.MySQLVersion, bm.UpgradeSafe, err)
+					continue
+				}
+			}
+
 			var backupTime time.Time
 			if checkBackupTime {
 				backupTime, err = time.Parse(time.RFC3339, bm.BackupTime)
@@ -461,14 +483,48 @@ func FindBackupToRestore(ctx context.Context, params RestoreParams, bhs []backup
 	// restore to a position (using incremental backups):
 	// we calculate a possible restore path based on the manifests. The resulting manifests are
 	// a sorted subsequence, with the full backup first, and zero or more incremental backups to follow.
-	manifests, err := FindPITRPath(params.RestoreToPos.GTIDSet, manifests)
+	restorePath.manifests, err = FindPITRPath(params.RestoreToPos.GTIDSet, manifests)
 	if err != nil {
 		return nil, err
 	}
-	restorePath.manifests = manifests
 	return restorePath, nil
 }
 
+func validateMySQLVersionUpgradeCompatible(to string, from string, upgradeSafe bool) error {
+	// It's always safe to use the same version.
+	if to == from {
+		return nil
+	}
+
+	flavorTo, parsedTo, err := ParseVersionString(to)
+	if err != nil {
+		return err
+	}
+
+	flavorFrom, parsedFrom, err := ParseVersionString(from)
+	if err != nil {
+		return err
+	}
+
+	if flavorTo != flavorFrom {
+		return fmt.Errorf("cannot use backup between different flavors: %q vs. %q", from, to)
+	}
+
+	if parsedTo == parsedFrom {
+		return nil
+	}
+
+	if !parsedTo.atLeast(parsedFrom) {
+		return fmt.Errorf("running MySQL version %q is older than backup MySQL version %q", to, from)
+	}
+
+	if upgradeSafe {
+		return nil
+	}
+
+	return fmt.Errorf("running MySQL version %q is newer than backup MySQL version %q which is not safe to upgrade", to, from)
+}
+
 func prepareToRestore(ctx context.Context, cnf *Mycnf, mysqld MysqlDaemon, logger logutil.Logger) error {
 	// shutdown mysqld if it is running
 	logger.Infof("Restore: shutdown mysqld")