From 8e084673c596c2e97c055c40e8b6c2cb18bd5944 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Tue, 18 Jul 2023 10:16:31 -0600 Subject: [PATCH 1/7] filesystem: let ZFS handle fstype / path --- subiquity/models/filesystem.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index 957c6fe50..1af61ba9a 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -28,6 +28,7 @@ from typing import List, Optional, Set, Union import more_itertools +import yaml from curtin import storage_config from curtin.block import partition_kname @@ -1110,6 +1111,21 @@ class ZFS: # options to pass to zfs dataset creation properties: Optional[dict] = None + @property + def fstype(self): + return 'zfs' + + @property + def path(self): + if self.properties is None: + return self.volume + if not yaml.safe_load(self.properties.get('canmount', 'on')): + return None + mountpoint = self.properties.get('mountpoint', None) + if mountpoint is not None: + return mountpoint + return self.volume + ConstructedDevice = Union[Raid, LVM_VolGroup, ZPool] From 203b9485a4d2efae08bace9dc58c51cbfded7f36 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Tue, 18 Jul 2023 10:16:04 -0600 Subject: [PATCH 2/7] filesystem: let ZPool be more like Mount --- subiquity/models/filesystem.py | 5 +- subiquity/models/tests/test_filesystem.py | 54 +++++++++++++------ .../controllers/tests/test_filesystem.py | 8 +-- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index 1af61ba9a..0e89dada1 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -1100,7 +1100,10 @@ def name(self): return self.pool @property - def mount(self): + def path(self): + if self.fs_properties is not None: + if not yaml.safe_load(self.fs_properties.get('canmount', 'off')): + return None return self.mountpoint diff --git a/subiquity/models/tests/test_filesystem.py b/subiquity/models/tests/test_filesystem.py index 512b22fea..34c49c01b 100644 --- a/subiquity/models/tests/test_filesystem.py +++ b/subiquity/models/tests/test_filesystem.py @@ -1299,30 +1299,52 @@ def test_zpool_to_action(self): def test_zpool_from_action(self): m = make_model() - d = make_disk(m) + d1 = make_disk(m) + d2 = make_disk(m) fake_up_blockdata(m) blockdevs = m._probe_data['blockdev'] config = [ - dict(type='disk', id=d.id, path=d.path, ptable=d.ptable, - serial=d.serial, info={d.path: blockdevs[d.path]}), - dict(type='zpool', id='zpool-1', vdevs=[d.id], pool='p1', + dict(type='disk', id=d1.id, path=d1.path, ptable=d1.ptable, + serial=d1.serial, info={d1.path: blockdevs[d1.path]}), + dict(type='disk', id=d2.id, path=d2.path, ptable=d2.ptable, + serial=d2.serial, info={d2.path: blockdevs[d2.path]}), + dict(type='zpool', id='zpool-1', vdevs=[d1.id], pool='p1', mountpoint='/'), + dict(type='zpool', id='zpool-2', vdevs=[d2.id], pool='p2', + mountpoint='/srv', fs_properties=dict(canmount='off')), dict(type='zfs', id='zfs-1', volume='/ROOT', pool='zpool-1'), + dict(type='zfs', id='zfs-2', volume='/SRV/srv', pool='zpool-2', + properties=dict(mountpoint='/srv', canmount='on')), ] objs = m._actions_from_config( config, blockdevs=None, is_probe_data=False) - actual_disk, zpool, zfs = objs - self.assertTrue(isinstance(zpool, ZPool)) - self.assertEqual('zpool-1', zpool.id) - self.assertEqual([actual_disk], zpool.vdevs) - self.assertEqual('p1', zpool.pool) - self.assertEqual('/', zpool.mountpoint) - self.assertEqual([zfs], zpool._zfses) - - self.assertTrue(isinstance(zfs, ZFS)) - self.assertEqual('zfs-1', zfs.id) - self.assertEqual(zpool, zfs.pool) - self.assertEqual('/ROOT', zfs.volume) + actual_d1, actual_d2, zp1, zp2, zfs_zp1, zfs_zp2 = objs + self.assertTrue(isinstance(zp1, ZPool)) + self.assertEqual('zpool-1', zp1.id) + self.assertEqual([actual_d1], zp1.vdevs) + self.assertEqual('p1', zp1.pool) + self.assertEqual('/', zp1.mountpoint) + self.assertEqual('/', zp1.path) + self.assertEqual([zfs_zp1], zp1._zfses) + + self.assertTrue(isinstance(zp2, ZPool)) + self.assertEqual('zpool-2', zp2.id) + self.assertEqual([actual_d2], zp2.vdevs) + self.assertEqual('p2', zp2.pool) + self.assertEqual('/srv', zp2.mountpoint) + self.assertEqual(None, zp2.path) + self.assertEqual([zfs_zp2], zp2._zfses) + + self.assertTrue(isinstance(zfs_zp1, ZFS)) + self.assertEqual('zfs-1', zfs_zp1.id) + self.assertEqual(zp1, zfs_zp1.pool) + self.assertEqual('/ROOT', zfs_zp1.volume) + + self.assertTrue(isinstance(zfs_zp2, ZFS)) + self.assertEqual('zfs-2', zfs_zp2.id) + self.assertEqual(zp2, zfs_zp2.pool) + self.assertEqual('/SRV/srv', zfs_zp2.volume) + self.assertEqual('/srv', zfs_zp2.path) class TestRootfs(SubiTestCase): diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index df2021180..12ed1f451 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -483,9 +483,9 @@ async def test_guided_zfs(self, bootloader, ptable, p1mnt): self.assertFalse(d1p2.preserve) self.assertFalse(d1p3.preserve) [rpool] = self.model._all(type='zpool', pool='rpool') - self.assertEqual('/', rpool.mount) + self.assertEqual('/', rpool.path) [bpool] = self.model._all(type='zpool', pool='bpool') - self.assertEqual('/boot', bpool.mount) + self.assertEqual('/boot', bpool.path) async def test_guided_zfs_BIOS_MSDOS(self): await self._guided_setup(Bootloader.BIOS, 'msdos') @@ -499,9 +499,9 @@ async def test_guided_zfs_BIOS_MSDOS(self): self.assertFalse(d1p1.preserve) self.assertFalse(d1p2.preserve) [rpool] = self.model._all(type='zpool', pool='rpool') - self.assertEqual('/', rpool.mount) + self.assertEqual('/', rpool.path) [bpool] = self.model._all(type='zpool', pool='bpool') - self.assertEqual('/boot', bpool.mount) + self.assertEqual('/boot', bpool.path) async def _guided_side_by_side(self, bl, ptable): await self._guided_setup(bl, ptable, storage_version=2) From 8e7499ba2ea2df742ffedecfa25ca48aae961888 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Tue, 18 Jul 2023 10:16:49 -0600 Subject: [PATCH 3/7] filesystem: ZFS/Zpool may be a mountpoint --- subiquity/models/filesystem.py | 25 ++++++++++++------- subiquity/models/tests/test_filesystem.py | 29 ++++++++++++++++++++--- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index 0e89dada1..bad056d18 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -1132,6 +1132,11 @@ def path(self): ConstructedDevice = Union[Raid, LVM_VolGroup, ZPool] +# A Mountlike is a literal Mount object, or one similar enough in behavior. +# Mountlikes have a path property that may return None if that given object is +# not actually mountable. +MountlikeNames = ('mount', 'zpool', 'zfs') + def align_up(size, block_size=1 << 20): return (size + block_size - 1) & ~(block_size - 1) @@ -1618,7 +1623,7 @@ def can_emit(obj): if dep.type in ['disk', 'raid']: ensure_partitions(dep) return False - if isinstance(obj, Mount): + if obj.type in MountlikeNames: # Any mount actions for a parent of this one have to be emitted # first. for parent in pathlib.Path(obj.path).parents: @@ -1631,7 +1636,7 @@ def can_emit(obj): return False return True - mountpoints = {m.path: m.id for m in self.all_mounts()} + mountpoints = {m.path: m.id for m in self.all_mountlikes()} log.debug('mountpoints %s', mountpoints) if mode == ActionRenderMode.FOR_API: @@ -1730,6 +1735,12 @@ def _all(self, **kw): def all_mounts(self): return self._all(type='mount') + def all_mountlikes(self): + ret = [] + for typename in MountlikeNames: + ret += self._all(type=typename) + return ret + def all_devices(self): # return: # compound devices, newest first @@ -1895,12 +1906,10 @@ def needs_bootloader_partition(self): "unknown bootloader type {}".format(self.bootloader)) def _mount_for_path(self, path): - mount = self._one(type='mount', path=path) - if mount is not None: - return mount - zpool = self._one(type='zpool', mountpoint=path) - if zpool is not None: - return zpool + for typename in MountlikeNames: + mount = self._one(type=typename, path=path) + if mount is not None: + return mount return None def is_root_mounted(self): diff --git a/subiquity/models/tests/test_filesystem.py b/subiquity/models/tests/test_filesystem.py index 34c49c01b..4a71a02b1 100644 --- a/subiquity/models/tests/test_filesystem.py +++ b/subiquity/models/tests/test_filesystem.py @@ -195,8 +195,8 @@ def make_partition(model, device=None, *, preserve=False, size=None, return partition -def make_filesystem(model, *, partition, **kw): - return Filesystem(m=model, volume=partition, **kw) +def make_filesystem(model, partition, *, fstype='ext4', **kw): + return Filesystem(m=model, volume=partition, fstype=fstype, **kw) def make_model_and_partition(bootloader=None): @@ -255,7 +255,8 @@ def make_zpool(model=None, device=None, pool=None, mountpoint=None, **kw): device = make_disk(model) if pool is None: pool = f'pool{len(model._actions)}' - zpool = ZPool(m=model, vdevs=[device], pool=pool, mountpoint=mountpoint) + zpool = ZPool(m=model, vdevs=[device], pool=pool, mountpoint=mountpoint, + **kw) model._actions.append(zpool) return zpool @@ -1348,11 +1349,33 @@ def test_zpool_from_action(self): class TestRootfs(SubiTestCase): + def test_mount_rootfs(self): + m, p = make_model_and_partition() + fs = make_filesystem(m, p) + m.add_mount(fs, '/') + self.assertTrue(m.is_root_mounted()) + + def test_mount_srv(self): + m, p = make_model_and_partition() + fs = make_filesystem(m, p) + m.add_mount(fs, '/srv') + self.assertFalse(m.is_root_mounted()) + def test_zpool_may_provide_rootfs(self): m = make_model() make_zpool(model=m, mountpoint='/') self.assertTrue(m.is_root_mounted()) + def test_zpool_not_rootfs_because_not_canmount(self): + m = make_model() + make_zpool(model=m, mountpoint='/', fs_properties=dict(canmount='no')) + self.assertFalse(m.is_root_mounted()) + + def test_zpool_rootfs_because_canmount(self): + m = make_model() + make_zpool(model=m, mountpoint='/', fs_properties=dict(canmount='yes')) + self.assertTrue(m.is_root_mounted()) + def test_zpool_nonrootfs_mountpoint(self): m = make_model() make_zpool(model=m, mountpoint='/srv') From 846098027c4705e1190cd4aa8989486725d66196 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Thu, 20 Jul 2023 11:40:41 -0600 Subject: [PATCH 4/7] filesystem: handle 'Mountlike.path is None' --- subiquity/models/filesystem.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index bad056d18..e7e955601 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -1623,7 +1623,7 @@ def can_emit(obj): if dep.type in ['disk', 'raid']: ensure_partitions(dep) return False - if obj.type in MountlikeNames: + if obj.type in MountlikeNames and obj.path is not None: # Any mount actions for a parent of this one have to be emitted # first. for parent in pathlib.Path(obj.path).parents: From 9dab634ade43f1485fdd5a23902ace894c886aae Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Thu, 20 Jul 2023 12:11:52 -0600 Subject: [PATCH 5/7] filesystem: stricter zfs canmount tests --- subiquity/models/tests/test_filesystem.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/subiquity/models/tests/test_filesystem.py b/subiquity/models/tests/test_filesystem.py index 4a71a02b1..33da4d2e1 100644 --- a/subiquity/models/tests/test_filesystem.py +++ b/subiquity/models/tests/test_filesystem.py @@ -1310,10 +1310,11 @@ def test_zpool_from_action(self): dict(type='disk', id=d2.id, path=d2.path, ptable=d2.ptable, serial=d2.serial, info={d2.path: blockdevs[d2.path]}), dict(type='zpool', id='zpool-1', vdevs=[d1.id], pool='p1', - mountpoint='/'), + mountpoint='/', fs_properties=dict(canmount='on')), dict(type='zpool', id='zpool-2', vdevs=[d2.id], pool='p2', mountpoint='/srv', fs_properties=dict(canmount='off')), - dict(type='zfs', id='zfs-1', volume='/ROOT', pool='zpool-1'), + dict(type='zfs', id='zfs-1', volume='/ROOT', pool='zpool-1', + properties=dict(canmount='off')), dict(type='zfs', id='zfs-2', volume='/SRV/srv', pool='zpool-2', properties=dict(mountpoint='/srv', canmount='on')), ] @@ -1340,6 +1341,7 @@ def test_zpool_from_action(self): self.assertEqual('zfs-1', zfs_zp1.id) self.assertEqual(zp1, zfs_zp1.pool) self.assertEqual('/ROOT', zfs_zp1.volume) + self.assertEqual(None, zfs_zp1.path) self.assertTrue(isinstance(zfs_zp2, ZFS)) self.assertEqual('zfs-2', zfs_zp2.id) @@ -1361,19 +1363,14 @@ def test_mount_srv(self): m.add_mount(fs, '/srv') self.assertFalse(m.is_root_mounted()) - def test_zpool_may_provide_rootfs(self): - m = make_model() - make_zpool(model=m, mountpoint='/') - self.assertTrue(m.is_root_mounted()) - def test_zpool_not_rootfs_because_not_canmount(self): m = make_model() - make_zpool(model=m, mountpoint='/', fs_properties=dict(canmount='no')) + make_zpool(model=m, mountpoint='/', fs_properties=dict(canmount='off')) self.assertFalse(m.is_root_mounted()) def test_zpool_rootfs_because_canmount(self): m = make_model() - make_zpool(model=m, mountpoint='/', fs_properties=dict(canmount='yes')) + make_zpool(model=m, mountpoint='/', fs_properties=dict(canmount='on')) self.assertTrue(m.is_root_mounted()) def test_zpool_nonrootfs_mountpoint(self): From 7da80945e9963d961ce2bde13cae71248018ccb1 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Thu, 20 Jul 2023 12:12:06 -0600 Subject: [PATCH 6/7] filesystem: let create_zpool be opinionated --- subiquity/common/filesystem/manipulator.py | 14 +++++++++++++- subiquity/models/filesystem.py | 13 +++---------- subiquity/models/tests/test_filesystem.py | 6 ++---- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/subiquity/common/filesystem/manipulator.py b/subiquity/common/filesystem/manipulator.py index 7980746a7..1ba8e373c 100644 --- a/subiquity/common/filesystem/manipulator.py +++ b/subiquity/common/filesystem/manipulator.py @@ -158,7 +158,19 @@ def delete_logical_volume(self, lv): delete_lvm_partition = delete_logical_volume def create_zpool(self, device, pool, mountpoint): - self.model.add_zpool(device, pool, mountpoint) + fs_properties = dict( + acltype='posixacl', + relatime='on', + canmount='on', + compression='gzip', + devices='off', + xattr='sa', + ) + pool_properties = dict(ashift=12) + + self.model.add_zpool( + device, pool, mountpoint, + fs_properties=fs_properties, pool_properties=pool_properties) def delete(self, obj): if obj is None: diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index e7e955601..fdba638d7 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -1929,21 +1929,14 @@ def should_add_swapfile(self): return False return True - def add_zpool(self, device, pool, mountpoint): - fs_properties = dict( - acltype='posixacl', - relatime='on', - canmount='on', - compression='gzip', - devices='off', - xattr='sa', - ) + def add_zpool(self, device, pool, mountpoint, *, + fs_properties=None, pool_properties=None): zpool = ZPool( m=self, vdevs=[device], pool=pool, mountpoint=mountpoint, - pool_properties=dict(ashift=12), + pool_properties=pool_properties, fs_properties=fs_properties) self._actions.append(zpool) return zpool diff --git a/subiquity/models/tests/test_filesystem.py b/subiquity/models/tests/test_filesystem.py index 33da4d2e1..d5535e4a8 100644 --- a/subiquity/models/tests/test_filesystem.py +++ b/subiquity/models/tests/test_filesystem.py @@ -255,10 +255,8 @@ def make_zpool(model=None, device=None, pool=None, mountpoint=None, **kw): device = make_disk(model) if pool is None: pool = f'pool{len(model._actions)}' - zpool = ZPool(m=model, vdevs=[device], pool=pool, mountpoint=mountpoint, - **kw) - model._actions.append(zpool) - return zpool + return model.add_zpool( + device=device, pool=pool, mountpoint=mountpoint, **kw) def make_zfs(model, *, pool, **kw): From a258f20e6a68fb03df45c13d6c284081ab198708 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Thu, 20 Jul 2023 11:42:00 -0600 Subject: [PATCH 7/7] filesystem: strict canmount --- subiquity/models/filesystem.py | 45 +++++++++++++++++------ subiquity/models/tests/test_filesystem.py | 41 +++++++++++++++++++++ 2 files changed, 74 insertions(+), 12 deletions(-) diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index fdba638d7..512bb1018 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -28,7 +28,6 @@ from typing import List, Optional, Set, Union import more_itertools -import yaml from curtin import storage_config from curtin.block import partition_kname @@ -1075,6 +1074,25 @@ def can_delete(self): return True +def get_canmount(properties: Optional[dict], default: bool) -> bool: + """Handle the possible values of the zfs canmount property, which should be + on/off/noauto. Due to yaml handling, on/off may be turned into a bool, so + handle that also.""" + if properties is None: + return default + vals = { + 'on': True, + 'off': False, + 'noauto': False, + True: True, + False: False, + } + result = vals.get(properties.get('canmount', default)) + if result is None: + raise ValueError('canmount must be one of "on", "off", or "noauto"') + return result + + @fsobj("zpool") class ZPool: vdevs: List[Union[Disk, Partition]] = attributes.reflist( @@ -1099,12 +1117,15 @@ def fstype(self): def name(self): return self.pool + @property + def canmount(self): + return get_canmount(self.fs_properties, False) + @property def path(self): - if self.fs_properties is not None: - if not yaml.safe_load(self.fs_properties.get('canmount', 'off')): - return None - return self.mountpoint + if self.canmount: + return self.mountpoint + return None @fsobj("zfs") @@ -1118,16 +1139,16 @@ class ZFS: def fstype(self): return 'zfs' + @property + def canmount(self): + return get_canmount(self.properties, False) + @property def path(self): - if self.properties is None: - return self.volume - if not yaml.safe_load(self.properties.get('canmount', 'on')): + if self.canmount: + return self.properties.get('mountpoint', self.volume) + else: return None - mountpoint = self.properties.get('mountpoint', None) - if mountpoint is not None: - return mountpoint - return self.volume ConstructedDevice = Union[Raid, LVM_VolGroup, ZPool] diff --git a/subiquity/models/tests/test_filesystem.py b/subiquity/models/tests/test_filesystem.py index d5535e4a8..4c89395d4 100644 --- a/subiquity/models/tests/test_filesystem.py +++ b/subiquity/models/tests/test_filesystem.py @@ -17,6 +17,7 @@ from unittest import mock import attr +import yaml from subiquitycore.tests import SubiTestCase from subiquitycore.tests.parameterized import parameterized @@ -29,6 +30,7 @@ Disk, Filesystem, FilesystemModel, + get_canmount, get_raid_size, humanize_size, NotFinalPartitionError, @@ -1279,6 +1281,45 @@ def test_is_logical(self): self.assertTrue(p7.is_logical) +class TestCanmount(SubiTestCase): + @parameterized.expand(( + ('on', True), + ('"on"', True), + ('true', True), + ('off', False), + ('"off"', False), + ('false', False), + ('noauto', False), + ('"noauto"', False), + )) + def test_present(self, value, expected): + property_yaml = f'canmount: {value}' + properties = yaml.safe_load(property_yaml) + for default in (True, False): + self.assertEqual(expected, get_canmount(properties, default), + f'yaml {property_yaml} default {default}') + + @parameterized.expand(( + ['{}'], + ['something-else: on'], + )) + def test_not_present(self, property_yaml): + properties = yaml.safe_load(property_yaml) + for default in (True, False): + self.assertEqual(default, get_canmount(properties, default), + f'yaml {property_yaml} default {default}') + + @parameterized.expand(( + ['asdf'], + ['"true"'], + ['"false"'], + )) + def test_invalid(self, value): + with self.assertRaises(ValueError): + properties = yaml.safe_load(f'canmount: {value}') + get_canmount(properties, False) + + class TestZPool(SubiTestCase): def test_zpool_to_action(self): m = make_model()