Fix missing register system on update.run, re-add shortcut if channel…

… unchanged

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,16 +309,29 @@ 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):
     nixos_mock.switch_to_system.side_effect = SwitchFailed(stdout="stdout")
 
     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(

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

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,27 +154,30 @@ 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."
             ),
         )
         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."

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)
 

pkgs/fc/agent/fc/maintenance/tests/test_maintenance.py

@@ -160,15 +160,16 @@ 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
     )
 
     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",
     )

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

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.