Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jsullivan3
Copy link

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.

@bebehei
Copy link
Contributor

bebehei commented May 20, 2016

Hi, thanks for the patch.

  • provide tests
  • use perltidy
  • write documentation (manpage only)

This is interesting in combination with #140 . But I see, I'm outing myself as a complete BTRFS-noob.

@bebehei
Copy link
Contributor

bebehei commented May 20, 2016

Sry for the long time without any response.

@jsullivan3
Copy link
Author

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?

@sgpinkus
Copy link
Member

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.

@MTecknology
Copy link

@jsullivan3 Is there any chance you have any interest taking another look at this pull request?

@jsullivan3
Copy link
Author

I'd be happy to do so! Sorry, I quite frankly lost track of this change with other issues that arose.
I'm a little rusty on my Perl at this point, unfortunately, so it may take a bit to get something that's acceptable to merge.
One concern I have is how to create tests to exercise this functionality. In other projects based in other languates, I've had success in creating loopback-based mounts that I could use to format BTRFS volumes. I can follow this strategy for rsnapshot testing as well, unless you can recommend a different strategy - any and all suggestions are welcome!

@MTecknology
Copy link

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

@jsullivan3
Copy link
Author

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...

@jsullivan3 jsullivan3 force-pushed the btrfs-rsnapshot branch 3 times, most recently from 2b7f6e7 to 41956c7 Compare December 12, 2019 20:13
@jsullivan3
Copy link
Author

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 perltidy and testing. I have a mock binary system to get around the requirement for privileged access to create btrfs file systems (and the minor detail that the Travis configuration doesn't appear to have btrfs available).

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 lvm tests, and I set up the mock btrfs and rsync system so that it can easily be extended to lvm and any other binaries.

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.

@jsullivan3
Copy link
Author

Should be good now - I fixed the Travis test failure... Thank you for your patience!

@jsullivan3
Copy link
Author

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?

@xBelladonna
Copy link

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.

@jsullivan3
Copy link
Author

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.

@xBelladonna
Copy link

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 - btrsnapshot maybe... I imagine you are not very interested in maintaining such fork but would you approve this?

@jsullivan3
Copy link
Author

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants