-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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/borgbackup: Document wrapper script and show excludes file path #107938
Conversation
Hi @dotlambda, @yorickvP, @ngiger, would you please be able to review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking so long to reply.
I must say I'm not a big fan of the wrapper script and wouldn't have merged the PR adding it.
@@ -112,7 +116,7 @@ let | |||
mkBorgWrapper = name: cfg: mkWrapperDrv { | |||
original = "${pkgs.borgbackup}/bin/borg"; | |||
name = "borg-job-${name}"; | |||
set = { BORG_REPO = cfg.repo; } // (mkPassEnv cfg) // cfg.environment; | |||
set = mkBorgEnvironment cfg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't always make sense: What if the user that runs the wrapper doesn't have permission to execute passCommand
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question applies to BORG_REPO
/BORG_RSH
- what if the user that runs the wrapper can't access the ssh key?
The wrapper script isn't meant to work when run by any user - that's not its purpose.
Users would have to run it as config.services.borgbackup.jobs.<name>.user
, which is fine.
Thanks for the feedback @dotlambda. The wrapper script was a good addition because it provides a convenient way for users to test their backups. |
> nixos-rebuild switch | ||
<section xml:id="opt-services-backup-borgbackup-testing"> | ||
<title>Testing remote backups</title> | ||
<para>Every job has a corresponding wrapper script installed into <xref linkend="opt-environment.systemPackages"/>. Inspect the wrapper variables by reading <filename>/run/current-system/sw/bin/borg-job-<NAME></filename>. This script can be used to test backups or manage your remote repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<para>Every job has a corresponding wrapper script installed into <xref linkend="opt-environment.systemPackages"/>. Inspect the wrapper variables by reading <filename>/run/current-system/sw/bin/borg-job-<NAME></filename>. This script can be used to test backups or manage your remote repo. | |
<para>Every job has a corresponding wrapper script installed into <xref linkend="opt-environment.systemPackages"/>. | |
It sets the same environment variables as the systemd unit. | |
You can inspect them by reading <filename>/run/current-system/sw/bin/borg-job-<name></filename>. | |
This script can be used to test backups, manage the remote repo, and do restores. | |
It is only guaranteed to work if run as the same user as the systemd unit. |
<title>Testing remote backups</title> | ||
<para>Every job has a corresponding wrapper script installed into <xref linkend="opt-environment.systemPackages"/>. Inspect the wrapper variables by reading <filename>/run/current-system/sw/bin/borg-job-<NAME></filename>. This script can be used to test backups or manage your remote repo. | ||
</para> | ||
<para>The following few commands (run as root) show how to test your backup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<para>The following few commands (run as root) show how to test your backup. | |
<para>The following few commands show how to test your backup. |
A #
should be clear enough.
> systemctl restart borgbackup-job-backupToLocalServer | ||
> export BORG_PASSPHRASE=topSecrect | ||
> borg list --rsh='ssh -i /run/keys/id_ed25519_my_borg_repo' borg@nixos:. | ||
<prompt># </prompt> systemctl restart borgbackup-job-backupToLocalServer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<prompt># </prompt> systemctl restart borgbackup-job-backupToLocalServer | |
<prompt># </prompt> systemctl start borgbackup-job-backupToLocalServer |
Why restart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea - that's how it was before. systemctl start
would be fine.
Thanks for the review @dotlambda - I have applied your suggestions. |
inherit (cfg) startAt; | ||
}; | ||
|
||
mkBorgEnvironment = cfg: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mkBorgEnvironment = cfg: { | |
mkEnvironment = cfg: { |
But actually, I don't know why I chose to let those functions have an argument cfg
. That can just be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK fixed.
cfg
is not actually the service module config, but the per-repo config. So it needs to be a variable one way or another.
@yorickvP didn't we merge the restore unit and env file we generate off our borg stuff? |
Yeah, this PR documents the generated wrappers. |
Please revert it if you don't want it here. I stopped caring about contributing to nixpkgs a long time ago. In case you wonder why, well, dude took time out of his day to document a thing and contribute said documentation for everyone else here, and your response is "wouldn't have merged this in the first place". What an attitude to have. |
I just mentioned my opinion, asked a valid question at the same time, and gave (hopefully) valuable feedback later. It's normal that people criticise but -- that's how I know Nixpkgs -- a more or less democratic compromise is found eventually. This is how every open source project should work and we would appreciate your input just as much as anyone else's. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as this PR is concerned, I think we can all appreciate things getting better documentation. Looks good to me.
I have no intention to do so, since multiple people seem to find this wrapper script useful.
I'm sure it would make your life easier if you did upstream at least some of them. And it makes the reviewers' lives easier if you do so in little bits. As I bet other people would profit from these changes, I would love to see you change your mind. If you do, make sure to ping me in those PRs and I'll be happy to drop a review! |
Worth making clear that I don't speak for Yorick, given that I've probably upset a few people. |
Ready to merge - @Ma27 - thanks. |
@rvl WOuld you be okay with removing the |
No @dotlambda, that's not reasonable - I'm not going to ask upstream to start making environment variables for every CLI option. |
Okay, fine. But at least squash the last two commits into one. |
For what it's worth, I don't think having environment variables for every option is unreasonable. Perhaps upstream has strong opinions on the subject (and that's their prerogative), but it's not a fundamentally wrong course of action, especially for things meant to run automatically on a server. Environment for configuration is one of the tenets of 12-factor apps, after all. |
OK thanks @dotlambda - rebased and squashed. It's not unreasonable to have environment variables for every CLI option, and sometimes it's quite helpful. But most of the software we package does not work like this. Borgbackup has a handful of settings which can be controlled by env var - generally things that apply to every |
This makes it simpler to get the path of the generated excludes file. Just cat the wrapper script.
Co-authored-by: Robert Schütz <rschuetz17@gmail.com>
003d298
to
5f2f9e2
Compare
Hello @dotlambda or @SuperSandro2000, I just rebased this on the latest Also in related borgbackup news, I have recently been using the |
This PR was recently brought to my attention again, and I would like to help to get it merged. If @dotlambda doesn't have the time, I can poke a few others and see if they would like to help.
That's a good idea, but perhaps you should put that in a separate PR (please ping me). |
Since I have zero knowledge about borgbackup someone else needs to verify the changes. |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-32/19865/1 |
Motivation for this change
I wrote my own
borg
wrapper scripts before realising that they are provided by the NixOS module.This adds documentation of the wrapper scripts.
It also adds the path to the excludes file as a variable, because that is useful to know when testing filters.
Things done
nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)