Improve restore #1044

lloeki · 2017-06-16T14:57:41Z

This improves restore performance by several orders of magniture by not going through the whole tree recursively when we can anticipate that no match will ever occur.

Here is a sample run restoring one file from a / backup of a Debian machine on S3:

time (DEBUG_LOG=/tmp/restic-debug.log ./restic restore --include /etc/resolv.conf 1141ffca --target restore)
debug log file /tmp/restic-debug.log
debug enabled
restoring <Snapshot 1141ffca of [/] at 2017-06-15 09:25:24.915676881 +0200 CEST by root@test> to restore
ignoring error for restore/etc/resolv.conf: Lchown: lchown restore/etc/resolv.conf: operation not permitted
There were 1 errors

real	0m5.075s
user	0m0.927s
sys	0m0.183s

time to reach the target file is significantly improved since it doesn't go down every directory before as well as inside /etc
time after having restored the file is very short since it doesn't go down every other directory

(The error about lchown is spurious since I'm not running this as root)

I have not included time from the current version since it just takes literally forever.

Timestamps and rights not being restored correctly will be the next unit of work once this is dealt with.

Fixes #982

lloeki · 2017-06-16T14:58:12Z

Please ignore SAVEPOINT commit, this is just WIP archival.

lloeki · 2017-06-16T14:59:34Z

src/restic/filter/filter.go

+	}
+
+	patterns := strings.Split(pattern, "/")
+	strs := strings.Split(str, "/")


There is duplication with Match here. This will be dealt with.

lloeki · 2017-06-16T15:03:01Z

src/restic/filter/filter_test.go

+	{"/foo", "/foo/bar", true}, // maybe this should be false
+	{"/*", "/foo", true},       // maybe this should be false
+	{"/*", "/foo/bar", true},   // maybe this should be false
+	{"/foo", "/foo/bar", true}, // maybe this should be false


There may be some semantic choice to be made here: /foo and /foo/* are equivalent to /foo/** currently. Maybe a single * could mean "restore just files at that level and foo or /foo mean "restore foo but if it's a dir, don't go down". I kept the existing behavior as is.

You're right, I like your suggestion. Make it so :) We'll need to document this, though.

lloeki · 2017-06-16T15:05:18Z

I have an integration test failing locally, but I can't make sense of it yet (I haven't looked into it, but I will).

lloeki · 2017-06-16T15:07:19Z

Good, Appveyor reproduced the same failure

lloeki · 2017-06-16T15:36:49Z

src/cmds/restic/cmd_restore.go

 		if err != nil {
 			Warnf("error for exclude pattern: %v", err)
 		}

-		return !matched
+		return !matched, !childMayMatch


Reverting that ! on childMayMatch fixes the integration test.

Do you know that you can just force-push reworked commits, and they will replace the commits here?

Yes I do, serial abuser of rebase -i over here :)

Had to leave pronto just as I found the fix but wanted to let you know (AFK for now)

Heh, nice. :)

fd0

First few comments are in, having an in depth look now.

fd0 · 2017-06-16T18:52:30Z

.gopath

@@ -0,0 +1 @@
+/Users/lloeki/Workspace/go


Remove this file from the PR

Yep, SAVEPOINT commit

fd0 · 2017-06-16T18:53:20Z

src/cmds/restic/cmd_restore.go

 	}

-	selectIncludeFilter := func(item string, dstpath string, node *restic.Node) bool {
-		matched, err := filter.List(opts.Include, item)
+	// This could return a `childMayMatch` bool and do some additional testing using filter.Match or something alike


Is this a work-in-progress comment?

Yep, SAVEPOINT commit

fd0 · 2017-06-16T19:02:02Z

src/restic/filter/filter_test.go

+	{"/foo", "/foo/bar", true}, // maybe this should be false
+	{"/*", "/foo", true},       // maybe this should be false
+	{"/*", "/foo/bar", true},   // maybe this should be false
+	{"/foo", "/foo/bar", true}, // maybe this should be false


You're right, I like your suggestion. Make it so :) We'll need to document this, though.

fd0 · 2017-06-16T19:59:57Z

I haven't found any other issues, good work already!

lloeki · 2017-07-06T16:07:41Z

Regarding this:

	{"/foo", "/foo/bar", true}, // maybe this should be false
	{"/*", "/foo", true},       // maybe this should be false
	{"/*", "/foo/bar", true},   // maybe this should be false
	{"/foo", "/foo/bar", true}, // maybe this should be false

You're right, I like your suggestion. Make it so :) We'll need to document this, though.

I'd rather handle it in a separate issue/PR as this seems a bit more involved and I don't want to hold this PR back.

lloeki · 2017-07-06T16:12:03Z

Timestamps and rights not being restored correctly will be the next unit of work once this is dealt with.

@fd0 Would you like me to split this fix in a separate PR? I have not worked on it yet although I have some ideas for a fix.

codecov-io · 2017-07-06T16:32:46Z

Codecov Report

Merging #1044 into master will decrease coverage by 5.67%.
The diff coverage is 74.57%.

@@            Coverage Diff             @@
##           master    #1044      +/-   ##
==========================================
- Coverage   52.96%   47.28%   -5.68%     
==========================================
  Files         126      126              
  Lines       12035    12077      +42     
==========================================
- Hits         6374     5711     -663     
- Misses       4921     5677     +756     
+ Partials      740      689      -51

Impacted Files	Coverage Δ
internal/restic/restorer.go	`0% <0%> (ø)`	⬆️
cmd/restic/cmd_backup.go	`24.29% <100%> (ø)`	⬆️
cmd/restic/cmd_restore.go	`62.5% <100%> (+2.5%)`	⬆️
internal/filter/filter.go	`72.64% <73.8%> (-0.22%)`	⬇️
internal/backend/b2/b2.go	`0% <0%> (-79.32%)`	⬇️
internal/backend/swift/swift.go	`0% <0%> (-73.9%)`	⬇️
internal/backend/azure/azure.go	`0% <0%> (-61.93%)`	⬇️
internal/backend/gs/gs.go	`0% <0%> (-60.79%)`	⬇️
internal/backend/swift/config.go	`34.37% <0%> (-56.25%)`	⬇️
internal/archiver/archiver.go	`64.95% <0%> (+0.16%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d87b2f1...f880ff2. Read the comment docs.

lloeki · 2017-07-07T08:32:39Z

Hmm, I had a hunch and tested this some more, and the behaviour for exclude filters is incorrect. ./restic restore --exclude /etc/resolv.conf 37b9c56d --target restored skips the whole of /etc

lloeki · 2017-07-07T08:35:51Z

Just to be sure I tested multiple include filters and it behaves as expected: ./restic restore --include /bin/bash --include /etc/resolv.conf 37b9c56d --target restored.

lloeki · 2017-07-07T10:01:09Z

The exclusion case is now fixed and its performance improved as well. I made some variable names more explicit (notably semantic of matched vs selected) to make it easier to reason about the code.

Tested cumulative excludes successfully: ./restic restore --exclude /bin/bash --exclude /boot/grub 37b9c56d --target restored

fd0 · 2017-07-08T08:14:18Z

Cool, let me know once you're satisfied with the PR and I'll have a look :)

lloeki · 2017-07-17T06:59:30Z

Sorry this fell off the radar, go for it!

fd0 · 2017-07-23T10:05:38Z

Hey, just a quick question: We're switching to the idiomatic Go repository layout (see #1116). May I convert your changes and force-push your branch?

lloeki · 2017-07-23T10:12:45Z

Sure :)

fd0 · 2017-08-06T06:38:47Z

Hm, it seems I'm not allowed to push into your branch. Could you please check if you've enabled this feature? https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

Alternatively, just do a git rebase master and force push yourself, all changes can be merged automatically.

This improves restore performance by several orders of magniture by not going through the whole tree recursively when we can anticipate that no match will ever occur.

An exclude filter is basically a 'wildcard but foo', so even if a childMayMatch, other children of a dir may not, therefore childMayMatch does not matter, but we should not go down unless the dir is selected for restore.

lloeki · 2017-08-16T13:30:25Z

Weird, that setting is active:

Anyway, I rebased.

fd0 · 2017-08-17T19:28:33Z

Thanks a lot, I'll have a look!

fd0 · 2017-09-04T19:54:02Z

Please excuse the long time I sat on this PR. It looks great and I've merged it. Thanks a lot for your work!

Improve restore

lloeki · 2017-09-05T07:43:56Z

Awesome, many thanks!

fd0 · 2017-09-05T08:37:22Z

Would you like to continue working on the timestamps issue?

lloeki · 2017-09-05T09:15:45Z

If time is not of the essence on that front, yes I can get my hands on it.

fd0 · 2017-09-05T09:28:16Z

It isn't, but we should probably document that in an issue (and that you'll work on it sometime).

lloeki · 2017-09-05T10:58:31Z

OK, I created the issue.

fd0 · 2017-09-05T11:17:58Z

Awesome, thanks!

lloeki commented Jun 16, 2017

View reviewed changes

lloeki mentioned this pull request Jun 16, 2017

Restore code is inefficient and produces inconsistent timestamps/permissions #982

Closed

lloeki commented Jun 16, 2017

View reviewed changes

fd0 requested changes Jun 16, 2017

View reviewed changes

fd0 added the PR:WIP label Jun 17, 2017

lloeki force-pushed the 982-improve-restore branch 2 times, most recently from 1daf014 to 23a950d Compare July 6, 2017 16:06

lloeki force-pushed the 982-improve-restore branch from b321293 to 45a2256 Compare July 7, 2017 09:55

lloeki force-pushed the 982-improve-restore branch from 45a2256 to 0813770 Compare July 8, 2017 07:30

fd0 removed the PR:WIP label Jul 21, 2017

lloeki added 2 commits August 16, 2017 15:25

Smarter filter when children won't match

4a36993

This improves restore performance by several orders of magniture by not going through the whole tree recursively when we can anticipate that no match will ever occur.

Fixing restore with excluded

f880ff2

An exclude filter is basically a 'wildcard but foo', so even if a childMayMatch, other children of a dir may not, therefore childMayMatch does not matter, but we should not go down unless the dir is selected for restore.

lloeki force-pushed the 982-improve-restore branch from 0813770 to f880ff2 Compare August 16, 2017 13:29

fd0 mentioned this pull request Aug 18, 2017

implement constrained restore that only walks paths specified in the --include pattern #1166

Closed

fd0 merged commit f880ff2 into restic:master Sep 4, 2017

fd0 added a commit that referenced this pull request Sep 4, 2017

Merge pull request #1044 from lloeki/982-improve-restore

fa2ee78

Improve restore

lloeki mentioned this pull request Sep 5, 2017

Restore code produces inconsistent timestamps/permissions #1212

Closed

lloeki deleted the 982-improve-restore branch November 7, 2017 08:35

		@@ -0,0 +1 @@
		/Users/lloeki/Workspace/go

Improve restore #1044

Improve restore #1044

Conversation

lloeki commented Jun 16, 2017

lloeki commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lloeki commented Jun 16, 2017

lloeki commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fd0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fd0 commented Jun 16, 2017

lloeki commented Jul 6, 2017

lloeki commented Jul 6, 2017

codecov-io commented Jul 6, 2017 • edited Loading

Codecov Report

lloeki commented Jul 7, 2017 • edited Loading

lloeki commented Jul 7, 2017

lloeki commented Jul 7, 2017 • edited Loading

fd0 commented Jul 8, 2017

lloeki commented Jul 17, 2017

fd0 commented Jul 23, 2017 • edited Loading

lloeki commented Jul 23, 2017

fd0 commented Aug 6, 2017 • edited Loading

lloeki commented Aug 16, 2017

fd0 commented Aug 17, 2017

fd0 commented Sep 4, 2017

lloeki commented Sep 5, 2017

fd0 commented Sep 5, 2017

lloeki commented Sep 5, 2017

fd0 commented Sep 5, 2017

lloeki commented Sep 5, 2017

fd0 commented Sep 5, 2017

codecov-io commented Jul 6, 2017 •

edited

Loading

lloeki commented Jul 7, 2017 •

edited

Loading

lloeki commented Jul 7, 2017 •

edited

Loading

fd0 commented Jul 23, 2017 •

edited

Loading

fd0 commented Aug 6, 2017 •

edited

Loading