From 393c72184986e66f2e72cf0d7e6c0476447c10b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janne=20He=C3=9F?= Date: Wed, 8 Dec 2021 13:14:40 +0100 Subject: [PATCH 1/5] nixos/switch-to-configuration: Move handleModifiedUnit into a sub --- .../activation/switch-to-configuration.pl | 132 +++++++++--------- 1 file changed, 69 insertions(+), 63 deletions(-) diff --git a/nixos/modules/system/activation/switch-to-configuration.pl b/nixos/modules/system/activation/switch-to-configuration.pl index 9bf7a5c0d427..5c75f2557714 100644 --- a/nixos/modules/system/activation/switch-to-configuration.pl +++ b/nixos/modules/system/activation/switch-to-configuration.pl @@ -146,6 +146,74 @@ 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? + } 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; + } + } + } +} + # Figure out what units need to be stopped, started, restarted or reloaded. my (%unitsToStop, %unitsToSkip, %unitsToStart, %unitsToRestart, %unitsToReload); @@ -218,69 +286,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$/) { - # 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; - } - } - } + handleModifiedUnit($unit, $baseName, $newUnitFile, $activePrev, \%unitsToStop, \%unitsToStart, \%unitsToReload, \%unitsToRestart, \%unitsToSkip); } } } From efcdc01d629b05e52137ecdc288d363ac5cb5128 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janne=20He=C3=9F?= Date: Thu, 9 Dec 2021 12:30:48 +0100 Subject: [PATCH 2/5] nixos/switchTest: Massively extend the test --- nixos/tests/switch-test.nix | 301 +++++++++++++++++++++++++++++++++++- 1 file changed, 296 insertions(+), 5 deletions(-) diff --git a/nixos/tests/switch-test.nix b/nixos/tests/switch-test.nix index 78adf7ffa7da..daad9134885f 100644 --- a/nixos/tests/switch-test.nix +++ b/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 ''; }) From 2024306048f284278c34599e563ebd8a20253559 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janne=20He=C3=9F?= Date: Thu, 9 Dec 2021 12:31:05 +0100 Subject: [PATCH 3/5] nixos/switch-to-configuration: Restart non-services --- nixos/modules/system/activation/switch-to-configuration.pl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nixos/modules/system/activation/switch-to-configuration.pl b/nixos/modules/system/activation/switch-to-configuration.pl index 5c75f2557714..5f92fb07c04a 100644 --- a/nixos/modules/system/activation/switch-to-configuration.pl +++ b/nixos/modules/system/activation/switch-to-configuration.pl @@ -170,7 +170,9 @@ sub handleModifiedUnit { 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")) { + # 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; From dac4f986ad9081b4ac5ead5064d65ca11e7f683c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janne=20He=C3=9F?= Date: Thu, 9 Dec 2021 12:39:30 +0100 Subject: [PATCH 4/5] systemd: Add switchTest to passthru --- pkgs/os-specific/linux/systemd/default.nix | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkgs/os-specific/linux/systemd/default.nix b/pkgs/os-specific/linux/systemd/default.nix index aa106ca1abaa..13a39f182c0e 100644 --- a/pkgs/os-specific/linux/systemd/default.nix +++ b/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"; From 68076287918ca9ee1c20e6289e77b2ce229cd493 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janne=20He=C3=9F?= Date: Thu, 9 Dec 2021 13:51:18 +0100 Subject: [PATCH 5/5] nixos/switch-to-configuraton: Add details about sockets --- nixos/modules/system/activation/switch-to-configuration.pl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nixos/modules/system/activation/switch-to-configuration.pl b/nixos/modules/system/activation/switch-to-configuration.pl index 5f92fb07c04a..707823dc54d3 100644 --- a/nixos/modules/system/activation/switch-to-configuration.pl +++ b/nixos/modules/system/activation/switch-to-configuration.pl @@ -161,6 +161,9 @@ sub handleModifiedUnit { 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")) {