From 76c8d4509934c8d32a8203a62c36df5aadf7de43 Mon Sep 17 00:00:00 2001 From: r-vdp Date: Wed, 20 May 2026 19:40:45 +0200 Subject: [PATCH] switch-to-configuration-ng: honour X-* directives in user-unit migration pass The post-activation pass added in 5cc82c4922f8 to handle units migrating from a per-user manager (home-manager) to NixOS unconditionally restarts or starts any candidate. dbus-broker.service explicitly opts out of restarts via reloadIfChanged because restarting the session bus kills running clients; the second pass ignored that and restarted it anyway. Apply the same X-ReloadIfChanged / X-RestartIfChanged / RefuseManualStop / RefuseManualStart / X-OnlyManualStart checks that handle_modified_unit performs, so a migrated unit is reloaded, skipped, restarted or started as its directives require. Covered by new switch-test specialisations for reloadIfChanged and restartIfChanged = false. --- nixos/tests/switch-test.nix | 39 ++++ .../sw/switch-to-configuration-ng/src/main.rs | 216 ++++++++++++++++-- 2 files changed, 238 insertions(+), 17 deletions(-) diff --git a/nixos/tests/switch-test.nix b/nixos/tests/switch-test.nix index 480d284ada1d..e83e43430fae 100644 --- a/nixos/tests/switch-test.nix +++ b/nixos/tests/switch-test.nix @@ -739,6 +739,22 @@ in ''; }; + # As above, but with reloadIfChanged: pass 2 must reload, not + # restart. + userServiceMigratedToNixosReloadOnly.configuration = { + imports = [ userServiceMigratedToNixosNoStop.configuration ]; + systemd.user.services.migrated = { + reloadIfChanged = true; + serviceConfig.ExecReload = "${pkgs.coreutils}/bin/true"; + }; + }; + + # As above, but with restartIfChanged = false: pass 2 must skip it. + userServiceMigratedToNixosNoRestart.configuration = { + imports = [ userServiceMigratedToNixosNoStop.configuration ]; + systemd.user.services.migrated.restartIfChanged = false; + }; + no_inhibitors.configuration.system.switch.inhibitors = lib.mkForce { }; inhibitors.configuration.system.switch.inhibitors = lib.mkForce { @@ -1818,6 +1834,29 @@ in out = machine.succeed(f"sudo -u usertest {user_env} cat /run/user/1001/migrated-owner") assert_contains(out, "nixos") + # Pass 2 must honour reloadIfChanged. + switch_to_specialisation("${machine}", "") + machine.fail(f"sudo -u usertest {user_env} systemctl --user is-active migrated.service") + seed_home_unit() + out = switch_to_specialisation("${machine}", "userServiceMigratedToNixosReloadOnly") + assert_lacks(out, "restarting (post-activation) the following user units: migrated.service") + assert_contains(out, "reloading (post-activation) the following user units: migrated.service") + user_systemctl("is-active migrated.service") + # Reloaded only, so the home ExecStart never re-ran. + out = machine.succeed(f"sudo -u usertest {user_env} cat /run/user/1001/migrated-owner") + assert_contains(out, "home") + + # Pass 2 must honour restartIfChanged = false. + switch_to_specialisation("${machine}", "") + machine.fail(f"sudo -u usertest {user_env} systemctl --user is-active migrated.service") + seed_home_unit() + out = switch_to_specialisation("${machine}", "userServiceMigratedToNixosNoRestart") + assert_lacks(out, "\nrestarting (post-activation) the following user units: migrated.service") + assert_contains(out, "NOT restarting (post-activation) the following user units: migrated.service") + user_systemctl("is-active migrated.service") + out = machine.succeed(f"sudo -u usertest {user_env} cat /run/user/1001/migrated-owner") + assert_contains(out, "home") + # Units that remain shadowed by ~/.config must be left alone in both # passes even though /etc now also defines them. switch_to_specialisation("${machine}", "") 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 d9568c7b5464..6046d3437ae0 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 @@ -688,12 +688,7 @@ fn handle_modified_unit( let reload_list = scope.reload_list_file(); let use_restart_as_stop_and_start = new_unit_info.is_none(); - if matches!( - unit, - "sysinit.target" | "basic.target" | "multi-user.target" | "graphical.target" - ) || unit.ends_with(".unit") - || unit.ends_with(".slice") - { + if cannot_be_restarted_directly(unit) { // Do nothing. These cannot be restarted directly. // Slices and Paths don't have to be restarted since properties (resource limits and @@ -940,6 +935,17 @@ fn parse_fstab(fstab: impl BufRead) -> (HashMap, HashMap bool { + matches!( + unit, + "sysinit.target" | "basic.target" | "multi-user.target" | "graphical.target" + ) || unit.ends_with(".path") + || unit.ends_with(".slice") +} + // Returns a HashMap containing the same contents as the passed in `units`, minus the units in // `units_to_filter`. fn filter_units( @@ -957,6 +963,50 @@ fn filter_units( res } +/// Action to take on a unit that migrated to NixOS ownership during the +/// post-activation pass. Honours the same X-* directives as +/// `handle_modified_unit`. +#[derive(Debug, PartialEq)] +enum MigrationAction { + Skip, + Reload, + Restart, + Start, +} + +impl MigrationAction { + /// Action to take on a migrated unit that is still active. + fn for_active_unit(unit: &str, new_unit_info: &UnitInfo) -> Self { + if cannot_be_restarted_directly(unit) { + return Self::Skip; + } + + if parse_systemd_bool(Some(new_unit_info), "Service", "X-ReloadIfChanged", false) { + return Self::Reload; + } + + if !parse_systemd_bool(Some(new_unit_info), "Service", "X-RestartIfChanged", true) + || parse_systemd_bool(Some(new_unit_info), "Unit", "RefuseManualStop", false) + || parse_systemd_bool(Some(new_unit_info), "Unit", "X-OnlyManualStart", false) + { + return Self::Skip; + } + + Self::Restart + } + + /// Action to take on a migrated unit that the previous owner stopped. + fn for_stopped_unit(new_unit_info: &UnitInfo) -> Self { + if parse_systemd_bool(Some(new_unit_info), "Unit", "RefuseManualStart", false) + || parse_systemd_bool(Some(new_unit_info), "Unit", "X-OnlyManualStart", false) + { + return Self::Skip; + } + + Self::Start + } +} + fn unit_is_active(conn: &LocalConnection, unit: &str) -> Result { let unit_object_path = conn .with_proxy( @@ -1510,29 +1560,40 @@ fn do_user_switch(parent_exe: String) -> anyhow::Result<()> { let active_after = get_active_units(&systemd)?; + let mut to_reload = HashMap::new(); let mut to_restart = HashMap::new(); let mut to_start = HashMap::new(); + let mut to_skip = HashMap::new(); for unit in &migration_candidates { - match active_after.get(unit) { + // Honour X-* directives so reloadIfChanged/restartIfChanged hold. + let new_unit_file = new_unit_dir.join(unit); + let new_unit_info = parse_unit(&new_unit_file, &new_unit_file)?; + + let action = match active_after.get(unit) { Some(unit_state) => { + // Only act if /etc now wins (i.e. the higher-priority + // copy is gone). Read errors are treated as "leave alone". let now_etc = unit_state .proxy .get("org.freedesktop.systemd1.Unit", "FragmentPath") .map(|p: String| p.starts_with(fragment_prefix)) .unwrap_or(false); - if now_etc { - // Still running with the previous manager's binary; - // restart so the /etc definition takes effect. - to_restart.insert(unit.clone(), ()); + if !now_etc { + // Still shadowed (or read error); leave it alone. + continue; } - // else: still shadowed by ~/.config, leave it alone. + MigrationAction::for_active_unit(unit, &new_unit_info) } - None => { - // Stopped by the previous manager; start the /etc copy. - to_start.insert(unit.clone(), ()); - } - } + // Stopped by the previous manager; start the /etc copy. + None => MigrationAction::for_stopped_unit(&new_unit_info), + }; + match action { + MigrationAction::Skip => to_skip.insert(unit.clone(), ()), + MigrationAction::Reload => to_reload.insert(unit.clone(), ()), + MigrationAction::Restart => to_restart.insert(unit.clone(), ()), + MigrationAction::Start => to_start.insert(unit.clone(), ()), + }; } // Re-start active targets so any other newly-unmasked dependencies are @@ -1543,6 +1604,24 @@ fn do_user_switch(parent_exe: String) -> anyhow::Result<()> { } } + if !to_skip.is_empty() { + print_units("NOT restarting (post-activation)", &to_skip); + } + + print_units("reloading (post-activation)", &to_reload); + for unit in to_reload.keys() { + match systemd.reload_unit(unit, "replace") { + Ok(job_path) => { + submitted_jobs.borrow_mut().insert(job_path, Job::Reload); + } + Err(err) => { + eprintln!("Failed to reload user unit {unit}: {err}"); + exit_code = 4; + } + } + } + block_on_jobs(&dbus_conn, &submitted_jobs); + print_units("restarting (post-activation)", &to_restart); for unit in to_restart.keys() { match systemd.restart_unit(unit, "replace") { @@ -2863,4 +2942,107 @@ After=dev-disk-by\x2dlabel-root.device ); } } + + fn unit_info( + sections: &[(&str, &[(&str, &str)])], + ) -> HashMap>> { + sections + .iter() + .map(|(section, kvs)| { + ( + section.to_string(), + kvs.iter() + .map(|(k, v)| (k.to_string(), vec![v.to_string()])) + .collect(), + ) + }) + .collect() + } + + #[test] + fn migration_action_for_active_unit() { + use super::MigrationAction; + + // Plain service: restart. + assert_eq!( + MigrationAction::for_active_unit("foo.service", &unit_info(&[])), + MigrationAction::Restart + ); + + // reloadIfChanged must reload, not restart. + assert_eq!( + MigrationAction::for_active_unit( + "foo.service", + &unit_info(&[("Service", &[("X-ReloadIfChanged", "true")])]) + ), + MigrationAction::Reload + ); + + // X-RestartIfChanged=false (restartIfChanged = false) must skip. + assert_eq!( + MigrationAction::for_active_unit( + "foo.service", + &unit_info(&[("Service", &[("X-RestartIfChanged", "false")])]) + ), + MigrationAction::Skip + ); + + // RefuseManualStop must skip. + assert_eq!( + MigrationAction::for_active_unit( + "foo.service", + &unit_info(&[("Unit", &[("RefuseManualStop", "yes")])]) + ), + MigrationAction::Skip + ); + + // X-OnlyManualStart must skip. + assert_eq!( + MigrationAction::for_active_unit( + "foo.service", + &unit_info(&[("Unit", &[("X-OnlyManualStart", "yes")])]) + ), + MigrationAction::Skip + ); + + // Units that cannot be restarted directly must skip. + for unit in [ + "sysinit.target", + "basic.target", + "multi-user.target", + "graphical.target", + "foo.path", + "bar.slice", + ] { + assert_eq!( + MigrationAction::for_active_unit(unit, &unit_info(&[])), + MigrationAction::Skip, + "{unit}" + ); + } + } + + #[test] + fn migration_action_for_stopped_unit() { + use super::MigrationAction; + + assert_eq!( + MigrationAction::for_stopped_unit(&unit_info(&[])), + MigrationAction::Start + ); + assert_eq!( + MigrationAction::for_stopped_unit(&unit_info(&[( + "Unit", + &[("RefuseManualStart", "true")] + )])), + MigrationAction::Skip + ); + assert_eq!( + MigrationAction::for_stopped_unit(&unit_info(&[( + "Unit", + &[("X-OnlyManualStart", "true")] + )])), + MigrationAction::Skip + ); + } }