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 957c6fe50..512bb1018 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -1074,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,8 +1118,14 @@ def name(self): return self.pool @property - def mount(self): - return self.mountpoint + def canmount(self): + return get_canmount(self.fs_properties, False) + + @property + def path(self): + if self.canmount: + return self.mountpoint + return None @fsobj("zfs") @@ -1110,9 +1135,29 @@ class ZFS: # options to pass to zfs dataset creation properties: Optional[dict] = None + @property + def fstype(self): + return 'zfs' + + @property + def canmount(self): + return get_canmount(self.properties, False) + + @property + def path(self): + if self.canmount: + return self.properties.get('mountpoint', self.volume) + else: + return None + 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) @@ -1599,7 +1644,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 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: @@ -1612,7 +1657,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: @@ -1711,6 +1756,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 @@ -1876,12 +1927,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): @@ -1901,21 +1950,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 512b22fea..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, @@ -195,8 +197,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,9 +257,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) - model._actions.append(zpool) - return zpool + return model.add_zpool( + device=device, pool=pool, mountpoint=mountpoint, **kw) def make_zfs(model, *, pool, **kw): @@ -1280,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() @@ -1299,36 +1339,77 @@ 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', - mountpoint='/'), - dict(type='zfs', id='zfs-1', volume='/ROOT', pool='zpool-1'), + 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='/', 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', + properties=dict(canmount='off')), + 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.assertEqual(None, zfs_zp1.path) + + 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): - def test_zpool_may_provide_rootfs(self): + 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_not_rootfs_because_not_canmount(self): + m = make_model() + 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='/') + make_zpool(model=m, mountpoint='/', fs_properties=dict(canmount='on')) self.assertTrue(m.is_root_mounted()) def test_zpool_nonrootfs_mountpoint(self): diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index f70c3092e..957f2ebb2 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)