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

nixos/btrbk: init #118629

Closed
wants to merge 1 commit into from
Closed

nixos/btrbk: init #118629

wants to merge 1 commit into from

Conversation

bratorange
Copy link
Contributor

Adding support for configuring the btrbk backup utility via
the configuration.nix. Atm this is more a less a direct mapping
from nix syntax into the configuration syntax of the tool.

The unit test requires a rather big testdisk (120M). This is
due to the fact, that btrfs images cant be smaller than that.

Motivation for this change

I am using btrbk for my backups and wanted a way do configure it via the normal nixos way.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Adding support for configuring the btrbk backup utility via
the configuration.nix. Atm this is more a less a direct mapping
from nix syntax into the configuration syntax of the tool.

The unit test requires a rather big testdisk (120M). This is
due to the fact, that btrfs images cant be smaller than that.
@symphorien
Copy link
Member

Thanks for the PR!
I have a competing implementation here: https://discourse.nixos.org/t/btrfs-backup-tools-in-nixos/11364/3
I'm obviously biased but I think mine is more complete

  • supports several configuration files, which is necessary to have snapshots at several frequencies
  • sets systemd timers up
  • supports serving snapshots via ssh
    Also did you take into account that the order of options matters:
volume ssh://foo
 snapshot_dir var/lib/btrbk/remote
 target /archive/btrbk
 subvolume var/lib/bitwarden_backup

iirc target must appear before subvolume because otherwise, subvolume will use the target of the enclosing context. If you map it to the obvious nix, it fails because nix sorts keys alphabetically.

Your module also has advantages over mine:

  • typechecking at evaluation time instead of build time
  • it has a test !

Again, I'm obviously biased, but what would you think about taking my module and your test?

@bratorange
Copy link
Contributor Author

bratorange commented Apr 7, 2021

Also did you take into account that the order of options matters:

volume ssh://foo

 snapshot_dir var/lib/btrbk/remote
 target /archive/btrbk
 subvolume var/lib/bitwarden_backup

iirc target must appear before subvolume because otherwise, subvolume will use the target of the enclosing context. If you map it to the obvious nix, it fails because nix sorts keys alphabetically.

Oh no didn't knew that this was important. Thanks for the notice!

I'm obviously biased but I think mine is more complete

  • supports several configuration files, which is necessary to have snapshots at several frequencies
  • sets systemd timers up
  • supports serving snapshots via ssh

Yeah I also so saw some smaller details, which were missing in my implementation. Like the extraPackages thing. I hadn't thought about this.

Your module also has advantages over mine:

  • typechecking at evaluation time instead of build time
  • it has a test !

I have to say at this point that a lot of work of mine went into this, so either way so im a also biased. What I would like to add to this list is, that options are explicitly declared. My original goal here was to make it possible, that the description and documentation would be enough to be used for more simple cases, and that configuration would be relativ strait forward, even if you had not writen a config before.

On the other side I have to say, that I really like this service-like approach. If i am not mistaken, it is inspired a little bit by the way borgbackup is implemented.

So my idea would be, to integrate my options into your module. For me it was a project over the last months. Especially the modules took a lot of time.

I know that this is no criteria for using or not using anything, but it leads to me being willing to put some more effort into it. And I think, from my obviously also biased view, that it would result in a better module after all.

But I don't know how committed you are to this. If you would like to I could close this pr, and I could make branch where we combine our approaches.

I am looking forward to hearing from you.

Edit: gramar

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/btrfs-backup-tools-in-nixos/11364/8

@symphorien
Copy link
Member

What I would like to add to this list is, that options are explicitly declared. My original goal here was to make it possible, that the description and documentation would be enough to be used for more simple cases, and that configuration would be relativ strait forward, even if you had not writen a config before.

This is indeed valuable, but https://github.com/NixOS/rfcs/blob/master/rfcs/0042-config-option.md only suggests to do so for a handful of options, not all of them because it makes modules really large and hard to maintain. I think having a good example for the rfc-42 style settings option can go a long way.

I have to say at this point that a lot of work of mine went into this, so either way so im a also biased.

Maybe the best is to wait for a third opinion then ;)

@symphorien symphorien mentioned this pull request Jun 19, 2021
11 tasks
@bbigras
Copy link
Contributor

bbigras commented Aug 14, 2021

Closing since #127479 was merged.

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.

4 participants