diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index 442e87adc43..a70d1804028 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -1255,8 +1255,9 @@ func waitForNumBackups(t *testing.T, expectNumBackups int) []string { } } -func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incrementalFromPos string, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) { +func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incrementalFromPos string, expectEmpty bool, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) { numBackups := len(waitForNumBackups(t, -1)) + output, err := localCluster.VtctldClientProcess.ExecuteCommandWithOutput("Backup", "--incremental-from-pos", incrementalFromPos, replica.Alias) if expectError != "" { require.Errorf(t, err, "expected: %v", expectError) @@ -1265,18 +1266,24 @@ func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incre } require.NoErrorf(t, err, "output: %v", output) + if expectEmpty { + require.Contains(t, output, mysqlctl.EmptyBackupMessage) + return nil, "" + } + backups := waitForNumBackups(t, numBackups+1) require.NotEmptyf(t, backups, "output: %v", output) verifyTabletBackupStats(t, replica.VttabletProcess.GetVars()) backupName = backups[len(backups)-1] + backupLocation := localCluster.CurrentVTDATAROOT + "/backups/" + shardKsName + "/" + backupName return readManifestFile(t, backupLocation), backupName } -func TestReplicaIncrementalBackup(t *testing.T, replicaIndex int, incrementalFromPos string, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) { +func TestReplicaIncrementalBackup(t *testing.T, replicaIndex int, incrementalFromPos string, expectEmpty bool, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) { replica := getReplica(t, replicaIndex) - return testReplicaIncrementalBackup(t, replica, incrementalFromPos, expectError) + return testReplicaIncrementalBackup(t, replica, incrementalFromPos, expectEmpty, expectError) } func TestReplicaFullRestore(t *testing.T, replicaIndex int, expectError string) { diff --git a/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go b/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go index 5effcc339d8..6270c023eab 100644 --- a/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go +++ b/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go @@ -138,6 +138,7 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase) name string writeBeforeBackup bool fromFullPosition bool + expectEmpty bool incrementalFrom incrementalFromPosType expectError string }{ @@ -146,14 +147,19 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase) incrementalFrom: incrementalFromPosPosition, }, { - name: "fail1", + name: "empty1", incrementalFrom: incrementalFromPosPosition, - expectError: "no binary logs to backup", + expectEmpty: true, }, { - name: "fail2", + name: "empty2", + incrementalFrom: incrementalFromPosAuto, + expectEmpty: true, + }, + { + name: "empty3", incrementalFrom: incrementalFromPosPosition, - expectError: "no binary logs to backup", + expectEmpty: true, }, { name: "make writes, succeed", @@ -161,9 +167,9 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase) incrementalFrom: incrementalFromPosPosition, }, { - name: "fail, no binary logs to backup", + name: "empty again", incrementalFrom: incrementalFromPosPosition, - expectError: "no binary logs to backup", + expectEmpty: true, }, { name: "make writes again, succeed", @@ -176,9 +182,9 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase) incrementalFrom: incrementalFromPosAuto, }, { - name: "fail auto position, no binary logs to backup", + name: "empty again, based on auto position", incrementalFrom: incrementalFromPosAuto, - expectError: "no binary logs to backup", + expectEmpty: true, }, { name: "auto position, make writes again, succeed", @@ -223,10 +229,15 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase) } } // always use same 1st replica - manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectError) + manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectEmpty, tc.expectError) if tc.expectError != "" { return } + if tc.expectEmpty { + assert.Nil(t, manifest) + return + } + require.NotNil(t, manifest) defer func() { lastBackupPos = manifest.Position lastBackupName = manifest.BackupName @@ -352,6 +363,7 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes name string writeBeforeBackup bool fromFullPosition bool + expectEmpty bool incrementalFrom incrementalFromPosType expectError string }{ @@ -360,14 +372,19 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes incrementalFrom: incrementalFromPosPosition, }, { - name: "fail1", + name: "empty1", incrementalFrom: incrementalFromPosPosition, - expectError: "no binary logs to backup", + expectEmpty: true, }, { - name: "fail2", + name: "empty2", + incrementalFrom: incrementalFromPosAuto, + expectEmpty: true, + }, + { + name: "empty3", incrementalFrom: incrementalFromPosPosition, - expectError: "no binary logs to backup", + expectEmpty: true, }, { name: "make writes, succeed", @@ -375,9 +392,9 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes incrementalFrom: incrementalFromPosPosition, }, { - name: "fail, no binary logs to backup", + name: "empty again", incrementalFrom: incrementalFromPosPosition, - expectError: "no binary logs to backup", + expectEmpty: true, }, { name: "make writes again, succeed", @@ -390,9 +407,9 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes incrementalFrom: incrementalFromPosAuto, }, { - name: "fail auto position, no binary logs to backup", + name: "empty again, based on auto position", incrementalFrom: incrementalFromPosAuto, - expectError: "no binary logs to backup", + expectEmpty: true, }, { name: "auto position, make writes again, succeed", @@ -434,11 +451,16 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes incrementalFromPos = replication.EncodePosition(fullBackupPos) } } - manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectError) + manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectEmpty, tc.expectError) if tc.expectError != "" { return } - // We wish to mark the current post-backup timestamp. We will later on retore to this point in time. + if tc.expectEmpty { + assert.Nil(t, manifest) + return + } + require.NotNil(t, manifest) + // We wish to mark the current post-backup timestamp. We will later on restore to this point in time. // However, the restore is up to and _exclusive_ of the timestamp. So for test's sake, we sleep // an extra few milliseconds just to ensure the timestamp we read is strictly after the backup time. // This is basicaly to avoid weird flakiness in CI. @@ -707,10 +729,11 @@ func ExecTestIncrementalBackupOnTwoTablets(t *testing.T, tcase *PITRTestCase) { lastBackupPos = fullBackupPos case operationIncrementalBackup: - manifest, _ := TestReplicaIncrementalBackup(t, tc.replicaIndex, "auto", tc.expectError) + manifest, _ := TestReplicaIncrementalBackup(t, tc.replicaIndex, "auto", false /* expectEmpty */, tc.expectError) if tc.expectError != "" { return } + require.NotNil(t, manifest) defer func() { lastBackupPos = manifest.Position }() diff --git a/go/vt/mysqlctl/backup.go b/go/vt/mysqlctl/backup.go index 5a9706b07f8..fb401966c50 100644 --- a/go/vt/mysqlctl/backup.go +++ b/go/vt/mysqlctl/backup.go @@ -86,6 +86,8 @@ var ( // backupCompressBlocks is the number of blocks that are processed // once before the writer blocks backupCompressBlocks = 2 + + EmptyBackupMessage = "no new data to backup, skipping it" ) func init() { @@ -168,14 +170,20 @@ func Backup(ctx context.Context, params BackupParams) error { } // Take the backup, and either AbortBackup or EndBackup. - usable, err := be.ExecuteBackup(ctx, beParams, bh) + backupResult, err := be.ExecuteBackup(ctx, beParams, bh) logger := params.Logger var finishErr error - if usable { - finishErr = bh.EndBackup(ctx) - } else { + switch backupResult { + case BackupUnusable: logger.Errorf2(err, "backup is not usable, aborting it") finishErr = bh.AbortBackup(ctx) + case BackupEmpty: + logger.Infof(EmptyBackupMessage) + // While an empty backup is considered "successful", it should leave no trace. + // We therefore ensire to clean up an backup files/directories/entries. + finishErr = bh.AbortBackup(ctx) + case BackupUsable: + finishErr = bh.EndBackup(ctx) } if err != nil { if finishErr != nil { diff --git a/go/vt/mysqlctl/backup_blackbox_test.go b/go/vt/mysqlctl/backup_blackbox_test.go index 116a4fe80f2..4508c4e4306 100644 --- a/go/vt/mysqlctl/backup_blackbox_test.go +++ b/go/vt/mysqlctl/backup_blackbox_test.go @@ -149,7 +149,7 @@ func TestExecuteBackup(t *testing.T) { fakeStats := backupstats.NewFakeStats() - ok, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{ + backupResult, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{ Logger: logutil.NewConsoleLogger(), Mysqld: mysqld, Cnf: &mysqlctl.Mycnf{ @@ -167,7 +167,7 @@ func TestExecuteBackup(t *testing.T) { }, bh) require.NoError(t, err) - assert.True(t, ok) + assert.Equal(t, mysqlctl.BackupUsable, backupResult) var destinationCloseStats int var destinationOpenStats int @@ -209,7 +209,7 @@ func TestExecuteBackup(t *testing.T) { mysqld.ExpectedExecuteSuperQueryCurrent = 0 // resest the index of what queries we've run mysqld.ShutdownTime = time.Minute // reminder that shutdownDeadline is 1s - ok, err = be.ExecuteBackup(ctx, mysqlctl.BackupParams{ + backupResult, err = be.ExecuteBackup(ctx, mysqlctl.BackupParams{ Logger: logutil.NewConsoleLogger(), Mysqld: mysqld, Cnf: &mysqlctl.Mycnf{ @@ -225,7 +225,7 @@ func TestExecuteBackup(t *testing.T) { }, bh) assert.Error(t, err) - assert.False(t, ok) + assert.Equal(t, mysqlctl.BackupUnusable, backupResult) } func TestExecuteBackupWithSafeUpgrade(t *testing.T) { @@ -296,7 +296,7 @@ func TestExecuteBackupWithSafeUpgrade(t *testing.T) { "SET GLOBAL innodb_fast_shutdown=0": {}, } - ok, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{ + backupResult, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{ Logger: logutil.NewConsoleLogger(), Mysqld: mysqld, Cnf: &mysqlctl.Mycnf{ @@ -314,7 +314,7 @@ func TestExecuteBackupWithSafeUpgrade(t *testing.T) { }, bh) require.NoError(t, err) - assert.True(t, ok) + assert.Equal(t, mysqlctl.BackupUsable, backupResult) } // TestExecuteBackupWithCanceledContext tests the ability of the backup function to gracefully handle cases where errors @@ -383,7 +383,7 @@ func TestExecuteBackupWithCanceledContext(t *testing.T) { cancelledCtx, cancelCtx := context.WithCancel(context.Background()) cancelCtx() - ok, err := be.ExecuteBackup(cancelledCtx, mysqlctl.BackupParams{ + backupResult, err := be.ExecuteBackup(cancelledCtx, mysqlctl.BackupParams{ Logger: logutil.NewConsoleLogger(), Mysqld: mysqld, Cnf: &mysqlctl.Mycnf{ @@ -403,7 +403,7 @@ func TestExecuteBackupWithCanceledContext(t *testing.T) { require.Error(t, err) // all four files will fail require.ErrorContains(t, err, "context canceled;context canceled;context canceled;context canceled") - assert.False(t, ok) + assert.Equal(t, mysqlctl.BackupUnusable, backupResult) } // TestExecuteRestoreWithCanceledContext tests the ability of the restore function to gracefully handle cases where errors @@ -468,7 +468,7 @@ func TestExecuteRestoreWithTimedOutContext(t *testing.T) { defer mysqld.Close() mysqld.ExpectedExecuteSuperQueryList = []string{"STOP SLAVE", "START SLAVE"} - ok, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{ + backupResult, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{ Logger: logutil.NewConsoleLogger(), Mysqld: mysqld, Cnf: &mysqlctl.Mycnf{ @@ -486,7 +486,7 @@ func TestExecuteRestoreWithTimedOutContext(t *testing.T) { }, bh) require.NoError(t, err) - assert.True(t, ok) + assert.Equal(t, mysqlctl.BackupUsable, backupResult) // Now try to restore the above backup. bh = filebackupstorage.NewBackupHandle(nil, "", "", true) diff --git a/go/vt/mysqlctl/backupengine.go b/go/vt/mysqlctl/backupengine.go index 0fbafb9a4a8..915b5e6894f 100644 --- a/go/vt/mysqlctl/backupengine.go +++ b/go/vt/mysqlctl/backupengine.go @@ -45,9 +45,17 @@ var ( backupEngineImplementation = builtinBackupEngineName ) +type BackupResult int + +const ( + BackupUnusable BackupResult = iota + BackupEmpty + BackupUsable +) + // BackupEngine is the interface to take a backup with a given engine. type BackupEngine interface { - ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error) + ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (BackupResult, error) ShouldDrainForBackup(req *tabletmanagerdatapb.BackupRequest) bool } diff --git a/go/vt/mysqlctl/binlogs_gtid.go b/go/vt/mysqlctl/binlogs_gtid.go index 3ea48663578..61ca0a87f70 100644 --- a/go/vt/mysqlctl/binlogs_gtid.go +++ b/go/vt/mysqlctl/binlogs_gtid.go @@ -104,8 +104,7 @@ func ChooseBinlogsForIncrementalBackup( // The other thing to validate, is that we can't allow a situation where the backup-GTIDs have entries not covered // by our binary log's Previous-GTIDs (padded with purged GTIDs). Because that means we can't possibly restore to // such position. - prevGTIDsUnionPurged := prevGTIDsUnion.Union(purgedGTIDSet) - if !prevGTIDsUnionPurged.Contains(backupFromGTIDSet) { + if prevGTIDsUnionPurged := prevGTIDsUnion.Union(purgedGTIDSet); !prevGTIDsUnionPurged.Contains(backupFromGTIDSet) { return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "Mismatching GTID entries. Requested backup pos has entries not found in the binary logs, and binary logs have entries not found in the requested backup pos. Neither fully contains the other.\n- Requested pos=%v\n- binlog pos=%v\n- purgedGTIDSet=%v\n- union=%v\n- union purged=%v", backupFromGTIDSet, previousGTIDsPos.GTIDSet, purgedGTIDSet, prevGTIDsUnion, prevGTIDsUnionPurged) @@ -133,7 +132,16 @@ func ChooseBinlogsForIncrementalBackup( } return binaryLogsToBackup, incrementalBackupFromGTID, incrementalBackupToGTID, nil } - return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "no binary logs to backup (increment is empty)") + if prevGTIDsUnion.Union(purgedGTIDSet).Equal(backupFromGTIDSet) { + // This means we've iterated over all binary logs, and as it turns out, the backup pos is + // identical to the Previous-GTIDs of the last binary log. But, we also know that we ourselves + // have flushed the binary logs so as to generate the new (now last) binary log. + // Which means, from the Pos of the backup till the time we issued FLUSH BINARY LOGS, there + // were no new GTID entries. The database was performed no writes during that period, + // so we have no entries to backup and the backup is therefore empty. + return nil, "", "", nil + } + return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "cannot find binary logs that cover requested GTID range. backupFromGTIDSet=%v, prevGTIDsUnion=%v", backupFromGTIDSet.String(), prevGTIDsUnion.String()) } // IsValidIncrementalBakcup determines whether the given manifest can be used to extend a backup diff --git a/go/vt/mysqlctl/binlogs_gtid_test.go b/go/vt/mysqlctl/binlogs_gtid_test.go index 655208e908e..ec1c220fd39 100644 --- a/go/vt/mysqlctl/binlogs_gtid_test.go +++ b/go/vt/mysqlctl/binlogs_gtid_test.go @@ -85,13 +85,13 @@ func TestChooseBinlogsForIncrementalBackup(t *testing.T) { name: "last binlog excluded, no binlogs found", previousGTIDs: basePreviousGTIDs, backupPos: "16b1039f-22b6-11ed-b765-0a43f95f28a3:1-331", - expectError: "no binary logs to backup", + expectBinlogs: nil, }, { name: "backup pos beyond all binlogs", previousGTIDs: basePreviousGTIDs, backupPos: "16b1039f-22b6-11ed-b765-0a43f95f28a3:1-630000", - expectError: "no binary logs to backup", + expectError: "cannot find binary logs that cover requested GTID range", }, { name: "missing GTID entries", @@ -294,13 +294,14 @@ func TestChooseBinlogsForIncrementalBackup(t *testing.T) { return } require.NoError(t, err) - require.NotEmpty(t, binlogsToBackup) assert.Equal(t, tc.expectBinlogs, binlogsToBackup) - if tc.previousGTIDs[binlogsToBackup[0]] != "" { - assert.Equal(t, tc.previousGTIDs[binlogsToBackup[0]], fromGTID) + if len(binlogsToBackup) > 0 { + if tc.previousGTIDs[binlogsToBackup[0]] != "" { + assert.Equal(t, tc.previousGTIDs[binlogsToBackup[0]], fromGTID) + } + assert.Equal(t, tc.previousGTIDs[binlogs[len(binlogs)-1]], toGTID) + assert.NotEqual(t, fromGTID, toGTID) } - assert.Equal(t, tc.previousGTIDs[binlogs[len(binlogs)-1]], toGTID) - assert.NotEqual(t, fromGTID, toGTID) }) } } diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index be4b33a4e4c..023439d1fff 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -203,8 +203,8 @@ func (fe *FileEntry) open(cnf *Mycnf, readOnly bool) (*os.File, error) { } // ExecuteBackup runs a backup based on given params. This could be a full or incremental backup. -// The function returns a boolean that indicates if the backup is usable, and an overall error. -func (be *BuiltinBackupEngine) ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error) { +// The function returns a BackupResult that indicates the usability of the backup, and an overall error. +func (be *BuiltinBackupEngine) ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (BackupResult, error) { params.Logger.Infof("Executing Backup at %v for keyspace/shard %v/%v on tablet %v, concurrency: %v, compress: %v, incrementalFromPos: %v", params.BackupTime, params.Keyspace, params.Shard, params.TabletAlias, params.Concurrency, backupStorageCompress, params.IncrementalFromPos) @@ -233,16 +233,17 @@ func getIncrementalFromPosGTIDSet(incrementalFromPos string) (replication.Mysql5 // executeIncrementalBackup runs an incremental backup, based on given 'incremental_from_pos', which can be: // - A valid position // - "auto", indicating the incremental backup should begin with last successful backup end position. -func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error) { +// The function returns a BackupResult that indicates the usability of the backup, and an overall error. +func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (BackupResult, error) { // Collect MySQL status: // UUID serverUUID, err := params.Mysqld.GetServerUUID(ctx) if err != nil { - return false, vterrors.Wrap(err, "can't get server uuid") + return BackupUnusable, vterrors.Wrap(err, "can't get server uuid") } mysqlVersion, err := params.Mysqld.GetVersionString(ctx) if err != nil { - return false, vterrors.Wrap(err, "can't get MySQL version") + return BackupUnusable, vterrors.Wrap(err, "can't get MySQL version") } // We now need to figure out the GTIDSet from which we want to take the incremental backup. The user may have @@ -254,7 +255,7 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par params.Logger.Infof("auto evaluating incremental_from_pos") backupName, pos, err := findLatestSuccessfulBackupPosition(ctx, params, bh.Name()) if err != nil { - return false, err + return BackupUnusable, err } fromBackupName = backupName params.IncrementalFromPos = replication.EncodePosition(pos) @@ -266,7 +267,7 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par backupName := params.IncrementalFromPos pos, err := findBackupPosition(ctx, params, backupName) if err != nil { - return false, err + return BackupUnusable, err } fromBackupName = backupName params.IncrementalFromPos = replication.EncodePosition(pos) @@ -276,7 +277,7 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par // params.IncrementalFromPos is a string. We want to turn that into a MySQL GTID backupFromGTIDSet, err := getIncrementalFromPosGTIDSet(params.IncrementalFromPos) if err != nil { - return false, err + return BackupUnusable, err } // OK, we now have the formal MySQL GTID from which we want to take the incremental backup. @@ -288,11 +289,11 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par // ignore the purged GTIDs: if err := params.Mysqld.FlushBinaryLogs(ctx); err != nil { - return false, vterrors.Wrapf(err, "cannot flush binary logs in incremental backup") + return BackupUnusable, vterrors.Wrapf(err, "cannot flush binary logs in incremental backup") } binaryLogs, err := params.Mysqld.GetBinaryLogs(ctx) if err != nil { - return false, vterrors.Wrapf(err, "cannot get binary logs in incremental backup") + return BackupUnusable, vterrors.Wrapf(err, "cannot get binary logs in incremental backup") } getPurgedGTIDSet := func() (replication.Position, replication.Mysql56GTIDSet, error) { @@ -311,7 +312,7 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par // command may also purge old logs, hence affecting the value of gtid_purged. gtidPurged, purgedGTIDSet, err := getPurgedGTIDSet() if err != nil { - return false, err + return BackupUnusable, err } previousGTIDs := map[string]string{} getBinlogPreviousGTIDs := func(ctx context.Context, binlog string) (gtids string, err error) { @@ -329,15 +330,19 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par } binaryLogsToBackup, incrementalBackupFromGTID, incrementalBackupToGTID, err := ChooseBinlogsForIncrementalBackup(ctx, backupFromGTIDSet, purgedGTIDSet, binaryLogs, getBinlogPreviousGTIDs) if err != nil { - return false, vterrors.Wrapf(err, "cannot get binary logs to backup in incremental backup") + return BackupUnusable, vterrors.Wrapf(err, "cannot get binary logs to backup in incremental backup") + } + if len(binaryLogsToBackup) == 0 { + // Empty backup. + return BackupEmpty, nil } incrementalBackupFromPosition, err := replication.ParsePosition(replication.Mysql56FlavorID, incrementalBackupFromGTID) if err != nil { - return false, vterrors.Wrapf(err, "cannot parse position %v", incrementalBackupFromGTID) + return BackupUnusable, vterrors.Wrapf(err, "cannot parse position %v", incrementalBackupFromGTID) } incrementalBackupToPosition, err := replication.ParsePosition(replication.Mysql56FlavorID, incrementalBackupToGTID) if err != nil { - return false, vterrors.Wrapf(err, "cannot parse position %v", incrementalBackupToGTID) + return BackupUnusable, vterrors.Wrapf(err, "cannot parse position %v", incrementalBackupToGTID) } // The backup position is the GTISset of the last binary log (taken from Previous-GTIDs of the one-next binary log), and we // also include gtid_purged ; this complies with the "standard" way MySQL "thinks" about GTIDs: there's gtid_executed, which includes @@ -352,16 +357,16 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par fe := FileEntry{Base: backupBinlogDir, Name: binlogFile} fullPath, err := fe.fullPath(params.Cnf) if err != nil { - return false, err + return BackupUnusable, err } req.BinlogFileNames = append(req.BinlogFileNames, fullPath) } resp, err := params.Mysqld.ReadBinlogFilesTimestamps(ctx, req) if err != nil { - return false, vterrors.Wrapf(err, "reading timestamps from binlog files %v", binaryLogsToBackup) + return BackupUnusable, vterrors.Wrapf(err, "reading timestamps from binlog files %v", binaryLogsToBackup) } if resp.FirstTimestampBinlog == "" || resp.LastTimestampBinlog == "" { - return false, vterrors.Errorf(vtrpc.Code_ABORTED, "empty binlog name in response. Request=%v, Response=%v", req, resp) + return BackupUnusable, vterrors.Errorf(vtrpc.Code_ABORTED, "empty binlog name in response. Request=%v, Response=%v", req, resp) } log.Infof("ReadBinlogFilesTimestampsResponse: %+v", resp) incrDetails := &IncrementalBackupDetails{ @@ -380,14 +385,14 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par // It is a fact that incrementalBackupFromGTID is earlier or equal to params.IncrementalFromPos. // In the backup manifest file, we document incrementalBackupFromGTID, not the user's requested position. if err := be.backupFiles(ctx, params, bh, incrementalBackupToPosition, gtidPurged, incrementalBackupFromPosition, fromBackupName, binaryLogsToBackup, serverUUID, mysqlVersion, incrDetails); err != nil { - return false, err + return BackupUnusable, err } - return true, nil + return BackupUsable, nil } -// executeFullBackup returns a boolean that indicates if the backup is usable, +// executeFullBackup returns a BackupResult that indicates the usability of the backup, // and an overall error. -func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error) { +func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (BackupResult, error) { if params.IncrementalFromPos != "" { return be.executeIncrementalBackup(ctx, params, bh) @@ -411,17 +416,17 @@ func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params Bac // keep going if we're the primary, might be a degenerate case sourceIsPrimary = true default: - return false, vterrors.Wrap(err, "can't get replica status") + return BackupUnusable, vterrors.Wrap(err, "can't get replica status") } // get the read-only flag readOnly, err = params.Mysqld.IsReadOnly() if err != nil { - return false, vterrors.Wrap(err, "failed to get read_only status") + return BackupUnusable, vterrors.Wrap(err, "failed to get read_only status") } superReadOnly, err = params.Mysqld.IsSuperReadOnly() if err != nil { - return false, vterrors.Wrap(err, "can't get super_read_only status") + return BackupUnusable, vterrors.Wrap(err, "can't get super_read_only status") } log.Infof("Flag values during full backup, read_only: %v, super_read_only:%t", readOnly, superReadOnly) @@ -431,7 +436,7 @@ func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params Bac if !superReadOnly { params.Logger.Infof("Enabling super_read_only on primary prior to backup") if _, err = params.Mysqld.SetSuperReadOnly(true); err != nil { - return false, vterrors.Wrap(err, "failed to enable super_read_only") + return BackupUnusable, vterrors.Wrap(err, "failed to enable super_read_only") } defer func() { // Resetting super_read_only back to its original value @@ -444,16 +449,16 @@ func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params Bac } replicationPosition, err = params.Mysqld.PrimaryPosition() if err != nil { - return false, vterrors.Wrap(err, "can't get position on primary") + return BackupUnusable, vterrors.Wrap(err, "can't get position on primary") } } else { // This is a replica if err := params.Mysqld.StopReplication(params.HookExtraEnv); err != nil { - return false, vterrors.Wrapf(err, "can't stop replica") + return BackupUnusable, vterrors.Wrapf(err, "can't stop replica") } replicaStatus, err := params.Mysqld.ReplicationStatus() if err != nil { - return false, vterrors.Wrap(err, "can't get replica status") + return BackupUnusable, vterrors.Wrap(err, "can't get replica status") } replicationPosition = replicaStatus.Position } @@ -461,23 +466,23 @@ func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params Bac gtidPurgedPosition, err := params.Mysqld.GetGTIDPurged(ctx) if err != nil { - return false, vterrors.Wrap(err, "can't get gtid_purged") + return BackupUnusable, vterrors.Wrap(err, "can't get gtid_purged") } serverUUID, err := params.Mysqld.GetServerUUID(ctx) if err != nil { - return false, vterrors.Wrap(err, "can't get server uuid") + return BackupUnusable, vterrors.Wrap(err, "can't get server uuid") } mysqlVersion, err := params.Mysqld.GetVersionString(ctx) if err != nil { - return false, vterrors.Wrap(err, "can't get MySQL version") + return BackupUnusable, vterrors.Wrap(err, "can't get MySQL version") } // check if we need to set innodb_fast_shutdown=0 for a backup safe for upgrades if params.UpgradeSafe { if _, err := params.Mysqld.FetchSuperQuery(ctx, "SET GLOBAL innodb_fast_shutdown=0"); err != nil { - return false, vterrors.Wrapf(err, "failed to disable fast shutdown") + return BackupUnusable, vterrors.Wrapf(err, "failed to disable fast shutdown") } } @@ -486,23 +491,26 @@ func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params Bac err = params.Mysqld.Shutdown(shutdownCtx, params.Cnf, true, params.MysqlShutdownTimeout) defer cancel() if err != nil { - return false, vterrors.Wrap(err, "can't shutdown mysqld") + return BackupUnusable, vterrors.Wrap(err, "can't shutdown mysqld") } // Backup everything, capture the error. backupErr := be.backupFiles(ctx, params, bh, replicationPosition, gtidPurgedPosition, replication.Position{}, "", nil, serverUUID, mysqlVersion, nil) - usable := backupErr == nil + backupResult := BackupUnusable + if backupErr == nil { + backupResult = BackupUsable + } // Try to restart mysqld, use background context in case we timed out the original context err = params.Mysqld.Start(context.Background(), params.Cnf) if err != nil { - return usable, vterrors.Wrap(err, "can't restart mysqld") + return backupResult, vterrors.Wrap(err, "can't restart mysqld") } // Resetting super_read_only back to its original value params.Logger.Infof("resetting mysqld super_read_only to %v", superReadOnly) if _, err := params.Mysqld.SetSuperReadOnly(superReadOnly); err != nil { - return usable, err + return backupResult, err } // Restore original mysqld state that we saved above. @@ -513,18 +521,18 @@ func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params Bac semiSyncSource, semiSyncReplica) err := params.Mysqld.SetSemiSyncEnabled(semiSyncSource, semiSyncReplica) if err != nil { - return usable, err + return backupResult, err } } if replicaStartRequired { params.Logger.Infof("restarting mysql replication") if err := params.Mysqld.StartReplication(params.HookExtraEnv); err != nil { - return usable, vterrors.Wrap(err, "cannot restart replica") + return backupResult, vterrors.Wrap(err, "cannot restart replica") } // this should be quick, but we might as well just wait if err := WaitForReplicationStart(params.Mysqld, replicationStartDeadline); err != nil { - return usable, vterrors.Wrap(err, "replica is not restarting") + return backupResult, vterrors.Wrap(err, "replica is not restarting") } // Wait for a reliable value for ReplicationLagSeconds from ReplicationStatus() @@ -542,16 +550,16 @@ func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params Bac pos, err := getPrimaryPosition(remoteCtx, tmc, params.TopoServer, params.Keyspace, params.Shard) // If we are unable to get the primary's position, return error. if err != nil { - return usable, err + return backupResult, err } if !replicationPosition.Equal(pos) { for { if err := ctx.Err(); err != nil { - return usable, err + return backupResult, err } status, err := params.Mysqld.ReplicationStatus() if err != nil { - return usable, err + return backupResult, err } newPos := status.Position if !newPos.Equal(replicationPosition) { @@ -562,7 +570,7 @@ func (be *BuiltinBackupEngine) executeFullBackup(ctx context.Context, params Bac } } - return usable, backupErr + return backupResult, backupErr } // backupFiles finds the list of files to backup, and creates the backup. diff --git a/go/vt/mysqlctl/fakebackupengine.go b/go/vt/mysqlctl/fakebackupengine.go index 2b8c3208ac5..d78282e6aff 100644 --- a/go/vt/mysqlctl/fakebackupengine.go +++ b/go/vt/mysqlctl/fakebackupengine.go @@ -41,7 +41,7 @@ type FakeBackupEngineExecuteBackupCall struct { } type FakeBackupEngineExecuteBackupReturn struct { - Ok bool + Res BackupResult Err error } @@ -59,14 +59,14 @@ func (be *FakeBackupEngine) ExecuteBackup( ctx context.Context, params BackupParams, bh backupstorage.BackupHandle, -) (bool, error) { +) (BackupResult, error) { be.ExecuteBackupCalls = append(be.ExecuteBackupCalls, FakeBackupEngineExecuteBackupCall{params, bh}) if be.ExecuteBackupDuration > 0 { time.Sleep(be.ExecuteBackupDuration) } - return be.ExecuteBackupReturn.Ok, be.ExecuteBackupReturn.Err + return be.ExecuteBackupReturn.Res, be.ExecuteBackupReturn.Err } func (be *FakeBackupEngine) ExecuteRestore( diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index 4aa2ab96b00..3f8491fdfb6 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -167,27 +167,27 @@ func closeFile(wc io.WriteCloser, fileName string, logger logutil.Logger, finalE } // ExecuteBackup runs a backup based on given params. This could be a full or incremental backup. -// The function returns a boolean that indicates if the backup is usable, and an overall error. -func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error) { +// The function returns a BackupResult that indicates the usability of the backup, and an overall error. +func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (BackupResult, error) { params.Logger.Infof("Executing Backup at %v for keyspace/shard %v/%v on tablet %v, concurrency: %v, compress: %v, incrementalFromPos: %v", params.BackupTime, params.Keyspace, params.Shard, params.TabletAlias, params.Concurrency, backupStorageCompress, params.IncrementalFromPos) return be.executeFullBackup(ctx, params, bh) } -// executeFullBackup returns a boolean that indicates if the backup is usable, +// executeFullBackup returns a BackupResult that indicates the usability of the backup, // and an overall error. -func (be *XtrabackupEngine) executeFullBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (complete bool, finalErr error) { +func (be *XtrabackupEngine) executeFullBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (backupResult BackupResult, finalErr error) { if params.IncrementalFromPos != "" { - return false, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "incremental backups not supported in xtrabackup engine.") + return BackupUnusable, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "incremental backups not supported in xtrabackup engine.") } if xtrabackupUser == "" { - return false, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "xtrabackupUser must be specified.") + return BackupUnusable, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "xtrabackupUser must be specified.") } // an extension is required when using an external compressor if backupStorageCompress && ExternalCompressorCmd != "" && ExternalCompressorExt == "" { - return false, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, + return BackupUnusable, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "flag --external-compressor-extension not provided when using an external compressor") } @@ -198,20 +198,20 @@ func (be *XtrabackupEngine) executeFullBackup(ctx context.Context, params Backup } if err != nil { - return false, vterrors.Wrap(err, "unable to obtain a connection to the database") + return BackupUnusable, vterrors.Wrap(err, "unable to obtain a connection to the database") } pos, err := conn.PrimaryPosition() if err != nil { - return false, vterrors.Wrap(err, "unable to obtain primary position") + return BackupUnusable, vterrors.Wrap(err, "unable to obtain primary position") } serverUUID, err := conn.GetServerUUID() if err != nil { - return false, vterrors.Wrap(err, "can't get server uuid") + return BackupUnusable, vterrors.Wrap(err, "can't get server uuid") } mysqlVersion, err := params.Mysqld.GetVersionString(ctx) if err != nil { - return false, vterrors.Wrap(err, "can't get MySQL version") + return BackupUnusable, vterrors.Wrap(err, "can't get MySQL version") } flavor := pos.GTIDSet.Flavor() @@ -229,14 +229,14 @@ func (be *XtrabackupEngine) executeFullBackup(ctx context.Context, params Backup params.Logger.Infof("Starting backup with %v stripe(s)", numStripes) replicationPosition, err := be.backupFiles(ctx, params, bh, backupFileName, numStripes, flavor) if err != nil { - return false, err + return BackupUnusable, err } // open the MANIFEST params.Logger.Infof("Writing backup MANIFEST") mwc, err := bh.AddFile(ctx, backupManifestFileName, backupstorage.FileSizeUnknown) if err != nil { - return false, vterrors.Wrapf(err, "cannot add %v to backup", backupManifestFileName) + return BackupUnusable, vterrors.Wrapf(err, "cannot add %v to backup", backupManifestFileName) } defer closeFile(mwc, backupManifestFileName, params.Logger, &finalErr) @@ -274,14 +274,14 @@ func (be *XtrabackupEngine) executeFullBackup(ctx context.Context, params Backup data, err := json.MarshalIndent(bm, "", " ") if err != nil { - return false, vterrors.Wrapf(err, "cannot JSON encode %v", backupManifestFileName) + return BackupUnusable, vterrors.Wrapf(err, "cannot JSON encode %v", backupManifestFileName) } if _, err := mwc.Write([]byte(data)); err != nil { - return false, vterrors.Wrapf(err, "cannot write %v", backupManifestFileName) + return BackupUnusable, vterrors.Wrapf(err, "cannot write %v", backupManifestFileName) } params.Logger.Infof("Backup completed") - return true, nil + return BackupUsable, nil } func (be *XtrabackupEngine) backupFiles(