From 1c15504b7a1e7dd6c6d060a64bcef33c8a822866 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 9 Sep 2024 10:39:15 +0200 Subject: [PATCH] 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 (cherry picked from commit 29064316230fe880594a3bc258360b451507c911) --- 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 a9cc78bea..5ec441303 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -1657,5 +1657,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 859f33a08..c15e3f242 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -361,9 +361,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): @@ -372,10 +373,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