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

Verify before restore #2012

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@stang
Copy link

stang commented Sep 27, 2018

What is the purpose of this change? What does it change?

  • extract the operations done by the --verify option into function of Restorer
  • verify the content of a node before restoring it (skip if already matching)

Was the change discussed in an issue or in the forum before?

#2011
Checklist

  • I have read the Contribution Guidelines
  • I have added tests for all changes in this PR
  • I have added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

stang added some commits Sep 5, 2018

Extract node verification as Restorer function
  This can be re-use later on to verify file integrity before
  attempting to restore them.

Signed-off-by: Stephane Tang <hi@stang.sh>
Verify local file content before restoring it
  Previously, it was unconditionnally restoring every single file.
  We now verifying if a file exist locally and if its content is valid
  before restoring it.
  Adopting a similar behavior as with 'rsync'.

Signed-off-by: Stephane Tang <hi@stang.sh>

@stang stang force-pushed the stang:verify-before-restore branch from 9b20ffc to 60dc0f4 Sep 27, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 27, 2018

Codecov Report

Merging #2012 into master will decrease coverage by 4.22%.
The diff coverage is 21.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2012      +/-   ##
==========================================
- Coverage   50.12%   45.89%   -4.23%     
==========================================
  Files         169      169              
  Lines       13563    13569       +6     
==========================================
- Hits         6798     6228     -570     
- Misses       5742     6372     +630     
+ Partials     1023      969      -54
Impacted Files Coverage Δ
internal/restorer/restorer.go 47.79% <21.42%> (+2.4%) ⬆️
internal/backend/b2/b2.go 0% <0%> (-78.95%) ⬇️
internal/backend/swift/swift.go 0% <0%> (-78.45%) ⬇️
internal/backend/gs/gs.go 0% <0%> (-73.61%) ⬇️
internal/backend/azure/azure.go 0% <0%> (-69.46%) ⬇️
internal/backend/swift/config.go 36.95% <0%> (-54.35%) ⬇️
cmd/restic/global.go 28.57% <0%> (+0.85%) ⬆️
internal/backend/s3/s3.go 62.4% <0%> (+2.25%) ⬆️
internal/archiver/blob_saver.go 97.53% <0%> (+2.46%) ⬆️
internal/archiver/file_saver.go 88.59% <0%> (+4.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0882aca...3c0bf62. Read the comment docs.

Add changelog for PR-2012
Signed-off-by: Stephane Tang <hi@stang.sh>
@ifedorenko

This comment has been minimized.

Copy link
Contributor

ifedorenko commented Sep 27, 2018

I suggest this wait until after #1719

@fd0

This comment has been minimized.

Copy link
Member

fd0 commented Sep 28, 2018

Thank you very much for the contribution! I think @ifedorenko is right: This needs to wait until #1719 is merged.

@bbashy

This comment has been minimized.

Copy link

bbashy commented Oct 16, 2018

That's been merged now but there's conflicts.

@fd0

This comment has been minimized.

Copy link
Member

fd0 commented Oct 16, 2018

That's expected: #1719 changed the same code. This needs to be resolved, preferably by @stang :)

@ifedorenko

This comment has been minimized.

Copy link
Contributor

ifedorenko commented Oct 16, 2018

Few notes/observations

  • I suggest to make verify-before-restore behaviour optional. For example, it is likely to slow down restore if the target file system is on 2.5" laptop hdd (i.e. slow mechanical disk) and the backend is on fast 1Gbps connection.
  • I think we should investigate if pre-restore file content verification should run on single or multiple threads. This likely depends on the of disks and filesystem used but I don't know if we should use different concurrency strategy on ssd vs hdd, for example.
  • It may be desirable to implement verify-before-restore at blob level, which will allow to resume interrupted restore and incremental restore of large files.
  • We need to decide what to do with existing files that are not part of the snapshot, i.e. should restore delete or keep them?
@stang

This comment has been minimized.

Copy link
Author

stang commented Nov 4, 2018

Sorry, I was on holiday for a little while.

I'll try to have a look and update this PR, taking @ifedorenko inputs in consideration.

@fd0

This comment has been minimized.

Copy link
Member

fd0 commented Jan 6, 2019

Hey, are you still interested in working on this? Or are you stuck and need help? :)

@stang

This comment has been minimized.

Copy link
Author

stang commented Jan 8, 2019

Sorry, TBH, I didn't manage to make much time to work on this since my last comment — we're still running on a forked build that seem to do the job for now, and got other stuff prioritised so I'm not sure when I'll be able to pick it up again.

If someone is keen to take-over, I won't say no. If not, I'll just keep it in my backlog.

@ifedorenko ifedorenko referenced this pull request Mar 15, 2019

Open

add --skip-unchanged option #2207

2 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.