From 385085d8f01673335b2be2d127098077c8d53b06 Mon Sep 17 00:00:00 2001 From: Arnout Engelen Date: Tue, 18 Nov 2025 19:46:12 +0100 Subject: [PATCH 1/2] nixos-rebuild-ng: use remote journalctl instead of pipe for logs Notably, `--pipe` would cause `nixos-rebuild-ng` to panic when stderr closes. That's especially painful when there's changes to the systemd unit that manages the network connection, since that wouldn't come back up. This materially improves the reliability of 'nixos-rebuild switch' and while it adds a bit of complexity it doesn't seem too bad. NixOS test coverage of nixos-rebuild is also pretty OK so that gives confidence it won't break too much, except for use cases that rely on passing information from the 'deployer' to the 'target' machine via stdin, which seems like an obscure pattern. Fixes #462179 --- .../nixos-rebuild-ng/src/nixos_rebuild/nix.py | 116 +++++++-- .../src/nixos_rebuild/process.py | 78 +++++- .../nixos-rebuild-ng/src/tests/test_main.py | 103 ++++++-- .../ni/nixos-rebuild-ng/src/tests/test_nix.py | 230 ++++++++++-------- 4 files changed, 369 insertions(+), 158 deletions(-) diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py index b1da9ef555dc..6598c69f259e 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/nix.py @@ -25,12 +25,14 @@ from .models import ( Profile, Remote, ) -from .process import PRESERVE_ENV, SSH_DEFAULT_OPTS, run_wrapper +from .process import PRESERVE_ENV, SSH_DEFAULT_OPTS, run_wrapper, run_wrapper_bg +from .process import Args as ProcessArgs from .utils import Args, dict_to_flags FLAKE_FLAGS: Final = ["--extra-experimental-features", "nix-command flakes"] FLAKE_REPL_TEMPLATE: Final = "repl.nix.template" -SWITCH_TO_CONFIGURATION_CMD_PREFIX: Final = [ +SYSTEMD_RUN_UNIT_PREFIX: Final = "nixos-rebuild-switch-to-configuration" +SYSTEMD_RUN_CMD_PREFIX: Final = [ "systemd-run", "-E", # Will be set to new value early in switch-to-configuration script, @@ -41,11 +43,10 @@ SWITCH_TO_CONFIGURATION_CMD_PREFIX: Final = [ "-E", "NIXOS_NO_CHECK", "--collect", + "--wait", "--no-ask-password", - "--pipe", "--quiet", "--service-type=exec", - "--unit=nixos-rebuild-switch-to-configuration", ] logger: Final = logging.getLogger(__name__) @@ -663,6 +664,78 @@ def set_profile( ) +def _has_systemd(target_host: Remote | None) -> bool: + r = run_wrapper( + ["test", "-d", "/run/systemd/system"], + remote=target_host, + check=False, + ) + return r.returncode == 0 + + +def _run_action( + action: Literal[Action.SWITCH, Action.BOOT, Action.TEST, Action.DRY_ACTIVATE], + path_to_config: Path, + install_bootloader: bool, + target_host: Remote | None, + sudo: bool, + prefix: ProcessArgs | None = None, +) -> None: + cmd: ProcessArgs = [path_to_config / "bin/switch-to-configuration", str(action)] + if prefix: + cmd = [*prefix, *cmd] + + run_wrapper( + cmd, + env={ + "LOCALE_ARCHIVE": PRESERVE_ENV, + "NIXOS_NO_CHECK": PRESERVE_ENV, + "NIXOS_INSTALL_BOOTLOADER": "1" if install_bootloader else "0", + }, + remote=target_host, + sudo=sudo, + ) + + +def _run_action_with_systemd( + action: Literal[Action.SWITCH, Action.BOOT, Action.TEST, Action.DRY_ACTIVATE], + path_to_config: Path, + install_bootloader: bool, + target_host: Remote | None, + sudo: bool, +) -> None: + unique_unit_name = SYSTEMD_RUN_UNIT_PREFIX + "-" + uuid.uuid4().hex[:8] + journalctl = run_wrapper_bg( + [ + "journalctl", + "-f", + f"--unit={unique_unit_name}", + "--output=cat", + ], + remote=target_host, + sudo=sudo, + ) + + try: + _run_action( + action, + path_to_config, + install_bootloader, + target_host, + sudo, + prefix=[*SYSTEMD_RUN_CMD_PREFIX, f"--unit={unique_unit_name}"], + ) + except KeyboardInterrupt: + run_wrapper( + ["systemctl", "stop", unique_unit_name], + remote=target_host, + sudo=sudo, + ) + raise + finally: + journalctl.terminate() + + def switch_to_configuration( path_to_config: Path, action: Literal[Action.SWITCH, Action.BOOT, Action.TEST, Action.DRY_ACTIVATE], @@ -686,29 +759,26 @@ def switch_to_configuration( if not path_to_config.exists(): raise NixOSRebuildError(f"specialisation not found: {specialisation}") - r = run_wrapper( - ["test", "-d", "/run/systemd/system"], - remote=target_host, - check=False, - ) - cmd = SWITCH_TO_CONFIGURATION_CMD_PREFIX - if r.returncode: + if _has_systemd(target_host): + _run_action_with_systemd( + action, + path_to_config, + install_bootloader, + target_host, + sudo, + ) + else: logger.debug( "skipping systemd-run to switch configuration since systemd is " "not working in target host" ) - cmd = [] - - run_wrapper( - [*cmd, path_to_config / "bin/switch-to-configuration", str(action)], - env={ - "LOCALE_ARCHIVE": PRESERVE_ENV, - "NIXOS_NO_CHECK": PRESERVE_ENV, - "NIXOS_INSTALL_BOOTLOADER": "1" if install_bootloader else "0", - }, - remote=target_host, - sudo=sudo, - ) + _run_action( + action, + path_to_config, + install_bootloader, + target_host, + sudo, + ) def upgrade_channels(all_channels: bool = False, sudo: bool = False) -> None: diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py index ac83d92039f7..aa32a20a7fac 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/nixos_rebuild/process.py @@ -9,7 +9,7 @@ from collections.abc import Mapping, Sequence from dataclasses import dataclass from enum import Enum from ipaddress import AddressValueError, IPv6Address -from typing import Final, Literal, Self, TextIO, TypedDict, Unpack, override +from typing import Final, Literal, NamedTuple, Self, TextIO, TypedDict, Unpack, override from . import tmpdir @@ -117,16 +117,18 @@ def cleanup_ssh() -> None: atexit.register(cleanup_ssh) -def run_wrapper( +class _RunParams(NamedTuple): + args: Args + popen_env: dict[str, str] | None + process_input: str | None + + +def _build_run_params( args: Args, - *, - check: bool = True, - env: Mapping[str, EnvValue] | None = None, - remote: Remote | None = None, - sudo: bool = False, - **kwargs: Unpack[RunKwargs], -) -> subprocess.CompletedProcess[str]: - "Wrapper around `subprocess.run` that supports extra functionality." + env: Mapping[str, EnvValue] | None, + remote: Remote | None, + sudo: bool, +) -> _RunParams: process_input = None run_args: list[Arg] = list(args) final_args: list[Arg] @@ -188,9 +190,63 @@ def run_wrapper( final_args = run_args popen_env = None if env is None else resolved_env + return _RunParams(final_args, popen_env, process_input) + + +def run_wrapper_bg( + args: Args, + env: Mapping[str, EnvValue] | None = None, + remote: Remote | None = None, + sudo: bool = False, +) -> subprocess.Popen[str]: + "Wrapper around `subprocess.Popen` that supports extra functionality." + (final_args, popen_env, process_input) = _build_run_params(args, env, remote, sudo) + + logger.debug( + "calling Popen with args=%r", + _sanitize_env_run_args(list(final_args)), + ) + + if process_input: + stdin = subprocess.PIPE + else: + stdin = None + + r = subprocess.Popen( + final_args, + env=popen_env, + stdin=stdin, + # Hope nobody is using NixOS with non-UTF8 encodings, but + # "surrogateescape" should still work in those systems. + text=True, + errors="surrogateescape", + ) + + if r.stdin and process_input: + r.stdin.write(process_input) + r.stdin.write("\n") + r.stdin.close() + + return r + + +def run_wrapper( + args: Args, + *, + check: bool = True, + env: Mapping[str, EnvValue] | None = None, + remote: Remote | None = None, + sudo: bool = False, + **kwargs: Unpack[RunKwargs], +) -> subprocess.CompletedProcess[str]: + "Wrapper around `subprocess.run` that supports extra functionality." + (final_args, popen_env, process_input) = _build_run_params( + list(args), env, remote, sudo + ) + logger.debug( "calling run with args=%r, kwargs=%r, env=%r", - _sanitize_env_run_args(remote_run_args if remote else run_args), + _sanitize_env_run_args(list(final_args)), kwargs, env, ) diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_main.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_main.py index 7f51c09d7497..986f6883a42d 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_main.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_main.py @@ -166,8 +166,15 @@ def test_parse_args() -> None: {"NIXOS_REBUILD_I_UNDERSTAND_THE_CONSEQUENCES_PLEASE_BREAK_MY_SYSTEM": "1"}, clear=True, ) +@patch("uuid.uuid4") @patch("subprocess.run", autospec=True) -def test_execute_nix_boot(mock_run: Mock, tmp_path: Path) -> None: +@patch("subprocess.Popen") +def test_execute_nix_boot( + mock_popen: Mock, mock_run: Mock, mock_uuid4: Mock, tmp_path: Path +) -> None: + test_uuid = uuid.UUID("58fe9784-f60a-42bc-aa94-eb8f1a7e5c17") + mock_uuid4.return_value = test_uuid + nixpkgs_path = tmp_path / "nixpkgs" (nixpkgs_path / ".git").mkdir(parents=True) config_path = tmp_path / "test" @@ -238,7 +245,8 @@ def test_execute_nix_boot(mock_run: Mock, tmp_path: Path) -> None: ), call( [ - *nr.nix.SWITCH_TO_CONFIGURATION_CMD_PREFIX, + *nr.nix.SYSTEMD_RUN_CMD_PREFIX, + f"--unit={nr.nix.SYSTEMD_RUN_UNIT_PREFIX}-{test_uuid.hex[:8]}", config_path / "bin/switch-to-configuration", "boot", ], @@ -259,7 +267,8 @@ def test_execute_nix_boot(mock_run: Mock, tmp_path: Path) -> None: # https://github.com/NixOS/nixpkgs/issues/437872 @patch.dict(os.environ, {}, clear=True) @patch("subprocess.run", autospec=True) -def test_execute_nix_build(mock_run: Mock, tmp_path: Path) -> None: +@patch("subprocess.Popen") +def test_execute_nix_build(mock_popen: Mock, mock_run: Mock, tmp_path: Path) -> None: config_path = tmp_path / "test" config_path.touch() @@ -301,7 +310,8 @@ def test_execute_nix_build(mock_run: Mock, tmp_path: Path) -> None: @patch.dict(os.environ, {}, clear=True) @patch("subprocess.run", autospec=True) -def test_execute_nix_build_vm(mock_run: Mock, tmp_path: Path) -> None: +@patch("subprocess.Popen") +def test_execute_nix_build_vm(mock_popen: Mock, mock_run: Mock, tmp_path: Path) -> None: config_path = tmp_path / "test" config_path.touch() @@ -350,7 +360,10 @@ def test_execute_nix_build_vm(mock_run: Mock, tmp_path: Path) -> None: @patch.dict(os.environ, {}, clear=True) @patch("subprocess.run", autospec=True) -def test_execute_nix_build_image_flake(mock_run: Mock, tmp_path: Path) -> None: +@patch("subprocess.Popen") +def test_execute_nix_build_image_flake( + mock_popen: Mock, mock_run: Mock, tmp_path: Path +) -> None: config_path = tmp_path / "test" config_path.touch() @@ -432,11 +445,18 @@ def test_execute_nix_build_image_flake(mock_run: Mock, tmp_path: Path) -> None: {"NIXOS_REBUILD_I_UNDERSTAND_THE_CONSEQUENCES_PLEASE_BREAK_MY_SYSTEM": "1"}, clear=True, ) +@patch("uuid.uuid4") @patch("subprocess.run", autospec=True) -def test_execute_nix_switch_flake(mock_run: Mock, tmp_path: Path) -> None: +@patch("subprocess.Popen") +def test_execute_nix_switch_flake( + mock_popen: Mock, mock_run: Mock, mock_uuid4: Mock, tmp_path: Path +) -> None: config_path = tmp_path / "test" config_path.touch() + test_uuid = uuid.UUID("58fe9784-f60a-42bc-aa94-eb8f1a7e5c17") + mock_uuid4.return_value = test_uuid + def run_side_effect(args: list[str], **kwargs: Any) -> CompletedProcess[str]: if args[0] == "nix": return CompletedProcess([], 0, str(config_path)) @@ -506,7 +526,8 @@ def test_execute_nix_switch_flake(mock_run: Mock, tmp_path: Path) -> None: "env", "-i", "NIXOS_INSTALL_BOOTLOADER=1", - *nr.nix.SWITCH_TO_CONFIGURATION_CMD_PREFIX, + *nr.nix.SYSTEMD_RUN_CMD_PREFIX, + f"--unit={nr.nix.SYSTEMD_RUN_UNIT_PREFIX}-{test_uuid.hex[:8]}", config_path / "bin/switch-to-configuration", "switch", ], @@ -523,11 +544,13 @@ def test_execute_nix_switch_flake(mock_run: Mock, tmp_path: Path) -> None: clear=True, ) @patch("subprocess.run", autospec=True) +@patch("subprocess.Popen") @patch("uuid.uuid4", autospec=True) @patch(get_qualified_name(nr.services.cleanup_ssh), autospec=True) def test_execute_nix_switch_build_target_host( mock_cleanup_ssh: Mock, mock_uuid4: Mock, + mock_popen: Mock, mock_run: Mock, tmp_path: Path, ) -> None: @@ -551,7 +574,8 @@ def test_execute_nix_switch_build_target_host( return CompletedProcess([], 0) mock_run.side_effect = run_side_effect - mock_uuid4.return_value = uuid.UUID(int=0) + test_uuid = uuid.UUID("58fe9784-f60a-42bc-aa94-eb8f1a7e5c17") + mock_uuid4.side_effect = [uuid.UUID(int=0), uuid.UUID(int=1), test_uuid] nr.execute( [ @@ -644,7 +668,7 @@ def test_execute_nix_switch_build_target_host( "--realise", str(config_path), "--add-root", - "/tmp/tmpdir/00000000000000000000000000000000", + "/tmp/tmpdir/00000000000000000000000000000001", ], check=True, stdout=PIPE, @@ -748,7 +772,8 @@ def test_execute_nix_switch_build_target_host( "-c", """'exec env -i PATH="${PATH-}" LOCALE_ARCHIVE="${LOCALE_ARCHIVE-}" NIXOS_NO_CHECK="${NIXOS_NO_CHECK-}" NIXOS_INSTALL_BOOTLOADER=0 "$@"'""", "sh", - *nr.nix.SWITCH_TO_CONFIGURATION_CMD_PREFIX, + *nr.nix.SYSTEMD_RUN_CMD_PREFIX, + f"--unit={nr.nix.SYSTEMD_RUN_UNIT_PREFIX}-{test_uuid.hex[:8]}", str(config_path / "bin/switch-to-configuration"), "switch", ], @@ -764,13 +789,20 @@ def test_execute_nix_switch_build_target_host( {"NIXOS_REBUILD_I_UNDERSTAND_THE_CONSEQUENCES_PLEASE_BREAK_MY_SYSTEM": "1"}, clear=True, ) +@patch("uuid.uuid4") @patch("subprocess.run", autospec=True) +@patch("subprocess.Popen") @patch(get_qualified_name(nr.services.cleanup_ssh), autospec=True) def test_execute_nix_switch_flake_target_host( mock_cleanup_ssh: Mock, + mock_popen: Mock, mock_run: Mock, + mock_uuid4: Mock, tmp_path: Path, ) -> None: + test_uuid = uuid.UUID("58fe9784-f60a-42bc-aa94-eb8f1a7e5c17") + mock_uuid4.return_value = test_uuid + config_path = tmp_path / "test" config_path.touch() @@ -865,7 +897,8 @@ def test_execute_nix_switch_flake_target_host( "-c", """'exec env -i PATH="${PATH-}" LOCALE_ARCHIVE="${LOCALE_ARCHIVE-}" NIXOS_NO_CHECK="${NIXOS_NO_CHECK-}" NIXOS_INSTALL_BOOTLOADER=0 "$@"'""", "sh", - *nr.nix.SWITCH_TO_CONFIGURATION_CMD_PREFIX, + *nr.nix.SYSTEMD_RUN_CMD_PREFIX, + f"--unit={nr.nix.SYSTEMD_RUN_UNIT_PREFIX}-{test_uuid.hex[:8]}", str(config_path / "bin/switch-to-configuration"), "switch", ], @@ -881,13 +914,20 @@ def test_execute_nix_switch_flake_target_host( {"NIXOS_REBUILD_I_UNDERSTAND_THE_CONSEQUENCES_PLEASE_BREAK_MY_SYSTEM": "1"}, clear=True, ) +@patch("uuid.uuid4") @patch("subprocess.run", autospec=True) +@patch("subprocess.Popen") @patch(get_qualified_name(nr.services.cleanup_ssh), autospec=True) def test_execute_nix_switch_flake_build_host( mock_cleanup_ssh: Mock, + mock_popen: Mock, mock_run: Mock, + mock_uuid4: Mock, tmp_path: Path, ) -> None: + test_uuid = uuid.UUID("58fe9784-f60a-42bc-aa94-eb8f1a7e5c17") + mock_uuid4.return_value = test_uuid + config_path = tmp_path / "test" config_path.touch() @@ -984,7 +1024,8 @@ def test_execute_nix_switch_flake_build_host( ), call( [ - *nr.nix.SWITCH_TO_CONFIGURATION_CMD_PREFIX, + *nr.nix.SYSTEMD_RUN_CMD_PREFIX, + f"--unit={nr.nix.SYSTEMD_RUN_UNIT_PREFIX}-{test_uuid.hex[:8]}", config_path / "bin/switch-to-configuration", "switch", ], @@ -996,7 +1037,10 @@ def test_execute_nix_switch_flake_build_host( @patch("subprocess.run", autospec=True) -def test_execute_switch_rollback(mock_run: Mock, tmp_path: Path) -> None: +@patch("subprocess.Popen") +def test_execute_switch_rollback( + mock_popen: Mock, mock_run: Mock, tmp_path: Path +) -> None: nixpkgs_path = tmp_path / "nixpkgs" (nixpkgs_path / ".git").mkdir(parents=True) @@ -1073,7 +1117,8 @@ def test_execute_switch_rollback(mock_run: Mock, tmp_path: Path) -> None: @patch("subprocess.run", autospec=True) -def test_execute_build(mock_run: Mock, tmp_path: Path) -> None: +@patch("subprocess.Popen") +def test_execute_build(mock_popen: Mock, mock_run: Mock, tmp_path: Path) -> None: config_path = tmp_path / "test" config_path.touch() mock_run.side_effect = [ @@ -1102,8 +1147,9 @@ def test_execute_build(mock_run: Mock, tmp_path: Path) -> None: @patch("subprocess.run", autospec=True) +@patch("subprocess.Popen") def test_execute_build_dry_run_build_and_target_remote( - mock_run: Mock, tmp_path: Path + mock_popen: Mock, mock_run: Mock, tmp_path: Path ) -> None: config_path = tmp_path / "test" config_path.touch() @@ -1174,7 +1220,8 @@ def test_execute_build_dry_run_build_and_target_remote( @patch("subprocess.run", autospec=True) -def test_execute_test_flake(mock_run: Mock, tmp_path: Path) -> None: +@patch("subprocess.Popen") +def test_execute_test_flake(mock_popen: Mock, mock_run: Mock, tmp_path: Path) -> None: config_path = tmp_path / "test" config_path.touch() @@ -1224,11 +1271,13 @@ def test_execute_test_flake(mock_run: Mock, tmp_path: Path) -> None: @patch("subprocess.run", autospec=True) +@patch("subprocess.Popen") @patch("pathlib.Path.exists", autospec=True, return_value=True) @patch("pathlib.Path.mkdir", autospec=True) def test_execute_test_rollback( mock_path_mkdir: Mock, mock_path_exists: Mock, + mock_popen: Mock, mock_run: Mock, ) -> None: def run_side_effect(args: list[str], **kwargs: Any) -> CompletedProcess[str]: @@ -1291,8 +1340,15 @@ def test_execute_test_rollback( {"NIXOS_REBUILD_I_UNDERSTAND_THE_CONSEQUENCES_PLEASE_BREAK_MY_SYSTEM": "1"}, clear=True, ) +@patch("uuid.uuid4", autospec=True) @patch("subprocess.run", autospec=True) -def test_execute_switch_store_path(mock_run: Mock, tmp_path: Path) -> None: +@patch("subprocess.Popen") +def test_execute_switch_store_path( + mock_popen: Mock, mock_run: Mock, mock_uuid4: Mock, tmp_path: Path +) -> None: + test_uuid = uuid.UUID("58fe9784-f60a-42bc-aa94-eb8f1a7e5c17") + mock_uuid4.return_value = test_uuid + config_path = tmp_path / "test-system" config_path.mkdir() @@ -1330,7 +1386,8 @@ def test_execute_switch_store_path(mock_run: Mock, tmp_path: Path) -> None: ), call( [ - *nr.nix.SWITCH_TO_CONFIGURATION_CMD_PREFIX, + *nr.nix.SYSTEMD_RUN_CMD_PREFIX, + f"--unit={nr.nix.SYSTEMD_RUN_UNIT_PREFIX}-{test_uuid.hex[:8]}", config_path / "bin/switch-to-configuration", "switch", ], @@ -1349,13 +1406,20 @@ def test_execute_switch_store_path(mock_run: Mock, tmp_path: Path) -> None: @patch.dict(os.environ, {}, clear=True) +@patch("uuid.uuid4") @patch("subprocess.run", autospec=True) @patch(get_qualified_name(nr.services.cleanup_ssh), autospec=True) +@patch("subprocess.Popen") def test_execute_switch_store_path_target_host( + mock_popen: Mock, mock_cleanup_ssh: Mock, mock_run: Mock, + mock_uuid4: Mock, tmp_path: Path, ) -> None: + test_uuid = uuid.UUID("58fe9784-f60a-42bc-aa94-eb8f1a7e5c17") + mock_uuid4.return_value = test_uuid + config_path = tmp_path / "test-system" config_path.mkdir() @@ -1448,7 +1512,8 @@ def test_execute_switch_store_path_target_host( "-c", """'exec env -i PATH="${PATH-}" LOCALE_ARCHIVE="${LOCALE_ARCHIVE-}" NIXOS_NO_CHECK="${NIXOS_NO_CHECK-}" NIXOS_INSTALL_BOOTLOADER=0 "$@"'""", "sh", - *nr.nix.SWITCH_TO_CONFIGURATION_CMD_PREFIX, + *nr.nix.SYSTEMD_RUN_CMD_PREFIX, + f"--unit={nr.nix.SYSTEMD_RUN_UNIT_PREFIX}-{test_uuid.hex[:8]}", str(config_path / "bin/switch-to-configuration"), "switch", ], diff --git a/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py b/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py index 8a69fd89fad7..60c6afa59c9d 100644 --- a/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py +++ b/pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_nix.py @@ -2,7 +2,7 @@ import sys import textwrap import uuid from pathlib import Path -from subprocess import PIPE, CompletedProcess +from subprocess import PIPE, CompletedProcess, Popen from typing import Any from unittest.mock import ANY, Mock, call, patch @@ -695,139 +695,159 @@ def test_set_profile(mock_run: Mock) -> None: @patch(get_qualified_name(n.run_wrapper, n), autospec=True) +@patch(get_qualified_name(n.run_wrapper_bg, n), autospec=True) def test_switch_to_configuration_without_systemd_run( - mock_run: Any, monkeypatch: MonkeyPatch + mock_run_bg: Mock, mock_run: Mock, monkeypatch: MonkeyPatch ) -> None: profile_path = Path("/path/to/profile") config_path = Path("/path/to/config") - mock_run.return_value = CompletedProcess([], 1) - with monkeypatch.context() as mp: - mp.setenv("LOCALE_ARCHIVE", "") + proc = Popen(["echo"]) + try: + mock_run_bg.return_value = p + mock_run.return_value = CompletedProcess([], 1) - n.switch_to_configuration( - profile_path, - m.Action.SWITCH, + with monkeypatch.context() as mp: + mp.setenv("LOCALE_ARCHIVE", "") + + n.switch_to_configuration( + profile_path, + m.Action.SWITCH, + sudo=False, + target_host=None, + specialisation=None, + install_bootloader=False, + ) + mock_run.assert_called_with( + [profile_path / "bin/switch-to-configuration", "switch"], + env={ + "LOCALE_ARCHIVE": p.PRESERVE_ENV, + "NIXOS_NO_CHECK": p.PRESERVE_ENV, + "NIXOS_INSTALL_BOOTLOADER": "0", + }, sudo=False, - target_host=None, - specialisation=None, - install_bootloader=False, + remote=None, ) - mock_run.assert_called_with( - [profile_path / "bin/switch-to-configuration", "switch"], - env={ - "LOCALE_ARCHIVE": p.PRESERVE_ENV, - "NIXOS_NO_CHECK": p.PRESERVE_ENV, - "NIXOS_INSTALL_BOOTLOADER": "0", - }, - sudo=False, - remote=None, - ) - with pytest.raises(m.NixOSRebuildError) as e: - n.switch_to_configuration( - config_path, - m.Action.BOOT, - sudo=False, - target_host=None, - specialisation="special", + with pytest.raises(m.NixOSRebuildError) as e: + n.switch_to_configuration( + config_path, + m.Action.BOOT, + sudo=False, + target_host=None, + specialisation="special", + ) + assert ( + str(e.value) + == "error: '--specialisation' can only be used with 'switch' and 'test'" ) - assert ( - str(e.value) - == "error: '--specialisation' can only be used with 'switch' and 'test'" - ) - target_host = m.Remote("user@localhost", [], None) - with monkeypatch.context() as mp: - mp.setenv("LOCALE_ARCHIVE", "/path/to/locale") - mp.setenv("PATH", "/path/to/bin") - mp.setattr(Path, Path.exists.__name__, lambda self: True) + target_host = m.Remote("user@localhost", [], None) + with monkeypatch.context() as mp: + mp.setenv("LOCALE_ARCHIVE", "/path/to/locale") + mp.setenv("PATH", "/path/to/bin") + mp.setattr(Path, Path.exists.__name__, lambda self: True) - n.switch_to_configuration( - Path("/path/to/config"), - m.Action.TEST, + n.switch_to_configuration( + Path("/path/to/config"), + m.Action.TEST, + sudo=True, + target_host=target_host, + install_bootloader=True, + specialisation="special", + ) + mock_run.assert_called_with( + [ + config_path / "specialisation/special/bin/switch-to-configuration", + "test", + ], + env={ + "LOCALE_ARCHIVE": p.PRESERVE_ENV, + "NIXOS_NO_CHECK": p.PRESERVE_ENV, + "NIXOS_INSTALL_BOOTLOADER": "1", + }, sudo=True, - target_host=target_host, - install_bootloader=True, - specialisation="special", + remote=target_host, ) - mock_run.assert_called_with( - [ - config_path / "specialisation/special/bin/switch-to-configuration", - "test", - ], - env={ - "LOCALE_ARCHIVE": p.PRESERVE_ENV, - "NIXOS_NO_CHECK": p.PRESERVE_ENV, - "NIXOS_INSTALL_BOOTLOADER": "1", - }, - sudo=True, - remote=target_host, - ) + finally: + proc.communicate(timeout=1) +@patch("uuid.uuid4") @patch(get_qualified_name(n.run_wrapper, n), autospec=True) +@patch(get_qualified_name(n.run_wrapper_bg, n), autospec=True) def test_switch_to_configuration_with_systemd_run( - mock_run: Mock, monkeypatch: MonkeyPatch + mock_run_bg: Mock, mock_run: Mock, mock_uuid4: Mock, monkeypatch: MonkeyPatch ) -> None: + test_uuid = uuid.UUID("58fe9784-f60a-42bc-aa94-eb8f1a7e5c17") + mock_uuid4.return_value = test_uuid + profile_path = Path("/path/to/profile") config_path = Path("/path/to/config") - mock_run.return_value = CompletedProcess([], 0) - with monkeypatch.context() as mp: - mp.setenv("LOCALE_ARCHIVE", "") + proc = Popen(["echo"]) + try: + mock_run_bg.return_value = proc + mock_run.return_value = CompletedProcess([], 0) - n.switch_to_configuration( - profile_path, - m.Action.SWITCH, + with monkeypatch.context() as mp: + mp.setenv("LOCALE_ARCHIVE", "") + + n.switch_to_configuration( + profile_path, + m.Action.SWITCH, + sudo=False, + target_host=None, + specialisation=None, + install_bootloader=False, + ) + mock_run.assert_called_with( + [ + *n.SYSTEMD_RUN_CMD_PREFIX, + f"--unit={n.SYSTEMD_RUN_UNIT_PREFIX}-{test_uuid.hex[:8]}", + profile_path / "bin/switch-to-configuration", + "switch", + ], + env={ + "LOCALE_ARCHIVE": p.PRESERVE_ENV, + "NIXOS_NO_CHECK": p.PRESERVE_ENV, + "NIXOS_INSTALL_BOOTLOADER": "0", + }, sudo=False, - target_host=None, - specialisation=None, - install_bootloader=False, + remote=None, ) - mock_run.assert_called_with( - [ - *n.SWITCH_TO_CONFIGURATION_CMD_PREFIX, - profile_path / "bin/switch-to-configuration", - "switch", - ], - env={ - "LOCALE_ARCHIVE": p.PRESERVE_ENV, - "NIXOS_NO_CHECK": p.PRESERVE_ENV, - "NIXOS_INSTALL_BOOTLOADER": "0", - }, - sudo=False, - remote=None, - ) - target_host = m.Remote("user@localhost", [], None) - with monkeypatch.context() as mp: - mp.setenv("LOCALE_ARCHIVE", "/path/to/locale") - mp.setenv("PATH", "/path/to/bin") - mp.setattr(Path, Path.exists.__name__, lambda self: True) + target_host = m.Remote("user@localhost", [], None) + with monkeypatch.context() as mp: + mp.setenv("LOCALE_ARCHIVE", "/path/to/locale") + mp.setenv("PATH", "/path/to/bin") + mp.setattr(Path, Path.exists.__name__, lambda self: True) - n.switch_to_configuration( - Path("/path/to/config"), - m.Action.TEST, + n.switch_to_configuration( + Path("/path/to/config"), + m.Action.TEST, + sudo=True, + target_host=target_host, + install_bootloader=True, + specialisation="special", + ) + mock_run.assert_called_with( + [ + *n.SYSTEMD_RUN_CMD_PREFIX, + f"--unit={n.SYSTEMD_RUN_UNIT_PREFIX}-{test_uuid.hex[:8]}", + config_path / "specialisation/special/bin/switch-to-configuration", + "test", + ], + env={ + "LOCALE_ARCHIVE": p.PRESERVE_ENV, + "NIXOS_NO_CHECK": p.PRESERVE_ENV, + "NIXOS_INSTALL_BOOTLOADER": "1", + }, sudo=True, - target_host=target_host, - install_bootloader=True, - specialisation="special", + remote=target_host, ) - mock_run.assert_called_with( - [ - *n.SWITCH_TO_CONFIGURATION_CMD_PREFIX, - config_path / "specialisation/special/bin/switch-to-configuration", - "test", - ], - env={ - "LOCALE_ARCHIVE": p.PRESERVE_ENV, - "NIXOS_NO_CHECK": p.PRESERVE_ENV, - "NIXOS_INSTALL_BOOTLOADER": "1", - }, - sudo=True, - remote=target_host, - ) + finally: + proc.communicate(timeout=1) @patch( From 15acaca6c64e3aa7328a286825f2091c82384c3b Mon Sep 17 00:00:00 2001 From: Arnout Engelen Date: Wed, 17 Dec 2025 17:42:33 +0100 Subject: [PATCH 2/2] nixos/tests/nixos-rebuild-target-host-interrupted: introduced Nixos test for the scenario that the connection breaks during the update --- nixos/tests/all-tests.nix | 3 + .../nixos-rebuild-target-host-interrupted.nix | 237 ++++++++++++++++++ 2 files changed, 240 insertions(+) create mode 100644 nixos/tests/nixos-rebuild-target-host-interrupted.nix diff --git a/nixos/tests/all-tests.nix b/nixos/tests/all-tests.nix index 34cc5e601125..623400a21787 100644 --- a/nixos/tests/all-tests.nix +++ b/nixos/tests/all-tests.nix @@ -1128,6 +1128,9 @@ in nixos-rebuild-target-host = runTest { imports = [ ./nixos-rebuild-target-host.nix ]; }; + nixos-rebuild-target-host-interrupted = runTest { + imports = [ ./nixos-rebuild-target-host-interrupted.nix ]; + }; nixpkgs = pkgs.callPackage ../modules/misc/nixpkgs/test.nix { inherit evalMinimalConfig; }; nixpkgs-config-allow-unfree = pkgs.callPackage ../modules/misc/nixpkgs/test-nixpkgs-config-allow-unfree.nix diff --git a/nixos/tests/nixos-rebuild-target-host-interrupted.nix b/nixos/tests/nixos-rebuild-target-host-interrupted.nix new file mode 100644 index 000000000000..e780019cc68d --- /dev/null +++ b/nixos/tests/nixos-rebuild-target-host-interrupted.nix @@ -0,0 +1,237 @@ +{ hostPkgs, ... }: + +# This test recreates a remote deployment scenario where the connection +# between deployer and target is closed during the deployment - in this +# case because the connection goes over a 'reverse ssh' tunnel service +# that has changes that are being deployed. + +# This is not seamless (the deployer doesn't get to see the logs after +# the disconnect), but is a lot better than the old behaviour, where +# the switch was aborted and the connection never restored. + +{ + name = "nixos-rebuild-target-host-interrupted"; + + # TODO: remove overlay from nixos/modules/profiles/installation-device.nix + # make it a _small package instead, then remove pkgsReadOnly = false;. + node.pkgsReadOnly = false; + + nodes = { + deployer = + { + nodes, + lib, + pkgs, + ... + }: + let + inherit (import ./ssh-keys.nix pkgs) snakeOilPrivateKey snakeOilPublicKey; + in + { + imports = [ + ../modules/profiles/installation-device.nix + ]; + + nix.settings = { + substituters = lib.mkForce [ ]; + hashed-mirrors = null; + connect-timeout = 1; + }; + + system.includeBuildDependencies = true; + + virtualisation = { + cores = 2; + memorySize = 3072; + }; + + services.openssh.enable = true; + users.users.root.openssh.authorizedKeys.keys = [ nodes.target.system.build.publicKey ]; + system.extraDependencies = [ + # so that it doesn't need to be built inside the test + pkgs.nixVersions.latest + ]; + + system.build.privateKey = snakeOilPrivateKey; + system.build.publicKey = snakeOilPublicKey; + system.switch.enable = true; + + services.getty.autologinUser = lib.mkForce "root"; + }; + + target = + { + nodes, + lib, + pkgs, + ... + }: + let + inherit (import ./ssh-keys.nix pkgs) snakeOilPrivateKey snakeOilPublicKey; + targetConfig = { + documentation.enable = false; + services.openssh.enable = true; + system.build.privateKey = snakeOilPrivateKey; + system.build.publicKey = snakeOilPublicKey; + + users.users.root.openssh.authorizedKeys.keys = [ nodes.deployer.system.build.publicKey ]; + users.users.alice.openssh.authorizedKeys.keys = [ nodes.deployer.system.build.publicKey ]; + users.users.bob.openssh.authorizedKeys.keys = [ nodes.deployer.system.build.publicKey ]; + + users.users.alice.extraGroups = [ "wheel" ]; + users.users.bob.extraGroups = [ "wheel" ]; + + # Disable sudo for root to ensure sudo isn't called without `--sudo` + security.sudo.extraRules = lib.mkForce [ + { + groups = [ "wheel" ]; + commands = [ { command = "ALL"; } ]; + } + { + users = [ "alice" ]; + commands = [ + { + command = "ALL"; + options = [ "NOPASSWD" ]; + } + ]; + } + ]; + + nix.settings.trusted-users = [ "@wheel" ]; + + systemd.services."autossh-ng" = { + after = [ "network.target" ]; + wantedBy = [ "multi-user.target" ]; + serviceConfig = { + User = "root"; + Restart = "always"; + RestartSec = "10s"; + ExecStart = "${pkgs.openssh}/bin/ssh -o \"ServerAliveInterval 30\" -o \"ServerAliveCountMax 3\" -o ExitOnForwardFailure=yes -N -R2222:localhost:22 deployer"; + }; + }; + + }; + in + { + imports = [ ./common/user-account.nix ]; + + config = lib.mkMerge [ + targetConfig + { + system.build = { + inherit targetConfig; + }; + system.switch.enable = true; + + networking.hostName = "target"; + } + ]; + }; + }; + + testScript = + { nodes, ... }: + let + sshConfig = builtins.toFile "ssh.conf" '' + UserKnownHostsFile=/dev/null + StrictHostKeyChecking=no + ''; + + targetConfigJSON = hostPkgs.writeText "target-configuration.json" ( + builtins.toJSON nodes.target.system.build.targetConfig + ); + + targetNetworkJSON = hostPkgs.writeText "target-network.json" ( + builtins.toJSON nodes.target.system.build.networkConfig + ); + + configFile = + hostname: + hostPkgs.writeText "configuration.nix" # nix + '' + { lib, pkgs, modulesPath, ... }: { + imports = [ + (modulesPath + "/virtualisation/qemu-vm.nix") + (modulesPath + "/testing/test-instrumentation.nix") + (modulesPath + "/../tests/common/user-account.nix") + (lib.modules.importJSON ./target-configuration.json) + (lib.modules.importJSON ./target-network.json) + ./hardware-configuration.nix + ]; + + boot.loader.grub = { + enable = true; + device = "/dev/vda"; + forceInstall = true; + }; + + # needed to make NIX_SSHOPTS work for nix-copy-closure + nix.package = pkgs.nixVersions.latest; + + # We're changing the '-E' parameter to the new hostname here, + # not because we care about the logs, but because we want to + # force the scenario where the connection is broken during the + # deployment (because the autossh-ng service is stopped and + # started): + systemd.services."autossh-ng".serviceConfig.ExecStart = + lib.mkForce "''${pkgs.openssh}/bin/ssh -o \"ServerAliveInterval 30\" -o \"ServerAliveCountMax 3\" -o ExitOnForwardFailure=yes -N -R2222:localhost:22 -E ${hostname} deployer"; + + # this will be asserted to validate the switch happened: + networking.hostName = "${hostname}"; + } + ''; + in + # python + '' + start_all() + target.wait_for_open_port(22) + + deployer.wait_until_succeeds("ping -c1 target") + deployer.succeed("install -Dm 600 ${nodes.deployer.system.build.privateKey} ~root/.ssh/id_ecdsa") + deployer.succeed("install ${sshConfig} ~root/.ssh/config") + + target.succeed("nixos-generate-config") + target.succeed("install -Dm 600 ${nodes.target.system.build.privateKey} ~root/.ssh/id_ecdsa") + target.succeed("install ${sshConfig} ~root/.ssh/config") + deployer.succeed("scp alice@target:/etc/nixos/hardware-configuration.nix /root/hardware-configuration.nix") + target.wait_for_unit("autossh-ng.service") + + deployer.copy_from_host("${configFile "config-1-deployed"}", "/root/configuration-1.nix") + deployer.copy_from_host("${configFile "config-2-deployed"}", "/root/configuration-2.nix") + deployer.copy_from_host("${targetNetworkJSON}", "/root/target-network.json") + deployer.copy_from_host("${targetConfigJSON}", "/root/target-configuration.json") + + with subtest("Deploy to alice@target via reverse ssh"): + deployer.wait_for_unit("multi-user.target") + # Uses TTY/send_chars instead of deployer.succeed to set NIX_SSHOPTS + deployer.send_chars("NIX_SSHOPTS=\"-p 2222\" nixos-rebuild switch -I nixos-config=/root/configuration-1.nix --target-host alice@localhost --sudo\n") + + # the connection breaks, but the 'switch' should now continue in the background: + deployer.wait_until_tty_matches("1", "error: while running command with remote sudo") + + def deployed(last_try: bool) -> bool: + target_hostname = deployer.succeed("ssh alice@target cat /etc/hostname", timeout=20).rstrip() + if last_try: + print(f"Still seeing hostname {target_hostname}") + return target_hostname == "config-1-deployed" + retry(deployed) + + with subtest("Deploy to bob@target via reverse ssh with password-based sudo"): + deployer.wait_for_unit("multi-user.target") + # Uses TTY/send_chars instead of deployer.succeed to set NIX_SSHOPTS and for ask-sudo-password + deployer.send_chars("NIX_SSHOPTS=\"-p 2222\" nixos-rebuild switch -I nixos-config=/root/configuration-2.nix --target-host bob@localhost --ask-sudo-password\n") + deployer.wait_until_tty_matches("1", "password for bob") + deployer.send_chars("${nodes.target.users.users.bob.password}\n") + + # the connection breaks, but the 'switch' should now continue in the background: + deployer.wait_until_tty_matches("1", "error: while running command with remote sudo") + + def deployed(last_try: bool) -> bool: + target_hostname = deployer.succeed("ssh alice@target cat /etc/hostname", timeout=20).rstrip() + if last_try: + print(f"Still seeing hostname {target_hostname}") + return target_hostname == "config-2-deployed" + retry(deployed) + ''; +}