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

Adds -persist option to check and restore commands to continue despite errors #595

Merged
merged 2 commits into from
Sep 9, 2020

Conversation

twlee79
Copy link
Contributor

@twlee79 twlee79 commented May 6, 2020

Addresses #157 and #205 and fixes #596

This PR adds a -persist option to the check and restore modes of duplicacy. The function of this option is to allow these processes to continue despite encountering errors when downloading or decrypting file chunks. This allows check or restore to continue until all chunks/files in the revision are processed. In check, this means all chunks with errors (-chunks mode) or all files with errors (-files mode) are reported. In restore, this means that duplicacy will attempt to restore all files rather than failing on first error. Only errors in file chunks can be recovered from; errors in metadata chunks will still cause fatal errors.

In my view this PR increases the robustness of backups as they can be restored as far as possible despite corrupt/missing file chunks. Assuming only a small number of chunks are missing/corrupted, this reduces a potentially a critical failure (inability to restore entire backup) to a lesser failure (only several files corrupted). It is worth noting that this increased robustness only applies to loss of file chunks. Loss of a metadata chunk is likely to still lead to loss of an entire backup. Therefore, this PR can be considered to mostly fix #157 and #205.

Duplicacy function without the -persist option is largely the same as the current version (the difference is for behaviour with restore -hash without -overwrite to fix #596, and additional statistics with restore -stats).

Internal working:
The -persist option is propagated to the Restore() and CheckSnapshops() through the use of a new parameter allowFailures (default allowFailures=false; allowFailures=true if -persist enabled).

Effect on chunk downloader
The allowFailures parameter is propagated to CreateChunkDownloader() by Restore() and CheckSnapshops(). With allowFailures==true, the chunk downloader will produce warnings on chunk download/decrypt failures rather than fatal errors (following the standard number of retries), with chunks marked with isBroken=true. Therefore, with -persist, missing/corrupted file chunks will only affect the files encoded by these chunks.

Effect on CheckSnapshots()
The allowFailures parameter is propagated to CreateChunkDownloader(). In CheckSnapshots() and in -chunks mode, file chunk failures will be output as warnings; verification will continue until all chunks processed (so warnings will be produced for all affected chunks). In -files mode, corrupted files will be reported as well as total number of files affected.

Effect on Restore()
The allowFailures parameter is propagated to CreateChunkDownloader() and RestoreFile(). In RestoreFile(), allowFailures=true will cause failures to reduce to warnings rather than errors, including existing file overwrite with overwrite==false and restored file hash failures. Chunks with isBroken==true are not used for reassembling the file and the resulting file will be corrupt and result in file hash errors. RestoreFile() is altered to return bool, Error (rather than just bool). The bool option indicates if the file was restored or not; the error option indicates if an error was encountered. The three return possibilities are true, nil: file restored; false, nil: file skipped (already present with correct hash); and false, error: file could not be restored. With this change, Restore() will track the downloadedFiles, skippedFiles and failedFiles, reporting totals for all if -stats requested.

Restore differences without -persist
Without -overwrite and with -hash, duplicacy would previously fail if file is present even if file has same hash (see #596). This behaviour has been changed by shifting the exiting file hash check further up in RestoreFile() and having RestoreFile() to signal a skipped file if an identical existing file is present (fixing #596); now -overwrite off will only cause failures if the file exists but has a different hash.

[This changes also works well with restore -hash -persist (overwrite=false) as it allows a user to restore all missing files without any overwrites; any files with different hashes that could not be restored due to existing files are reported.]

In addition, restore will report more statistics (skippedFiles and failedFiles) regardless of -persist.

Additional unit tests
Additional unit tests for allowFailures=true have been added to duplicacy_backupmanager_test.go: TestBackupManagerPersist

@CLAassistant
Copy link

CLAassistant commented May 6, 2020

CLA assistant check
All committers have signed the CLA.

@twlee79 twlee79 marked this pull request as ready for review May 7, 2020 01:48
@gilbertchen
Copy link
Owner

This pull request has been mentioned on Duplicacy Forum. There might be relevant details there:

https://forum.duplicacy.com/t/improving-robustness-of-duplicacy-to-missing-corrupt-chunks-with-a-secondary-snapshot-table/3627/1

@gilbertchen
Copy link
Owner

This pull request has been mentioned on Duplicacy Forum. There might be relevant details there:

https://forum.duplicacy.com/t/add-option-for-restore-command-to-pass-filters-file/3916/1

@gboudreau
Copy link
Contributor

@gilbertchen Any particular reason this has not been merged yet? Would you maybe prefer this to be handled differently, or maybe merged separately?
I was about to look into the code, to see how I could create a PR that would allow -check to report all invalid chunks, instead of dying at the first invalid chunk found. But this PR goes further.

@gilbertchen gilbertchen merged commit 948994c into gilbertchen:master Sep 9, 2020
@gilbertchen
Copy link
Owner

Sorry I wasn't really paying attention to this PR. This is exactly what we need to fix issues caused by the sftp bug.

@twlee79 Thank you for the contribution! This is a high quality PR that includes excellent tests and docs. There are a few minor changes that I will make after the merge.

@gilbertchen
Copy link
Owner

This pull request has been mentioned on Duplicacy Forum. There might be relevant details there:

https://forum.duplicacy.com/t/cli-2-7-0-is-now-available/4260/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants