restorer: allow writing target file blobs out of order #2195
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2195 +/- ##
=========================================
Coverage ? 45.82%
=========================================
Files ? 174
Lines ? 14290
Branches ? 0
=========================================
Hits ? 6549
Misses ? 6730
Partials ? 1011
Continue to review full report at Codecov.
|
|
@fd0 @mholt can you review this PR when you get a chance? It is the code discussed in #2074, and should be ready or almost ready to be merged to master. Compared to master, this PR does not guarantee target files are written sequentially, start to finish, which allowed much simpler implementation and faster multithreaded restore of very large files. |
|
Sure -- I just barely finished my masters thesis so I'm in major catch-up mode, but at a glance I really like the ~1K LoC reduction. Don't wait up on me, but I will try to give it a high-level once over soon. |
ifedorenko
force-pushed
the
out-of-order-restore-no-progress
branch
from
f4c7335
to
9491b27
Compare
ifedorenko
force-pushed
the
out-of-order-restore-no-progress
branch
from
9491b27
to
5db73a6
Compare
|
Hello everyone, and specially @ifedorenko. Just to make sure: this PR objective is speed optimization, not memory usage, right? I ask because it has been implied in the forum that it could help me save some memory during the restore, but I'm seeing the exact opposite (about double memory usage as compared to "standard" v0.9.5 restic). PS: if you need anything, I compiled it with Cheers, |
|
This PR is supposed to use little less memory compared to 0.9.5, can't think of why it takes 2x memory instead. I'd need to know what data structures use the memory in your tests to suggest anything specific. |
|
Hello @ifedorenko,
Perhaps my assertion of 2X memory was premature: it used 2x the memory in the first 15m, but that could have been because it just progressed farther in that interval than standard 0.9.5. I'm monitoring it as it runs, and after ~1h05m since then, it seems to have plateaued at about the same 36GB of RAM it reached in those first 15 mins. If it has really plateaued, it would be using only 90% (ie, 10% less) than the ~40GB that standard 0.9.5 ended up using up to the point I had to kill it. I will continue to monitor it and report back on its memory use.
Would the output of Cheers, |
The current Sorry for the false alarm. |
|
The restore finished a few hours later and, in a word: WOW! :-) In more words: it finished in 4h08m the same restore that standard 0.9.5 restic took over 72 hours and reached only 2/3 -- so, performance-wise, congratulations @ifedorenko! Thanks to your work, I escaped having to junk restic and all the hard work I invested implementing and tuning it here -- it would not have been viable to keep using a backup method that was so slow when restoring things. On a less positive note, I detected two corrupted files -- more details on my forum post. To sum it up, I think there's a good chance it wasn't caused by restic -- I will do some more tests to try and locate the issue, and then come back to report. Cheers, |
More info memory-wise: when I re-run the exact same |
| // of concurrent writeToFile invocations plus cacheCap. | ||
| // writes blobs to target files. | ||
| // multiple files can be written to concurrently. | ||
| // multiple blobs can be concurrently written to the same file. |
There was a problem hiding this comment.
This might cause issues with FUSE based systems, especially FUSE mounted high-latency systems.
There was a problem hiding this comment.
@saikpr Can you please elaborate a bit on this? In what way can it cause issues, for what reason in FUSE?
|
This code restored my ~60GB backup in half an hour, whereas the old restorer code would have finished in months (no hyperbole) at the rate it was going. In both cases the entire repo was already downloaded. As a user, I'd highly recommend merging this. |
|
This did improve the RAM and speed (but not that much if you increase the Chunk Size to 128MB, the difference was significant at low chunk sizes) But Rclone was throwing a lot more Eventually both the restores were exactly same, byte-by-byte only the stderr was polluted with the above error. You can find the logs here out_of_band.txt |
|
Hi, I was having an issue with this:
The last error was because I cancelled the restore process. And also sometimes the restore fail, those backups are about 45G, sometimes the restore only does 35G But after using the branch from @ifedorenko it worked perfectly:
As you may notice we are using rest-server (I had to blur url and customer name). Hope this add any value to the discussion. Regards |
|
One quick fix will be to reduce the number of workers (here
https://github.com/restic/restic/blob/master/internal/restorer/filerestorer.go#L24)
or get more RAM :p
--
Sainyam Kapoor
…On Tue, Aug 6, 2019 at 10:58 AM Tulio Ruiz ***@***.***> wrote:
Hi,
I was having an issue with this:
[image: image]
<https://user-images.githubusercontent.com/2739530/62550112-4ad5a580-b82f-11e9-94a0-c010bc72d7e4.png>
The last error was because I cancelled the restore process. And also
sometimes the restore fail, those backups are about 45G, sometimes the
restore only does 35G
But after using the branch from @ifedorenko
<https://github.com/ifedorenko> it worked perfectly:
[image: image]
<https://user-images.githubusercontent.com/2739530/62550574-16aeb480-b830-11e9-92b3-21613eccd4c8.png>
As you may notice we are using rest-server (I had to blur url and customer
name). Hope this add any value to the discussion.
Regards
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2195?email_source=notifications&email_token=AA6K6YLXAWNT7AHKTN7DIPLQDGGSDA5CNFSM4G3K4YYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3VNRBY#issuecomment-518707335>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA6K6YLOANEYR7Y4BBUFH6LQDGGSDANCNFSM4G3K4YYA>
.
|
|
Having already demo'd this branch at work, I definitely think it needs to be merged in. Allowed me to circumvent a ton of |
ifedorenko
force-pushed
the
out-of-order-restore-no-progress
branch
from
5db73a6
to
ab23ab9
Compare
|
fyi, rebased my branch on the current master, also split |
|
Really waiting this become production to start using restic in my company. We have some huge VM disks to synchronize to GDrive and actually they don't perform well to restore. I've tested this branch and worked perfectly, but until become stable I can't use in production. |
|
This branch is an absolute life saver. I had to use PR2195 in order to successfully restore about 50 TB of business data. The stock restic restore had been running for weeks with no end in sight. This version restored 9 TB of files overnight! imo, in it's current state, restic-master is not a usable backup solution. It's terrible restore performance is unacceptable, making it useless on large repositories. This pull request should be merged to master as soon as possible. This deserves to be prioritized above all other pending requests. Thanks ifedorenko. This saved my bacon. |
|
For the record, I wrote the current broken implementation too, so I am to blame for the current state of affairs. There is O(N^2) loop there, which gets progressively slower with number of blobs in a file. It can be fixed, but the fix would add complexity to already complex code and I think this PR is overall better deal. |
He's probably referring to the comments which were addressed by commit 430e058. |
rawtaz
force-pushed
the
out-of-order-restore-no-progress
branch
from
c83044f
to
9dbafbf
Compare
|
@ifedorenko I'm terribly sorry for saying there were unresolved lines. You indeed fixed that a while ago. GitHub for some reason still showed them as unresolved, so I thought they weren't. I clicked the resolve button now, so no more confusion on that part :P I took the liberty of updating the changelog file a bit and squashed some commits. From where I stand this PR is ready for merging, just going to get @fd0 on it in a short while before we push the button. I have no doubts it's going to be merged soon. |
|
@rawtaz I would prefer two commits, to be honest, one for the code changes including changelog, and the other for |
Much simpler implementation that guarantees each required pack is downloaded only once (and hence does not need to manage pack cache). Also improves large file restore performance. Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
rawtaz
force-pushed
the
out-of-order-restore-no-progress
branch
from
9dbafbf
to
c52198d
Compare
There was a problem hiding this comment.
Awesome work, thank you very much @ifedorenko! And I'm very sorry it took us so long to get this merged :)
|
@fd0: We have the same problem. Restoring from 1TB repository brings up the caching errors on serveral files. We are using 0.9.6. Please release this fix ASAP if not done already! |
@buhkguj Please be more specific. The same problem as what? Is the problem you are referring to already documented somewhere? If so, please provide a link to it. Thanks. |
We got restore errors like this: Issue: #2244 |
|
Guys, |
|
@i0x71 @rawtaz You can find the binaries on the beta website here : https://beta.restic.net/restic-v0.9.6-160-gf033850a/ |
|
@i0x71 That is 11 months during which you and everyone else who had this specific problem has had all the opportunity in the world to try/use this fix, and in fact some users who wanted to try it actually did so and have been using it ever since. All you had to do was build restic yourself (which is just one command assuming you have Go installed), and if you didn't know about this you just had to ask. We'd be happy to help you in case you didn't know how to do it. That said, can you tell me in what way the team that maintains restic is less trustworthy due to it taking a long time to get this fix into master (and released)? In what way does us having a life to manage, sometimes with severe issues to deal with that you cannot even imagine, and having kids, family, relatives and a full time job including travels to work out during the approximately 15 hours we're awake, make us less trustworthy? On the contrary I would suggest that we'd be less trustworthy if we merged this and furthermore put it out as an official release the day after the PR was created. I hope you understand why that would be more of an issue. FWIW, there will always be changes and fixes that are either waiting to be merged or that have been merged but not put into a release, this was just one of them. I agree it took too long, but there are reasons for that and anyone who needed this fix had the possibility to use it, you don't need a release for that. At the very least just ask if you can try it beforehand. Thanks! |
|
I tested the beta bin and the restore was done without errors! |
|
I'm sorry but I sort of have to agree with the criticism. Today I had to panic-restore some data and then stumbled across the errors and that really did not lighten my day as it increased my panic even more to find that the data was not fully restored. It took me some googling and clicking through issues here to find this and the link to the beta binaries which then could properly restore the data. While I understand some of the reasoning I really think that if there is an issue with the released version not restoring (all) data it is a good idea to release the fixed version in time. People in my situation will clearly be thankful because when you are in need of a restore it really does not lighten the day when the restore fails. At minimum imho there should be a note about the issue and pointing to the beta release. And yes, my initial reaction was along the lines of "ok, forget about restic, need to check for a better backup tool later". I'm glad that in the end it turned out all good, but still. I think some might be underestimating the weight of this issue and how it makes the endusers feel. And I certainly feel that "there will always be changes and fixes that are either waiting to be merged or that have been merged but not put into a release, this was just one of them" seems like a odd statement considering that we are not talking about some oddball feature but the core functionality of a backup tool: restoring data. Just my 2ct, thanks for considering my thoughts and of course thanks for all the work behind this project. I really do not mean this in a rude way please take it as constructive criticism. |
|
It might help if there would be some kind of roadmap where users would be able to see when the new version will be released. |
|
@dhilgarth One it's released, you will see the release tag for the commits in this pull request (StackOverflow). In addition, some roadmap to see which issues are blocking a release would be nice. |
No. And as you can see there's a huge flow of issues, comments and PRs taking up a lot of time for the maintainers. It's hard to manage all that and we need the flexibility in releasing when we think that we are able to. Having to maintain a roadmap would in my personal opinion only be counter-productive, it would not help. |
|
@rawtaz All these project management tools help to stay focused and solve the important issues first instead of being overwhelmed by the huge flow of issues. Overall productivity will increase. If it was my project, I would make use of them. But the team probably knows best what works for them... |
|
I spent some time with one of them and it turned to not work at all. It was a complete waste of time, and I was honestly a bit annoyed by the fact that it advertises itself as this great thing, yet when you try it it's utter garbage. I guess this hasn't helped raising my love for such tools :D To be fair, it wasn't a project management tool specifically, more like a GitHub issues+PR management tool. Anyway, I hear you, and my comment was only about how I feel about it, perhaps others in the team want to or use such tools. Regardless, I truly don't think having a roadmap would help us. A release will come when it's ready. |
|
@rawtaz To get me right, I am not requesting some sort of timeline when to expect the next release. It would be more to assign issues to milestones (which represent the next minor, major and future releases) and cheer for your great work for every solved issue. This way, users can see which issues are blocking a release and estimate how much work is left before a release can happen (it will also avoid users posting questions like this). Using such tools allows to separate communication on (new) issues and working on / fixing issues. This way one can stay focused while working on issues which usually increases productivity a lot. |
|
That's great. I am not going to add any further comments on this. Thanks for your suggestion! |
What is the purpose of this change? What does it change?
Much simpler implementation that guarantees each required pack
is downloaded only once (and hence does not need to manage
pack cache). Also improves large file restore performance.
Was the change discussed in an issue or in the forum before?
See #2074
https://forum.restic.net/t/restore-using-rclone-gdrive-backend-is-slow/1112/8
https://forum.restic.net/t/degraded-restore-performance-s3-backend/1400
Checklist
changelog/unreleased/that describes the changes for our users (template here)gofmton the code in all commits