restore: Add progress bar to 'restore --verify'#4989
restore: Add progress bar to 'restore --verify'#4989MichaelEischer merged 1 commit intorestic:masterfrom
Conversation
0f028fd to
32640eb
Compare
|
@MichaelEischer please review :) Before you do, please advise on the following:
|
MichaelEischer
left a comment
There was a problem hiding this comment.
3. Do you think the way I've implemented now is enough, or should I try doing 2? The change if involving
restoreui.NewProgresswould have a larger footprint, and thus I have avoided it for now.
The answer depends on whether we want the progress bar to also work for JSON or not. For just text output the current approach would be good enough.
To add JSON support, we essentially have two options. We could extend newTerminalProgressMax to actually support JSON progress bars (maybe with a format similar to progress_percent from borgbackup). The other option would be to extend the restore progress UI. But that has the big downside of requiring yet another handcrafted progress implementation.
So I'm leaning towards starting with newTerminalProgressMax and separately adding JSON support.
32640eb to
82dad5b
Compare
|
Thanks @MichaelEischer for the review and the feedback on the implementation
|
MichaelEischer
left a comment
There was a problem hiding this comment.
I just had another look at how the maximum for the progress bar is determined and noticed that the current approach can't work, see below for details. No idea how I've missed that yesterday.
internal/restorer/restorer.go
Outdated
| ) | ||
|
|
||
| if p != nil { | ||
| numFilesVerified := uint64(res.sn.Summary.TotalFilesProcessed) |
There was a problem hiding this comment.
I just had another look and we'll need a different way to determine the number of files to verify. Just restore the same snapshot twice, then you'll end up with something like the following:
verifying files in output
[0:00] 0.00% 0 / 1 files verified
finished verifying 0 files in output (took 0s)
This is rather confusing as the progress will never reach 100% in most cases. Or even worse, just restore an old snapshot without that metadata and restic would just crash.
What could work is the following. In RestoreTo there's a res.trackFile(location, updateMetadataOnly) call. At that place the restorer could also count all files that are not updateMetadataOnly updates. Then RestoreTo could return that number similar to VerifyFiles.
That will also remove the hacks that were necessary to make the tests work.
There was a problem hiding this comment.
Thanks for the catching this, and the guidance!
I've updated the code as per your suggestions.
Please review again when you have the time :)
5f64cc2 to
c9a5594
Compare
c9a5594 to
f1407af
Compare
What does this PR change? What problem does it solve?
This MR introduces a progress bar to the
restic restore --verifyflow when verify is in-progress. Earlier, there was no output when files were being verified.Was the change previously discussed in an issue or on the forum?
#4795
Checklist
changelog/unreleased/that describes the changes for our users (see template).gofmton the code in all commits.