From f2e1cadf7d4b9ab08d8b1915687e265f08301b12 Mon Sep 17 00:00:00 2001 From: Tobias Stenzel Date: Tue, 11 Jul 2023 16:25:51 +0200 Subject: [PATCH] Fix missing register system on update.run, re-add shortcut if channel unchanged --- .../maintenance/activity/tests/test_update.py | 34 ++++++++++++--- .../agent/fc/maintenance/activity/update.py | 25 ++++++++--- pkgs/fc/agent/fc/maintenance/maintenance.py | 17 +++++--- pkgs/fc/agent/fc/maintenance/request.py | 43 +++++++++++++------ .../fc/maintenance/tests/test_maintenance.py | 16 ++++--- .../fc/maintenance/tests/test_request.py | 14 +++++- pkgs/fc/agent/fc/util/nixos.py | 27 +----------- 7 files changed, 110 insertions(+), 66 deletions(-) diff --git a/pkgs/fc/agent/fc/maintenance/activity/tests/test_update.py b/pkgs/fc/agent/fc/maintenance/activity/tests/test_update.py index 4f3c50410..dc933a4b8 100644 --- a/pkgs/fc/agent/fc/maintenance/activity/tests/test_update.py +++ b/pkgs/fc/agent/fc/maintenance/activity/tests/test_update.py @@ -266,19 +266,26 @@ def test_update_activity_run(log, nixos_mock, activity, logger): nixos_mock.build_system.assert_called_with( activity.next_channel_url, log=activity.log ) + nixos_mock.register_system_profile.assert_called_with( + NEXT_SYSTEM_PATH, log=activity.log + ) nixos_mock.switch_to_system.assert_called_with( NEXT_SYSTEM_PATH, lazy=False, log=activity.log ) - log.has("update-run-succeeded") + assert log.has("update-run-succeeded") def test_update_activity_run_unchanged(log, nixos_mock, activity): - nixos_mock.running_system_version.return_value = activity.next_version + activity.current_system = activity.next_system activity.run() + nixos_mock.update_system_channel.assert_called_with( + activity.next_channel_url, log=activity.log + ) + nixos_mock.build_system.assert_not_called() + assert activity.returncode == 0 - log.has("update-run-skip") def test_update_activity_run_update_system_channel_fails( @@ -291,7 +298,7 @@ def test_update_activity_run_update_system_channel_fails( activity.run() assert activity.returncode == 1 - log.has("update-run-failed", returncode=1) + assert log.has("update-run-failed", returncode=1) def test_update_activity_build_system_fails(log, nixos_mock, activity): @@ -302,7 +309,20 @@ def test_update_activity_build_system_fails(log, nixos_mock, activity): activity.run() assert activity.returncode == 2 - log.has("update-run-failed", returncode=2) + assert log.has("update-run-failed", returncode=2) + + +def test_update_activity_register_system_profile_fails( + log, nixos_mock, activity +): + nixos_mock.register_system_profile.side_effect = RegisterFailed( + msg="msg", stdout="stdout", stderr="stderr" + ) + + activity.run() + + assert activity.returncode == 3 + assert log.has("update-run-failed", returncode=3) def test_update_activity_switch_to_system_fails(log, nixos_mock, activity): @@ -310,8 +330,8 @@ def test_update_activity_switch_to_system_fails(log, nixos_mock, activity): activity.run() - assert activity.returncode == 3 - log.has("update-run-failed", returncode=3) + assert activity.returncode == 4 + assert log.has("update-run-failed", returncode=4) def test_update_activity_from_enc( diff --git a/pkgs/fc/agent/fc/maintenance/activity/update.py b/pkgs/fc/agent/fc/maintenance/activity/update.py index 57bdf92cc..69038b4f5 100644 --- a/pkgs/fc/agent/fc/maintenance/activity/update.py +++ b/pkgs/fc/agent/fc/maintenance/activity/update.py @@ -172,12 +172,25 @@ def update_system_channel(self): nixos.update_system_channel(self.next_channel_url, self.log) @property - def is_identical_to_running_system(self): + def identical_to_current_channel_url(self) -> bool: + if self.current_channel_url == self.next_channel_url: + self.log.info( + "update-identical-channel", + channel=self.next_channel_url, + _replace_msg=( + "Current system already uses the wanted channel URL." + ), + ) + return True + + return False + + @property + def identical_to_current_system(self) -> bool: if self.current_system == self.next_system: self.log.info( - "update-identical", - current_version=self.next_version, - next_version=self.next_version, + "update-identical-system", + version=self.next_version, system=self.next_system.removeprefix("/nix/store/"), _replace_msg=( "Running system {system} is already the wanted system." @@ -193,7 +206,7 @@ def run(self): try: self.update_system_channel() - if self.is_identical_to_running_system: + if self.identical_to_current_system: self.returncode = 0 return @@ -202,6 +215,8 @@ def run(self): self.next_channel_url, log=self.log ) step = 3 + nixos.register_system_profile(system_path, log=self.log) + step = 4 nixos.switch_to_system(system_path, lazy=False, log=self.log) except nixos.ChannelException as e: self.stdout = e.stdout diff --git a/pkgs/fc/agent/fc/maintenance/maintenance.py b/pkgs/fc/agent/fc/maintenance/maintenance.py index 7fca7098f..4383f0498 100644 --- a/pkgs/fc/agent/fc/maintenance/maintenance.py +++ b/pkgs/fc/agent/fc/maintenance/maintenance.py @@ -128,7 +128,7 @@ def request_update(log, enc, current_requests) -> Optional[Request]: activity = UpdateActivity.from_enc(log, enc) if activity is None: - log.debug("update-no-activity") + log.debug("request-update-no-activity") return other_planned_requests = [ @@ -145,7 +145,7 @@ def request_update(log, enc, current_requests) -> Optional[Request]: if equivalent_planned_requests: log.info( - "update-found-equivalent", + "request-update-found-equivalent", _replace_msg=( "Existing request {request} with same channel URL: {channel_url}" ), @@ -154,17 +154,20 @@ def request_update(log, enc, current_requests) -> Optional[Request]: ) return + if not other_planned_requests and activity.identical_to_current_channel_url: + # Shortcut to save time preparing an activity which will have no effect. + return + activity.prepare() # Always request an update if other updates are planned at the moment. Adding # this activity can cancel out an existing activity by going back to the # current system state. This is useful when someone requests an update by error # and wants to undo it. Let the merge algorithm figure this out. - if not other_planned_requests and activity.is_identical_to_running_system: + if not other_planned_requests and activity.identical_to_current_system: log.info( - "update-system-unchanged", + "request-update-shortcut", _replace_msg=( - "System produced by this update is identical to the running system." "As there are no other update requests, skip the update and set the " "system channel directly." ), @@ -172,9 +175,9 @@ def request_update(log, enc, current_requests) -> Optional[Request]: activity.update_system_channel() return - if not activity.is_identical_to_running_system: + if not activity.identical_to_current_system: log.info( - "update-system-changed", + "request-update-prepared", _replace_msg=( "Update preparation was successful. This update will apply " "changes to the system." diff --git a/pkgs/fc/agent/fc/maintenance/request.py b/pkgs/fc/agent/fc/maintenance/request.py index d702b5d59..93c3940dc 100644 --- a/pkgs/fc/agent/fc/maintenance/request.py +++ b/pkgs/fc/agent/fc/maintenance/request.py @@ -175,19 +175,36 @@ def load(cls, dir, log): with open(p.join(dir, "request.yaml")) as f: instance = yaml.load(f, Loader=yaml.UnsafeLoader) - instance.added_at = ensure_timezone_present(instance.added_at) - # These attributes are not present on legacy requests. For newer requests, - # they are None after deserialization. - if hasattr(instance, "next_due"): - instance.next_due = ensure_timezone_present(instance.next_due) - if hasattr(instance, "updated_at"): - instance.updated_at = ensure_timezone_present( - instance.updated_at - ) - if hasattr(instance, "last_scheduled_at"): - instance.last_scheduled_at = ensure_timezone_present( - instance.last_scheduled_at - ) + + instance.added_at = ensure_timezone_present(instance.added_at) + # Some attributes are not present on legacy requests. For newer requests, + # they are None after deserialization. + if hasattr(instance, "next_due"): + instance.next_due = ensure_timezone_present(instance.next_due) + else: + instance.next_due = None + + if hasattr(instance, "updated_at"): + instance.updated_at = ensure_timezone_present(instance.updated_at) + else: + instance.updated_at = None + + if hasattr(instance, "last_scheduled_at"): + instance.last_scheduled_at = ensure_timezone_present( + instance.last_scheduled_at + ) + else: + instance.last_scheduled_at = None + + if not hasattr(instance, "_comment"): + instance._comment = None + + if not hasattr(instance, "_estimate"): + instance._estimate = None + + if not hasattr(instance, "state"): + instance.state = State.pending + instance.dir = dir instance.set_up_logging(log) diff --git a/pkgs/fc/agent/fc/maintenance/tests/test_maintenance.py b/pkgs/fc/agent/fc/maintenance/tests/test_maintenance.py index 665ffb286..f99bdc723 100644 --- a/pkgs/fc/agent/fc/maintenance/tests/test_maintenance.py +++ b/pkgs/fc/agent/fc/maintenance/tests/test_maintenance.py @@ -160,7 +160,8 @@ def fake_changed_kernel_version(path): def test_request_update(log, logger, monkeypatch): enc = {} from_enc_mock = MagicMock() - from_enc_mock.return_value.is_identical_to_running_system = False + from_enc_mock.return_value.identical_to_current_channel_url = False + from_enc_mock.return_value.identical_to_current_system = False monkeypatch.setattr( "fc.maintenance.maintenance.UpdateActivity.from_enc", from_enc_mock ) @@ -168,7 +169,7 @@ def test_request_update(log, logger, monkeypatch): request = fc.maintenance.maintenance.request_update(logger, enc, []) assert request - assert log.has("update-system-changed") + assert log.has("request-update-prepared") def test_request_update_unchanged(log, logger, monkeypatch): @@ -185,14 +186,15 @@ def test_request_update_unchanged(log, logger, monkeypatch): ) assert request is None - assert log.has("update-no-activity") + assert log.has("request-update-no-activity") def test_request_update_unchanged_system_and_no_other_requests_skips_request( log, logger, monkeypatch ): from_enc_mock = MagicMock() - from_enc_mock.return_value.is_identical_to_running_system = True + from_enc_mock.return_value.identical_to_current_channel_url = False + from_enc_mock.return_value.identical_to_current_system = True monkeypatch.setattr( "fc.maintenance.maintenance.UpdateActivity.from_enc", from_enc_mock ) @@ -204,7 +206,7 @@ def test_request_update_unchanged_system_and_no_other_requests_skips_request( ) assert request is None - assert log.has("update-system-unchanged") + assert log.has("request-update-shortcut") class FakeUpdateActivity(UpdateActivity): @@ -219,7 +221,7 @@ def test_request_update_unchanged_system_and_other_requests_produces_request( log, logger, monkeypatch ): from_enc_mock = MagicMock() - from_enc_mock.return_value.is_identical_to_running_system = True + from_enc_mock.return_value.identical_to_current_system = True monkeypatch.setattr( "fc.maintenance.maintenance.UpdateActivity.from_enc", from_enc_mock ) @@ -254,7 +256,7 @@ def test_request_update_equivalent_existing_request_skip_request( assert request is None assert log.has( - "update-found-equivalent", + "request-update-found-equivalent", request=existing_request.id, channel_url="https://fake", ) diff --git a/pkgs/fc/agent/fc/maintenance/tests/test_request.py b/pkgs/fc/agent/fc/maintenance/tests/test_request.py index 1437529ba..bba7f1276 100644 --- a/pkgs/fc/agent/fc/maintenance/tests/test_request.py +++ b/pkgs/fc/agent/fc/maintenance/tests/test_request.py @@ -1,13 +1,15 @@ import datetime import unittest.mock +from io import StringIO import freezegun import pytest import pytz import structlog -from fc.maintenance.activity import Activity +from fc.maintenance.activity import Activity, RebootType from fc.maintenance.request import Attempt, Request from fc.maintenance.state import ARCHIVE, EXIT_TEMPFAIL, State +from rich.console import Console def test_overdue_not_scheduled(): @@ -223,3 +225,13 @@ def test_update_state_overdue_request(monkeypatch): monkeypatch.setattr("fc.maintenance.request.Request.overdue", True) r.update_state() assert r.state == State.postpone + + +def test_show(): + r = Request(Activity()) + r.activity.reboot_needed = RebootType.WARM + + console = Console(file=StringIO()) + console.print(r) + out = console.file.getvalue() + assert "fc.maintenance.activity.Activity (warm reboot needed)" in out diff --git a/pkgs/fc/agent/fc/util/nixos.py b/pkgs/fc/agent/fc/util/nixos.py index 4786740ce..62e67aed4 100644 --- a/pkgs/fc/agent/fc/util/nixos.py +++ b/pkgs/fc/agent/fc/util/nixos.py @@ -98,11 +98,9 @@ def current_fc_environment_name(log=_log): def channel_version(channel_url, log=_log): - log.debug("channel_version", channel=channel_url) - final_channel_url = resolve_url_redirects(channel_url) try: nixpkgs_path = subprocess.run( - ["nix-instantiate", "-I", final_channel_url, "--find-file", "."], + ["nix-instantiate", "-I", channel_url, "--find-file", "."], check=True, capture_output=True, text=True, @@ -297,29 +295,6 @@ def update_system_channel(channel_url, log=_log): raise ChannelUpdateFailed(stdout=stdout, stderr=stderr) -def switch_to_channel(channel_url, lazy=False, log=_log): - final_channel_url = resolve_url_redirects(channel_url) - """ - Build system with this channel and switch to it. - Replicates the behaviour of nixos-rebuild switch and adds an optional - lazy mode which only switches to the built system if it actually changed. - """ - log.info( - "channel-switch", - channel=channel_url, - resolved_channel=final_channel_url, - ) - # Put a temporary result link in /run to avoid a race condition - # with the garbage collector which may remove the system we just built. - # If register fails, we still hold a GC root until the next reboot. - out_link = "/run/fc-agent-built-system" - built_system = build_system(final_channel_url, out_link) - register_system_profile(built_system) - # New system is registered, delete the temporary result link. - os.unlink(out_link) - return switch_to_system(built_system, lazy) - - def find_nix_build_error(stderr: str, log=_log): """Returns the (hopefully) interesting part of the error message from Nix build output or a generic message if nothing is found.