switch-to-configuration-ng: Stop drop-in template instances

As the test shows, stc-ng previously did not correctly account for the
removal of template instances defined with overrideStrategy "asDropin".
A real use case does exist: Defining systemd-nspawn container units
which inherit the settings from the vendored systemd-nspawn@.service.

I tried to find the least intrusvie but stable solution here. The
globbing is technically unnecessary but it mimics the behaviour and
implementation in parse_unit.

nixos/tests/activation/template-dropin.nix

@@ -0,0 +1,91 @@
+{ lib, ... }:
+{
+  name = "stc-template-dropin";
+
+  nodes.machine =
+    { pkgs, lib, ... }:
+    {
+      # Define the base template. This file exists in both generations.
+      systemd.services."test-template@" = {
+        description = "A base template for testing";
+        serviceConfig.ExecStart = "${pkgs.coreutils}/bin/sleep infinity";
+      };
+
+      # Define the managed instance using drop-ins.
+      systemd.services."test-template@managed" = {
+        overrideStrategy = "asDropin";
+        wantedBy = [ "multi-user.target" ];
+        serviceConfig.Environment = "TEST_VAR=1";
+      };
+
+      # Also define a service which will be changed
+      systemd.services."test-template@changed" = {
+        overrideStrategy = "asDropin";
+        wantedBy = [ "multi-user.target" ];
+        serviceConfig.Environment = "TEST_VAR=1";
+      };
+
+      # Create a new generation that explicitly removes the managed instance
+      specialisation.new-generation.configuration = {
+        systemd.services."test-template@managed" = {
+          enable = lib.mkForce false;
+          wantedBy = lib.mkForce [ ];
+        };
+        systemd.services."test-template@changed" = {
+          serviceConfig.Environment = lib.mkForce "TEST_VAR=2";
+        };
+      };
+    };
+
+  testScript = # python
+    ''
+      managed_unit = "test-template@managed.service"
+      changed_unit = "test-template@changed.service"
+      manual_unit = "test-template@manual.service"
+
+      with subtest("Start the machine and ensure the managed instance is running"):
+          machine.wait_for_unit("multi-user.target")
+          machine.wait_for_unit(managed_unit)
+          machine.wait_for_unit(changed_unit)
+
+      with subtest("Imperatively start an unmanaged instance"):
+          machine.succeed(f"systemctl start {manual_unit}")
+          machine.wait_for_unit(manual_unit)
+
+      with subtest("Run dry-activate on the new generation"):
+          new_gen = "/run/booted-system/specialisation/new-generation"
+
+          # switch-to-configuration prints to stderr, so we redirect it to stdout for parsing
+          output = machine.succeed(f"{new_gen}/bin/switch-to-configuration dry-activate 2>&1")
+          machine.log("dry-activate output:\n" + output)
+
+          found_stop = False
+          found_start = False
+          found_changed = False
+          found_manual_stop = False
+          for line in output.splitlines():
+              if line.startswith("would stop"):
+                  found_stop = found_stop or managed_unit in line
+                  found_manual_stop = found_manual_stop or manual_unit in line
+              elif line.startswith("would start"):
+                  found_start = found_start or managed_unit in line
+                  found_changed = found_changed or changed_unit in line
+
+          assert found_stop, "The managed instance was not marked for stopping."
+          assert found_changed, "The changed unit was not marked for stopping + starting (restarting)."
+          assert not found_start, "switch-to-configuration wants to start the removed managed instance!"
+          assert not found_manual_stop, "switch-to-configuration wants to stop the manual instance!"
+
+      with subtest("Perform the actual switch and verify system state"):
+          machine.succeed(f"{new_gen}/bin/switch-to-configuration switch")
+
+          # The managed instance should be dead
+          machine.fail(f"systemctl is-active {managed_unit}")
+
+          # The changed instance should be running
+          machine.succeed(f"systemctl is-active {changed_unit}")
+
+          # The manual instance should survive the configuration switch untouched
+          machine.succeed(f"systemctl is-active {manual_unit}")
+    '';
+}

nixos/tests/all-tests.nix

@@ -189,6 +189,7 @@ in
   activation-nix-channel = runTest ./activation/nix-channel.nix;
   activation-nixos-init = runTest ./activation/nixos-init.nix;
   activation-perlless = runTest ./activation/perlless.nix;
+  activation-template-dropin = runTest ./activation/template-dropin.nix;
   activation-var = runTest ./activation/var.nix;
   actual = runTest ./actual.nix;
   adguardhome = runTest ./adguardhome.nix;

pkgs/by-name/sw/switch-to-configuration-ng/package.nix

@@ -30,7 +30,7 @@ rustPlatform.buildRustPackage {
     cargo clippy -- -Dwarnings
   '';
 
-  passthru.tests = { inherit (nixosTests) switchTest; };
+  passthru.tests = { inherit (nixosTests) switchTest activation-template-dropin; };
 
   meta = {
     description = "NixOS switch-to-configuration program";

pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs

@@ -998,6 +998,14 @@ fn remove_file_if_exists(p: impl AsRef<Path>) -> std::io::Result<()> {
     }
 }
 
+/// Checks if a unit has been disabled in configuration
+fn is_unit_disabled(unit_file: PathBuf) -> bool {
+    unit_file
+        .canonicalize()
+        .map(|full_path| full_path == Path::new("/dev/null"))
+        .unwrap_or(true)
+}
+
 /// Iterate over currently active units in the given scope, compare the unit
 /// file in `old_unit_dir` against the one in `new_unit_dir`, and populate the
 /// action maps accordingly.
@@ -1050,6 +1058,7 @@ fn collect_unit_changes(
         let mut base_unit = unit.clone();
         let mut current_base_unit_file = current_unit_file.clone();
         let mut new_base_unit_file = new_unit_file.clone();
+        let mut dropins_removed = false;
 
         // Detect template instances
         if let Some((Some(template_name), Some(template_instance))) =
@@ -1064,6 +1073,22 @@ fn collect_unit_changes(
                 base_unit = format!("{template_name}@.{template_instance}");
                 current_base_unit_file = old_unit_dir.join(&base_unit);
                 new_base_unit_file = new_unit_dir.join(&base_unit);
+
+                // Handle instances defined as drop-ins
+                let mut current_dropins =
+                    glob(&format!("{}.d/*.conf", current_unit_file.display()))
+                        .context("Invalid glob pattern")?
+                        .filter_map(|v| v.ok());
+                let mut new_dropins = glob(&format!("{}.d/*.conf", new_unit_file.display()))
+                    .context("Invalid glob pattern")?
+                    .filter_map(|v| v.ok());
+
+                // When the unit is disabled, the override files will be a symlink to /dev/null instead.
+                dropins_removed =
+                    // True if either no existing files or no disabled files (unit existed)
+                    !current_dropins.all(is_unit_disabled)
+                    // True if either no new files or all disabled new files (unit gone)
+                    && new_dropins.all(is_unit_disabled);
             }
         }
 
@@ -1078,11 +1103,9 @@ fn collect_unit_changes(
         if current_base_unit_file.exists()
             && (unit_state.state == "active" || unit_state.state == "activating")
         {
-            if new_base_unit_file
-                .canonicalize()
-                .map(|full_path| full_path == Path::new("/dev/null"))
-                .unwrap_or(true)
-            {
+            // Account for template unit instances where overrideStrategy == "asDropin"
+            // whilst also allowing manual instances to keep running.
+            if dropins_removed || is_unit_disabled(new_base_unit_file.clone()) {
                 let current_unit_info = parse_unit(&current_unit_file, &current_base_unit_file)?;
                 if parse_systemd_bool(Some(&current_unit_info), "Unit", "X-StopOnRemoval", true) {
                     _ = units_to_stop.insert(unit.to_string(), ());