select backup engine in Backup() and ignore engines in RestoreFromBackup() #16428

rvrangel · 2024-07-18T15:04:37Z

Description

This PR will allow calls to the vttablet and vtctld Backup() API to specify which backup engine to be used. You might want to specify flags for both and have a default one defined in --backup_engine_implementation, but then you do not need to restart the vttablet to be able to switch from one engine to the other.

Calls to Backup() without the backup engine will just use the default backup engine the vttablet was started with.

It also implements a filter on RestoreFromBackup() as to allow certain backup engine to be ignored (e.g. you take backups with both the xtrabackupengine and the mysqlshell engine proposed in #16294) so as to have both physical and logical backups, but you only want to be able to restore from the physical backups unless you specify a particular backup.

Related Issue(s)

Fixes #16429

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: Renan Rangel <rrangel@slack-corp.com>

vitess-bot · 2024-07-18T15:04:40Z

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

codecov · 2024-08-15T15:45:11Z

Codecov Report

Attention: Patch coverage is 35.71429% with 27 lines in your changes missing coverage. Please review.

Project coverage is 69.45%. Comparing base (3743f09) to head (a6dff4b).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
go/cmd/vtctldclient/command/backups.go	18.18%	9 Missing ⚠️
go/vt/vttablet/tabletmanager/restore.go	0.00%	6 Missing ⚠️
go/vt/vttablet/tabletmanager/rpc_backup.go	0.00%	5 Missing ⚠️
go/vt/mysqlctl/backupengine.go	50.00%	3 Missing ⚠️
go/vt/mysqlctl/builtinbackupengine.go	0.00%	1 Missing ⚠️
go/vt/mysqlctl/mysqlshellbackupengine.go	0.00%	1 Missing ⚠️
go/vt/mysqlctl/xtrabackupengine.go	0.00%	1 Missing ⚠️
go/vt/vttablet/tabletmanager/tm_init.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16428      +/-   ##
==========================================
+ Coverage   69.44%   69.45%   +0.01%     
==========================================
  Files        1571     1571              
  Lines      203004   203021      +17     
==========================================
+ Hits       140974   141013      +39     
+ Misses      62030    62008      -22

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

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

…ackup Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

shlomi-noach

Thank you for this, I've left a few comments throughout the code.
Some major points:

cluster_endtoend_vtctlbackup_sharded_clustertest_heavy.yml may not be modified into using xtrabackup because this means we're losing trust in Vitess' functionality on vanilla MySQL.
Also, this is (as the name suggests) on of the heaviest endtoend tests we have; consider creating a new and shorter test.
Like @deepthi , I prefer allow lists over deny lists. If there's a strong argument for deny lists, please let us know. But IMO if next week someone introduces a new backup method which does not match your requirements, you have to rush to edit your deployment/scripts/tools to ensure it's not being used. Whereas if next week someone introduces a new backup method which does comply with your requirements, then you can at leisure add it.
Let's use meaningful data in tests, and more explicit tests over such data.

shlomi-noach · 2024-08-28T05:18:06Z

go/cmd/vtctldclient/command/backups.go

@@ -299,6 +312,7 @@ func init() {
 	Root.AddCommand(RemoveBackup)

 	RestoreFromBackup.Flags().StringVarP(&restoreFromBackupOptions.BackupTimestamp, "backup-timestamp", "t", "", "Use the backup taken at, or closest before, this timestamp. Omit to use the latest backup. Timestamp format is \"YYYY-mm-DD.HHMMSS\".")
+	RestoreFromBackup.Flags().StringVar(&restoreFromBackupOptions.IgnoredBackupEngines, "ignored-backup-engines", "", "Ignore backups created with this list of backup engines, sepparated by a comma")


There's a StringSliceVar flag type that handles comma separated lists.

go/cmd/vtctldclient/command/backups.go

shlomi-noach · 2024-08-28T05:23:12Z

go/test/endtoend/backup/vtctlbackup/backup_test.go

+	lastBackup := getLastBackup(t)
+
+	// open the Manifest and retrieve the backup engine that was used
+	f, err := os.Open(path.Join(localCluster.CurrentVTDATAROOT,


There's an existing readManifestFile() function in backup_utils.go. Please reuse.

shlomi-noach · 2024-08-28T05:24:51Z

go/test/endtoend/backup/vtctlbackup/backup_test.go

+	return manifest.BackupMethod
+}
+
+func getLastBackup(t *testing.T) string {


Consider reusing (or refactoring) func (cluster LocalProcessCluster) ListBackups(shardKsName string) ([]string, error)

shlomi-noach · 2024-08-28T05:25:13Z

go/test/endtoend/backup/vtctlbackup/backup_test.go

+	// lets take two backups, each using a different backup engine
+	err = localCluster.VtctldClientProcess.ExecuteCommand("Backup", "--allow-primary", "--backup-engine=builtin", primary.Alias)
+	require.NoError(t, err)
+	// firstBackup := getLastBackup(t)


Remove the above line?

shlomi-noach · 2024-08-28T05:30:18Z

go/test/endtoend/backup/vtctlbackup/backup_test.go

+	// check the new replica has the data
+	cluster.VerifyRowsInTablet(t, replica2, keyspaceName, 2)
+
+	// now lets break the last backup in the shard


These are different tests here. Please separate them via properly named t.Run(..., ) subtests.

shlomi-noach · 2024-08-28T05:31:47Z

go/test/endtoend/backup/vtctlbackup/backup_test.go

+	// make sure we are replicating after the restore is done
+	err = replica2.VttabletProcess.WaitForTabletStatusesForTimeout([]string{"SERVING"}, timeout)
+	require.NoError(t, err)
+	cluster.VerifyRowsInTablet(t, replica2, keyspaceName, 2)


cluster.VerifyRowsInTablet is a valid way to test the restore content, and strong enough as the test goes, but not clear enough for future us. It's better to explicitly require "there's a row with this specific string value that means something".

shlomi-noach · 2024-08-28T05:34:15Z

go/vt/mysqlctl/backupengine.go

@@ -213,8 +219,14 @@ func registerBackupEngineFlags(fs *pflag.FlagSet) {
 // a particular backup by calling GetRestoreEngine().
 //
 // This must only be called after flags have been parsed.
-func GetBackupEngine() (BackupEngine, error) {
-	name := backupEngineImplementation
+func GetBackupEngine(backupEngine *string) (BackupEngine, error) {


Let's never pass a *string. This function should accept a string. If it's empty, use the default value.

the reason it is a *string is because that's how grpc optional arguments are created (as it should avoid backward compatibility issues). just to confirm, you prefer me to get rid of it completely and make it mandatory or just avoid passing a *string to GetBackupEngine()? I will keep the grpc optional parameter for now, but let me know if you prefer the other way

or just avoid passing a *string to GetBackupEngine()

This.

shlomi-noach · 2024-08-28T05:35:26Z

go/vt/mysqlctl/backupengine.go

+	var name string
+	if backupEngine == nil || *backupEngine == "" {
+		name = backupEngineImplementation
+	} else {
+		name = *backupEngine
+	}
+


Let's make this more canonical (and also more idiomatic):

Suggested change

var name string

if backupEngine == nil || *backupEngine == "" {

name = backupEngineImplementation

} else {

name = *backupEngine

}

name := backupEngineImplementation

if backupEngine != "" {

name = backupEngine

}

shlomi-noach · 2024-08-28T05:36:01Z

go/vt/vttablet/tabletmanager/restore.go


 	statsRestoreBackupTime     *stats.String
 	statsRestoreBackupPosition *stats.String
 )

 func registerRestoreFlags(fs *pflag.FlagSet) {
 	fs.BoolVar(&restoreFromBackup, "restore_from_backup", restoreFromBackup, "(init restore parameter) will check BackupStorage for a recent backup at startup and start there")
+	fs.StringVar(&restoreFromBackupIgnoreEngines, "restore_from_backup_ignore_engines", restoreFromBackupIgnoreEngines, "ignore backups taken with the backup engines provided in this comma-separated list")


Use StringSliceVar

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

rvrangel · 2024-09-24T11:06:06Z

@shlomi-noach @deepthi thanks again for the review! I believe I have addressed all your comments, but let me know if I missed anything.

deepthi

Looks good now except for the flag name and help text. I can approve once those are fixed.

deepthi · 2024-09-24T22:39:04Z

go/flags/endtoend/vttablet.txt

@@ -300,6 +300,7 @@ Flags:
      --restore-to-timestamp string                                      (init incremental restore parameter) if set, run a point in time recovery that restores up to the given timestamp, if possible. Given timestamp in RFC3339 format. Example: '2006-01-02T15:04:05Z07:00'
      --restore_concurrency int                                          (init restore parameter) how many concurrent files to restore at once (default 4)
      --restore_from_backup                                              (init restore parameter) will check BackupStorage for a recent backup at startup and start there
+      --restore_from_backup_allowed_engines strings                      if present will filter out any backups taken with engines not included in the list


New flags should use dashes, not underscores. Sorry, we haven't got around to deprecating and replacing the old-style flags, so we have this pitfall where you follow what looks like the standard pattern, but it isn't.

For future reference, the vitess-bot checklist is helpful here. It tells you what is expected under New flags.

aha, thanks @deepthi! I will update it

deepthi · 2024-09-24T22:41:31Z

go/flags/endtoend/vttablet.txt

@@ -300,6 +300,7 @@ Flags:
      --restore-to-timestamp string                                      (init incremental restore parameter) if set, run a point in time recovery that restores up to the given timestamp, if possible. Given timestamp in RFC3339 format. Example: '2006-01-02T15:04:05Z07:00'
      --restore_concurrency int                                          (init restore parameter) how many concurrent files to restore at once (default 4)
      --restore_from_backup                                              (init restore parameter) will check BackupStorage for a recent backup at startup and start there
+      --restore_from_backup_allowed_engines strings                      if present will filter out any backups taken with engines not included in the list


Suggested change

--restore_from_backup_allowed_engines strings if present will filter out any backups taken with engines not included in the list

--restore-from-backup-allowed-engines strings (init restore parameter) if set, only backups taken with the specified engines are eligible to be restored

deepthi · 2024-09-24T22:42:49Z

go/cmd/vtctldclient/command/backups.go

@@ -299,6 +309,7 @@ func init() {
 	Root.AddCommand(RemoveBackup)

 	RestoreFromBackup.Flags().StringVarP(&restoreFromBackupOptions.BackupTimestamp, "backup-timestamp", "t", "", "Use the backup taken at, or closest before, this timestamp. Omit to use the latest backup. Timestamp format is \"YYYY-mm-DD.HHMMSS\".")
+	RestoreFromBackup.Flags().StringSliceVar(&restoreFromBackupOptions.AllowedBackupEngines, "allowed-backup-engines", restoreFromBackupOptions.AllowedBackupEngines, "if present will filter out any backups taken with engines not included in the list")


Suggested change

RestoreFromBackup.Flags().StringSliceVar(&restoreFromBackupOptions.AllowedBackupEngines, "allowed-backup-engines", restoreFromBackupOptions.AllowedBackupEngines, "if present will filter out any backups taken with engines not included in the list")

RestoreFromBackup.Flags().StringSliceVar(&restoreFromBackupOptions.AllowedBackupEngines, "allowed-backup-engines", restoreFromBackupOptions.AllowedBackupEngines, "if set, only backups taken with the specified engines are eligible to be restored")

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

deepthi · 2024-09-25T17:57:54Z

We'll update all the command reference docs before release.

shlomi-noach · 2024-09-26T08:51:24Z

go/test/endtoend/backup/vtctlbackup/backup_utils.go

+		err = localCluster.VtctldClientProcess.ExecuteCommand("Backup", "--allow-primary", "--backup-engine=builtin", primary.Alias)
+		require.NoError(t, err)
+		engineUsed := getBackupEngineOfLastBackup(t)
+		require.Equal(t, "builtin", engineUsed)


shlomi-noach · 2024-09-26T08:51:42Z

go/test/endtoend/backup/vtctlbackup/backup_utils.go

+		err = localCluster.VtctldClientProcess.ExecuteCommand("Backup", "--allow-primary", "--backup-engine=xtrabackup", primary.Alias)
+		require.NoError(t, err)
+		engineUsed := getBackupEngineOfLastBackup(t)
+		require.Equal(t, "xtrabackup", engineUsed)


shlomi-noach · 2024-09-26T13:16:04Z

go/vt/mysqlctl/backupengine.go

+		if len(params.AllowedBackupEngines) > 0 && !slices.Contains(params.AllowedBackupEngines, bm.BackupMethod) {
+			params.Logger.Warningf("Ignoring backup %v because it is using %q backup engine", bh.Name(), bm.BackupMethod)
+			continue
+		}


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

…kup() (vitessio#16428) Signed-off-by: Renan Rangel <rrangel@slack-corp.com> Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Signed-off-by: 'Renan Rangel' <rrangel@slack-corp.com>

rvrangel added 2 commits July 18, 2024 06:52

allow backup engine to be specified on Backup() API

675922a

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

add test for the --ignored-backup-engines flag

b4fb33d

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

github-actions bot added this to the v21.0.0 milestone Jul 18, 2024

rvrangel changed the title ~~Select backup engine~~ select backup engine in Backup() and ignore engines in RestoreFromBackup() Jul 18, 2024

rvrangel changed the title ~~select backup engine in Backup() and ignore engines in RestoreFromBackup()~~ select backup engine in Backup() and ignore engines in RestoreFromBackup() Jul 18, 2024

Merge branch 'master' into select-backup-engine

b2b83f5

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

rvrangel added the Component: Backup and Restore label Jul 18, 2024

rvrangel mentioned this pull request Jul 19, 2024

adding new mysql shell backup engine #16295

Merged

5 tasks

rvrangel added 2 commits August 15, 2024 08:16

fixing tests

1369c4a

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

Merge branch 'master' into select-backup-engine

5f12132

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

rvrangel added 5 commits August 16, 2024 05:26

installed xtrabackup for additional tests

2738f7f

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

fix xtrabackup install

a73bf44

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

add vtctlbackup_sharded_clustertest_heavy to list that requires xtrab…

a08b8e1

…ackup Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

fix unit test

f4f6451

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

reset common args for each test

4c70b4e

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

rvrangel marked this pull request as ready for review August 23, 2024 14:51

rvrangel requested review from deepthi, harshit-gangal, mattlord, GuptaManan100, frouioui, rohit-nayak-ps, ajm188 and notfelineit as code owners August 23, 2024 14:51

deepthi removed request for frouioui, GuptaManan100 and rohit-nayak-ps August 27, 2024 23:18

shlomi-noach requested changes Aug 28, 2024

View reviewed changes

deepthi 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 Aug 28, 2024

rvrangel added 6 commits September 20, 2024 06:22

Merge branch 'master' into select-backup-engine

4613bf5

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

use allow list

2f174cc

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

improve tests

d4a53a9

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

moving tests to endtoend/backup/xtrabackup

631bb65

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

Merge branch 'master' into select-backup-engine

f3b9e83

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

fix tests

41cd68e

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

deepthi reviewed Sep 24, 2024

View reviewed changes

rvrangel added 3 commits September 25, 2024 03:56

Merge branch 'master' into select-backup-engine

c187e36

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

fix mysqlshell engine

b2a540e

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

update test and flags

ff3dc51

Signed-off-by: Renan Rangel <rrangel@slack-corp.com>

deepthi approved these changes Sep 25, 2024

View reviewed changes

deepthi added Type: Feature and removed NeedsWebsiteDocsUpdate What it says labels Sep 25, 2024

shlomi-noach approved these changes Sep 26, 2024

View reviewed changes

empty commit to kick CI

a6dff4b

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

shlomi-noach merged commit 2e2b223 into vitessio:main Sep 26, 2024
101 checks passed

This was referenced Oct 25, 2024

Backport select engine #17075

Closed

Backport select backup engine in Backup() and ignore engines in RestoreFromBackup() slackhq/vitess#542

Merged

tanjinx mentioned this pull request Nov 18, 2024

backport upstream 16428 (logical backup part2) slackhq/vitess#562

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

select backup engine in Backup() and ignore engines in RestoreFromBackup() #16428

select backup engine in Backup() and ignore engines in RestoreFromBackup() #16428

rvrangel commented Jul 18, 2024 •

edited

Loading

vitess-bot bot commented Jul 18, 2024

codecov bot commented Aug 15, 2024 •

edited

Loading

shlomi-noach left a comment

shlomi-noach Aug 28, 2024

shlomi-noach Aug 28, 2024

shlomi-noach Aug 28, 2024

shlomi-noach Aug 28, 2024

shlomi-noach Aug 28, 2024

shlomi-noach Aug 28, 2024

shlomi-noach Aug 28, 2024

rvrangel Sep 20, 2024

shlomi-noach Sep 22, 2024

shlomi-noach Aug 28, 2024

shlomi-noach Aug 28, 2024

rvrangel commented Sep 24, 2024

deepthi left a comment

deepthi Sep 24, 2024

deepthi Sep 24, 2024

rvrangel Sep 25, 2024

deepthi Sep 24, 2024

deepthi Sep 24, 2024

deepthi commented Sep 25, 2024

shlomi-noach Sep 26, 2024

shlomi-noach Sep 26, 2024

shlomi-noach Sep 26, 2024

	--restore_from_backup_allowed_engines strings if present will filter out any backups taken with engines not included in the list
	--restore-from-backup-allowed-engines strings (init restore parameter) if set, only backups taken with the specified engines are eligible to be restored

	RestoreFromBackup.Flags().StringSliceVar(&restoreFromBackupOptions.AllowedBackupEngines, "allowed-backup-engines", restoreFromBackupOptions.AllowedBackupEngines, "if present will filter out any backups taken with engines not included in the list")
	RestoreFromBackup.Flags().StringSliceVar(&restoreFromBackupOptions.AllowedBackupEngines, "allowed-backup-engines", restoreFromBackupOptions.AllowedBackupEngines, "if set, only backups taken with the specified engines are eligible to be restored")

select backup engine in Backup() and ignore engines in RestoreFromBackup() #16428

select backup engine in Backup() and ignore engines in RestoreFromBackup() #16428

Conversation

rvrangel commented Jul 18, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jul 18, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Aug 15, 2024 • edited Loading

Codecov Report

shlomi-noach 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

rvrangel commented Sep 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

deepthi commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvrangel commented Jul 18, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading