restic copy --stream: run one large copy operation crossing snapshot boundaries - issue #5453#5472
Conversation
MichaelEischer
left a comment
There was a problem hiding this comment.
The --stream and non-stream code paths MUST be unified or this will become a maintenance nightmare.
25f9c88 to
6b2a614
Compare
MichaelEischer
left a comment
There was a problem hiding this comment.
This will require further rework to be compatible with the changes from #5558 . While looking closer at copyTreeBatched I've noticed that it's actually possible to let it automatically batch multiple snapshots together. That should be possible with limited extra complexity and would have to benefit of just working out of the box.
| copyBlobs.Insert(h) | ||
| for _, p := range pb { | ||
| packList.Insert(p.PackID) | ||
| if !seenBlobs.Has(h.ID) { |
There was a problem hiding this comment.
I don't think we need seenBlobs at all. Once repository.Repack(Inner) has run, all copied blobs will immediately also be returned by dstRepo.LookupBlobSize, even if the repository was not flushed yet.
There was a problem hiding this comment.
Well, that wasn't actually the case and will be fixed by #5614 .
Instead of rebasing my code, I decided to start fresh, since WithBlobUploader() has been introduced. changelog/unreleased/issue-5453: doc/045_working_with_repos.rst: the usual cmd/restic/cmd_copy.go: gather all snaps to be collected - collectAllSnapshots() run overall copy step - func copyTreeBatched() helper copySaveSnapshot() to save the corresponding snapshot internal/repository/repack.go: introduce wrapper CopyBlobs(), which passes parameter `uploader restic.BlobSaver` from WithBlobUploader() via copyTreeBatched() to repack(). internal/backend/local/local_windows.go: I did not touch it, but gofmt did: whitespace
5976608 to
b87f758
Compare
internal/repository/repack.go: I have to please the mighty linter.
I cave in - no double comment
|
In principle, one could make |
… function signature
MichaelEischer
left a comment
There was a problem hiding this comment.
Looks like I should have replied here a bit earlier. Adding a --all option is completely the opposite direction of what this PR should move into, sorry. I actually think that even a --batch option is already slightly too much. In particular, there's an apparent way that let's copy perform well even without additional configuration.
I might partially change my mind if a relevant performance benefit or other benefit can be demonstrated, but this will be an uphill battle.
As I want to merge this PR before I introduce even more merge conflicts (the next 2 or 3 are already prepared), I'll take over cleaning up the code here (including my comments).
|
|
||
| // save all the snapshots | ||
| for _, sn := range selectedSnapshots { | ||
| err := copySaveSnapshot(ctx, sn, dstRepo, printer) |
There was a problem hiding this comment.
Saving the snapshot must happen outside of WithBlobUploader, otherwise some blobs might still be missing.
| visitedTrees restic.IDSet, selectedSnapshots []*data.Snapshot, opts CopyOptions, | ||
| printer progress.Printer) error { | ||
|
|
||
| // seenBlobs is necessary in about 1 of 10000 blobs, in the other 99.99% the check |
There was a problem hiding this comment.
This is a problem that should be properly investigated instead of adding workaround. Stating that something randomly doesn't work basically admits defeat, when something important could be broken.
There was a problem hiding this comment.
Sorry, I trusted your statement about the pending blobs. I tried to investigate the issue (since I knew that seenBlobswas a circumvention), but I could not see an apparent pattern. So - in order to deliver a correct result - I used seenBlobs. I never suspected that the master index was the culprit.
a79c11e to
37dc626
Compare
37dc626 to
7d08c92
Compare
| printer := ui.NewProgressPrinter(gopts.JSON, gopts.Verbosity, gopts.Term) | ||
| _, repo3, unlock3, err = openWithReadLock(ctx, gopts, false, printer) | ||
| rtest.OK(t, err) | ||
| defer unlock3() |
There was a problem hiding this comment.
unlock3 has already been called once withTermStatus returns. After that point the repository must not be used anymore.
|
Michael, thank you for cleaning up my messes :) |
|
May I point out, after copying your modified code to my git repository, that we still have a long tail of small packfiles after running whereas the original repository looks like this: In total about 110 more packfiles, and many more index files. This can easily be remedied with a |
Thanks for testing! Looks like I've effectively broken the PR for the snapshots you've tested with 🙈 . The latest commit makes the batching quite a bit more aggressive, which should be enough to yield a single digit number of indexes and get rid of most of the difference in the pack files number. Can you give that another try? |
|
Hi Michael, I just saw your note and I have tested again. Excellent news! Excellent work! |
restic copy now allows to optimize the repository packsize across multiple snapshots when copying from one repository to another repository.
changelog/unreleased/issue-5453:
announce new option
--batch.cmd/restic/cmd_check_integration_test.go:
add stream test
cmd/restic/cmd_copy.go:
new functions collectAllSnapshots(), copyTreeBatched() to perfrom the batch copy.
cmd/restic/cmd_copy_integration_test.go:
a new streaming test
doc/045_working_with_repos.rst:
a short note about option
--batchWhat does this PR change? What problem does it solve?
This PR modifies the way the
copyoperation works. With the--batchoption, one large request list is built and sent to repository.Repack(), instead of one request list per snapshot to be copied. This allows optimizing the repository packsize while copyingrestic backupwith only minor changes between different snapshots. Normally, one new index file is created per snapshots, together with at least two packfiles for each new snapshot. When changes between backups are small, this can result in a lot of smallish packfiles.Yes, this can always be fixed by running
restic prune, but this is an extra step.Was the change previously discussed in an issue or on the forum?
closes #5453
Checklist
changelog/unreleased/that describes the changes for our users (see template).