Improve recover command #2880

aawsome · 2020-08-07T04:03:59Z

What does this PR change? What problem does it solve?

Enhance the recover command to make it useful in more recovery situations:

only save data not referenced by any snapshot
dont't write an empty snapshot
use progress bar for scanning trees

OLD AND REMOVED:

allow to combine all found directories in one snapshot (equals the current behavior; default) or make one snapshot per "hanging" root tree

allow to add user-defined tags to newly generated snapshots

add dry-run possibility

Especially the flags --split=true seem very useful for me: If you forget a couple of snapshots and run restic recover --split=true, you'll get the removed snapshots back (but deduplicated in the sense that multiple completely unchanged snapshots will resurrect only once and without snapshot metadata).

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

no.

Checklist

I have read the Contribution Guidelines
I have enabled maintainer edits for this PR
I have not added tests for all changes in this PR
I have not added documentation for the changes (in the manual) - there is no docu for recover so far
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

aawsome · 2020-11-22T21:10:13Z

rebased this to resolve the merge conflicts.
I have to add a changelog entry and maybe docu for the new recover command (there is no docu for the current recover, though), but besides that, this PR is ready for review.

MichaelEischer

I don't think that the recover command needs that many flags. For me it looks like --all=false is the behavior everyone would. --all=true just clutters the recovered snapshot(s). Now with the new prune implementation, there will be hundred or thousands of "root" trees in the repository, which makes --combine=false essentially useless in my opinion. When recover only creates a single snapshot, I don't see much use for a --dry-run option either, in the worst case recover will create a single snapshot which is also tagged accordingly.

--all=false --combine=false will not really recover deleted snapshots, these still miss all metadata.

cmd/restic/cmd_recover.go

MichaelEischer · 2020-12-31T13:03:11Z

cmd/restic/cmd_recover.go

+	recoverFlags := cmdRecover.Flags()
+	recoverFlags.BoolVar(&recoverOptions.All, "all", false, "add all directories, not only unreferenced ones")
+	recoverFlags.BoolVar(&recoverOptions.Combine, "combine", true, "if true, combine all directories into one snapshot, else generate multple snapshots")
+	recoverFlags.BoolVarP(&recoverOptions.DryRun, "dry-run", "n", false, "only show what would be done")


--dry-run only seems to be useful in combination with --combine=false?

Another use case would be to use it for debugging/troubleshooting which would also work on read-only repositories.

cmd/restic/cmd_recover.go

rawtaz · 2021-07-17T21:43:34Z

What's the use case of tagging a snapshot created by recover? This command is something you'd run manually when needed, so why would you need to tag it? Even if you do, you could just use the existing tag command so there's little need for adding code to tag these manually produced snapshots IMO.

Also, I don't understand the point of having a boolean argument to --split. If the default is to not split, then just --split would be enough, no?

aawsome · 2021-07-20T18:32:34Z

What's the use case of tagging a snapshot created by recover? This command is something you'd run manually when needed, so why would you need to tag it? Even if you do, you could just use the existing tag command so there's little need for adding code to tag these manually produced snapshots IMO.

With --split you can get quite a lot of new snapshots. I thought it might be handy to let recover tag them all.
And it is exactly 3 extra lines of code...

Also, I don't understand the point of having a boolean argument to --split. If the default is to not split, then just --split would be enough, no?

You are right. Of course --split also works.

aawsome · 2021-07-20T18:38:22Z

rebased to solve merge conflicts.

MichaelEischer

I'm still not convinced about the --split option as it just feels like a crutch to me. After all, the whole metadata of a snapshot is still lost in the process. So, I'd like to limit this PR to the code cleanups and tweaks, but defer new options to a discussion on how "un-deleting" a snapshot could/should work.

Please remove the --split and the then obsolete --dry-run and --tags options. The factored out createSnapshot function can stay though.

MichaelEischer · 2021-07-17T14:06:58Z

cmd/restic/cmd_recover.go

 		tree, err := repo.LoadTree(gopts.ctx, id)
 		if err != nil {
 			Warnf("unable to load tree %v: %v\n", id.Str(), err)
 			continue
 		}

 		for _, node := range tree.Nodes {
-			if node.Type != "dir" || node.Subtree == nil {
-				continue
+			if node.Type == "dir" && node.Subtree != nil {


As a follow-up PR it would be possible to introduce a ForAllTrees function similar to ForAllSnapshots etc.

Very true 👍
Let's see when I'll find some time for it 😉

- only save directories not referenced by any snapshot - dont't write empty snapshot - use progress bar

aawsome · 2021-09-03T19:41:50Z

I'm fine with postponing the '--split' or something else to a future PR.

Please remove the --split and the then obsolete --dry-run and --tags options. The factored out createSnapshot function can stay though.

Is removed. Hope these small improvements are nevertheless helpful!

MichaelEischer

LGTM.

Excluding trees belonging to snapshot might be quite helpful, especially for a repository containing lots of snapshots. And the code cleanup is also a plus :-) .

aawsome changed the title ~~Improve recover~~ Improve recover command Aug 7, 2020

aawsome force-pushed the enhance-recover branch from 2c80985 to bea3a5f Compare November 22, 2020 21:07

aawsome force-pushed the enhance-recover branch from bea3a5f to bf5931a Compare November 22, 2020 21:13

MichaelEischer requested changes Dec 31, 2020

View reviewed changes

aawsome force-pushed the enhance-recover branch 2 times, most recently from ae8dc42 to 0f2fe6a Compare December 31, 2020 22:51

This was referenced Jan 15, 2021

prune: Remove all unused trees #3228

Merged

backup: Add --resume option #3229

Closed

aawsome force-pushed the enhance-recover branch from 0f2fe6a to fac12d3 Compare July 20, 2021 18:37

MichaelEischer requested changes Jul 23, 2021

View reviewed changes

Improve recover

bce8792

- only save directories not referenced by any snapshot - dont't write empty snapshot - use progress bar

aawsome force-pushed the enhance-recover branch from fac12d3 to bce8792 Compare September 3, 2021 19:38

MichaelEischer approved these changes Sep 3, 2021

View reviewed changes

MichaelEischer merged commit bc4cbd7 into restic:master Sep 3, 2021

aawsome deleted the enhance-recover branch February 24, 2024 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve recover command #2880

Improve recover command #2880

aawsome commented Aug 7, 2020 •

edited

aawsome commented Nov 22, 2020

MichaelEischer left a comment

MichaelEischer Dec 31, 2020

aawsome Dec 31, 2020

rawtaz commented Jul 17, 2021 •

edited

aawsome commented Jul 20, 2021

aawsome commented Jul 20, 2021

MichaelEischer left a comment

MichaelEischer Jul 17, 2021

aawsome Sep 3, 2021

aawsome commented Sep 3, 2021

MichaelEischer left a comment

Improve recover command #2880

Improve recover command #2880

Conversation

aawsome commented Aug 7, 2020 • edited

What does this PR change? What problem does it solve?

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

Checklist

aawsome commented Nov 22, 2020

MichaelEischer left a comment

Choose a reason for hiding this comment

MichaelEischer Dec 31, 2020

Choose a reason for hiding this comment

aawsome Dec 31, 2020

Choose a reason for hiding this comment

rawtaz commented Jul 17, 2021 • edited

aawsome commented Jul 20, 2021

aawsome commented Jul 20, 2021

MichaelEischer left a comment

Choose a reason for hiding this comment

MichaelEischer Jul 17, 2021

Choose a reason for hiding this comment

aawsome Sep 3, 2021

Choose a reason for hiding this comment

aawsome commented Sep 3, 2021

MichaelEischer left a comment

Choose a reason for hiding this comment

aawsome commented Aug 7, 2020 •

edited

rawtaz commented Jul 17, 2021 •

edited