Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 20 commits into from
Sep 26, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add test for the --ignored-backup-engines flag
Signed-off-by: Renan Rangel <[email protected]>
rvrangel committed Jul 18, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit b4fb33db8b17dd0e8029596ee2828ba84b6b6ecf
73 changes: 68 additions & 5 deletions go/test/endtoend/backup/vtctlbackup/backup_test.go
Original file line number Diff line number Diff line change
@@ -116,15 +116,12 @@ func TestBackupEngineSelector(t *testing.T) {

// fetch the backup engine used on the last backup triggered by the end-to-end tests.
func getBackupEngineOfLastBackup(t *testing.T) string {
output, err := localCluster.VtctldClientProcess.ExecuteCommandWithOutput("GetBackups", shardKsName)

// split the backups response into a slice of strings
backups := strings.Split(strings.TrimSpace(output), "\n")
lastBackup := getLastBackup(t)

// open the Manifest and retrieve the backup engine that was used
f, err := os.Open(path.Join(localCluster.CurrentVTDATAROOT,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"backups", keyspaceName, shardName,
backups[len(backups)-1], "MANIFEST",
lastBackup, "MANIFEST",
))
require.NoError(t, err)
defer f.Close()
@@ -138,3 +135,69 @@ func getBackupEngineOfLastBackup(t *testing.T) string {

return manifest.BackupMethod
}

func getLastBackup(t *testing.T) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

output, err := localCluster.VtctldClientProcess.ExecuteCommandWithOutput("GetBackups", shardKsName)
require.NoError(t, err)

// split the backups response into a slice of strings
backups := strings.Split(strings.TrimSpace(output), "\n")

return backups[len(backups)-1]
}

func TestRestoreIgnoreBackups(t *testing.T) {
defer setDefaultCompressionFlag()
defer cluster.PanicHandler(t)

// launch the custer with xtrabackup as the default engine
code, err := LaunchCluster(XtraBackup, "xbstream", 0, &CompressionDetails{CompressorEngineName: "pgzip"})
require.Nilf(t, err, "setup failed with status code %d", code)

defer TearDownCluster()

localCluster.DisableVTOrcRecoveries(t)
defer func() {
localCluster.EnableVTOrcRecoveries(t)
}()
verifyInitialReplication(t)

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the above line?


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

// insert more data on the primary
_, err = primary.VttabletProcess.QueryTablet("insert into vt_insert_test (msg) values ('test2')", keyspaceName, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally insert data that is meaningful to the test. So instead of "test2" insert a "right after xtrabackup backup" (or whatever makes sense here).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see test2 being tested. I assume that if we insert this value after the full backup, we want to validate that it does not exists after restore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly because there are other cases where we validate on vitess by checking numbers of rows, but I agree it would be better testing the values themselves, so I added that

require.NoError(t, err)

// now bring up the other replica, letting it restore from backup.
restoreWaitForBackup(t, "replica", nil, true)
err = replica2.VttabletProcess.WaitForTabletStatusesForTimeout([]string{"SERVING"}, timeout)
require.NoError(t, err)

// check the new replica has the data
cluster.VerifyRowsInTablet(t, replica2, keyspaceName, 2)

// now lets break the last backup in the shard
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

err = os.Remove(path.Join(localCluster.CurrentVTDATAROOT,
"backups", keyspaceName, shardName,
getLastBackup(t), "backup.xbstream.gz"))
require.NoError(t, err)

// and try to restore from it
err = localCluster.VtctldClientProcess.ExecuteCommand("RestoreFromBackup", replica2.Alias)
require.ErrorContains(t, err, "exit status 1") // this should fail

// now we retry but trying the earlier backup
err = localCluster.VtctldClientProcess.ExecuteCommand("RestoreFromBackup", "--ignored-backup-engines=xtrabackup", replica2.Alias)
require.NoError(t, err) // this should succeed

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}