From 9deb674a0d931c4fb4dc2e19b8c8bcf9caa04bdf Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 9 Sep 2024 10:15:46 +0200 Subject: [PATCH 1/3] Revert "storage: lean on systemd-shutdown to export zpools on shutdown" This reverts commit 2a330206421103176aa9ee0c598a13cb9f369770. --- subiquity/models/filesystem.py | 7 ------- subiquity/server/controllers/filesystem.py | 2 ++ subiquity/server/controllers/install.py | 22 ---------------------- 3 files changed, 2 insertions(+), 29 deletions(-) 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 82452a25e..4c72be7ac 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -1768,3 +1768,5 @@ async def _pre_shutdown(self): ) else: await self.app.command_runner.run(["umount", "--recursive", "/target"]) + if len(self.model._all(type="zpool")) > 0: + 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"], From f36c828debaea61e6649c3f938e92f57f6b94016 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 9 Sep 2024 10:35:42 +0200 Subject: [PATCH 2/3] filesystem: document _pre_shutdown and link to systemd-shutdown impl We decided to revert the use of systemd-shutdown for exporting zpools because it didn't address the issue we were trying to solve. However, dropping scripts for systemd-shutdown is an interesting implementation from an architecture standpoint, because it can be executed even if Subiquity does not cleanly exit. Let's document the _pre_shutdown function and link to the original systemd-shutdown implementation that we came up with for exporting zpools earlier. Signed-off-by: Olivier Gayot --- subiquity/server/controllers/filesystem.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 4c72be7ac..fdd4029d1 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -1757,6 +1757,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: From 29064316230fe880594a3bc258360b451507c911 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 9 Sep 2024 10:39:15 +0200 Subject: [PATCH 3/3] storage: export zpools unconditionally In the _pre_shutdown function, we used to invoke `zpool export -a` if we found the presence of a zpool in the storage model. That being said, zpools can be implicitly declared by users using {"type": "format", "fstype": "zfsroot"} in the curtin actions. This is valid curtin configuration and unfortunately causes Subiquity to think that there is no zpool. When that happened, Subiquity did not invoke `zpool export -a` and therefore the target system couldn't boot without a call to `zpool import -f`. We now do the call to `zpool export -a` unconditionally. Running this command when there is no zpool is a noop so it should not be a problem. In theory there is a risk that we could export a zpool that was not meant for exporting. However, that would involve somebody importing (or force importing) a zpool in the live installer environment and not wanting it exported at the end. This sounds like an very unlikely use case. Furthermore, this could already be a problem today since we invoke `zpool export` with the `-a` option (which all pools imported on the system). LP: #2073772 Signed-off-by: Olivier Gayot --- subiquity/server/controllers/filesystem.py | 3 +-- .../server/controllers/tests/test_filesystem.py | 14 ++++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index fdd4029d1..5d168942b 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -1781,5 +1781,4 @@ async def _pre_shutdown(self): ) else: await self.app.command_runner.run(["umount", "--recursive", "/target"]) - if len(self.model._all(type="zpool")) > 0: - await self.app.command_runner.run(["zpool", "export", "-a"]) + await self.app.command_runner.run(["zpool", "export", "-a"]) diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index 97b9ae7ba..84f8fec1f 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -468,9 +468,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): @@ -479,10 +480,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