nixos/home-assistant: reload the daemon when configuration changed

[andir]

Motivation for this change

Reload the service when configuration changes. This means that we don't
have a potentially slow startup for every small configuration change.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• 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/)
• 22.05 Release Notes (or backporting 21.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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[andir]

Mhm, this should probably depend on !configWritable && !lovelaceConfigWritable?

[aanderse]

Additionally, you should add a note somewhere reminding users of the security implications here - "Please restart this service whenever a security update is released" or some such.

[andir]

Actually that is already happening and I'm testing that in the VM test. As soon as the binary changes the service restarts anyway.

[aanderse]

What? With reloadIfChanged? I'm pretty sure it never used to... That is great.

ping @dasJ who is really deep into the switching script right now.
ping @mweinelt who maybe thought this wasn't the case?

[dasJ]

Until #49528 is properly fixed there is actually a workaround we are using downstream. It's ugly but works and it doesn't add security implications.
Just add a second service that reloads the main service and give the reload service the restart triggers. Then don't give the main service restartIfChanged=true

I have included parts of our exim module as an example:

exim module

{
  systemd.services.exim-reload = {
    description = "Exim config reload";
    wantedBy = [ "multi-user.target" ];
    stopIfChanged = false;

    restartTriggers = [ config.environment.etc."exim.conf".source ];

    script = ''
      # Ensure the unit is already active
      ${config.systemd.package}/bin/systemctl is-active exim.service >/dev/null || exit 0
      ${config.systemd.package}/bin/systemctl reload-or-restart exim.service
    '';

    sandbox = 2;
    serviceConfig = {
      Type = "oneshot";
      RemainAfterExit = true;
      User = "exim";
      Group = "exim";
      SystemCallFilter = "@system-service";
    };

    apparmor = {
      enable = true;
      extraConfig = ''
        /run/dbus/system_bus_socket rw,
      '';
    };
  };

  security.polkit.extraConfig = ''
    polkit.addRule(function(action, subject) {
      if (action.id == "org.freedesktop.systemd1.manage-units" &&
        action.lookup("unit") == "exim.service" &&
        action.lookup("verb") == "reload-or-restart" &&
        subject.user == "exim") {
          return polkit.Result.YES;
      }
    });
  '';
}

[andir]

@dasJ can you explain why this works as expected then? Did you have a look at the test code that I added to this PR?

[dasJ]

I looked at the code now. I have really no idea why that works. Are you 100% sure it gets reloaded once and restarted the second time and it's not hass restarting itself? Printing the switch-to-configuration.pl call should explain what it's asking systemd to do

[andir]

Unfortunately the reloadIfChanged feature isn't really documented well (or correct). My current working theory is that the restartIfChanged list is an additional list of restart triggers and the reloadIfChanged boolean switches to reloading instead of restarting when the list changes. Meaning that it actually does exactly what we want in this case. The naming of these options is very unfortunate but I think it is working as expected.

I'll see if I can get this deployed tonight and give it some real-world testing as well was writing a VM test for the restartIfChanged semantics.

[aanderse]

ping @flokli because of past conversations about how we thought if ExecStart ever changes the service should be restarted... we were both under the impression reloadIfChanged forces the service to never restart. Brief conversations with @mweinelt leave me with the impression that they also think this... 🤔

Thanks for offering to look into this @andir! And thanks for your feedback @dasJ!

[mweinelt]

Just found this PR and I really have to wonder how I missed it or forgot looking into it.

Having a proper reload mechanism would be awesome to have. I think I'll annoy Janne, who has been working on this topic quite recently.

[dasJ]

Oh no

[mweinelt]

Rebased and did a few cosmetical changes. The use of specializations for the test scenario is neat. Thanks for providing these.

@ofborg test home-assistant