From 3a05a41b9fb0343f47a7dc62bfb49a1fbc5114c8 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Thu, 29 Aug 2019 10:11:20 -0700 Subject: [PATCH] vtbackup: Make initial backup idempotent again. The check for a complete backup already existing was supposed to make the initial backup idempotent, such that running it after an initial backup already exists will simply do nothing and return success. However, that property was broken when I added the check for serving tablets above the check for a complete backup. I should have kept the check for a complete backup first, because if there's already a complete backup and we're doing nothing, then it's fine that some tablets may already be serving. Signed-off-by: Anthony Yeh --- go/cmd/vtbackup/vtbackup.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/go/cmd/vtbackup/vtbackup.go b/go/cmd/vtbackup/vtbackup.go index 690271a0e63..6f5e72813a1 100644 --- a/go/cmd/vtbackup/vtbackup.go +++ b/go/cmd/vtbackup/vtbackup.go @@ -468,13 +468,21 @@ func parseBackupTime(name string) (time.Time, error) { } func shouldBackup(ctx context.Context, topoServer *topo.Server, backupStorage backupstorage.BackupStorage, backupDir string) (bool, error) { + // Look for the most recent, complete backup. backups, err := backupStorage.ListBackups(ctx, backupDir) if err != nil { return false, fmt.Errorf("can't list backups: %v", err) } + lastBackup := lastCompleteBackup(ctx, backups) // Check preconditions for initial_backup mode. if *initialBackup { + // Check if any backups for the shard already exist in this backup storage location. + if lastBackup != nil { + log.Infof("At least one complete backup already exists, so there's no need to seed an empty backup. Doing nothing.") + return false, nil + } + // Check whether the shard exists. _, shardErr := topoServer.GetShard(ctx, *initKeyspace, *initShard) switch { @@ -505,11 +513,6 @@ func shouldBackup(ctx context.Context, topoServer *topo.Server, backupStorage ba return false, fmt.Errorf("failed to check whether shard %v/%v exists before doing initial backup: %v", *initKeyspace, *initShard, err) } - // Check if any backups for the shard exist in this backup storage location. - if len(backups) > 0 { - log.Infof("At least one backup already exists, so there's no need to seed an empty backup. Doing nothing.") - return false, nil - } log.Infof("Shard %v/%v has no existing backups. Creating initial backup.", *initKeyspace, *initShard) return true, nil } @@ -518,8 +521,6 @@ func shouldBackup(ctx context.Context, topoServer *topo.Server, backupStorage ba if len(backups) == 0 && !*allowFirstBackup { return false, fmt.Errorf("no existing backups to restore from; backup is not possible since -initial_backup flag was not enabled") } - // Look for the most recent, complete backup. - lastBackup := lastCompleteBackup(ctx, backups) if lastBackup == nil { if *allowFirstBackup { // There's no complete backup, but we were told to take one from scratch anyway.