Skip to content

Commit

Permalink
storage: export zpools unconditionally
Browse files Browse the repository at this point in the history
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 <[email protected]>
(cherry picked from commit 2906431)
  • Loading branch information
ogayot committed Sep 9, 2024
1 parent f9b3400 commit 1c15504
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 6 deletions.
3 changes: 1 addition & 2 deletions subiquity/server/controllers/filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
14 changes: 10 additions & 4 deletions subiquity/server/controllers/tests/test_filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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
Expand Down

0 comments on commit 1c15504

Please sign in to comment.