From cb68885328acc28a0fc04a8a711312d6320b19f4 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Thu, 8 Mar 2018 11:27:15 -0800 Subject: [PATCH 1/2] If backup expiration determines that force is required but the force parameter is not set, it will no longer throw an error unless the backup contains data from prior to the expire_before_version. --- fdbclient/BackupContainer.actor.cpp | 46 ++++++++++++++++++++++------- fdbclient/Knobs.cpp | 1 + fdbclient/Knobs.h | 1 + 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/fdbclient/BackupContainer.actor.cpp b/fdbclient/BackupContainer.actor.cpp index 88290726804..e7087155196 100644 --- a/fdbclient/BackupContainer.actor.cpp +++ b/fdbclient/BackupContainer.actor.cpp @@ -564,7 +564,7 @@ class BackupContainerFileSystem : public IBackupContainer { // Force is required if there is not a restorable snapshot which both // - begins at or after expireEndVersion // - ends at or before restorableBeginVersion - bool forceNeeded = true; + state bool forceNeeded = true; for(KeyspaceSnapshotFile &s : desc.snapshots) { if(s.restorable.orDefault(false) && s.beginVersion >= expireEndVersion && s.endVersion <= restorableBeginVersion) { forceNeeded = false; @@ -572,9 +572,6 @@ class BackupContainerFileSystem : public IBackupContainer { } } - if(forceNeeded && !force) - throw backup_cannot_expire(); - // Get metadata state Optional expiredEnd; state Optional logBegin; @@ -631,25 +628,54 @@ class BackupContainerFileSystem : public IBackupContainer { Void _ = wait(bc->logBeginVersion().clear()); } - // Delete files - state std::vector> deletes; + // Make a list of files to delete + state std::vector toDelete; + // Move filenames out of vector then destroy it to save memory for(auto const &f : logs) { - deletes.push_back(bc->deleteFile(f.fileName)); + toDelete.push_back(std::move(f.fileName)); } + logs.clear(); + // Move filenames out of vector then destroy it to save memory for(auto const &f : ranges) { // Must recheck version because list returns data up to and including the given endVersion if(f.version < expireEndVersion) - deletes.push_back(bc->deleteFile(f.fileName)); + toDelete.push_back(std::move(f.fileName)); } + ranges.clear(); for(auto const &f : desc.snapshots) { if(f.endVersion < expireEndVersion) - deletes.push_back(bc->deleteFile(f.fileName)); + toDelete.push_back(std::move(f.fileName)); } - Void _ = wait(waitForAll(deletes)); + // If some files to delete were found AND force is needed AND the force option is NOT set, then fail + if(!toDelete.empty() && forceNeeded && !force) + throw backup_cannot_expire(); + + // Delete files, but limit parallelism because the file list could use a lot of memory and the corresponding + // delete actor states would use even more if they all existed at the same time. + state std::list> deleteFutures; + + while(!toDelete.empty() || !deleteFutures.empty()) { + + // While there are files to delete and budget in the deleteFutures list, start a delete + while(!toDelete.empty() && deleteFutures.size() < CLIENT_KNOBS->BACKUP_CONCURRENT_DELETES) { + deleteFutures.push_back(bc->deleteFile(toDelete.back())); + toDelete.pop_back(); + } + + // Wait for deletes to finish until there are only targetDeletesInFlight remaining. + // If there are no files left to start then this value is 0, otherwise it is one less + // than the delete concurrency limit. + state int targetFuturesSize = toDelete.empty() ? 0 : (CLIENT_KNOBS->BACKUP_CONCURRENT_DELETES - 1); + + while(deleteFutures.size() > targetFuturesSize) { + Void _ = wait(deleteFutures.front()); + deleteFutures.pop_front(); + } + } // Update the expiredEndVersion property. Void _ = wait(bc->expiredEndVersion().set(expireEndVersion)); diff --git a/fdbclient/Knobs.cpp b/fdbclient/Knobs.cpp index 6c6232089fe..e22f4550e2f 100644 --- a/fdbclient/Knobs.cpp +++ b/fdbclient/Knobs.cpp @@ -91,6 +91,7 @@ ClientKnobs::ClientKnobs(bool randomize) { init( TASKBUCKET_MAX_TASK_KEYS, 1000 ); if( randomize && BUGGIFY ) TASKBUCKET_MAX_TASK_KEYS = 20; //Backup + init( BACKUP_CONCURRENT_DELETES, 100 ); init( BACKUP_SIMULATED_LIMIT_BYTES, 1e6 ); if( randomize && BUGGIFY ) BACKUP_SIMULATED_LIMIT_BYTES = 1000; init( BACKUP_GET_RANGE_LIMIT_BYTES, 1e6 ); init( BACKUP_LOCK_BYTES, 1e8 ); diff --git a/fdbclient/Knobs.h b/fdbclient/Knobs.h index d24c6717c61..1ce0cec1ea0 100644 --- a/fdbclient/Knobs.h +++ b/fdbclient/Knobs.h @@ -93,6 +93,7 @@ class ClientKnobs : public Knobs { int TASKBUCKET_MAX_TASK_KEYS; // Backup + int BACKUP_CONCURRENT_DELETES; int BACKUP_SIMULATED_LIMIT_BYTES; int BACKUP_GET_RANGE_LIMIT_BYTES; int BACKUP_LOCK_BYTES; From 3a7288924a10f47acc165373db81e2b0edbbecdc Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Fri, 9 Mar 2018 11:29:23 -0800 Subject: [PATCH 2/2] Bug fixes. During expiration, the backup container's log range metadata could be updated incorrectly if force was required and not specified or if a backup had no log begin metadata and an expire was done which covered 1 or more log file. In the latter case a backup could be left in a state where the container metadata suggests the backup has more log coverage than it actually does. --- fdbclient/BackupContainer.actor.cpp | 52 ++++++++++++++++++----------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/fdbclient/BackupContainer.actor.cpp b/fdbclient/BackupContainer.actor.cpp index e7087155196..90e9dd41d35 100644 --- a/fdbclient/BackupContainer.actor.cpp +++ b/fdbclient/BackupContainer.actor.cpp @@ -601,33 +601,16 @@ class BackupContainerFileSystem : public IBackupContainer { newLogBeginVersion = expireEndVersion; } else { - // If the last log overlaps the expiredEnd then use the log's begin version + // If the last log overlaps the expiredEnd then use the log's begin version and move the expiredEnd + // back to match it. if(last.endVersion > expireEndVersion) { newLogBeginVersion = last.beginVersion; logs.pop_back(); + expireEndVersion = newLogBeginVersion.get(); } } } - // If we have a new log begin version then potentially update the property but definitely set - // expireEndVersion to the new log begin because we will only be deleting files up to but not - // including that version. - if(newLogBeginVersion.present()) { - expireEndVersion = newLogBeginVersion.get(); - // If the new version is greater than the existing one or the - // existing one is not present then write the new value - if(logBegin.orDefault(0) < newLogBeginVersion.get()) { - Void _ = wait(bc->logBeginVersion().set(newLogBeginVersion.get())); - } - } - else { - // Otherwise, if the old logBeginVersion is present and older than expireEndVersion then clear it because - // it refers to a version in a range we're about to delete and apparently continuity through - // expireEndVersion is broken. - if(logBegin.present() && logBegin.get() < expireEndVersion) - Void _ = wait(bc->logBeginVersion().clear()); - } - // Make a list of files to delete state std::vector toDelete; @@ -654,6 +637,35 @@ class BackupContainerFileSystem : public IBackupContainer { if(!toDelete.empty() && forceNeeded && !force) throw backup_cannot_expire(); + // We are about to start deleting files, at which point no data prior to the expire end version can be + // safely assumed to exist. The [logBegin, logEnd) range from the container's metadata describes + // a range of log versions which can be assumed to exist, so if the range of data being deleted overlaps + // that range then the metadata range must be updated. + + // If we're expiring the entire log range described by the metadata then clear both metadata values + if(logEnd.present() && logEnd.get() < expireEndVersion) { + if(logBegin.present()) + Void _ = wait(bc->logBeginVersion().clear()); + if(logEnd.present()) + Void _ = wait(bc->logEndVersion().clear()); + } + else { + // If we are expiring to a point within the metadata range then update the begin if we have a new + // log begin version (which we should!) or clear the metadata range if we do not (which would be + // repairing the metadata from an incorrect state) + if(logBegin.present() && logBegin.get() < expireEndVersion) { + if(newLogBeginVersion.present()) { + Void _ = wait(bc->logBeginVersion().set(newLogBeginVersion.get())); + } + else { + if(logBegin.present()) + Void _ = wait(bc->logBeginVersion().clear()); + if(logEnd.present()) + Void _ = wait(bc->logEndVersion().clear()); + } + } + } + // Delete files, but limit parallelism because the file list could use a lot of memory and the corresponding // delete actor states would use even more if they all existed at the same time. state std::list> deleteFutures;