nixos/restic: add option to inhibit going to sleep

[cheriimoya]

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[lolbinarycat]

this documents the option more clearly than the option's actual documentation, although false = don't inhibit should probably be obvious from the option name, so i its probably fine as-is.

[cheriimoya]

What do you think about the updated option description?

[MinerSebas]

Why split the string beforhand?

Suggested change

	inhibitCmd = concatStringsSep " " [
	"${pkgs.systemd}/bin/systemd-inhibit"
	"--mode='block'"
	"--who='restic'"
	"--what='sleep'"
	"--why='Scheduled backup ${name}' "
	];
	inhibitCmd = "${pkgs.systemd}/bin/systemd-inhibit --mode='block' --who='restic' --what='sleep' --why='Scheduled backup ${name}' ";

[cheriimoya]

I split it up for readability, no other reason. I could change it, but the next reviewer would probably tell me to split it up as it is too long:D

[networkException]

Looks good to me, needs another rebase though

[emilylange]

@ofborg test restic

[emilylange]

The newly added VM test section for this seems to fail, but the VM test itself reports as successful anyway.
Can you look into this?

server # [   68.052903] systemd[1]: Starting restic-backups-inhibit-test.service...
server: waiting for success: systemd-inhibit --no-legend --no-pager | grep -q restic
(finished: waiting for success: systemd-inhibit --no-legend --no-pager | grep -q restic, in 0.08 seconds)
(finished: run the VM test script, in 68.89 seconds)
server # [   68.177458] restic-backups-inhibit-test-pre-start[1788]: Fatal: unable to open config file: stat /root/restic-backup-inhibit-test/config: no such file or directory
server # [   68.180268] restic-backups-inhibit-test-pre-start[1788]: Is there a repository at the following location?
server # [   68.181806] restic-backups-inhibit-test-pre-start[1788]: /root/restic-backup-inhibit-test
server # [   68.186114] restic-backups-inhibit-test-pre-start[1785]: /nix/store/3jwc6w0dbsfwcnl4d91ssq77x8phyihd-restic-0.16.5/bin/restic failed with exit status 1.
test script finished in 68.96s

See https://logs.ofborg.org/?key=nixos/nixpkgs.296691&attempt_id=d81aedde-c33a-40c0-9060-b638f8a1048a

Also, what is the benefit of wrapping the command with systemd-inhitbit instead of preventing sleep like this:

diff --git a/nixos/modules/services/backup/restic.nix b/nixos/modules/services/backup/restic.nix
index 7b10d7d38d20..903eb9ceeafc 100644
--- a/nixos/modules/services/backup/restic.nix
+++ b/nixos/modules/services/backup/restic.nix
@@ -308,14 +308,7 @@ in
         (name: backup:
           let
             extraOptions = concatMapStrings (arg: " -o ${arg}") backup.extraOptions;
-            inhibitCmd = concatStringsSep " " [
-              "${pkgs.systemd}/bin/systemd-inhibit"
-              "--mode='block'"
-              "--who='restic'"
-              "--what='sleep'"
-              "--why=${escapeShellArg "Scheduled backup ${name}"} "
-            ];
-            resticCmd = "${optionalString backup.inhibitsSleep inhibitCmd}${backup.package}/bin/restic${extraOptions}";
+            resticCmd = "${backup.package}/bin/restic${extraOptions}";
             excludeFlags = optional (backup.exclude != []) "--exclude-file=${pkgs.writeText "exclude-patterns" (concatStringsSep "\n" backup.exclude)}";
             filesFromTmpFile = "/run/restic-backups-${name}/includes";
             doBackup = (backup.dynamicFilesFrom != null) || (backup.paths != null && backup.paths != []);
@@ -353,6 +346,18 @@ in
             restartIfChanged = false;
             wants = [ "network-online.target" ];
             after = [ "network-online.target" ];
+            before = optionals backup.inhibitsSleep [
+              "suspend.target"
+              "hibernate.target"
+              "hybrid-sleep.target"
+              "suspend-then-hibernate.target"
+            ];
+            wantedBy = optionals backup.inhibitsSleep [
+              "suspend.target"
+              "hibernate.target"
+              "hybrid-sleep.target"
+              "suspend-then-hibernate.target"
+            ];
             serviceConfig = {
               Type = "oneshot";
               ExecStart = (optionals doBackup [ "${resticCmd} backup ${concatStringsSep " " (backup.extraBackupArgs ++ excludeFlags)} --files-from=${filesFromTmpFile}" ])

I would assume it does not show up in systemd-inhibit that way, but is that really of that much use?

[cheriimoya]

Also, what is the benefit of wrapping the command with systemd-inhitbit instead of preventing sleep like this:
...
I would assume it does not show up in systemd-inhibit that way, but is that really of that much use?

Another benefit is, that if I execute systemctl suspend, systemd tells me that there is an inhibitor instead of not giving any output and seemingly hanging.
What is an advantage of the before/wantedBy relation over the wrapped version?

Thanks for pointing out the issue with the test, I was only seeing the succeeded test tbh.
Will look into that.

[emilylange]

What is an advantage of the before/wantedBy relation over the wrapped version?

The current wrapped version is silently failing the test, so I wanted to provide another approach that I know works.

Another benefit is, that if I execute systemctl suspend, systemd tells me that there is an inhibitor instead of not giving any output and seemingly hanging.

Hmm I see. That's a solid reason. Keep your systemd-inhibit approach then. Sounds good.

Feel free to undraft and ping when you fixed the VM test :)

[cheriimoya]

Okay, after some debugging I found the cause of the exit 1 output...
It's the output of systemd-inhibit.
The initialize option does a resticCmd snapshot || resticCmd init and as the first command fails if the repo is not initialized, the systemd-inhibit version fails verbosely than the version without.
I could leave it as is or alter resticCmd snapshot || resticCmd init to resticCmd snapshot ${lib.optionalString backup.inhibit "2> /dev/null"} || resticCmd init.
What do you think?

$ RESTIC_REPOSITORY=/none/restic-backup-inhibit-test systemd-inhibit /nix/store/0lvsqlp1n5c0mckcglfbxih43x0amysi-restic-0.16.4/bin/restic snapshots                                                       [9:43:52]
Fatal: unable to open config file: stat /none/restic-backup-inhibit-test/config: no such file or directory
Is there a repository at the following location?
/none/restic-backup-inhibit-test
/nix/store/0lvsqlp1n5c0mckcglfbxih43x0amysi-restic-0.16.4/bin/restic failed with exit status 1.
[1]    2674739 exit 1     RESTIC_REPOSITORY=/none/restic-backup-inhibit-test systemd-inhibit  snapshots
$ RESTIC_REPOSITORY=/none/restic-backup-inhibit-test /nix/store/0lvsqlp1n5c0mckcglfbxih43x0amysi-restic-0.16.4/bin/restic snapshots                                                                       [9:43:54]
Fatal: unable to open config file: stat /none/restic-backup-inhibit-test/config: no such file or directory
Is there a repository at the following location?
/none/restic-backup-inhibit-test
[1]    2674760 exit 1     RESTIC_REPOSITORY=/none/restic-backup-inhibit-test  snapshots

[emilylange]

Ah I see, this is a misunderstanding then.

The error line comes from resticCmd snapshot (which is intended) and just happens to be the last line the VM test prints.

Makes sense to me now!

[emilylange]

Wondering if a 5 second timeout is too low for hydra.nixos.org.

I suppose we will see how it goes and can create a follow-up PR if needed :)

[emilylange]

@ofborg test restic