Incremental backup: do not error on empty backup #15022

shlomi-noach · 2024-01-24T06:15:48Z

Description

An incremental backup backs up the delta of binary logs from last known good backup up to the point where the incremental backup is requested. An incremental backup flushes the binary logs and sees what binary log content needs to be backed-up.

It is possible for an incremental backup to be empty. This can happen when the database is sitting idly doing absolutely nothing from the last good backup to the point of incremental backup.

Up till now, this resulted with an error: vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "no binary logs to backup (increment is empty)").

But this is an undesired behavior. It is possible for databases to sit idly. If someone were to set up a cronjob running an incremental backup at, say, every 1min or every 5min, then they'd have to deal with a lot of errors that really are not indicative of anything wrong per se in the database or in the backup process.

The decision to error on an empty backup was never formalized and was left as an arbitrary behavior. We now fomalize the behavior for an empty backup:

An empty backup is possible for incremental backups only. It is irrelevant to full backups.
An empty backup happens when there's no new GTIDs since last good backup, ie no content in the binary logs that needs to be copied.
An empty backup exits with return code 0, ie not an error.
An empty backup clearly indicates "backup is empty" in its standard output (wording might change)
An empty backup leaves no files. There is no MANIFEST or otherwise any files to such backup.
An empty backup is expected to leave no trace, if possible. There will be a backup path (or the respective comparable on non-file system targets such as s3) created at the beginning of the process, but it is then removed once the backup is found to be empty. Such cleanup could fail, leaving an empty path with no files.
The backup only has a name, used internally, during its runtime. But once found to be empty, that name is discarded.

To that effect, this PR:

Formalizes a BackupResult enum (backup is unusable, is empty, or is usable)
Distinguishes between the two scenarios: no-binlogs-need-any-backup and cannot-find-binlogs-required-for-backup. The former is now an indication of an empty backup and the process exits without error.
Updated the PITR backup framework to test empty backup results, does not "wait-for" new backup manifest after empty backup, etc.

Related Issue(s)

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

…te MANIFEST file Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot · 2024-01-24T06:15:51Z

codecov · 2024-01-24T07:05:34Z

Codecov Report

Attention: 61 lines in your changes are missing coverage. Please review.

Comparison is base (b977e51) 47.64% compared to head (6744449) 47.68%.
Report is 9 commits behind head on main.

Files	Patch %	Lines
go/vt/mysqlctl/builtinbackupengine.go	16.66%	40 Missing ⚠️
go/vt/mysqlctl/xtrabackupengine.go	0.00%	14 Missing ⚠️
go/vt/mysqlctl/backup.go	30.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15022      +/-   ##
==========================================
+ Coverage   47.64%   47.68%   +0.04%     
==========================================
  Files        1151     1155       +4     
  Lines      239843   240169     +326     
==========================================
+ Hits       114268   114522     +254     
- Misses     116975   117045      +70     
- Partials     8600     8602       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mattlord

LGTM! I only had minor comments and suggestions so will let you address those as you prefer.

mattlord · 2024-01-24T22:00:25Z

go/vt/mysqlctl/backup.go

 		logger.Errorf2(err, "backup is not usable, aborting it")
 		finishErr = bh.AbortBackup(ctx)
+	case BackupEmpty:
+		logger.Infof("backup is empty")


Something like no new data to backup, skipping it is more informative, IMO.

mattlord · 2024-01-24T22:04:51Z

go/vt/mysqlctl/binlogs_gtid.go

+		// 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.


Slightly more explanative to note in this and similar places that the database performed no writes vs "did nothing" / "was silent" etc.

mattlord · 2024-01-24T22:06:13Z

go/vt/mysqlctl/binlogs_gtid.go

+		// 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())


It's more accurate to say that there are no new GTIDs to backup as flush logs could have been done N times (along with mysqld restarts etc). Right? Or we could say:

return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "no binary logs containing new GTIDs to backup. backupFromGTIDSet=%v, prevGTIDsUnion=%v", backupFromGTIDSet.String(), prevGTIDsUnion.String())

Reworded as "cannot find binary logs that cover requested GTID range"

mattlord · 2024-01-24T22:09:31Z

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.


We should update the comment.

Reworded as "The function returns a BackupResult that indicates the usability of the backup"

mattlord · 2024-01-24T22:13:03Z

go/vt/mysqlctl/builtinbackupengine.go

@@ -233,24 +233,25 @@ 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 boolean that indicates if the backup is usable, and an overall error.


This comment is also off now, right?

mattlord · 2024-01-24T22:14:15Z

go/vt/mysqlctl/builtinbackupengine.go

 	}
-	return true, nil
+	return BackupUsable, nil
 }

 // executeFullBackup returns a boolean that indicates if the backup is usable,


Comment off note again.

mattlord · 2024-01-24T22:16:25Z

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.


Another comment :)

mattlord · 2024-01-24T22:16:37Z

go/vt/mysqlctl/xtrabackupengine.go

@@ -177,17 +177,17 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara

 // executeFullBackup returns a boolean that indicates if the backup is usable,


You guess it

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2024-01-25T10:40:51Z

Docs PR: vitessio/website#1668

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vmg

Looks good to me!

shlomi-noach added 6 commits January 23, 2024 14:54

Allow empty incremental backup: exit without error, but do not genera…

248f026

…te MANIFEST file Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

test empty incremental backups

c6a9bcd

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

ExecuteBackup returns BackupResult enum rather than bool

f109074

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

remove empty backup

eb3d2b8

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

log info entry that backup is empty.

4364e45

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

test empty backups, catch empty backup message

5381d05

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Backup and Restore labels Jan 24, 2024

shlomi-noach requested review from deepthi and mattlord as code owners January 24, 2024 06:15

shlomi-noach requested a review from a team January 24, 2024 06:15

github-actions bot added this to the v19.0.0 milestone Jan 24, 2024

shlomi-noach removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Jan 24, 2024

mattlord approved these changes Jan 24, 2024

View reviewed changes

shlomi-noach added 4 commits January 25, 2024 07:52

rewording empty backup message

b680769

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

empty backup message extracted as a variable

0e85d1d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rewording

16585e4

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

reword comment

aa2909a

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach requested a review from a team January 25, 2024 06:03

shlomi-noach mentioned this pull request Jan 25, 2024

Docs: empty incremental backup vitessio/website#1668

Merged

shlomi-noach removed the NeedsWebsiteDocsUpdate What it says label Jan 25, 2024

adapt expected error message

eb20df0

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach requested review from a team and removed request for a team January 29, 2024 05:35

shlomi-noach added 2 commits January 30, 2024 09:10

resolved conflict

bd2182d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

comment

6744449

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vmg approved these changes Jan 30, 2024

View reviewed changes

shlomi-noach merged commit 9e27038 into vitessio:main Jan 30, 2024
101 of 102 checks passed

shlomi-noach deleted the empty-incremental-backup-no-error branch January 30, 2024 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incremental backup: do not error on empty backup #15022

Incremental backup: do not error on empty backup #15022

shlomi-noach commented Jan 24, 2024 •

edited

Loading

vitess-bot bot commented Jan 24, 2024

codecov bot commented Jan 24, 2024 •

edited

Loading

mattlord left a comment

mattlord Jan 24, 2024

mattlord Jan 24, 2024

shlomi-noach Jan 25, 2024

mattlord Jan 24, 2024

shlomi-noach Jan 25, 2024

mattlord Jan 24, 2024

shlomi-noach Jan 25, 2024

mattlord Jan 24, 2024

shlomi-noach Jan 25, 2024

mattlord Jan 24, 2024

shlomi-noach Jan 25, 2024

mattlord Jan 24, 2024

shlomi-noach Jan 25, 2024

mattlord Jan 24, 2024

shlomi-noach Jan 25, 2024

shlomi-noach commented Jan 25, 2024

vmg left a comment

		@@ -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.

		@@ -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.

		@@ -177,17 +177,17 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara

		// executeFullBackup returns a boolean that indicates if the backup is usable,

Incremental backup: do not error on empty backup #15022

Incremental backup: do not error on empty backup #15022

Conversation

shlomi-noach commented Jan 24, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 24, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

mattlord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach commented Jan 25, 2024

vmg left a comment

Choose a reason for hiding this comment

shlomi-noach commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading