Fix non-intuitive repo behavior by aawsome · Pull Request #2773 · restic/restic

aawsome · 2020-06-07T07:21:58Z

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

The current restic.Repository implementation has some non-intuitive behavior. While you can use
the rather high-level method SaveBlob to save a blob it does not check for already saved blobs by default. If you do the check yourself by using repo.Index().Has() you encounter the problem that saved blobs in unfinished packs are not yet present in the index and therefore Has() usually returns false for just saved blobs.
Moreover full indexes are not saved by default but need to be manually saved by regularily calling SaveFullIndex().

All these issues are at the moment solved separately in the internal/archiver codebase but will not work by default for new implementations.

This PR moves the needed logic to repository (and the index implementation therein)
Moreover it implements a new method SavePack in repository/master_index.go which saves all index entry for a whole pack.
As a side-effect it reduces the code base and simplifies the archiver logic.

Note that most code changes in this PR are needed modifications in tests, e.g. to make tests generating duplicate blobs or counting the saved blobs still work.

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

See #2523 where the ideas are explained.
This PR is a prerequisite for #2606 (see discussion there)

Checklist

I have read the Contribution Guidelines
I have enabled maintainer edits for this PR
I have changed all affected 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

MichaelEischer

I like the cleanups of this PR. For the method names I've suggested a few alternatives, which I'd prefer over the current ones. CheckSetKnown and the existing SaveFullIndex need some fixes for race conditions before this can land, but besides these (and some minor cleanups) I don't see any other blockers.

Inlining the intermediate index upload into the packerManager is actually much simpler than what I had thought about, but it nevertheless looks like a good solution.

internal/restic/repository.go

internal/repository/repository.go

internal/repository/index.go

internal/repository/master_index.go

internal/repository/packer_manager.go

internal/repository/master_index.go

aawsome · 2020-06-08T05:56:09Z

I like the cleanups of this PR. For the method names I've suggested a few alternatives, which I'd prefer over the current ones. CheckSetKnown and the existing SaveFullIndex need some fixes for race conditions before this can land, but besides these (and some minor cleanups) I don't see any other blockers.

@MichaelEischer Thank you very much for your suggestions - I especially like the naming which is IMO now much clearer (and also allowed for some clearer code).

I changed everything except that I still kept Index.Store. I think we should leave this open to further index optimization.

internal/repository/index.go

internal/repository/master_index.go

greatroar · 2020-06-09T17:37:18Z

I didn't check for race conditions, but at a glance this PR looks very clean. Nice work!

aawsome · 2020-06-09T18:54:24Z

I didn't check for race conditions, but at a glance this PR looks very clean. Nice work!

Thank you for your feedback! I changed all mentioned issues.

MichaelEischer

The PR is nearly ready to go, I've just found a few further minor issue. There's also a left-over call to SaveIndex in cmd_recover.go which should be removed.

Please add a note to the commit message that the commit removes the verbose "uploaded intermediate index" messages. Also a short explanation of the "why" of the commit might be useful, in case someone stumbles across the change later on. The hint at the race conditions should be a bit more specific, just saying that there are some race condition in this 400 lines diff makes it hard to find the problematic spots.

For future PRs: Please split your changes into smaller steps, a single commit that changes at least 5 different things (merged flush, knownBlobs, StorePacks, replacement of the index uploader, the finalization race condition) at once is too large (it should still be a single PR, just with some intermediate steps). It's way easier to review 5 commits with 80 lines diff than a single one with 400 lines.

internal/repository/repository.go

internal/archiver/blob_saver.go

internal/repository/index.go

internal/repository/master_index.go

MichaelEischer · 2020-06-10T19:13:45Z

We probably also want a short changelog entry to highlight the memory usage improvements of the change to knownBlobs. This should be especially visible during a backup that adds a lot of data or the initial backup run.

aawsome · 2020-06-11T10:58:06Z

@MichaelEischer Thanks for reviewing and thank you very much for your valuable hints.
I changed everything and hope this is now ready for merge.

I also realized that the changes were larger than originally expected. In fact I just wanted to change the "pending blobs" and automatic saving of intermediate index files. When doing the changes I realized that changing SaveBlob and Flush may be quite convenient...
I also realized that I should invest a bit more time in getting to know more git functionalities 😏

rawtaz · 2020-06-11T11:04:07Z

@aawsome Please add a changelog entry that we can check as well :)

- The SaveBlob method now checks for duplicates. - Moves handling of pending blobs to MasterIndex. -> also cleans up pending index entries when they are saved in the index -> when using SaveBlob no need to care about index any longer - Always check for full index and save it when storing packs. -> removes the need of an index uploader -> also removes the verbose "uploaded intermediate index" messages - The Flush method now also saves the index - Fix race condition when checking and saving full/non-finalized indexes

aawsome · 2020-06-11T11:06:43Z

@aawsome Please add a changelog entry that we can check as well :)

ups - sorry, forgot to add this in the commit. Is now in the PR.

MichaelEischer

I noticed that I did overlook a side-effect on the prune command: repository.Repack previously did not write index files, which has changed now. For the current prune implementation this means that several index files will be uploaded and afterwards deleted again during the final index rebuild. The cleanest simple solution I see at the moment is to add a DisableAutomaticIndexUpload method to the Repository, which would pass that flag on to the packer manager. The disable call would then happen somewhere in the prune command before the call to Repack. I explicitly want to avoid any structural changes of the prune command.

For creating my git commits, I usually selectively add changed lines to rather small git commits right from the start. Rearranging and combining the commits is then possible with git rebase -i. Using the latter command it's also possible to split a commit into multiple parts, but that feels rather cumbersome to me. For learning a lot about git I can recommend the book that's freely available on the official git scm website.

changelog/unreleased/pull-2773

rawtaz · 2020-06-11T20:50:05Z

For creating my git commits, I usually selectively add changed lines to rather small git commits right from the start.

There's also git add -p with which you can add small parts of your changes, perhaps this is what @MichaelEischer was talking about.

Rearranging and combining the commits is then possible with git rebase -i

I love this command.

+ modifications of changelog

aawsome · 2020-06-12T08:02:48Z

I noticed that I did overlook a side-effect on the prune command: repository.Repack previously did not write index files, which has changed now. For the current prune implementation this means that several index files will be uploaded and afterwards deleted again during the final index rebuild.

I also realized that this would happen and came to the conclusion that it shouldn't harm: at the end rebuild-index will rebuild the index from pack files ignoring all present index files.
However, I added it as you suggested, so that we can deal with prune e.g. in #2718

Thanks @MichaelEischer and @rawtaz for your suggestions about git. I'm actually quite new to working with git and again learned something new 😄 So far I did not try out the full functionality of git rebase -i and did not even know there's a git add -p. Will save a lot of fiddling with code in the editor in future!

MichaelEischer

LGTM. Thanks a lot for the cleanups and optimizations!

aawsome mentioned this pull request Jun 7, 2020

Add copy functionality. #2606

Merged

7 tasks

MichaelEischer requested changes Jun 7, 2020

View reviewed changes

aawsome force-pushed the index-uploads+knownblobs branch 2 times, most recently from 7b298a9 to 14bd06f Compare June 8, 2020 05:51

aawsome requested a review from MichaelEischer June 8, 2020 05:59

greatroar reviewed Jun 8, 2020

View reviewed changes

internal/repository/index.go Outdated Show resolved Hide resolved

greatroar reviewed Jun 8, 2020

View reviewed changes

internal/repository/master_index.go Outdated Show resolved Hide resolved

aawsome force-pushed the index-uploads+knownblobs branch from 14bd06f to 671537e Compare June 8, 2020 17:30

greatroar reviewed Jun 9, 2020

View reviewed changes

internal/repository/master_index.go Outdated Show resolved Hide resolved

aawsome force-pushed the index-uploads+knownblobs branch from 671537e to 4509c58 Compare June 9, 2020 18:50

MichaelEischer requested changes Jun 10, 2020

View reviewed changes

aawsome force-pushed the index-uploads+knownblobs branch from 4509c58 to 1a7f3c0 Compare June 11, 2020 10:48

aawsome force-pushed the index-uploads+knownblobs branch from 1a7f3c0 to 9190691 Compare June 11, 2020 11:05

MichaelEischer reviewed Jun 11, 2020

View reviewed changes

changelog/unreleased/pull-2773 Outdated Show resolved Hide resolved

changelog/unreleased/pull-2773 Outdated Show resolved Hide resolved

changelog/unreleased/pull-2773 Outdated Show resolved Hide resolved

disable index uploads for prune command

70347e9

+ modifications of changelog

This was referenced Jun 12, 2020

Reduce index memory #2781

Merged

Comparison of approaches to tackle index memory usage #2523

Closed

MichaelEischer approved these changes Jun 12, 2020

View reviewed changes

MichaelEischer merged commit 735a807 into restic:master Jun 12, 2020

aawsome deleted the index-uploads+knownblobs branch June 13, 2020 05:58

aawsome mentioned this pull request Jun 13, 2020

Reimplementation of prune #2718

Merged

11 tasks

aawsome mentioned this pull request Jul 10, 2020

Implement 'rewrite' command to exclude files from existing snapshots #2731

Merged

8 tasks

MichaelEischer mentioned this pull request Jan 9, 2021

Tree xxxxxx not found in repository and abort #1798

Closed

Conversation

aawsome commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

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

Checklist

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aawsome commented Jun 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greatroar commented Jun 9, 2020

Uh oh!

aawsome commented Jun 9, 2020

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaelEischer commented Jun 10, 2020

Uh oh!

aawsome commented Jun 11, 2020

Uh oh!

rawtaz commented Jun 11, 2020

Uh oh!

aawsome commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rawtaz commented Jun 11, 2020

Uh oh!

aawsome commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aawsome commented Jun 7, 2020 •

edited

Loading

aawsome commented Jun 11, 2020 •

edited

Loading

aawsome commented Jun 12, 2020 •

edited

Loading