-
Notifications
You must be signed in to change notification settings - Fork 259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for BTRFS snapshots #138
base: master
Are you sure you want to change the base?
Conversation
Hi, thanks for the patch.
This is interesting in combination with #140 . But I see, I'm outing myself as a complete BTRFS-noob. |
Sry for the long time without any response. |
Thank you for the feedback, no worries regarding the response time. I'm excited about the chance to contribute back to this project - I've been using it for years and appreciate the hard work that's been put into making it a solid backup system. I hadn't heard of perltidy until just now - I'll run it against the changes and commit any suggested modifications. Would you prefer that I add a commit to this PR for the perltidy changes, or would you prefer that I close this PR and submit a new one with all commits squashed into one? I can go either way - please let me know which strategy would make things easier for you. As for tests - I didn't see any tests for the LVM functionality (which is Linux-specific), so I wasn't sure if Linux-specific tests for the BTRFS snapshot functionality would even be feasible. If I submit tests that exercise the BTRFS snapshot functionality, may I request help in making them specific to Linux so that my changes do not break the test harness on non-Linux platforms? |
Please don't open another PR. Use this one and squash the perltidy commit into it. On tests; the LVM and a lot of other stuff was added before @bebehei became responsible for this repo, and gave it some new legs. The policy now is that everything should be tested. All that old stuff really should have tests for it for regressions, but to do that retrospective is unrealistic. As for new stuff, its preferred that there are tests for every new bit of functionality. Or where a new commit touches old functionality, tests are added to ensure the behavior is the same before and after. Currently we are only testing on Linux. There was Mac OSX in there too, but I disabled it because they were broken anyway. I'm fine with only covering Linux. If we could get OSX coverage back, that would be a bonus. When we get to that, we'll figure out how to disable platform specific tests where necessary. Should just be a simple switch. Also aside: I also have no knowledge of BTRFS, and don't have an opinion one way or the other on this PR. Yet to look it over in detail. |
@jsullivan3 Is there any chance you have any interest taking another look at this pull request? |
I'd be happy to do so! Sorry, I quite frankly lost track of this change with other issues that arose. |
This project has been unmaintained for a while. I think all PRs were kinda lost to the back burner. Note- I'm not a maintainer so I can't exactly accept any changes, but maintenance seems to be getting revived. What you described seems to match how other tests are performed. I don't see any reason that this approach should be avoided. Hey! If you end up having a lot of fun with that approach, perhaps you'd be interested in writing some tests for the other btrfs PRs? :D |
50f22fe
to
5d165b7
Compare
Well, I figured out perltidy at least, and updated the PR to resolve conflicts and conform to the perltidy guidelines. This set of updates should revive the code a bit and make it more feasible to test. My concerns about using loopback devices include requiring some type of privileged access to run tests. I'm going to dig around the SysWrap functionality that other tests are using to see if I can use it to implement non-privileged tests. Other thoughts and ideas would be greatly appreciated. I'll start with this PR and if I can find time to do others, I'd certainly be happy to do so - but I can't guarantee anything... |
2b7f6e7
to
41956c7
Compare
I'm working through a few iterations to debug the test failures on Travis (the tests work fine in my development environment - though isn't that always the case), but I think the PR is in a better state with Any thoughts you may have regarding the way I implemented the tests would be greatly appreciated. I suspect a similar mechanism would be helpful for I apologize for the churn as I learn how to generate unit tests in Perl and include information that is helpful when triaging test failures as reported by Travis. |
41956c7
to
6943a92
Compare
…hots of subvolumes.
6943a92
to
38dbd9c
Compare
Should be good now - I fixed the Travis test failure... Thank you for your patience! |
Hi team - I believe this PR should be ready for a final review, and that the "incomplete" and "needs tests" labels can be removed at this point. Is anything else required before this change set is considered ready? Is it still in consideration to be merged? |
I cannot reasonably ask for any precise timeframe but is it planned to merge this at all? I have just lost an entire VM and all its backups because of rsnapshot failing once I had chosen btrfs as root filesystem. Thankfully I had block-level VM backup but this happened a few hours before the weekly backup was due and I lost a whole week of writes. This patch would have prevented all this and will be able to prevent it in future but until this or similar PR is merged then I am confined to ext4/xfs on lvm. Please let me know. |
Hi @xBelladonna - I'm sorry to hear that you had that experience. I just updated the branch with latest commits from the upstream rsnapshot repository. If you're interested in trying it out, please feel free to test drive https://github.com/jsullivan3/rsnapshot/tree/btrfs-rsnapshot. In the meantime, I believe that the PR has been updated to address the requests for documentation and tests. @MTecknology and @sgpinkus - can you please advise as to what needs to be done to merge this PR? I have since moved on to other backup mechanisms and no longer rely on rsnapshot, but I'm willing to continue working on this change if others would find it useful and helpful. |
Hi @jsullivan3, apologies for the late reply, I have been busy with work. Thanks very much for being willing to continue this! I have also since started trying other backup mechanisms such as btrbk but still use rsnapshot as it is the simplest tool which allows me to specify exactly what to back up instead of "just this huge subvoume here". I will build and test your fork and see how it goes. If there are not any activity here from maintainers for some time I am considering maintaining a fork with your changes - |
I would not have the time or ability to maintain such a fork, but I'd be happy for somebody to be able to make use of this set of changes. If something goes wrong with the build that includes this PR, please let me know and I will do what I can to assist. Though I also can't guarantee speedy responses due to my own commitments... |
The LVM snapshot functionality is extremely useful, but unfortunately it cannot be used for LVM volumes that are formatted as BTRFS due to the fact that the snapshot and the source volume would share the same UUID. The BTRFS Wiki's "Gotchas" page warns to never mount two different file systems that share the same UUID because of the strong potential for data corruption.
The changes in this PR model the LVM snapshot functionality to add the ability to create BTRFS snapshots so that data stored on a BTRFS subvolume can safely be archived using rsnapshot.
While BTRFS supports snapshot and send/receive functionality that may provide a more efficient backup mechanism, support for BTRFS snapshots in the rsnapshot utility will be very useful in environments that use a mix of BTRFS and other file systems so that a system administrator can rely on a single backup scheme rather than requiring separate backup schemes for different file systems.
Commits from PR#136 have been squashed and summarized as requested in that PR.