Backup: `--incremental-from-pos` supports backup name #14923

shlomi-noach · 2024-01-10T08:56:43Z

Description

Running an incremental backup, up till now the flag --incremental-from-pos supported:

A position
The magic word "auto"

It now also supports a backup name. If such named backup exists, then the incremental backup will start with the position of the named backup.

A bit of refactoring made, especially around endtoend tests, to accommodate.

Docs: ~~vitessio/website#1662~~ vitessio/website#1667

Related Issue(s)

Fixes #14905

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

Signed-off-by: Shlomi Noach <[email protected]>

…Pos public Signed-off-by: Shlomi Noach <[email protected]>

Signed-off-by: Shlomi Noach <[email protected]>

vitess-bot · 2024-01-10T08:56:46Z

codecov · 2024-01-10T09:22:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@3091de1). Click here to learn what that means.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #14923   +/-   ##
=======================================
  Coverage        ?   47.64%           
=======================================
  Files           ?     1151           
  Lines           ?   239843           
  Branches        ?        0           
=======================================
  Hits            ?   114281           
  Misses          ?   116963           
  Partials        ?     8599

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

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach · 2024-01-14T06:15:10Z

Documented in ~~vitessio/website#1662~~ vitessio/website#1667

mattlord

LGTM. Just a few minor suggestions.

At first I wasn't crazy about re-using the existing flag, but after reading through it and thinking about it I think it's fine. The backup name is a pointer to a pos, whatever pos that file had. We'll just need to document it of course here: https://vitess.io/docs/19.0/user-guides/operating-vitess/backup-and-restore/creating-a-backup/#create-an-incremental-backup-with-vtctl

The only reason that I didn't approve here, is that we should update the --incremental-from-pos/--incremental_from_pos flag related text:

Unless I'm missing something here?

go/vt/mysqlctl/backupengine.go

go/vt/mysqlctl/builtinbackupengine.go

Co-authored-by: Matt Lord <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

…/vitess into incremental-from-backup-name Signed-off-by: Shlomi Noach <[email protected]>

Co-authored-by: Matt Lord <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

…/vitess into incremental-from-backup-name Signed-off-by: Shlomi Noach <[email protected]>

Co-authored-by: Matt Lord <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

…/vitess into incremental-from-backup-name Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach · 2024-01-18T08:51:20Z

The only reason that I didn't approve here, is that we should update the --incremental-from-pos/--incremental_from_pos flag related text:

I'm not sure what's going on. I'm absolutely certain I updated the flag docs in all places 😱 . Anyway, updating again.

shlomi-noach · 2024-01-18T08:52:59Z

At first I wasn't crazy about re-using the existing flag, but after reading through it and thinking about it I think it's fine.

I think ideally the flag should be renamed --incremental-from, and then it makes a ton of sense. I can work on transitioning the flag through two versions, in a followup PR.

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach · 2024-01-18T09:23:10Z

Updated all CLI docs.

Signed-off-by: Shlomi Noach <[email protected]>

mattlord

Thanks, @shlomi-noach ! ❤️

shlomi-noach · 2024-01-24T15:01:52Z

New docs PR in vitessio/website#1667, cleaner.

deepthi

All my comments are minor. Please feel free to merge once they are addressed.

go/cmd/vtbackup/cli/vtbackup.go

go/cmd/vtctldclient/command/backups.go

deepthi · 2024-01-29T12:18:17Z

go/cmd/vtctldclient/command/backups.go


 	Backup.Flags().BoolVar(&backupOptions.UpgradeSafe, "upgrade-safe", false, "Whether to use innodb_fast_shutdown=0 for the backup so it is safe to use for MySQL upgrades.")
 	Root.AddCommand(Backup)

 	BackupShard.Flags().BoolVar(&backupShardOptions.AllowPrimary, "allow-primary", false, "Allow the primary of a shard to be used for the backup. WARNING: If using the builtin backup engine, this will shutdown mysqld on the primary and stop writes for the duration of the backup.")
 	BackupShard.Flags().Int32Var(&backupShardOptions.Concurrency, "concurrency", 4, "Specifies the number of compression/checksum jobs to run simultaneously.")
-	BackupShard.Flags().StringVar(&backupShardOptions.IncrementalFromPos, "incremental-from-pos", "", "Position of previous backup. Default: empty. If given, then this backup becomes an incremental backup from given position. If value is 'auto', backup taken from last successful backup position")
+	BackupShard.Flags().StringVar(&backupShardOptions.IncrementalFromPos, "incremental-from-pos", "", "Position, or name of backup from which to create an incremental backup. Default: empty. If given, then this backup becomes an incremental backup from given position or given backup. If value is 'auto', backup taken from last successful backup position.")


Make this consistent with the other places.

deepthi · 2024-01-29T12:21:24Z

go/vt/mysqlctl/builtinbackupengine.go

@@ -291,6 +294,19 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par
 	if err != nil {
 		return false, vterrors.Wrapf(err, "cannot get binary logs in incremental backup")
 	}
+
+	// @@gtid_purged


Is this comment line needed? Doesn't seem to add anything.

deepthi · 2024-01-29T12:21:46Z

go/vt/vtctl/backup.go

@@ -72,7 +72,7 @@ func init() {
 func commandBackup(ctx context.Context, wr *wrangler.Wrangler, subFlags *pflag.FlagSet, args []string) error {
 	concurrency := subFlags.Int32("concurrency", 4, "Specifies the number of compression/checksum jobs to run simultaneously")
 	allowPrimary := subFlags.Bool("allow_primary", false, "Allows backups to be taken on primary. Warning!! If you are using the builtin backup engine, this will shutdown your primary mysql for as long as it takes to create a backup.")
-	incrementalFromPos := subFlags.String("incremental_from_pos", "", "Position of previous backup. Default: empty. If given, then this backup becomes an incremental backup from given position. If value is 'auto', backup taken from last successful backup position")
+	incrementalFromPos := subFlags.String("incremental_from_pos", "", "Position, or name of backup from which to create an incremental backup. Default: empty. If given, then this backup becomes an incremental backup from given position or given backup. If value is 'auto', backup taken from last successful backup position.")


Make this consistent with all other flag docs.

deepthi · 2024-01-29T12:21:55Z

go/vt/vtctl/backup.go

@@ -114,7 +114,7 @@ func (b *backupEventStreamLogger) Send(resp *vtctldatapb.BackupResponse) error {
 func commandBackupShard(ctx context.Context, wr *wrangler.Wrangler, subFlags *pflag.FlagSet, args []string) error {
 	concurrency := subFlags.Int32("concurrency", 4, "Specifies the number of compression/checksum jobs to run simultaneously")
 	allowPrimary := subFlags.Bool("allow_primary", false, "Whether to use primary tablet for backup. Warning!! If you are using the builtin backup engine, this will shutdown your primary mysql for as long as it takes to create a backup.")
-	incrementalFromPos := subFlags.String("incremental_from_pos", "", "Position of previous backup. Default: empty. If given, then this backup becomes an incremental backup from given position. If value is 'auto', backup taken from last successful backup position")
+	incrementalFromPos := subFlags.String("incremental_from_pos", "", "Position, or name of backup from which to create an incremental backup. Default: empty. If given, then this backup becomes an incremental backup from given position or given backup. If value is 'auto', backup taken from last successful backup position.")


And this too.

Co-authored-by: Deepthi Sigireddi <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach added 6 commits January 9, 2024 09:35

Backup manifest: add backup name

f4e851e

Signed-off-by: Shlomi Noach <[email protected]>

typo

2e6e914

Signed-off-by: Shlomi Noach <[email protected]>

minor refactor: relocated function downwards

37c9d16

Signed-off-by: Shlomi Noach <[email protected]>

--incremental-from-pos support an existing backup name

456cce6

Signed-off-by: Shlomi Noach <[email protected]>

hide function that don't need public access. Make AutoIncrementalFrom…

558e485

…Pos public Signed-off-by: Shlomi Noach <[email protected]>

backup_pitr framework: test incremental from backup name

05ca90c

Signed-off-by: Shlomi Noach <[email protected]>

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

shlomi-noach requested review from deepthi and mattlord as code owners January 10, 2024 08:56

github-actions bot added this to the v19.0.0 milestone Jan 10, 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 10, 2024

shlomi-noach requested a review from a team January 11, 2024 06:37

Updated command line doclets

9e545ec

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach mentioned this pull request Jan 14, 2024

Docs: Backup/BackupShard --incremental-from-pos accepts backup name vitessio/website#1662

Closed

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

mattlord reviewed Jan 17, 2024

View reviewed changes

go/vt/mysqlctl/backupengine.go Outdated Show resolved Hide resolved

go/vt/mysqlctl/builtinbackupengine.go Outdated Show resolved Hide resolved

go/vt/mysqlctl/builtinbackupengine.go Outdated Show resolved Hide resolved

shlomi-noach and others added 4 commits January 18, 2024 10:46

Update go/vt/mysqlctl/backupengine.go

77850e9

Co-authored-by: Matt Lord <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

Merge branch 'incremental-from-backup-name' of github.com:planetscale…

347b478

…/vitess into incremental-from-backup-name Signed-off-by: Shlomi Noach <[email protected]>

Update go/vt/mysqlctl/builtinbackupengine.go

b501256

Co-authored-by: Matt Lord <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

Merge branch 'incremental-from-backup-name' of github.com:planetscale…

8e995c2

…/vitess into incremental-from-backup-name Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach and others added 2 commits January 18, 2024 10:49

Update go/vt/mysqlctl/builtinbackupengine.go

685e53d

Co-authored-by: Matt Lord <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

Merge branch 'incremental-from-backup-name' of github.com:planetscale…

20d87e0

…/vitess into incremental-from-backup-name Signed-off-by: Shlomi Noach <[email protected]>

update docs

edf5e64

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach requested review from ajm188 and rohit-nayak-ps as code owners January 18, 2024 09:23

fix vtbackup CLI expected output

a85c612

Signed-off-by: Shlomi Noach <[email protected]>

mattlord approved these changes Jan 22, 2024

View reviewed changes

shlomi-noach mentioned this pull request Jan 24, 2024

Docs: Backup/BackupShard --incremental-from-pos accepts backup name vitessio/website#1667

Merged

frouioui assigned deepthi Jan 24, 2024

shlomi-noach requested a review from a team January 28, 2024 14:51

deepthi approved these changes Jan 29, 2024

View reviewed changes

shlomi-noach and others added 3 commits January 29, 2024 15:05

Update go/cmd/vtctldclient/command/backups.go

de304af

Co-authored-by: Deepthi Sigireddi <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

Update go/cmd/vtbackup/cli/vtbackup.go

3ba33bb

Co-authored-by: Deepthi Sigireddi <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

update --incremental_from_pos flag doclet

20b1e46

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach merged commit b977e51 into vitessio:main Jan 29, 2024
102 checks passed

shlomi-noach deleted the incremental-from-backup-name branch January 29, 2024 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backup: `--incremental-from-pos` supports backup name #14923

Backup: `--incremental-from-pos` supports backup name #14923

shlomi-noach commented Jan 10, 2024 •

edited

Loading

vitess-bot bot commented Jan 10, 2024

codecov bot commented Jan 10, 2024 •

edited

Loading

shlomi-noach commented Jan 14, 2024 •

edited

Loading

mattlord left a comment

shlomi-noach commented Jan 18, 2024

shlomi-noach commented Jan 18, 2024

shlomi-noach commented Jan 18, 2024

mattlord left a comment

shlomi-noach commented Jan 24, 2024

deepthi left a comment

deepthi Jan 29, 2024

shlomi-noach Jan 29, 2024

deepthi Jan 29, 2024

shlomi-noach Jan 29, 2024

deepthi Jan 29, 2024

shlomi-noach Jan 29, 2024

deepthi Jan 29, 2024

shlomi-noach Jan 29, 2024

Backup: --incremental-from-pos supports backup name #14923

Backup: --incremental-from-pos supports backup name #14923

Conversation

shlomi-noach commented Jan 10, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 10, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jan 10, 2024 • edited Loading

Codecov Report

shlomi-noach commented Jan 14, 2024 • edited Loading

mattlord left a comment

Choose a reason for hiding this comment

shlomi-noach commented Jan 18, 2024

shlomi-noach commented Jan 18, 2024

shlomi-noach commented Jan 18, 2024

mattlord left a comment

Choose a reason for hiding this comment

shlomi-noach commented Jan 24, 2024

deepthi 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

Backup: `--incremental-from-pos` supports backup name #14923

Backup: `--incremental-from-pos` supports backup name #14923

shlomi-noach commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

shlomi-noach commented Jan 14, 2024 •

edited

Loading