From 418a0ae8f9e7e28dec3386e923bab98919c712a0 Mon Sep 17 00:00:00 2001 From: Lucas Savva Date: Tue, 31 Mar 2026 22:28:57 +0100 Subject: [PATCH 1/5] 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 | 91 +++++++++++++++++++ nixos/tests/all-tests.nix | 1 + .../sw/switch-to-configuration-ng/package.nix | 2 +- .../sw/switch-to-configuration-ng/src/main.rs | 33 ++++++- 4 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 nixos/tests/activation/template-dropin.nix diff --git a/nixos/tests/activation/template-dropin.nix b/nixos/tests/activation/template-dropin.nix new file mode 100644 index 000000000000..b4c3de4a29c6 --- /dev/null +++ b/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}") + ''; +} diff --git a/nixos/tests/all-tests.nix b/nixos/tests/all-tests.nix index f6093eb7e14a..80199d0731d9 100644 --- a/nixos/tests/all-tests.nix +++ b/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; diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/package.nix b/pkgs/by-name/sw/switch-to-configuration-ng/package.nix index cffc771405f4..ccb3409d7eea 100644 --- a/pkgs/by-name/sw/switch-to-configuration-ng/package.nix +++ b/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"; diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs index 22b340895f56..5145b54b5a19 100644 --- a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs +++ b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs @@ -998,6 +998,14 @@ fn remove_file_if_exists(p: impl AsRef) -> 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(¤t_unit_file, ¤t_base_unit_file)?; if parse_systemd_bool(Some(¤t_unit_info), "Unit", "X-StopOnRemoval", true) { _ = units_to_stop.insert(unit.to_string(), ()); From 575bbbbc76d5ef0b272127cc871ad7108477b9e2 Mon Sep 17 00:00:00 2001 From: r-vdp Date: Wed, 22 Apr 2026 13:25:05 +0200 Subject: [PATCH 2/5] switch-to-configuration-ng: Escape unit paths in drop-in glob Unit names may contain backslashes (e.g. from systemd-escape) and the toplevel path is user-influenced; both were previously fed unescaped into glob() where they could be misinterpreted as metacharacters and silently match nothing. Factor the pattern into a helper that escapes the path prefix and use it at all four call sites. --- .../sw/switch-to-configuration-ng/src/main.rs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs index 5145b54b5a19..4d243693eaa6 100644 --- a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs +++ b/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 `.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 { + 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 { ) })?; - 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 { // 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; }; @@ -1076,12 +1080,8 @@ fn collect_unit_changes( // 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()); + unit_dropin_glob(¤t_unit_file)?.filter_map(|v| v.ok()); + let mut new_dropins = unit_dropin_glob(&new_unit_file)?.filter_map(|v| v.ok()); // When the unit is disabled, the override files will be a symlink to /dev/null instead. dropins_removed = From 959218abd6adaa06b80e9adde009ca71ef16694b Mon Sep 17 00:00:00 2001 From: r-vdp Date: Thu, 30 Apr 2026 09:58:22 +0200 Subject: [PATCH 3/5] switch-to-configuration-ng: take &Path in is_unit_disabled canonicalize() only needs &self, so there is no reason to take ownership of the PathBuf. Accepting impl AsRef lets the glob iterator call sites keep passing owned PathBufs to .all() while the direct call on new_base_unit_file no longer needs to clone. --- pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs index 4d243693eaa6..e125a67a29f7 100644 --- a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs +++ b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs @@ -1003,8 +1003,9 @@ fn remove_file_if_exists(p: impl AsRef) -> std::io::Result<()> { } /// Checks if a unit has been disabled in configuration -fn is_unit_disabled(unit_file: PathBuf) -> bool { +fn is_unit_disabled(unit_file: impl AsRef) -> bool { unit_file + .as_ref() .canonicalize() .map(|full_path| full_path == Path::new("/dev/null")) .unwrap_or(true) @@ -1105,7 +1106,7 @@ fn collect_unit_changes( { // 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()) { + if dropins_removed || is_unit_disabled(&new_base_unit_file) { let current_unit_info = parse_unit(¤t_unit_file, ¤t_base_unit_file)?; if parse_systemd_bool(Some(¤t_unit_info), "Unit", "X-StopOnRemoval", true) { _ = units_to_stop.insert(unit.to_string(), ()); From b12732fb6ca5b4a08046984b612f948a68db4a2a Mon Sep 17 00:00:00 2001 From: r-vdp Date: Thu, 30 Apr 2026 10:58:22 +0200 Subject: [PATCH 4/5] switch-to-configuration-ng: fix dropins_removed comments --- pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs index e125a67a29f7..63867a9b9322 100644 --- a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs +++ b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs @@ -1086,9 +1086,11 @@ fn collect_unit_changes( // 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) + // True iff at least one current drop-in is not /dev/null, + // i.e. the instance was NixOS-managed in the old generation. !current_dropins.all(is_unit_disabled) - // True if either no new files or all disabled new files (unit gone) + // True iff there are no new drop-ins, or all of them are + // /dev/null, i.e. the instance is gone in the new generation. && new_dropins.all(is_unit_disabled); } } From 99b70cdb1dbd54af799d2839f0081e7bdfd3fede Mon Sep 17 00:00:00 2001 From: r-vdp Date: Thu, 30 Apr 2026 11:27:30 +0200 Subject: [PATCH 5/5] switch-to-configuration-ng: replace is_unit_disabled with UnitFileState is_unit_disabled mapped any canonicalize() error to "disabled" via unwrap_or(true), conflating ENOENT with transient I/O failures. For new_dropins.all(is_unit_disabled) that meant a transient error on a still-present drop-in could read as "all masked" and stop a unit that is still configured. Replace the boolean helper with a tri-state UnitFileState (Present / Masked / Missing) returned from unit_file_state(). Only ENOENT maps to Missing; other errors are propagated so callers cannot accidentally pick the wrong default. --- .../sw/switch-to-configuration-ng/src/main.rs | 75 ++++++++++++++----- 1 file changed, 55 insertions(+), 20 deletions(-) diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs index 63867a9b9322..af3771561055 100644 --- a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs +++ b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs @@ -1002,13 +1002,35 @@ fn remove_file_if_exists(p: impl AsRef) -> std::io::Result<()> { } } -/// Checks if a unit has been disabled in configuration -fn is_unit_disabled(unit_file: impl AsRef) -> bool { - unit_file - .as_ref() - .canonicalize() - .map(|full_path| full_path == Path::new("/dev/null")) - .unwrap_or(true) +#[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) -> Result { + 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 @@ -1079,19 +1101,32 @@ fn collect_unit_changes( 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 = - unit_dropin_glob(¤t_unit_file)?.filter_map(|v| v.ok()); - let mut new_dropins = unit_dropin_glob(&new_unit_file)?.filter_map(|v| v.ok()); + // Handle instances defined as drop-ins. When the unit is + // disabled, the override files will be a symlink to + // /dev/null instead. - // When the unit is disabled, the override files will be a symlink to /dev/null instead. - dropins_removed = - // True iff at least one current drop-in is not /dev/null, - // i.e. the instance was NixOS-managed in the old generation. - !current_dropins.all(is_unit_disabled) - // True iff there are no new drop-ins, or all of them are - // /dev/null, i.e. the instance is gone in the new generation. - && new_dropins.all(is_unit_disabled); + // 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(¤t_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; + } + } + } } } @@ -1108,7 +1143,7 @@ fn collect_unit_changes( { // 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) { + if dropins_removed || unit_file_state(&new_base_unit_file)?.is_gone() { let current_unit_info = parse_unit(¤t_unit_file, ¤t_base_unit_file)?; if parse_systemd_bool(Some(¤t_unit_info), "Unit", "X-StopOnRemoval", true) { _ = units_to_stop.insert(unit.to_string(), ());