nixos/switch-to-configuration: fix ignoring of template unit specialization dropins

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 <<EOF >> "$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"):