Merge pull request #1730 from dbungert/zfs-action-ordering

Zfs action ordering

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:

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

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):

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)