nixos/borgbackup: Document wrapper script and show excludes file path #107938

rvl · 2020-12-30T04:36:33Z

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

Built on platform(s)
- NixOS
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.

rvl · 2021-05-12T23:50:02Z

Hi @dotlambda, @yorickvP, @ngiger, would you please be able to review this?

dotlambda

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.

dotlambda · 2021-05-14T16:47:48Z

nixos/modules/services/backup/borgbackup.nix

@@ -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;


This doesn't always make sense: What if the user that runs the wrapper doesn't have permission to execute passCommand?

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.

rvl · 2021-05-17T04:31:36Z

Thanks for the feedback @dotlambda.

The wrapper script was a good addition because it provides a convenient way for users to test their backups.
As you know, a backup can't be trusted unless it's regularly tested.
If there is a better way of testing backups made by the borgbackup module, please let us know and we can document it.

dotlambda · 2021-05-17T09:08:52Z

nixos/modules/services/backup/borgbackup.xml

-> 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-&lt;NAME&gt;</filename>. This script can be used to test backups or manage your remote repo.


Suggested change

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

dotlambda · 2021-05-17T09:09:20Z

nixos/modules/services/backup/borgbackup.xml

+   <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-&lt;NAME&gt;</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.


Suggested change

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

dotlambda · 2021-05-17T09:09:44Z

nixos/modules/services/backup/borgbackup.xml

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


Suggested change

<prompt># </prompt> systemctl restart borgbackup-job-backupToLocalServer

<prompt># </prompt> systemctl start borgbackup-job-backupToLocalServer

Why restart?

No idea - that's how it was before. systemctl start would be fine.

rvl · 2021-05-18T00:04:34Z

Thanks for the review @dotlambda - I have applied your suggestions.

dotlambda · 2021-05-18T08:10:28Z

nixos/modules/services/backup/borgbackup.nix

      inherit (cfg) startAt;
    };

+  mkBorgEnvironment = cfg: {


Suggested change

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.

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.

mkaito · 2021-05-18T15:19:00Z

@yorickvP didn't we merge the restore unit and env file we generate off our borg stuff?

yorickvP · 2021-05-18T16:31:33Z

Yeah, this PR documents the generated wrappers.

mkaito · 2021-05-18T18:26:56Z

I must say I'm not a big fan of the wrapper script and wouldn't have merged the PR adding it.

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.

dotlambda · 2021-05-18T19:13:33Z

I must say I'm not a big fan of the wrapper script and wouldn't have merged the PR adding it.

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.

mkaito

As far as this PR is concerned, I think we can all appreciate things getting better documentation. Looks good to me.

dotlambda · 2021-05-18T19:35:26Z

And again, feel free to file a PR reverting everything about this script if you don't like it.

I have no intention to do so, since multiple people seem to find this wrapper script useful.

The only reason it's even here is that we had some misguided and deluded attempt to upstream all our nixpkgs modifications some time ago. Don't worry, it won't happen again.

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!

rvl · 2021-05-19T02:35:35Z

@mkaito @yorickvP Your efforts are appreciated by me, at least.

mkaito · 2021-05-19T02:58:29Z

Worth making clear that I don't speak for Yorick, given that I've probably upset a few people.

rvl · 2021-06-01T03:58:42Z

Ready to merge - @Ma27 - thanks.

dotlambda · 2021-06-13T17:46:21Z

@rvl WOuld you be okay with removing the $excludeFile hack? I think we should first add such an environment variable upstream.

rvl · 2021-06-14T06:03:24Z

No @dotlambda, that's not reasonable - I'm not going to ask upstream to start making environment variables for every CLI option.
We can and should fix it in our NixOS service scripts.

dotlambda · 2021-06-14T12:59:28Z

Okay, fine. But at least squash the last two commits into one.

mkaito · 2021-06-14T13:49:26Z

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.

rvl · 2021-06-15T02:27:43Z

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 borg subcommand. Our purpose is to package the software and make it easy to configure and use, not to fix every upstream package.

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>

rvl · 2021-12-01T04:49:29Z

Hello @dotlambda or @SuperSandro2000, I just rebased this on the latest nixos-unstable. Could we please merge it? 🙏

Also in related borgbackup news, I have recently been using the borgbackup create --patterns-from=patterns.lst option, which seems like a much more sane way of specifying files to backup. I could add an option to the nixos module for this - if someone is willing to merge it.

mkaito · 2022-06-10T12:02:54Z

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.

I have recently been using the borgbackup create --patterns-from=patterns.lst option

That's a good idea, but perhaps you should put that in a separate PR (please ping me).

SuperSandro2000 · 2022-06-11T12:39:50Z

Since I have zero knowledge about borgbackup someone else needs to verify the changes.

nixos-discourse · 2022-06-20T13:21:10Z

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

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Dec 30, 2020

rvl requested a review from dotlambda January 5, 2021 01:54

dotlambda reviewed May 14, 2021

View reviewed changes

dotlambda reviewed May 17, 2021

View reviewed changes

ofborg bot removed the 10.rebuild-linux: 1 label May 18, 2021

rvl force-pushed the borgbackup-wrapper branch from c20feb7 to 88cb027 Compare May 18, 2021 00:04

dotlambda reviewed May 18, 2021

View reviewed changes

dotlambda requested review from mweinelt and yorickvP May 18, 2021 08:26

yorickvP requested a review from mkaito May 18, 2021 12:00

mkaito approved these changes May 18, 2021

View reviewed changes

rvl force-pushed the borgbackup-wrapper branch from 88418e9 to 003d298 Compare June 15, 2021 02:19

rvl and others added 2 commits December 1, 2021 12:37

nixos/borgbackup: Put excludes file path in job wrapper script

f2d7a78

This makes it simpler to get the path of the generated excludes file. Just cat the wrapper script.

nixos/borgbackup: Document the job wrapper script

5f2f9e2

Co-authored-by: Robert Schütz <rschuetz17@gmail.com>

rvl force-pushed the borgbackup-wrapper branch from 003d298 to 5f2f9e2 Compare December 1, 2021 04:41

SuperSandro2000 added the 2.status: merge conflict label Jun 20, 2022

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 7, 2023

rvl closed this Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/borgbackup: Document wrapper script and show excludes file path #107938

nixos/borgbackup: Document wrapper script and show excludes file path #107938

rvl commented Dec 30, 2020

rvl commented May 12, 2021

dotlambda left a comment •

edited

dotlambda May 14, 2021

rvl May 17, 2021

rvl commented May 17, 2021

dotlambda May 17, 2021

dotlambda May 17, 2021

dotlambda May 17, 2021

rvl May 18, 2021

rvl commented May 18, 2021

dotlambda May 18, 2021

rvl May 19, 2021

mkaito commented May 18, 2021

yorickvP commented May 18, 2021

mkaito commented May 18, 2021 •

edited

dotlambda commented May 18, 2021

mkaito left a comment •

edited

dotlambda commented May 18, 2021

rvl commented May 19, 2021

mkaito commented May 19, 2021

rvl commented Jun 1, 2021

dotlambda commented Jun 13, 2021

rvl commented Jun 14, 2021

dotlambda commented Jun 14, 2021

mkaito commented Jun 14, 2021 •

edited

rvl commented Jun 15, 2021

rvl commented Dec 1, 2021

mkaito commented Jun 10, 2022

SuperSandro2000 commented Jun 11, 2022

nixos-discourse commented Jun 20, 2022

-   <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-&lt;NAME&gt;</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-&lt;name&gt;</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.

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

	<prompt># </prompt> systemctl restart borgbackup-job-backupToLocalServer
	<prompt># </prompt> systemctl start borgbackup-job-backupToLocalServer

nixos/borgbackup: Document wrapper script and show excludes file path #107938

nixos/borgbackup: Document wrapper script and show excludes file path #107938

Conversation

rvl commented Dec 30, 2020

Motivation for this change

Things done

rvl commented May 12, 2021

dotlambda left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl commented May 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkaito commented May 18, 2021

yorickvP commented May 18, 2021

mkaito commented May 18, 2021 • edited

dotlambda commented May 18, 2021

mkaito left a comment • edited

Choose a reason for hiding this comment

dotlambda commented May 18, 2021

rvl commented May 19, 2021

mkaito commented May 19, 2021

rvl commented Jun 1, 2021

dotlambda commented Jun 13, 2021

rvl commented Jun 14, 2021

dotlambda commented Jun 14, 2021

mkaito commented Jun 14, 2021 • edited

rvl commented Jun 15, 2021

rvl commented Dec 1, 2021

mkaito commented Jun 10, 2022

SuperSandro2000 commented Jun 11, 2022

nixos-discourse commented Jun 20, 2022

dotlambda left a comment •

edited

mkaito commented May 18, 2021 •

edited

mkaito left a comment •

edited

mkaito commented Jun 14, 2021 •

edited