From d073105d6b346e4bdc27a22ad50cffcdc8a4b630 Mon Sep 17 00:00:00 2001 From: oddlama Date: Tue, 16 May 2023 14:35:41 +0200 Subject: nixos/switch-to-configuration: fix ignoring of template unit specialization dropins --- .../system/activation/switch-to-configuration.pl | 58 +++++---- nixos/tests/switch-test.nix | 130 +++++++++++++++++++-- 2 files changed, 156 insertions(+), 32 deletions(-) diff --git a/nixos/modules/system/activation/switch-to-configuration.pl b/nixos/modules/system/activation/switch-to-configuration.pl index cfad6403986..d7f50448ed1 100755 --- a/nixos/modules/system/activation/switch-to-configuration.pl +++ b/nixos/modules/system/activation/switch-to-configuration.pl @@ -253,16 +253,24 @@ sub parse_systemd_ini { # If a directory with the same basename ending in .d exists next to the unit file, it will be # assumed to contain override files which will be parsed as well and handled properly. sub parse_unit { - my ($unit_path) = @_; + my ($unit_path, $base_unit_path) = @_; # Parse the main unit and all overrides my %unit_data; # Replace \ with \\ so glob() still works with units that have a \ in them # Valid characters in unit names are ASCII letters, digits, ":", "-", "_", ".", and "\" + $base_unit_path =~ s/\\/\\\\/gmsx; $unit_path =~ s/\\/\\\\/gmsx; - foreach (glob("${unit_path}{,.d/*.conf}")) { + + foreach (glob("${base_unit_path}{,.d/*.conf}")) { parse_systemd_ini(\%unit_data, "$_") } + # Handle drop-in template-unit instance overrides + if ($unit_path ne $base_unit_path) { + foreach (glob("${unit_path}.d/*.conf")) { + parse_systemd_ini(\%unit_data, "$_") + } + } return %unit_data; } @@ -423,7 +431,7 @@ sub compare_units { ## no critic(Subroutines::ProhibitExcessComplexity) # Called when a unit exists in both the old systemd and the new system and the units # differ. This figures out of what units are to be stopped, restarted, reloaded, started, and skipped. sub handle_modified_unit { ## no critic(Subroutines::ProhibitManyArgs, Subroutines::ProhibitExcessComplexity) - my ($unit, $base_name, $new_unit_file, $new_unit_info, $active_cur, $units_to_stop, $units_to_start, $units_to_reload, $units_to_restart, $units_to_skip) = @_; + my ($unit, $base_name, $new_unit_file, $new_base_unit_file, $new_unit_info, $active_cur, $units_to_stop, $units_to_start, $units_to_reload, $units_to_restart, $units_to_skip) = @_; if ($unit eq "sysinit.target" || $unit eq "basic.target" || $unit eq "multi-user.target" || $unit eq "graphical.target" || $unit =~ /\.path$/msx || $unit =~ /\.slice$/msx) { # Do nothing. These cannot be restarted directly. @@ -442,7 +450,7 @@ sub handle_modified_unit { ## no critic(Subroutines::ProhibitManyArgs, Subroutin # Revert of the attempt: https://github.com/NixOS/nixpkgs/pull/147609 # More details: https://github.com/NixOS/nixpkgs/issues/74899#issuecomment-981142430 } else { - my %new_unit_info = $new_unit_info ? %{$new_unit_info} : parse_unit($new_unit_file); + my %new_unit_info = $new_unit_info ? %{$new_unit_info} : parse_unit($new_unit_file, $new_base_unit_file); if (parse_systemd_bool(\%new_unit_info, "Service", "X-ReloadIfChanged", 0) and not $units_to_restart->{$unit} and not $units_to_stop->{$unit}) { $units_to_reload->{$unit} = 1; record_unit($reload_list_file, $unit); @@ -538,31 +546,33 @@ my %units_to_filter; # units not shown my $active_cur = get_active_units(); while (my ($unit, $state) = each(%{$active_cur})) { - my $base_unit = $unit; + my $cur_unit_file = "/etc/systemd/system/$unit"; + my $new_unit_file = "$toplevel/etc/systemd/system/$unit"; - my $cur_unit_file = "/etc/systemd/system/$base_unit"; - my $new_unit_file = "$toplevel/etc/systemd/system/$base_unit"; + my $base_unit = $unit; + my $cur_base_unit_file = $cur_unit_file; + my $new_base_unit_file = $new_unit_file; # Detect template instances. if (!-e $cur_unit_file && !-e $new_unit_file && $unit =~ /^(.*)@[^\.]*\.(.*)$/msx) { $base_unit = "$1\@.$2"; - $cur_unit_file = "/etc/systemd/system/$base_unit"; - $new_unit_file = "$toplevel/etc/systemd/system/$base_unit"; + $cur_base_unit_file = "/etc/systemd/system/$base_unit"; + $new_base_unit_file = "$toplevel/etc/systemd/system/$base_unit"; } my $base_name = $base_unit; $base_name =~ s/\.[[:lower:]]*$//msx; - if (-e $cur_unit_file && ($state->{state} eq "active" || $state->{state} eq "activating")) { - if (! -e $new_unit_file || abs_path($new_unit_file) eq "/dev/null") { - my %cur_unit_info = parse_unit($cur_unit_file); + if (-e $cur_base_unit_file && ($state->{state} eq "active" || $state->{state} eq "activating")) { + if (! -e $new_base_unit_file || abs_path($new_base_unit_file) eq "/dev/null") { + my %cur_unit_info = parse_unit($cur_unit_file, $cur_base_unit_file); if (parse_systemd_bool(\%cur_unit_info, "Unit", "X-StopOnRemoval", 1)) { $units_to_stop{$unit} = 1; } } elsif ($unit =~ /\.target$/msx) { - my %new_unit_info = parse_unit($new_unit_file); + my %new_unit_info = parse_unit($new_unit_file, $new_base_unit_file); # Cause all active target units to be restarted below. # This should start most changed units we stop here as @@ -596,11 +606,11 @@ while (my ($unit, $state) = each(%{$active_cur})) { } else { - my %cur_unit_info = parse_unit($cur_unit_file); - my %new_unit_info = parse_unit($new_unit_file); + my %cur_unit_info = parse_unit($cur_unit_file, $cur_base_unit_file); + my %new_unit_info = parse_unit($new_unit_file, $new_base_unit_file); my $diff = compare_units(\%cur_unit_info, \%new_unit_info); if ($diff == 1) { - handle_modified_unit($unit, $base_name, $new_unit_file, \%new_unit_info, $active_cur, \%units_to_stop, \%units_to_start, \%units_to_reload, \%units_to_restart, \%units_to_skip); + handle_modified_unit($unit, $base_name, $new_unit_file, $new_base_unit_file, \%new_unit_info, $active_cur, \%units_to_stop, \%units_to_start, \%units_to_reload, \%units_to_restart, \%units_to_skip); } elsif ($diff == 2 and not $units_to_restart{$unit}) { $units_to_reload{$unit} = 1; record_unit($reload_list_file, $unit); @@ -710,13 +720,14 @@ if ($action eq "dry-activate") { # Handle the activation script requesting the restart or reload of a unit. foreach (split(/\n/msx, read_file($dry_restart_by_activation_file, err_mode => "quiet") // "")) { my $unit = $_; + my $new_unit_file = "$toplevel/etc/systemd/system/$unit"; my $base_unit = $unit; - my $new_unit_file = "$toplevel/etc/systemd/system/$base_unit"; + my $new_base_unit_file = $new_unit_file; # Detect template instances. if (!-e $new_unit_file && $unit =~ /^(.*)@[^\.]*\.(.*)$/msx) { $base_unit = "$1\@.$2"; - $new_unit_file = "$toplevel/etc/systemd/system/$base_unit"; + $new_base_unit_file = "$toplevel/etc/systemd/system/$base_unit"; } my $base_name = $base_unit; @@ -728,7 +739,7 @@ if ($action eq "dry-activate") { next; } - handle_modified_unit($unit, $base_name, $new_unit_file, undef, $active_cur, \%units_to_restart, \%units_to_restart, \%units_to_reload, \%units_to_restart, \%units_to_skip); + handle_modified_unit($unit, $base_name, $new_unit_file, $new_base_unit_file, undef, $active_cur, \%units_to_restart, \%units_to_restart, \%units_to_reload, \%units_to_restart, \%units_to_skip); } unlink($dry_restart_by_activation_file); @@ -782,13 +793,14 @@ system("$out/activate", "$out") == 0 or $res = 2; # Handle the activation script requesting the restart or reload of a unit. foreach (split(/\n/msx, read_file($restart_by_activation_file, err_mode => "quiet") // "")) { my $unit = $_; + my $new_unit_file = "$toplevel/etc/systemd/system/$unit"; my $base_unit = $unit; - my $new_unit_file = "$toplevel/etc/systemd/system/$base_unit"; + my $new_base_unit_file = $new_unit_file; # Detect template instances. if (!-e $new_unit_file && $unit =~ /^(.*)@[^\.]*\.(.*)$/msx) { $base_unit = "$1\@.$2"; - $new_unit_file = "$toplevel/etc/systemd/system/$base_unit"; + $new_base_unit_file = "$toplevel/etc/systemd/system/$base_unit"; } my $base_name = $base_unit; @@ -801,7 +813,7 @@ foreach (split(/\n/msx, read_file($restart_by_activation_file, err_mode => "quie next; } - handle_modified_unit($unit, $base_name, $new_unit_file, undef, $active_cur, \%units_to_restart, \%units_to_restart, \%units_to_reload, \%units_to_restart, \%units_to_skip); + handle_modified_unit($unit, $base_name, $new_unit_file, $new_base_unit_file, undef, $active_cur, \%units_to_restart, \%units_to_restart, \%units_to_reload, \%units_to_restart, \%units_to_skip); } # We can remove the file now because it has been propagated to the other restart/reload files unlink($restart_by_activation_file); @@ -859,7 +871,7 @@ if (scalar(keys(%units_to_reload)) > 0) { for my $unit (keys(%units_to_reload)) { if (!unit_is_active($unit)) { # Figure out if we need to start the unit - my %unit_info = parse_unit("$toplevel/etc/systemd/system/$unit"); + my %unit_info = parse_unit("$toplevel/etc/systemd/system/$unit", "$out/etc/systemd/system/$unit"); if (!(parse_systemd_bool(\%unit_info, "Unit", "RefuseManualStart", 0) || parse_systemd_bool(\%unit_info, "Unit", "X-OnlyManualStart", 0))) { $units_to_start{$unit} = 1; record_unit($start_list_file, $unit); diff --git a/nixos/tests/switch-test.nix b/nixos/tests/switch-test.nix index f44dede7fef..fe6bad1ac05 100644 --- a/nixos/tests/switch-test.nix +++ b/nixos/tests/switch-test.nix @@ -1,6 +1,6 @@ # Test configuration switching. -import ./make-test-python.nix ({ pkgs, ...} : let +import ./make-test-python.nix ({ lib, pkgs, ...} : let # Simple service that can either be socket-activated or that will # listen on port 1234 if not socket-activated. @@ -290,29 +290,50 @@ in { ExecReload = "${pkgs.coreutils}/bin/true"; }; }; + "templated-simple-service@" = simple-service; + "templated-simple-service@instance".overrideStrategy = "asDropin"; simple-restart-service = simple-service // { stopIfChanged = false; }; + "templated-simple-restart-service@" = simple-restart-service; + "templated-simple-restart-service@instance".overrideStrategy = "asDropin"; simple-reload-service = simple-service // { reloadIfChanged = true; }; + "templated-simple-reload-service@" = simple-reload-service; + "templated-simple-reload-service@instance".overrideStrategy = "asDropin"; no-restart-service = simple-service // { restartIfChanged = false; }; + "templated-no-restart-service@" = no-restart-service; + "templated-no-restart-service@instance".overrideStrategy = "asDropin"; reload-triggers = simple-service // { wantedBy = [ "multi-user.target" ]; }; + "templated-reload-triggers@" = simple-service; + "templated-reload-triggers@instance" = { + overrideStrategy = "asDropin"; + wantedBy = [ "multi-user.target" ]; + }; reload-triggers-and-restart-by-as = simple-service; + "templated-reload-triggers-and-restart-by-as@" = reload-triggers-and-restart-by-as; + "templated-reload-triggers-and-restart-by-as@instance".overrideStrategy = "asDropin"; reload-triggers-and-restart = simple-service // { stopIfChanged = false; # easier to check for this wantedBy = [ "multi-user.target" ]; }; + "templated-reload-triggers-and-restart@" = simple-service; + "templated-reload-triggers-and-restart@instance" = { + overrideStrategy = "asDropin"; + stopIfChanged = false; # easier to check for this + wantedBy = [ "multi-user.target" ]; + }; }; system.activationScripts.restart-and-reload-test = { @@ -332,12 +353,20 @@ in { simple-reload-service.service no-restart-service.service reload-triggers-and-restart-by-as.service + templated-simple-service@instance.service + templated-simple-restart-service@instance.service + templated-simple-reload-service@instance.service + templated-no-restart-service@instance.service + templated-reload-triggers-and-restart-by-as@instance.service EOF cat <> "$g" reload-triggers.service reload-triggers-and-restart-by-as.service reload-triggers-and-restart.service + templated-reload-triggers@instance.service + templated-reload-triggers-and-restart-by-as@instance.service + templated-reload-triggers-and-restart@instance.service EOF ''; }; @@ -346,6 +375,10 @@ in { restart-and-reload-by-activation-script-modified.configuration = { imports = [ restart-and-reload-by-activation-script.configuration ]; systemd.services.reload-triggers-and-restart.serviceConfig.X-Modified = "test"; + systemd.services."templated-reload-triggers-and-restart@instance" = { + overrideStrategy = "asDropin"; + serviceConfig.X-Modified = "test"; + }; }; simple-socket.configuration = { @@ -507,6 +540,10 @@ in { set -o pipefail exec env -i "$@" | tee /dev/stderr ''; + + # Returns a comma separated representation of the given list in sorted + # order, that matches the output format of switch-to-configuration.pl + sortedUnits = xs: lib.concatStringsSep ", " (builtins.sort builtins.lessThan xs); in /* python */ '' def switch_to_specialisation(system, name, action="test", fail=False): if name == "": @@ -896,15 +933,62 @@ in { assert_lacks(out, "NOT restarting the following changed units:") assert_lacks(out, "reloading the following units:") assert_lacks(out, "restarting the following units:") - assert_contains(out, "\nstarting the following units: no-restart-service.service, reload-triggers-and-restart-by-as.service, simple-reload-service.service, simple-restart-service.service, simple-service.service\n") - assert_contains(out, "the following new units were started: no-restart-service.service, reload-triggers-and-restart-by-as.service, reload-triggers-and-restart.service, reload-triggers.service, simple-reload-service.service, simple-restart-service.service, simple-service.service\n") + assert_contains(out, "\nstarting the following units: ${sortedUnits [ + "no-restart-service.service" + "reload-triggers-and-restart-by-as.service" + "simple-reload-service.service" + "simple-restart-service.service" + "simple-service.service" + "templated-no-restart-service@instance.service" + "templated-reload-triggers-and-restart-by-as@instance.service" + "templated-simple-reload-service@instance.service" + "templated-simple-restart-service@instance.service" + "templated-simple-service@instance.service" + ]}\n") + assert_contains(out, "the following new units were started: ${sortedUnits [ + "no-restart-service.service" + "reload-triggers-and-restart-by-as.service" + "reload-triggers-and-restart.service" + "reload-triggers.service" + "simple-reload-service.service" + "simple-restart-service.service" + "simple-service.service" + "system-templated\\\\x2dno\\\\x2drestart\\\\x2dservice.slice" + "system-templated\\\\x2dreload\\\\x2dtriggers.slice" + "system-templated\\\\x2dreload\\\\x2dtriggers\\\\x2dand\\\\x2drestart.slice" + "system-templated\\\\x2dreload\\\\x2dtriggers\\\\x2dand\\\\x2drestart\\\\x2dby\\\\x2das.slice" + "system-templated\\\\x2dsimple\\\\x2dreload\\\\x2dservice.slice" + "system-templated\\\\x2dsimple\\\\x2drestart\\\\x2dservice.slice" + "system-templated\\\\x2dsimple\\\\x2dservice.slice" + "templated-no-restart-service@instance.service" + "templated-reload-triggers-and-restart-by-as@instance.service" + "templated-reload-triggers-and-restart@instance.service" + "templated-reload-triggers@instance.service" + "templated-simple-reload-service@instance.service" + "templated-simple-restart-service@instance.service" + "templated-simple-service@instance.service" + ]}\n") # Switch to the same system where the example services get restarted # and reloaded by the activation script out = switch_to_specialisation("${machine}", "restart-and-reload-by-activation-script") assert_lacks(out, "stopping the following units:") assert_lacks(out, "NOT restarting the following changed units:") - assert_contains(out, "reloading the following units: reload-triggers-and-restart.service, reload-triggers.service, simple-reload-service.service\n") - assert_contains(out, "restarting the following units: reload-triggers-and-restart-by-as.service, simple-restart-service.service, simple-service.service\n") + assert_contains(out, "reloading the following units: ${sortedUnits [ + "reload-triggers-and-restart.service" + "reload-triggers.service" + "simple-reload-service.service" + "templated-reload-triggers-and-restart@instance.service" + "templated-reload-triggers@instance.service" + "templated-simple-reload-service@instance.service" + ]}\n") + assert_contains(out, "restarting the following units: ${sortedUnits [ + "reload-triggers-and-restart-by-as.service" + "simple-restart-service.service" + "simple-service.service" + "templated-reload-triggers-and-restart-by-as@instance.service" + "templated-simple-restart-service@instance.service" + "templated-simple-service@instance.service" + ]}\n") assert_lacks(out, "\nstarting the following units:") assert_lacks(out, "the following new units were started:") # Switch to the same system and see if the service gets restarted when it's modified @@ -912,16 +996,44 @@ in { out = switch_to_specialisation("${machine}", "restart-and-reload-by-activation-script-modified") assert_lacks(out, "stopping the following units:") assert_lacks(out, "NOT restarting the following changed units:") - assert_contains(out, "reloading the following units: reload-triggers.service, simple-reload-service.service\n") - assert_contains(out, "restarting the following units: reload-triggers-and-restart-by-as.service, reload-triggers-and-restart.service, simple-restart-service.service, simple-service.service\n") + assert_contains(out, "reloading the following units: ${sortedUnits [ + "reload-triggers.service" + "simple-reload-service.service" + "templated-reload-triggers@instance.service" + "templated-simple-reload-service@instance.service" + ]}\n") + assert_contains(out, "restarting the following units: ${sortedUnits [ + "reload-triggers-and-restart-by-as.service" + "reload-triggers-and-restart.service" + "simple-restart-service.service" + "simple-service.service" + "templated-reload-triggers-and-restart-by-as@instance.service" + "templated-reload-triggers-and-restart@instance.service" + "templated-simple-restart-service@instance.service" + "templated-simple-service@instance.service" + ]}\n") assert_lacks(out, "\nstarting the following units:") assert_lacks(out, "the following new units were started:") # The same, but in dry mode out = switch_to_specialisation("${machine}", "restart-and-reload-by-activation-script", action="dry-activate") assert_lacks(out, "would stop the following units:") assert_lacks(out, "would NOT stop the following changed units:") - 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_contains(out, "would reload the following units: ${sortedUnits [ + "reload-triggers.service" + "simple-reload-service.service" + "templated-reload-triggers@instance.service" + "templated-simple-reload-service@instance.service" + ]}\n") + assert_contains(out, "would restart the following units: ${sortedUnits [ + "reload-triggers-and-restart-by-as.service" + "reload-triggers-and-restart.service" + "simple-restart-service.service" + "simple-service.service" + "templated-reload-triggers-and-restart-by-as@instance.service" + "templated-reload-triggers-and-restart@instance.service" + "templated-simple-restart-service@instance.service" + "templated-simple-service@instance.service" + ]}\n") assert_lacks(out, "\nwould start the following units:") with subtest("socket-activated services"): -- cgit 1.4.1