nixos/switch-to-configuration: Document and test socket-activated services

4 files changed, 169 insertions, 20 deletions

diff --git a/nixos/doc/manual/development/unit-handling.section.md b/nixos/doc/manual/development/unit-handling.section.md
index d477f2c860f..bd4fe9e670f 100644
--- a/nixos/doc/manual/development/unit-handling.section.md
+++ b/nixos/doc/manual/development/unit-handling.section.md
@@ -41,17 +41,18 @@ checks:
     `RefuseManualStop` in the `[Unit]` section, and `X-OnlyManualStart` in the
     `[Unit]` section.
 
-  - The rest of the behavior is decided whether the unit has `X-StopIfChanged`
-    in the `[Service]` section set (exposed via
+  - Further behavior depends on the unit having `X-StopIfChanged` in the
+    `[Service]` section set to `true` (exposed via
     [systemd.services.\<name\>.stopIfChanged](#opt-systemd.services)). This is
     set to `true` by default and must be explicitly turned off if not wanted.
     If the flag is enabled, the unit is **stop**ped and then **start**ed. If
     not, the unit is **restart**ed. The goal of the flag is to make sure that
     the new unit never runs in the old environment which is still in place
-    before the activation script is run.
+    before the activation script is run. This behavior is different when the
+    service is socket-activated, as outlined in the following steps.
 
   - The last thing that is taken into account is whether the unit is a service
-    and socket-activated. Due to a bug, this is currently only done when
-    `X-StopIfChanged` is set. If the unit is socket-activated, the socket is
-    stopped and started, and the service is stopped and to be started by socket
-    activation.
+    and socket-activated. If `X-StopIfChanged` is **not** set, the service
+    is **restart**ed with the others. If it is set, both the service and the
+    socket are **stop**ped and the socket is **start**ed, leaving socket
+    activation to start the service when it's needed.
diff --git a/nixos/doc/manual/from_md/development/unit-handling.section.xml b/nixos/doc/manual/from_md/development/unit-handling.section.xml
index a6a654042f6..57c4754c001 100644
--- a/nixos/doc/manual/from_md/development/unit-handling.section.xml
+++ b/nixos/doc/manual/from_md/development/unit-handling.section.xml
@@ -88,9 +88,10 @@
         </listitem>
         <listitem>
           <para>
-            The rest of the behavior is decided whether the unit has
+            Further behavior depends on the unit having
             <literal>X-StopIfChanged</literal> in the
-            <literal>[Service]</literal> section set (exposed via
+            <literal>[Service]</literal> section set to
+            <literal>true</literal> (exposed via
             <link linkend="opt-systemd.services">systemd.services.&lt;name&gt;.stopIfChanged</link>).
             This is set to <literal>true</literal> by default and must
             be explicitly turned off if not wanted. If the flag is
@@ -100,17 +101,22 @@
             is <emphasis role="strong">restart</emphasis>ed. The goal of
             the flag is to make sure that the new unit never runs in the
             old environment which is still in place before the
-            activation script is run.
+            activation script is run. This behavior is different when
+            the service is socket-activated, as outlined in the
+            following steps.
           </para>
         </listitem>
         <listitem>
           <para>
             The last thing that is taken into account is whether the
-            unit is a service and socket-activated. Due to a bug, this
-            is currently only done when
-            <literal>X-StopIfChanged</literal> is set. If the unit is
-            socket-activated, the socket is stopped and started, and the
-            service is stopped and to be started by socket activation.
+            unit is a service and socket-activated. If
+            <literal>X-StopIfChanged</literal> is
+            <emphasis role="strong">not</emphasis> set, the service is
+            <emphasis role="strong">restart</emphasis>ed with the
+            others. If it is set, both the service and the socket are
+            <emphasis role="strong">stop</emphasis>ped and the socket is
+            <emphasis role="strong">start</emphasis>ed, leaving socket
+            activation to start the service when it’s needed.
           </para>
         </listitem>
       </itemizedlist>
diff --git a/nixos/modules/system/activation/switch-to-configuration.pl b/nixos/modules/system/activation/switch-to-configuration.pl
index a8fe14c58f0..3a5ffe822ed 100644
--- a/nixos/modules/system/activation/switch-to-configuration.pl
+++ b/nixos/modules/system/activation/switch-to-configuration.pl
@@ -307,6 +307,7 @@ sub handleModifiedUnit {
         # seem to get applied on daemon-reload.
     } elsif ($unit =~ /\.mount$/) {
         # Reload the changed mount unit to force a remount.
+        # FIXME: only reload when Options= changed, restart otherwise
         $unitsToReload->{$unit} = 1;
         recordUnit($reloadListFile, $unit);
     } elsif ($unit =~ /\.socket$/) {
@@ -339,7 +340,7 @@ sub handleModifiedUnit {
                 # If this unit is socket-activated, then stop the
                 # socket unit(s) as well, and restart the
                 # socket(s) instead of the service.
-                my $socketActivated = 0;
+                my $socket_activated = 0;
                 if ($unit =~ /\.service$/) {
                     my @sockets = split(/ /, join(" ", @{$unitInfo{Service}{Sockets} // []}));
                     if (scalar @sockets == 0) {
@@ -347,13 +348,15 @@ sub handleModifiedUnit {
                     }
                     foreach my $socket (@sockets) {
                         if (defined $activePrev->{$socket}) {
+                            # We can now be sure this is a socket-activate unit
+
                             $unitsToStop->{$socket} = 1;
                             # Only restart sockets that actually
                             # exist in new configuration:
                             if (-e "$out/etc/systemd/system/$socket") {
                                 $unitsToStart->{$socket} = 1;
                                 recordUnit($startListFile, $socket);
-                                $socketActivated = 1;
+                                $socket_activated = 1;
                             }
                             # Remove from units to reload so we don't restart and reload
                             if ($unitsToReload->{$unit}) {
@@ -368,7 +371,7 @@ sub handleModifiedUnit {
                 # that this unit needs to be started below.
                 # We write this to a file to ensure that the
                 # service gets restarted if we're interrupted.
-                if (!$socketActivated) {
+                if (!$socket_activated) {
                     $unitsToStart->{$unit} = 1;
                     recordUnit($startListFile, $unit);
                 }
diff --git a/nixos/tests/switch-test.nix b/nixos/tests/switch-test.nix
index b429babce83..090bbe298a4 100644
--- a/nixos/tests/switch-test.nix
+++ b/nixos/tests/switch-test.nix
@@ -1,6 +1,46 @@
 # Test configuration switching.
 
-import ./make-test-python.nix ({ pkgs, ...} : {
+import ./make-test-python.nix ({ pkgs, ...} : let
+
+  # Simple service that can either be socket-activated or that will
+  # listen on port 1234 if not socket-activated.
+  # A connection to the socket causes 'hello' to be written to the client.
+  socketTest = pkgs.writeScript "socket-test.py" /* python */ ''
+    #!${pkgs.python3}/bin/python3
+
+    from socketserver import TCPServer, StreamRequestHandler
+    import socket
+    import os
+
+
+    class Handler(StreamRequestHandler):
+        def handle(self):
+            self.wfile.write("hello".encode("utf-8"))
+
+
+    class Server(TCPServer):
+        def __init__(self, server_address, handler_cls):
+            listenFds = os.getenv('LISTEN_FDS')
+            if listenFds is None or int(listenFds) < 1:
+                print(f'Binding to {server_address}')
+                TCPServer.__init__(
+                        self, server_address, handler_cls, bind_and_activate=True)
+            else:
+                TCPServer.__init__(
+                        self, server_address, handler_cls, bind_and_activate=False)
+                # Override socket
+                print(f'Got activated by {os.getenv("LISTEN_FDNAMES")} '
+                      f'with {listenFds} FDs')
+                self.socket = socket.fromfd(3, self.address_family,
+                                            self.socket_type)
+
+
+    if __name__ == "__main__":
+        server = Server(("localhost", 1234), Handler)
+        server.serve_forever()
+  '';
+
+in {
   name = "switch-test";
   meta = with pkgs.lib.maintainers; {
     maintainers = [ gleber das_j ];
@@ -8,6 +48,7 @@ import ./make-test-python.nix ({ pkgs, ...} : {
 
   nodes = {
     machine = { pkgs, lib, ... }: {
+      environment.systemPackages = [ pkgs.socat ]; # for the socket activation stuff
       users.mutableUsers = false;
 
       specialisation = rec {
@@ -231,6 +272,40 @@ import ./make-test-python.nix ({ pkgs, ...} : {
           systemd.services.reload-triggers-and-restart.serviceConfig.X-Modified = "test";
         };
 
+        simple-socket.configuration = {
+          systemd.services.socket-activated = {
+            description = "A socket-activated service";
+            stopIfChanged = lib.mkDefault false;
+            serviceConfig = {
+              ExecStart = socketTest;
+              ExecReload = "${pkgs.coreutils}/bin/true";
+            };
+          };
+          systemd.sockets.socket-activated = {
+            wantedBy = [ "sockets.target" ];
+            listenStreams = [ "/run/test.sock" ];
+            socketConfig.SocketMode = lib.mkDefault "0777";
+          };
+        };
+
+        simple-socket-service-modified.configuration = {
+          imports = [ simple-socket.configuration ];
+          systemd.services.socket-activated.serviceConfig.X-Test = "test";
+        };
+
+        simple-socket-stop-if-changed.configuration = {
+          imports = [ simple-socket.configuration ];
+          systemd.services.socket-activated.stopIfChanged = true;
+        };
+
+        simple-socket-stop-if-changed-and-reloadtrigger.configuration = {
+          imports = [ simple-socket.configuration ];
+          systemd.services.socket-activated = {
+            stopIfChanged = true;
+            reloadTriggers = [ "test" ];
+          };
+        };
+
         mount.configuration = {
           systemd.mounts = [
             {
@@ -676,7 +751,71 @@ import ./make-test-python.nix ({ pkgs, ...} : {
         assert_contains(out, "would reload the following units: reload-triggers.service, simple-reload-service.service\n")
         assert_contains(out, "would restart the following units: reload-triggers-and-restart-by-as.service, reload-triggers-and-restart.service, simple-restart-service.service, simple-service.service\n")
         assert_lacks(out, "\nwould start the following units:")
-        assert_lacks(out, "as well:")
+
+    with subtest("socket-activated services"):
+        # Socket-activated services don't get started, just the socket
+        machine.fail("[ -S /run/test.sock ]")
+        out = switch_to_specialisation("${machine}", "simple-socket")
+        # assert_lacks(out, "stopping the following units:") nobody cares
+        assert_lacks(out, "NOT restarting the following changed units:")
+        assert_lacks(out, "reloading the following units:")
+        assert_lacks(out, "\nrestarting the following units:")
+        assert_lacks(out, "\nstarting the following units:")
+        assert_contains(out, "the following new units were started: socket-activated.socket\n")
+        machine.succeed("[ -S /run/test.sock ]")
+
+        # Changing a non-activated service does nothing
+        out = switch_to_specialisation("${machine}", "simple-socket-service-modified")
+        assert_lacks(out, "stopping the following units:")
+        assert_lacks(out, "NOT restarting the following changed units:")
+        assert_lacks(out, "reloading the following units:")
+        assert_lacks(out, "\nrestarting the following units:")
+        assert_lacks(out, "\nstarting the following units:")
+        assert_lacks(out, "the following new units were started:")
+        machine.succeed("[ -S /run/test.sock ]")
+        # The unit is properly activated when the socket is accessed
+        if machine.succeed("socat - UNIX-CONNECT:/run/test.sock") != "hello":
+            raise Exception("Socket was not properly activated")  # idk how that would happen tbh
+
+        # Changing an activated service with stopIfChanged=false restarts the service
+        out = switch_to_specialisation("${machine}", "simple-socket")
+        assert_lacks(out, "stopping the following units:")
+        assert_lacks(out, "NOT restarting the following changed units:")
+        assert_lacks(out, "reloading the following units:")
+        assert_contains(out, "\nrestarting the following units: socket-activated.service\n")
+        assert_lacks(out, "\nstarting the following units:")
+        assert_lacks(out, "the following new units were started:")
+        machine.succeed("[ -S /run/test.sock ]")
+        # Socket-activation of the unit still works
+        if machine.succeed("socat - UNIX-CONNECT:/run/test.sock") != "hello":
+            raise Exception("Socket was not properly activated after the service was restarted")
+
+        # Changing an activated service with stopIfChanged=true stops the service and
+        # socket and starts the socket
+        out = switch_to_specialisation("${machine}", "simple-socket-stop-if-changed")
+        assert_contains(out, "stopping the following units: socket-activated.service, socket-activated.socket\n")
+        assert_lacks(out, "NOT restarting the following changed units:")
+        assert_lacks(out, "reloading the following units:")
+        assert_lacks(out, "\nrestarting the following units:")
+        assert_contains(out, "\nstarting the following units: socket-activated.socket\n")
+        assert_lacks(out, "the following new units were started:")
+        machine.succeed("[ -S /run/test.sock ]")
+        # Socket-activation of the unit still works
+        if machine.succeed("socat - UNIX-CONNECT:/run/test.sock") != "hello":
+            raise Exception("Socket was not properly activated after the service was restarted")
+
+        # Changing a reload trigger of a socket-activated unit only reloads it
+        out = switch_to_specialisation("${machine}", "simple-socket-stop-if-changed-and-reloadtrigger")
+        assert_lacks(out, "stopping the following units:")
+        assert_lacks(out, "NOT restarting the following changed units:")
+        assert_contains(out, "reloading the following units: socket-activated.service\n")
+        assert_lacks(out, "\nrestarting the following units:")
+        assert_lacks(out, "\nstarting the following units: socket-activated.socket")
+        assert_lacks(out, "the following new units were started:")
+        machine.succeed("[ -S /run/test.sock ]")
+        # Socket-activation of the unit still works
+        if machine.succeed("socat - UNIX-CONNECT:/run/test.sock") != "hello":
+            raise Exception("Socket was not properly activated after the service was restarted")
 
     with subtest("mounts"):
         switch_to_specialisation("${machine}", "mount")