diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index fea082101..72bffc8d5 100644 --- a/subiquity/models/filesystem.py +++ b/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: diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index b8b7ad558..fd7f3af41 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/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"]) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 08f067a99..373bae154 100644 --- a/subiquity/server/controllers/install.py +++ b/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"], diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index b80e9be68..bad407afd 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/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