From 248f0267944c635728a4f2322bcccd17fe39ad29 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 23 Jan 2024 14:54:49 +0200 Subject: [PATCH 01/12] Allow empty incremental backup: exit without error, but do not generate MANIFEST file Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/binlogs_gtid.go | 14 +++++++++++--- go/vt/mysqlctl/binlogs_gtid_test.go | 13 +++++++------ go/vt/mysqlctl/builtinbackupengine.go | 5 +++++ 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/go/vt/mysqlctl/binlogs_gtid.go b/go/vt/mysqlctl/binlogs_gtid.go index 3ea48663578..d1258f32117 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 completely silent during that period. + // We have nothing to backup. The backup is empty. + return nil, "", "", nil + } + return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "no binary logs to backup. 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..628454f7a76 100644 --- a/go/vt/mysqlctl/binlogs_gtid_test.go +++ b/go/vt/mysqlctl/binlogs_gtid_test.go @@ -85,7 +85,7 @@ 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", @@ -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 57f55cefb2f..6f7114ecf02 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -233,6 +233,7 @@ 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. +// The function returns a boolean that indicates if the backup is usable, and an overall error. func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error) { // Collect MySQL status: // UUID @@ -316,6 +317,10 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par if err != nil { return false, vterrors.Wrapf(err, "cannot get binary logs to backup in incremental backup") } + if len(binaryLogsToBackup) == 0 { + // Empty backup. + return true, nil + } incrementalBackupFromPosition, err := replication.ParsePosition(replication.Mysql56FlavorID, incrementalBackupFromGTID) if err != nil { return false, vterrors.Wrapf(err, "cannot parse position %v", incrementalBackupFromGTID) From c6a9bcd2f424ab34e9cd33ec387fd9873d75db7a Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 23 Jan 2024 14:55:19 +0200 Subject: [PATCH 02/12] test empty incremental backups Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../backup/vtctlbackup/backup_utils.go | 19 +++++- .../backup/vtctlbackup/pitr_test_framework.go | 66 +++++++++++++------ 2 files changed, 62 insertions(+), 23 deletions(-) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index e470652f7ee..40158e0bbc1 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -1218,6 +1218,13 @@ func readManifestFile(t *testing.T, backupLocation string) (manifest *mysqlctl.B return manifest } +func expectNoManifest(t *testing.T, backupLocation string) { + // reading manifest + fullPath := backupLocation + "/MANIFEST" + _, err := os.ReadFile(fullPath) + assert.Truef(t, os.IsNotExist(err), "Expected ErrNotExist, got %v", err) +} + func TestReplicaFullBackup(t *testing.T, replicaIndex int) (manifest *mysqlctl.BackupManifest) { backups := vtctlBackupReplicaNoDestroyNoWrites(t, replicaIndex) @@ -1255,8 +1262,9 @@ func waitForNumBackups(t *testing.T, expectNumBackups int) []string { } } -func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incrementalFromPos replication.Position, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) { +func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incrementalFromPos replication.Position, expectEmpty bool, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) { numBackups := len(waitForNumBackups(t, -1)) + incrementalFromPosArg := "auto" if !incrementalFromPos.IsZero() { incrementalFromPosArg = replication.EncodePosition(incrementalFromPos) @@ -1275,12 +1283,17 @@ func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incre verifyTabletBackupStats(t, replica.VttabletProcess.GetVars()) backupName = backups[len(backups)-1] backupLocation := localCluster.CurrentVTDATAROOT + "/backups/" + shardKsName + "/" + backupName + + if expectEmpty { + expectNoManifest(t, backupLocation) + return nil, backupName + } return readManifestFile(t, backupLocation), backupName } -func TestReplicaIncrementalBackup(t *testing.T, replicaIndex int, incrementalFromPos replication.Position, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) { +func TestReplicaIncrementalBackup(t *testing.T, replicaIndex int, incrementalFromPos replication.Position, 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 8b9014e7f8c..0ec1e6f41b3 100644 --- a/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go +++ b/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go @@ -128,26 +128,33 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase) writeBeforeBackup bool fromFullPosition bool autoPosition bool + expectEmpty bool expectError string }{ { name: "first incremental backup", }, { - name: "fail1", - expectError: "no binary logs to backup", + name: "empty1", + expectEmpty: true, }, { - name: "fail2", - expectError: "no binary logs to backup", + name: "empty2", + autoPosition: true, + expectEmpty: true, + }, + { + name: "empty3", + expectEmpty: true, }, { name: "make writes, succeed", writeBeforeBackup: true, }, { - name: "fail, no binary logs to backup", - expectError: "no binary logs to backup", + name: "empty again", + autoPosition: true, + expectEmpty: true, }, { name: "make writes again, succeed", @@ -159,9 +166,9 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase) autoPosition: true, }, { - name: "fail auto position, no binary logs to backup", + name: "empty again, based on auto position", autoPosition: true, - expectError: "no binary logs to backup", + expectEmpty: true, }, { name: "auto position, make writes again, succeed", @@ -200,10 +207,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 }() @@ -324,26 +336,33 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes writeBeforeBackup bool fromFullPosition bool autoPosition bool + expectEmpty bool expectError string }{ { name: "first incremental backup", }, { - name: "fail1", - expectError: "no binary logs to backup", + name: "empty1", + expectEmpty: true, }, { - name: "fail2", - expectError: "no binary logs to backup", + name: "empty2", + autoPosition: true, + expectEmpty: true, + }, + { + name: "empty3", + expectEmpty: true, }, { name: "make writes, succeed", writeBeforeBackup: true, }, { - name: "fail, no binary logs to backup", - expectError: "no binary logs to backup", + name: "empty again", + autoPosition: true, + expectEmpty: true, }, { name: "make writes again, succeed", @@ -355,9 +374,9 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes autoPosition: true, }, { - name: "fail auto position, no binary logs to backup", + name: "empty again, based on auto position", autoPosition: true, - expectError: "no binary logs to backup", + expectEmpty: true, }, { name: "auto position, make writes again, succeed", @@ -393,11 +412,17 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes incrementalFromPos = fullBackupPos } } - manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectError) + expectEmpty := !tc.writeBeforeBackup + manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, 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 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. @@ -664,10 +689,11 @@ func ExecTestIncrementalBackupOnTwoTablets(t *testing.T, tcase *PITRTestCase) { lastBackupPos = fullBackupPos case operationIncrementalBackup: var incrementalFromPos replication.Position // keep zero, we will use "auto" - manifest, _ := TestReplicaIncrementalBackup(t, tc.replicaIndex, incrementalFromPos, tc.expectError) + manifest, _ := TestReplicaIncrementalBackup(t, tc.replicaIndex, incrementalFromPos, false /* expectEmpty */, tc.expectError) if tc.expectError != "" { return } + require.NotNil(t, manifest) defer func() { lastBackupPos = manifest.Position }() From f109074748dfc8de308fe1085dd01d1b0988cb20 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 23 Jan 2024 15:33:24 +0200 Subject: [PATCH 03/12] ExecuteBackup returns BackupResult enum rather than bool Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../backup/vtctlbackup/pitr_test_framework.go | 5 +- go/vt/mysqlctl/backup.go | 11 ++- go/vt/mysqlctl/backup_blackbox_test.go | 20 ++--- go/vt/mysqlctl/backupengine.go | 10 ++- go/vt/mysqlctl/builtinbackupengine.go | 85 ++++++++++--------- go/vt/mysqlctl/fakebackupengine.go | 6 +- go/vt/mysqlctl/xtrabackupengine.go | 28 +++--- 7 files changed, 89 insertions(+), 76 deletions(-) diff --git a/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go b/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go index 0ec1e6f41b3..b7ba0b0d917 100644 --- a/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go +++ b/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go @@ -412,12 +412,11 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes incrementalFromPos = fullBackupPos } } - expectEmpty := !tc.writeBeforeBackup - manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, expectEmpty, tc.expectError) + manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectEmpty, tc.expectError) if tc.expectError != "" { return } - if expectEmpty { + if tc.expectEmpty { assert.Nil(t, manifest) return } diff --git a/go/vt/mysqlctl/backup.go b/go/vt/mysqlctl/backup.go index 5a9706b07f8..2fb01e0c52e 100644 --- a/go/vt/mysqlctl/backup.go +++ b/go/vt/mysqlctl/backup.go @@ -168,14 +168,17 @@ 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: + finishErr = bh.EndBackup(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 cf6065cd733..45a355f51ba 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/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index 6f7114ecf02..41a0fcac89e 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -204,7 +204,7 @@ 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) { +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) @@ -234,16 +234,16 @@ func getIncrementalFromPosGTIDSet(incrementalFromPos string) (replication.Mysql5 // - A valid position // - "auto", indicating the incremental backup should begin with last successful backup end position. // The function returns a boolean that indicates if the backup is usable, and an overall error. -func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, 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") } var fromBackupName string @@ -251,7 +251,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) @@ -274,7 +274,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 backip. @@ -286,18 +286,18 @@ 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") } // gtid_purged is important information. The restore flow uses this info to to complement binary logs' Previous-GTIDs. // It is important to only get gtid_purged _after_ we've rotated into the new binary log, because the `FLUSH BINARY LOGS` // 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) { @@ -315,19 +315,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 true, nil + 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 @@ -342,16 +342,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{ @@ -370,14 +370,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, // 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) @@ -401,17 +401,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) @@ -421,7 +421,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 @@ -434,16 +434,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 } @@ -451,23 +451,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") } } @@ -476,23 +476,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. @@ -503,18 +506,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() @@ -532,16 +535,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) { @@ -552,7 +555,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 a61551c3dcf..dd171e66b73 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -168,7 +168,7 @@ 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) { +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) @@ -177,17 +177,17 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara // executeFullBackup returns a boolean that indicates if the backup is usable, // 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) @@ -273,14 +273,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( From eb3d2b89f589818a10997377ad57e2697ba84f0a Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 23 Jan 2024 16:11:27 +0200 Subject: [PATCH 04/12] remove empty backup Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/backup.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/mysqlctl/backup.go b/go/vt/mysqlctl/backup.go index 2fb01e0c52e..821999c7893 100644 --- a/go/vt/mysqlctl/backup.go +++ b/go/vt/mysqlctl/backup.go @@ -176,7 +176,7 @@ func Backup(ctx context.Context, params BackupParams) error { logger.Errorf2(err, "backup is not usable, aborting it") finishErr = bh.AbortBackup(ctx) case BackupEmpty: - finishErr = bh.EndBackup(ctx) + finishErr = bh.AbortBackup(ctx) case BackupUsable: finishErr = bh.EndBackup(ctx) } From 4364e450db6ce4771c2413664f7f6d39d3e5b611 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 23 Jan 2024 18:34:26 +0200 Subject: [PATCH 05/12] log info entry that backup is empty. Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/backup.go | 1 + 1 file changed, 1 insertion(+) diff --git a/go/vt/mysqlctl/backup.go b/go/vt/mysqlctl/backup.go index 821999c7893..345efcbc988 100644 --- a/go/vt/mysqlctl/backup.go +++ b/go/vt/mysqlctl/backup.go @@ -176,6 +176,7 @@ func Backup(ctx context.Context, params BackupParams) error { logger.Errorf2(err, "backup is not usable, aborting it") finishErr = bh.AbortBackup(ctx) case BackupEmpty: + logger.Infof("backup is empty") finishErr = bh.AbortBackup(ctx) case BackupUsable: finishErr = bh.EndBackup(ctx) From 5381d05ed42bd189fb0e23772f533f3d0e14382b Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 23 Jan 2024 18:34:42 +0200 Subject: [PATCH 06/12] test empty backups, catch empty backup message Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../backup/vtctlbackup/backup_utils.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index 40158e0bbc1..879b98d05ef 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -1218,13 +1218,6 @@ func readManifestFile(t *testing.T, backupLocation string) (manifest *mysqlctl.B return manifest } -func expectNoManifest(t *testing.T, backupLocation string) { - // reading manifest - fullPath := backupLocation + "/MANIFEST" - _, err := os.ReadFile(fullPath) - assert.Truef(t, os.IsNotExist(err), "Expected ErrNotExist, got %v", err) -} - func TestReplicaFullBackup(t *testing.T, replicaIndex int) (manifest *mysqlctl.BackupManifest) { backups := vtctlBackupReplicaNoDestroyNoWrites(t, replicaIndex) @@ -1277,17 +1270,18 @@ func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incre } require.NoErrorf(t, err, "output: %v", output) + if expectEmpty { + require.Contains(t, output, "backup is empty") + 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 - if expectEmpty { - expectNoManifest(t, backupLocation) - return nil, backupName - } + backupLocation := localCluster.CurrentVTDATAROOT + "/backups/" + shardKsName + "/" + backupName return readManifestFile(t, backupLocation), backupName } From b680769641d95c134996aa5eb939280fb7adaa8e Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 25 Jan 2024 07:52:10 +0200 Subject: [PATCH 07/12] rewording empty backup message Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/test/endtoend/backup/vtctlbackup/backup_utils.go | 2 +- go/vt/mysqlctl/backup.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index 879b98d05ef..5c5659b0e5c 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -1271,7 +1271,7 @@ func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incre require.NoErrorf(t, err, "output: %v", output) if expectEmpty { - require.Contains(t, output, "backup is empty") + require.Contains(t, output, "no new data to backup, skipping it") return nil, "" } diff --git a/go/vt/mysqlctl/backup.go b/go/vt/mysqlctl/backup.go index 345efcbc988..bc48e64e492 100644 --- a/go/vt/mysqlctl/backup.go +++ b/go/vt/mysqlctl/backup.go @@ -176,7 +176,7 @@ func Backup(ctx context.Context, params BackupParams) error { logger.Errorf2(err, "backup is not usable, aborting it") finishErr = bh.AbortBackup(ctx) case BackupEmpty: - logger.Infof("backup is empty") + logger.Infof("no new data to backup, skipping it") finishErr = bh.AbortBackup(ctx) case BackupUsable: finishErr = bh.EndBackup(ctx) From 0e85d1da8b644f27892be37d58ad1564154806f2 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 25 Jan 2024 07:53:54 +0200 Subject: [PATCH 08/12] empty backup message extracted as a variable Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/test/endtoend/backup/vtctlbackup/backup_utils.go | 2 +- go/vt/mysqlctl/backup.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index 5c5659b0e5c..212bb725a49 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -1271,7 +1271,7 @@ func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incre require.NoErrorf(t, err, "output: %v", output) if expectEmpty { - require.Contains(t, output, "no new data to backup, skipping it") + require.Contains(t, output, mysqlctl.EmptyBackupMessage) return nil, "" } diff --git a/go/vt/mysqlctl/backup.go b/go/vt/mysqlctl/backup.go index bc48e64e492..c6b83774235 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() { @@ -176,7 +178,7 @@ func Backup(ctx context.Context, params BackupParams) error { logger.Errorf2(err, "backup is not usable, aborting it") finishErr = bh.AbortBackup(ctx) case BackupEmpty: - logger.Infof("no new data to backup, skipping it") + logger.Infof(EmptyBackupMessage) finishErr = bh.AbortBackup(ctx) case BackupUsable: finishErr = bh.EndBackup(ctx) From 16585e4b7a6e8b8702968d05bcfe037e312c388d Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 25 Jan 2024 07:58:52 +0200 Subject: [PATCH 09/12] rewording Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/binlogs_gtid.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/go/vt/mysqlctl/binlogs_gtid.go b/go/vt/mysqlctl/binlogs_gtid.go index d1258f32117..61ca0a87f70 100644 --- a/go/vt/mysqlctl/binlogs_gtid.go +++ b/go/vt/mysqlctl/binlogs_gtid.go @@ -137,11 +137,11 @@ func ChooseBinlogsForIncrementalBackup( // 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 completely silent during that period. - // We have nothing to backup. The backup is empty. + // 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, "no binary logs to backup. backupFromGTIDSet=%v, prevGTIDsUnion=%v", backupFromGTIDSet.String(), prevGTIDsUnion.String()) + 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 From aa2909ab1d32f1750fd013fbfa51955208dea911 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 25 Jan 2024 08:02:32 +0200 Subject: [PATCH 10/12] reword comment Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/builtinbackupengine.go | 6 +++--- go/vt/mysqlctl/xtrabackupengine.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index 41a0fcac89e..57b8f36d982 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -203,7 +203,7 @@ 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. +// 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,7 +233,7 @@ 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. -// The function returns a boolean that indicates if the backup is usable, and an overall 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 @@ -375,7 +375,7 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par 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) (BackupResult, error) { diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index dd171e66b73..ef434047718 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -167,7 +167,7 @@ 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. +// 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) @@ -175,7 +175,7 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara 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) (backupResult BackupResult, finalErr error) { if params.IncrementalFromPos != "" { From eb20df0aa7031329d66ab435a0a2507c32babf9d Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 28 Jan 2024 16:50:09 +0200 Subject: [PATCH 11/12] adapt expected error message Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/binlogs_gtid_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/mysqlctl/binlogs_gtid_test.go b/go/vt/mysqlctl/binlogs_gtid_test.go index 628454f7a76..ec1c220fd39 100644 --- a/go/vt/mysqlctl/binlogs_gtid_test.go +++ b/go/vt/mysqlctl/binlogs_gtid_test.go @@ -91,7 +91,7 @@ func TestChooseBinlogsForIncrementalBackup(t *testing.T) { 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", From 674444951303d6896d52eac5228d5b76e31f18b1 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 30 Jan 2024 10:04:56 +0200 Subject: [PATCH 12/12] comment Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go | 4 ++-- go/vt/mysqlctl/backup.go | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go b/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go index da2be0e2321..6270c023eab 100644 --- a/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go +++ b/go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go @@ -168,7 +168,7 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase) }, { name: "empty again", - incrementalFrom: incrementalFromPosAuto, + incrementalFrom: incrementalFromPosPosition, expectEmpty: true, }, { @@ -393,7 +393,7 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes }, { name: "empty again", - incrementalFrom: incrementalFromPosAuto, + incrementalFrom: incrementalFromPosPosition, expectEmpty: true, }, { diff --git a/go/vt/mysqlctl/backup.go b/go/vt/mysqlctl/backup.go index c6b83774235..fb401966c50 100644 --- a/go/vt/mysqlctl/backup.go +++ b/go/vt/mysqlctl/backup.go @@ -179,6 +179,8 @@ func Backup(ctx context.Context, params BackupParams) error { 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)