switch-to-configuration-ng: stop removed drop-in template instances (#512360)

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

@@ -410,6 +410,14 @@ fn parse_systemd_ini(data: &mut UnitInfo, mut unit_file: impl Read) -> Result<()
     Ok(())
 }
 
+/// Glob for `<unit_path>.d/*.conf`, escaping any glob metacharacters in the
+/// path prefix so unit names containing e.g. `\` (from systemd-escape) are
+/// matched literally.
+fn unit_dropin_glob(unit_path: &Path) -> Result<glob::Paths> {
+    let prefix = glob::Pattern::escape(&format!("{}.d", unit_path.display()));
+    glob(&format!("{prefix}/*.conf")).context("Invalid glob pattern")
+}
+
 // This function takes the path to a systemd configuration file (like a unit configuration) and
 // parses it into a UnitInfo structure.
 //
@@ -428,9 +436,7 @@ fn parse_unit(unit_file: &Path, base_unit_path: &Path) -> Result<UnitInfo> {
         )
     })?;
 
-    for entry in
-        glob(&format!("{}.d/*.conf", base_unit_path.display())).context("Invalid glob pattern")?
-    {
+    for entry in unit_dropin_glob(base_unit_path)? {
         let Ok(entry) = entry else {
             continue;
         };
@@ -442,9 +448,7 @@ fn parse_unit(unit_file: &Path, base_unit_path: &Path) -> Result<UnitInfo> {
 
     // Handle drop-in template-unit instance overrides
     if unit_file != base_unit_path {
-        for entry in
-            glob(&format!("{}.d/*.conf", unit_file.display())).context("Invalid glob pattern")?
-        {
+        for entry in unit_dropin_glob(unit_file)? {
             let Ok(entry) = entry else {
                 continue;
             };
@@ -1036,6 +1040,37 @@ fn remove_file_if_exists(p: impl AsRef<Path>) -> std::io::Result<()> {
     }
 }
 
+#[derive(Debug, PartialEq)]
+enum UnitFileState {
+    /// The file exists and resolves to a real unit file.
+    Present,
+    /// The file is a (chain of) symlink(s) to /dev/null, i.e. masked.
+    Masked,
+    /// The file does not exist, or is a dangling symlink.
+    Missing,
+}
+
+impl UnitFileState {
+    /// Whether the unit file is absent from the configuration, either because
+    /// it does not exist or because it has been masked to /dev/null.
+    fn is_gone(&self) -> bool {
+        matches!(self, UnitFileState::Masked | UnitFileState::Missing)
+    }
+}
+
+/// Classify a unit-file path. Unexpected I/O errors are propagated.
+fn unit_file_state(unit_file: impl AsRef<Path>) -> Result<UnitFileState> {
+    let path = unit_file.as_ref();
+    match path.canonicalize() {
+        Ok(target) if target == Path::new("/dev/null") => Ok(UnitFileState::Masked),
+        Ok(_) => Ok(UnitFileState::Present),
+        Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(UnitFileState::Missing),
+        Err(err) => {
+            Err(err).with_context(|| format!("Failed to canonicalize unit file {}", path.display()))
+        }
+    }
+}
+
 /// 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.
@@ -1088,6 +1123,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))) =
@@ -1102,6 +1138,33 @@ 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. When the unit is
+                // disabled, the override files will be a symlink to
+                // /dev/null instead.
+
+                // The instance was NixOS-managed in the old generation iff
+                // at least one current drop-in is a real file (not masked).
+                let mut currently_managed = false;
+                for entry in unit_dropin_glob(&current_unit_file)?.flatten() {
+                    if unit_file_state(&entry)? == UnitFileState::Present {
+                        currently_managed = true;
+                        break;
+                    }
+                }
+
+                // If the instance was managed before, check whether it
+                // still is: gone iff no new drop-ins remain (or all are
+                // masked to /dev/null).
+                if currently_managed {
+                    dropins_removed = true;
+                    for entry in unit_dropin_glob(&new_unit_file)?.flatten() {
+                        if unit_file_state(&entry)? == UnitFileState::Present {
+                            dropins_removed = false;
+                            break;
+                        }
+                    }
+                }
             }
         }
 
@@ -1116,11 +1179,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 || unit_file_state(&new_base_unit_file)?.is_gone() {
                 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(), ());