Issue: 4942: cmd_rewrite: add snapshot summary data to an existing snapshot.#5185
Issue: 4942: cmd_rewrite: add snapshot summary data to an existing snapshot.#5185MichaelEischer merged 15 commits intorestic:masterfrom
Conversation
|
There will be a subsequent PR which will allow include filters to be used for the restic rewrite command, see #4278 for details. |
MichaelEischer
left a comment
There was a problem hiding this comment.
I found quite a few issues with the code changes, see below for details.
MichaelEischer
left a comment
There was a problem hiding this comment.
Oh and please change the commit description such that the first line is cmd_rewrite: add capability to add snapshot summary data to an existing snapshot.. The Issue" 4942 part is not useful as summary.
|
@MichaelEischer: Thank you for reviewing my code. I have updated the code as you requested. The new tag for creating snapshot summary data is "snap-summary". See changes in PR #5185. |
There was a problem hiding this comment.
Sorry for the review delay and asking for further significant changes.
Revert the addition of the "snap-summary" tag. As discussed in the comments it is only applied inconsistently and rather appears to be a hack. The root cause is that the filter (...) function currently lacks a way to track modifications of the snapshot summary. So the clean way to implement this change is to add this modification tracking. Also please add an integration test that checks that a snapshot that has a summary is not modified unnecessarily.
[Edit]Please also update the docs at doc/045_working_with_repos.rst with the new functionality.[/Edit]
Other than that, please rebase the PR onto the latest master branch.
|
I rebased my work. |
MichaelEischer
left a comment
There was a problem hiding this comment.
I've added quite a few comments how to fix the implementation and how to remove the SnapshotSummary for a snapshot. Please don't introduce random backwards-incompatible changes in the PR, this will just result in additional work for everyone and reverting those changes.
cmd/restic/cmd_rewrite.go
Outdated
| return false, err | ||
| } | ||
|
|
||
| // bug: do not destroy old snapshot! |
There was a problem hiding this comment.
This is once again intended functionality and not a bug, see restic repair snapshots. (just look for all callers of filterAndReplaceSnapshot and do a bit of tracing the handling of broken trees from there). If a filter unintentionally does not return a filteredTree, then the filter is broken.
Please try to focus on the change this PR intends to make and don't make apparently random changes (at least from my perspective). While the existing code can definitely have bugs, start by assuming that it is sufficiently correct. It's getting annoying that each review round leads to a new bunch of unrelated changes which then have to be reverted.
There was a problem hiding this comment.
Sorry Michael,
I do not intend to annoy you. Forgive me for erring into areas which I am not supposed to touch.
I have reverted all the changes and I went back to square one to started fresh.
However I hit a buffer stop, since the changes I have made since upset tests
TestRepairSnapshotsIntact and TestRewriteUnchanged.
The condition filteredTree == *sn.Tree && newMetadata == nil is met when option --snapshot-summary is active by itself. However I cannot check it there. The only check I can think of is sn.Summary == nil.
See lines
//if filteredTree == *sn.Tree && newMetadata == nil && sn.Summary == nil { if filteredTree == *sn.Tree && newMetadata == nil {.
How do I proceed from here?
There was a problem hiding this comment.
I've reworked the PR quite a bit. The summary is now checked for changes via matchingSummary = sn.Summary != nil && *summary == *sn.Summary.
Went back to square one and implemeted the change which I had made before. I hit a buffer stop. See more details in restic#5185
cmd_rewrite: add capability to add snapshot summary data to an existing snapshot. Add option --snapshot-summary [-s] to enable this enhancement.
…ng snapshot Add option --snapshot-summary to enable this feature
…ng snapshot. Changes to the code as requested.
added error checking to --new-host and blank stripping to --new-time As a consequence of the change in the return parameter to the filter processing, the files cmd/restic/cmd_repair_snapshots.go, internal/walker/rewriter.go had to be touched as well I also include here a modified version of cmd/restic/testdata/backup-data.tar.gz, mainly used for cmd_ls integration testing.
changed wording - posed a question
if snapshot has already got history, just skip quietly and continue to next snapshot to be checked.
fixed error in hostname check highlighted bug when new tree is empty: don't delete a valid snapshot!
reset git repository to defined state
Went back to square one and implemeted the change which I had made before. I hit a buffer stop. See more details in restic#5185
corrected typo and implemented requested change.
implemented change that Michael Eischer recommended. It took a while until I understood what his comment implied. :)
made --snapshot-summary an exclusive option and therefore can deal with identical trees. Added parameter snapshotSummary (bool) to filterAndReplaceSnapshot. All integration tests pass.
fixed bug when --snapshot-summary is used on its own and snapshots have summary data. Needs to be skipped early. Added a short note in the documentation
b0b4d4e to
b88af22
Compare
|
I've reverted a bunch of the changes to an older state and made the refactoring I had in mind during the last round of reviews. I'll take another look for a final review in the next days. P.S.: the first line of a commit description should include a short summary of the changes introduced by that commit. Ideally, just reading through the summary of a bunch of commits later on provides a rough idea what has changed. |
MichaelEischer
left a comment
There was a problem hiding this comment.
LGTM. Thanks for proof of concept for the command!
…apshot. (restic#5185) Co-authored-by: Michael Eischer <michael.eischer@fau.de>
cmd_rewrite: add capability to add snapshot summary data to an existing snapshot.
Add option --snapshot-summary [-s] to enable this enhancement.
What does this PR change? What problem does it solve?
This pull request add the capability of adding snapshot summary data to an existing snapshot.
Was the change previously discussed in an issue or on the forum?
#4942
closes #4942
Checklist
changelog/unreleased/that describes the changes for our users (see template).