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/systemd): Add Persistent for services. #123158

Closed
wants to merge 1 commit into from
Closed

Conversation

blargg
Copy link
Contributor

@blargg blargg commented May 15, 2021

Persistent will check on startup if a timer trigger was missed while the
timer was inactive and trigger the timer once if it was. This is useful
for computers to catch up on missed automated tasks from while they were
shut down.

Motivation for this change

This will enable us to provide the persistent option for system.autoUpgrade or services.borgbackup.jobs. That way, if you have a daily update or backup that is sometimes missed because your personal computer is off, it will catch up the next time you turn the computer on.

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 (n/a, nixos change)
    • other Linux distributions (n/a)
  • 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/). (I manually checked that the timer would work correctly for a service)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date (I added docs for the new options, not sure if that is everything).
  • Fits CONTRIBUTING.md.

Persistent will check on startup if a timer trigger was missed while the
timer was inactive and trigger the timer once if it was.  This is useful
for computers to catch up on missed automated tasks from while they were
shut down.
@blargg
Copy link
Contributor Author

blargg commented May 15, 2021

Some thoughts on this PR. I went with the current pattern of wrapping the option on the service configuration.

I think it could also make sense to let you fully specify systemd.timers..timerConfig from the service, but this conflicts with the current pattern, and I would need more guidance on how to manage that.

@aanderse
Copy link
Member

I'm not opposed to this change, but I'm not sure I see the value. I would have assumed this value is intended to be directly used by sysadmins for their own systemd units (presumably for discoverability, let people know this option exists, because it is a common problem to solve), but you explicitly mention other NixOS services utilizing this. Since other NixOS services often write their own systemd units I think it makes sense that NixOS developers likely know this systemd option exists already and there isn't much value.

Just my thoughts, at least.

cc @NixOS/systemd

@blargg
Copy link
Contributor Author

blargg commented May 16, 2021

My end goal is just to enable Persistent for services.borgbackup.jobs, so whichever way is best to get that done I'm good with.

Looking through a number of nixos services, they already configure both their systemd services through systemd.services and timer through systemd.services.<name>.startAt.

Some examples of startAt:

For these modules, adding this option gives them an easy way to make use of persistent without rewriting their module.

In addition, it still takes a bit of dev work to take a nixos option, convert that into a string formatted for a systemd service or timer, then figure out what file to write that to so systemd picks it up. By using these options instead of directly writing the files, we can share that work between developers.

But at the same time, I'm not an expert on nixos, so I'll go along with whatever people think is best.

@flokli
Copy link
Contributor

flokli commented May 16, 2021

systemd.services.<name>.startAt is self-explanatory enough so people understand it creates a timer for the service unit.

systemd.services.<name>.persistent feels very unintuitive to me - it's configuring the generated timer, available in systemd.timers.<name>.

I'd feel more comfortable if the borgbackup/… modules could just access systemd.timers.<name> directly (in addition to the startAt if they want to configure the generated timer in more detail.

In fact, that's what nix-gc is already doing today:

    systemd.services.nix-gc = {
      description = "Nix Garbage Collector";
      script = "exec ${config.nix.package.out}/bin/nix-collect-garbage ${cfg.options}";
      startAt = optional cfg.automatic cfg.dates;
    };

    systemd.timers.nix-gc = lib.mkIf cfg.automatic {
      timerConfig = {
        RandomizedDelaySec = cfg.randomizedDelaySec;
        Persistent = cfg.persistent;
      };
    };

@blargg
Copy link
Contributor Author

blargg commented May 16, 2021

Ah thanks, I didn't find any examples of this, I should have been looking through the unstable options instead.

Yeah, I like the idea of directly setting the service and the timer in the module. Seems like an better way to make all of the timer functionality available to a service.

I'm going to close this PR and work on implementing that instead.

@blargg blargg closed this May 16, 2021
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

3 participants