Merge pull request #149837 from helsinki-systems/feat/redo-activation-script-restarting

nixos/switch-to-configuration: Add a massive test and do a slight refactor

nixos/modules/system/activation/switch-to-configuration.pl

@@ -146,6 +146,79 @@ sub fingerprintUnit {
     return abs_path($s) . (-f "${s}.d/overrides.conf" ? " " . abs_path "${s}.d/overrides.conf" : "");
 }
 
+sub handleModifiedUnit {
+    my ($unit, $baseName, $newUnitFile, $activePrev, $unitsToStop, $unitsToStart, $unitsToReload, $unitsToRestart, $unitsToSkip) = @_;
+
+    if ($unit eq "sysinit.target" || $unit eq "basic.target" || $unit eq "multi-user.target" || $unit eq "graphical.target" || $unit =~ /\.path$/ || $unit =~ /\.slice$/) {
+        # Do nothing.  These cannot be restarted directly.
+
+        # Slices and Paths don't have to be restarted since
+        # properties (resource limits and inotify watches)
+        # seem to get applied on daemon-reload.
+    } elsif ($unit =~ /\.mount$/) {
+        # Reload the changed mount unit to force a remount.
+        $unitsToReload->{$unit} = 1;
+        recordUnit($reloadListFile, $unit);
+    } elsif ($unit =~ /\.socket$/) {
+        # FIXME: do something?
+        # Attempt to fix this: https://github.com/NixOS/nixpkgs/pull/141192
+        # Revert of the attempt: https://github.com/NixOS/nixpkgs/pull/147609
+        # More details: https://github.com/NixOS/nixpkgs/issues/74899#issuecomment-981142430
+    } else {
+        my $unitInfo = parseUnit($newUnitFile);
+        if (boolIsTrue($unitInfo->{'X-ReloadIfChanged'} // "no")) {
+            $unitsToReload->{$unit} = 1;
+            recordUnit($reloadListFile, $unit);
+        }
+        elsif (!boolIsTrue($unitInfo->{'X-RestartIfChanged'} // "yes") || boolIsTrue($unitInfo->{'RefuseManualStop'} // "no") || boolIsTrue($unitInfo->{'X-OnlyManualStart'} // "no")) {
+            $unitsToSkip->{$unit} = 1;
+        } else {
+            # It doesn't make sense to stop and start non-services because
+            # they can't have ExecStop=
+            if (!boolIsTrue($unitInfo->{'X-StopIfChanged'} // "yes") || $unit !~ /\.service$/) {
+                # This unit should be restarted instead of
+                # stopped and started.
+                $unitsToRestart->{$unit} = 1;
+                recordUnit($restartListFile, $unit);
+            } else {
+                # 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;
+                if ($unit =~ /\.service$/) {
+                    my @sockets = split / /, ($unitInfo->{Sockets} // "");
+                    if (scalar @sockets == 0) {
+                        @sockets = ("$baseName.socket");
+                    }
+                    foreach my $socket (@sockets) {
+                        if (defined $activePrev->{$socket}) {
+                            $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;
+                            }
+                        }
+                    }
+                }
+
+                # If the unit is not socket-activated, record
+                # 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) {
+                    $unitsToStart->{$unit} = 1;
+                    recordUnit($startListFile, $unit);
+                }
+
+                $unitsToStop->{$unit} = 1;
+            }
+        }
+    }
+}
+
 # Figure out what units need to be stopped, started, restarted or reloaded.
 my (%unitsToStop, %unitsToSkip, %unitsToStart, %unitsToRestart, %unitsToReload);
 
@@ -218,69 +291,7 @@ while (my ($unit, $state) = each %{$activePrev}) {
         }
 
         elsif (fingerprintUnit($prevUnitFile) ne fingerprintUnit($newUnitFile)) {
-            if ($unit eq "sysinit.target" || $unit eq "basic.target" || $unit eq "multi-user.target" || $unit eq "graphical.target" || $unit =~ /\.path$/ || $unit =~ /\.slice$/) {
+            handleModifiedUnit($unit, $baseName, $newUnitFile, $activePrev, \%unitsToStop, \%unitsToStart, \%unitsToReload, \%unitsToRestart, \%unitsToSkip);
-                # Do nothing.  These cannot be restarted directly.
-
-                # Slices and Paths don't have to be restarted since
-                # properties (resource limits and inotify watches)
-                # seem to get applied on daemon-reload.
-            } elsif ($unit =~ /\.mount$/) {
-                # Reload the changed mount unit to force a remount.
-                $unitsToReload{$unit} = 1;
-                recordUnit($reloadListFile, $unit);
-            } elsif ($unit =~ /\.socket$/) {
-                # FIXME: do something?
-            } else {
-                my $unitInfo = parseUnit($newUnitFile);
-                if (boolIsTrue($unitInfo->{'X-ReloadIfChanged'} // "no")) {
-                    $unitsToReload{$unit} = 1;
-                    recordUnit($reloadListFile, $unit);
-                }
-                elsif (!boolIsTrue($unitInfo->{'X-RestartIfChanged'} // "yes") || boolIsTrue($unitInfo->{'RefuseManualStop'} // "no") || boolIsTrue($unitInfo->{'X-OnlyManualStart'} // "no")) {
-                    $unitsToSkip{$unit} = 1;
-                } else {
-                    if (!boolIsTrue($unitInfo->{'X-StopIfChanged'} // "yes")) {
-                        # This unit should be restarted instead of
-                        # stopped and started.
-                        $unitsToRestart{$unit} = 1;
-                        recordUnit($restartListFile, $unit);
-                    } else {
-                        # 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;
-                        if ($unit =~ /\.service$/) {
-                            my @sockets = split / /, ($unitInfo->{Sockets} // "");
-                            if (scalar @sockets == 0) {
-                                @sockets = ("$baseName.socket");
-                            }
-                            foreach my $socket (@sockets) {
-                                if (defined $activePrev->{$socket}) {
-                                    $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;
-                                    }
-                                }
-                            }
-                        }
-
-                        # If the unit is not socket-activated, record
-                        # 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) {
-                            $unitsToStart{$unit} = 1;
-                            recordUnit($startListFile, $unit);
-                        }
-
-                        $unitsToStop{$unit} = 1;
-                    }
-                }
-            }
         }
     }
 }

nixos/tests/switch-test.nix

@@ -3,21 +3,138 @@
 import ./make-test-python.nix ({ pkgs, ...} : {
   name = "switch-test";
   meta = with pkgs.lib.maintainers; {
-    maintainers = [ gleber ];
+    maintainers = [ gleber das_j ];
   };
 
   nodes = {
-    machine = { ... }: {
+    machine = { pkgs, lib, ... }: {
       users.mutableUsers = false;
+
+      specialisation = rec {
+        simpleService.configuration = {
+          systemd.services.test = {
+            wantedBy = [ "multi-user.target" ];
+            serviceConfig = {
+              Type = "oneshot";
+              RemainAfterExit = true;
+              ExecStart = "${pkgs.coreutils}/bin/true";
+            };
+          };
+        };
+
+        simpleServiceModified.configuration = {
+          imports = [ simpleService.configuration ];
+          systemd.services.test.serviceConfig.X-Test = true;
+        };
+
+        simpleServiceNostop.configuration = {
+          imports = [ simpleService.configuration ];
+          systemd.services.test.stopIfChanged = false;
+        };
+
+        simpleServiceReload.configuration = {
+          imports = [ simpleService.configuration ];
+          systemd.services.test = {
+            reloadIfChanged = true;
+            serviceConfig.ExecReload = "${pkgs.coreutils}/bin/true";
+          };
+        };
+
+        simpleServiceNorestart.configuration = {
+          imports = [ simpleService.configuration ];
+          systemd.services.test.restartIfChanged = false;
+        };
+
+        mount.configuration = {
+          systemd.mounts = [
+            {
+              description = "Testmount";
+              what = "tmpfs";
+              type = "tmpfs";
+              where = "/testmount";
+              options = "size=1M";
+              wantedBy = [ "local-fs.target" ];
+            }
+          ];
+        };
+
+        mountModified.configuration = {
+          systemd.mounts = [
+            {
+              description = "Testmount";
+              what = "tmpfs";
+              type = "tmpfs";
+              where = "/testmount";
+              options = "size=10M";
+              wantedBy = [ "local-fs.target" ];
+            }
+          ];
+        };
+
+        timer.configuration = {
+          systemd.timers.test-timer = {
+            wantedBy = [ "timers.target" ];
+            timerConfig.OnCalendar = "@1395716396"; # chosen by fair dice roll
+          };
+          systemd.services.test-timer = {
+            serviceConfig = {
+              Type = "oneshot";
+              ExecStart = "${pkgs.coreutils}/bin/true";
+            };
+          };
+        };
+
+        timerModified.configuration = {
+          imports = [ timer.configuration ];
+          systemd.timers.test-timer.timerConfig.OnCalendar = lib.mkForce "Fri 2012-11-23 16:00:00";
+        };
+
+        path.configuration = {
+          systemd.paths.test-watch = {
+            wantedBy = [ "paths.target" ];
+            pathConfig.PathExists = "/testpath";
+          };
+          systemd.services.test-watch = {
+            serviceConfig = {
+              Type = "oneshot";
+              ExecStart = "${pkgs.coreutils}/bin/touch /testpath-modified";
+            };
+          };
+        };
+
+        pathModified.configuration = {
+          imports = [ path.configuration ];
+          systemd.paths.test-watch.pathConfig.PathExists = lib.mkForce "/testpath2";
+        };
+
+        slice.configuration = {
+          systemd.slices.testslice.sliceConfig.MemoryMax = "1"; # don't allow memory allocation
+          systemd.services.testservice = {
+            serviceConfig = {
+              Type = "oneshot";
+              RemainAfterExit = true;
+              ExecStart = "${pkgs.coreutils}/bin/true";
+              Slice = "testslice.slice";
+            };
+          };
+        };
+
+        sliceModified.configuration = {
+          imports = [ slice.configuration ];
+          systemd.slices.testslice.sliceConfig.MemoryMax = lib.mkForce null;
+        };
+      };
     };
-    other = { ... }: {
+
+    other = {
       users.mutableUsers = true;
     };
   };
 
-  testScript = {nodes, ...}: let
+  testScript = { nodes, ... }: let
     originalSystem = nodes.machine.config.system.build.toplevel;
     otherSystem = nodes.other.config.system.build.toplevel;
+    machine = nodes.machine.config.system.build.toplevel;
 
     # Ensures failures pass through using pipefail, otherwise failing to
     # switch-to-configuration is hidden by the success of `tee`.
@@ -27,12 +144,186 @@ import ./make-test-python.nix ({ pkgs, ...} : {
       set -o pipefail
       exec env -i "$@" | tee /dev/stderr
     '';
-  in ''
+  in /* python */ ''
+    def switch_to_specialisation(system, name, action="test"):
+        if name == "":
+            stc = f"{system}/bin/switch-to-configuration"
+        else:
+            stc = f"{system}/specialisation/{name}/bin/switch-to-configuration"
+        out = machine.succeed(f"{stc} {action} 2>&1")
+        assert_lacks(out, "switch-to-configuration line")  # Perl warnings
+        return out
+
+    def assert_contains(haystack, needle):
+        if needle not in haystack:
+            print("The haystack that will cause the following exception is:")
+            print("---")
+            print(haystack)
+            print("---")
+            raise Exception(f"Expected string '{needle}' was not found")
+
+    def assert_lacks(haystack, needle):
+        if needle in haystack:
+            print("The haystack that will cause the following exception is:")
+            print("---")
+            print(haystack, end="")
+            print("---")
+            raise Exception(f"Unexpected string '{needle}' was found")
+
+
     machine.succeed(
         "${stderrRunner} ${originalSystem}/bin/switch-to-configuration test"
     )
     machine.succeed(
         "${stderrRunner} ${otherSystem}/bin/switch-to-configuration test"
     )
+
+    with subtest("services"):
+        switch_to_specialisation("${machine}", "")
+        # Nothing happens when nothing is changed
+        out = switch_to_specialisation("${machine}", "")
+        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:")
+        assert_lacks(out, "as well:")
+
+        # Start a simple service
+        out = switch_to_specialisation("${machine}", "simpleService")
+        assert_lacks(out, "stopping the following units:")
+        assert_lacks(out, "NOT restarting the following changed units:")
+        assert_contains(out, "reloading the following units: dbus.service\n")  # huh
+        assert_lacks(out, "\nrestarting the following units:")
+        assert_lacks(out, "\nstarting the following units:")
+        assert_contains(out, "the following new units were started: test.service\n")
+        assert_lacks(out, "as well:")
+
+        # Not changing anything doesn't do anything
+        out = switch_to_specialisation("${machine}", "simpleService")
+        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:")
+        assert_lacks(out, "as well:")
+
+        # Restart the simple service
+        out = switch_to_specialisation("${machine}", "simpleServiceModified")
+        assert_contains(out, "stopping the following units: test.service\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: test.service\n")
+        assert_lacks(out, "the following new units were started:")
+        assert_lacks(out, "as well:")
+
+        # Restart the service with stopIfChanged=false
+        out = switch_to_specialisation("${machine}", "simpleServiceNostop")
+        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: test.service\n")
+        assert_lacks(out, "\nstarting the following units:")
+        assert_lacks(out, "the following new units were started:")
+        assert_lacks(out, "as well:")
+
+        # Reload the service with reloadIfChanged=true
+        out = switch_to_specialisation("${machine}", "simpleServiceReload")
+        assert_lacks(out, "stopping the following units:")
+        assert_lacks(out, "NOT restarting the following changed units:")
+        assert_contains(out, "reloading the following units: test.service\n")
+        assert_lacks(out, "\nrestarting the following units:")
+        assert_lacks(out, "\nstarting the following units:")
+        assert_lacks(out, "the following new units were started:")
+        assert_lacks(out, "as well:")
+
+        # Nothing happens when restartIfChanged=false
+        out = switch_to_specialisation("${machine}", "simpleServiceNorestart")
+        assert_lacks(out, "stopping the following units:")
+        assert_contains(out, "NOT restarting the following changed units: test.service\n")
+        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:")
+        assert_lacks(out, "as well:")
+
+        # Dry mode shows different messages
+        out = switch_to_specialisation("${machine}", "simpleService", action="dry-activate")
+        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:")
+        assert_lacks(out, "as well:")
+        assert_contains(out, "would start the following units: test.service\n")
+
+    with subtest("mounts"):
+        switch_to_specialisation("${machine}", "mount")
+        out = machine.succeed("mount | grep 'on /testmount'")
+        assert_contains(out, "size=1024k")
+        out = switch_to_specialisation("${machine}", "mountModified")
+        assert_lacks(out, "stopping the following units:")
+        assert_lacks(out, "NOT restarting the following changed units:")
+        assert_contains(out, "reloading the following units: testmount.mount\n")
+        assert_lacks(out, "\nrestarting the following units:")
+        assert_lacks(out, "\nstarting the following units:")
+        assert_lacks(out, "the following new units were started:")
+        assert_lacks(out, "as well:")
+        # It changed
+        out = machine.succeed("mount | grep 'on /testmount'")
+        assert_contains(out, "size=10240k")
+
+    with subtest("timers"):
+        switch_to_specialisation("${machine}", "timer")
+        out = machine.succeed("systemctl show test-timer.timer")
+        assert_contains(out, "OnCalendar=2014-03-25 02:59:56 UTC")
+        out = switch_to_specialisation("${machine}", "timerModified")
+        assert_lacks(out, "stopping the following units:")
+        assert_lacks(out, "reloading the following units:")
+        assert_contains(out, "restarting the following units: test-timer.timer\n")
+        assert_lacks(out, "\nstarting the following units:")
+        assert_lacks(out, "the following new units were started:")
+        assert_lacks(out, "as well:")
+        # It changed
+        out = machine.succeed("systemctl show test-timer.timer")
+        assert_contains(out, "OnCalendar=Fri 2012-11-23 16:00:00")
+
+    with subtest("paths"):
+        out = switch_to_specialisation("${machine}", "path")
+        assert_contains(out, "stopping the following units: test-timer.timer\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_lacks(out, "\nstarting the following units:")
+        assert_contains(out, "the following new units were started: test-watch.path")
+        assert_lacks(out, "as well:")
+        machine.fail("test -f /testpath-modified")
+
+        # touch the file, unit should be triggered
+        machine.succeed("touch /testpath")
+        machine.wait_until_succeeds("test -f /testpath-modified")
+        machine.succeed("rm /testpath /testpath-modified")
+        switch_to_specialisation("${machine}", "pathModified")
+        machine.succeed("touch /testpath")
+        machine.fail("test -f /testpath-modified")
+        machine.succeed("touch /testpath2")
+        machine.wait_until_succeeds("test -f /testpath-modified")
+
+    # This test ensures that changes to slice configuration get applied.
+    # We test this by having a slice that allows no memory allocation at
+    # all and starting a service within it. If the service crashes, the slice
+    # is applied and if we modify the slice to allow memory allocation, the
+    # service should successfully start.
+    with subtest("slices"):
+        machine.succeed("echo 0 > /proc/sys/vm/panic_on_oom")  # allow OOMing
+        out = switch_to_specialisation("${machine}", "slice")
+        machine.fail("systemctl start testservice.service")
+        out = switch_to_specialisation("${machine}", "sliceModified")
+        machine.succeed("systemctl start testservice.service")
+        machine.succeed("echo 1 > /proc/sys/vm/panic_on_oom")  # disallow OOMing
   '';
 })

pkgs/os-specific/linux/systemd/default.nix

@@ -2,6 +2,7 @@
 
 { stdenv
 , lib
+, nixosTests
 , fetchFromGitHub
 , fetchpatch
 , fetchzip
@@ -613,6 +614,10 @@ stdenv.mkDerivation {
   # runtime; otherwise we can't and we need to reboot.
   passthru.interfaceVersion = 2;
 
+  passthru.tests = {
+    inherit (nixosTests) switchTest;
+  };
+
   meta = with lib; {
     homepage = "https://www.freedesktop.org/wiki/Software/systemd/";
     description = "A system and service manager for Linux";