Merge pull request #2077 from ogayot/zpool-export-always

Unconditionally run `zpool export -a` / revert systemd-shutdown implementation

subiquity/models/filesystem.py

@@ -2053,13 +2053,6 @@ def render(self, mode: ActionRenderMode = ActionRenderMode.DEFAULT):
             config["grub"] = self.grub
         return config
 
-    def systemd_shutdown_commands(self) -> list[list[str]]:
-        """Return a list of commands meant to be executed by systemd-shutdown.
-        We entrust the execution of `zpool export` commands to systemd-shutdown
-        instead of subiquity's _pre_shutdown hook, in hope that the commands
-        will more likely succeed."""
-        return [["zpool", "export", zpool.name] for zpool in self._all(type="zpool")]
-
     def load_probe_data(self, probe_data):
         for devname, devdata in probe_data["blockdev"].items():
             if int(devdata["attrs"]["size"]) != 0:

subiquity/server/controllers/filesystem.py

@@ -1767,6 +1767,19 @@ def make_autoinstall(self):
         return r
 
     async def _pre_shutdown(self):
+        """This function is executed just before rebooting and after copying
+        logs to the target. This means bugs reports are unlikely to include
+        execution logs from this function and therefore diagnosing issues is a
+        challenge. Let's try to keep it as simple as possible.
+
+        Another approach to execute commands before reboot is to place scripts in
+        /usr/lib/systemd/system-shutdown and lean on systemd-shutdown(8) to
+        execute them after unmounting most file-systems.
+
+        See this PR for an example (the PR was eventually reverted because it
+        didn't address the issue we tried to solve at the time).
+        https://github.com/canonical/subiquity/pull/2064
+        """
         if not self.reset_partition_only:
             # /target is mounted only if the installation was actually started.
             try:
@@ -1778,3 +1791,4 @@ async def _pre_shutdown(self):
                 )
             else:
                 await self.app.command_runner.run(["umount", "--recursive", "/target"])
+        await self.app.command_runner.run(["zpool", "export", "-a"])

subiquity/server/controllers/install.py

@@ -345,25 +345,6 @@ async def curtin_install(self, *, context, source):
 
         fs_controller = self.app.controllers.Filesystem
 
-        def register_shutdown_commands() -> None:
-            cmds: list[list[str]] = fs_controller.model.systemd_shutdown_commands()
-
-            if not cmds:
-                return
-
-            shutdown_dir = root / "usr/lib/systemd/system-shutdown"
-            shutdown_script = shutdown_dir / "subiquity-storage.shutdown"
-            shutdown_dir.mkdir(parents=True, exist_ok=True)
-            with shutdown_script.open(mode="w", encoding="utf-8") as stream:
-                # Generating a bash script is .. ewww
-                # Should we place the commands in a JSON file and hardcode a
-                # script that calls jq or something?
-                stream.write("#!/bin/bash\n")
-                for cmd in cmds:
-                    stream.write(shlex.join(cmd))
-                    stream.write("\n")
-            shutdown_script.chmod(0o755)
-
         async def run_curtin_step(name, stages, step_config, source=None):
             config = copy.deepcopy(base_config)
             filename = f"subiquity-{name.replace(' ', '-')}.conf"
@@ -387,7 +368,6 @@ async def run_curtin_step(name, stages, step_config, source=None):
                     device_map_path=logs_dir / "device-map.json",
                 ),
             )
-            register_shutdown_commands()
         elif fs_controller.use_snapd_install_api():
             await run_curtin_step(
                 name="partitioning",
@@ -407,7 +387,6 @@ async def run_curtin_step(name, stages, step_config, source=None):
                     device_map_path=logs_dir / "device-map-format.json",
                 ),
             )
-            register_shutdown_commands()
             if source is not None:
                 await run_curtin_step(
                     name="extract",
@@ -441,7 +420,6 @@ async def run_curtin_step(name, stages, step_config, source=None):
                 ),
                 source=source,
             )
-            register_shutdown_commands()
             await run_curtin_step(
                 name="extract",
                 stages=["extract"],

subiquity/server/controllers/tests/test_filesystem.py

@@ -484,9 +484,10 @@ async def test__pre_shutdown_install_started(self):
             [
                 mock.call(["mountpoint", "/target"]),
                 mock.call(["umount", "--recursive", "/target"]),
+                mock.call(["zpool", "export", "-a"]),
             ]
         )
-        self.assertEqual(len(mock_run.mock_calls), 2)
+        self.assertEqual(3, mock_run.call_count)
 
     async def test__pre_shutdown_install_not_started(self):
         async def fake_run(cmd, **kwargs):
@@ -495,10 +496,15 @@ async def fake_run(cmd, **kwargs):
 
         self.fsc.reset_partition_only = False
         run = mock.patch.object(self.app.command_runner, "run", side_effect=fake_run)
-        _all = mock.patch.object(self.fsc.model, "_all")
-        with run as mock_run, _all:
+        with run as mock_run:
             await self.fsc._pre_shutdown()
-        mock_run.assert_called_once_with(["mountpoint", "/target"])
+        mock_run.assert_has_calls(
+            [
+                mock.call(["mountpoint", "/target"]),
+                mock.call(["zpool", "export", "-a"]),
+            ]
+        )
+        self.assertEqual(2, mock_run.call_count)
 
     async def test_examine_systems(self):
         # In LP: #2037723 and other similar reports, the user selects the