Merge "Don't fstrim vfat partitions"

diskimage_builder/block_device/level3/mount.py

@@ -95,17 +95,20 @@ class MountPointNode(NodeBase):
 
     def umount(self):
         logger.info("Called for [%s]", self.name)
-        # Before calling umount call 'fstrim' on the mounted file
+        # Before calling umount, call 'fstrim' on suitable mounted
-        # system.  This discards unused blocks from the mounted
+        # file systems.  This discards unused blocks from the mounted
         # file system and therefore decreases the resulting image
         # size.
-        # A race condition can occur when trying to fstrim immediately after
+        #
-        # deleting a file resulting in that free space not being reclaimed.
+        # A race condition can occur when trying to fstrim immediately
-        # Calling sync before fstrim is a workaround for this behaviour.
+        # after deleting a file resulting in that free space not being
+        # reclaimed.  Calling sync before fstrim is a workaround for
+        # this behaviour.
         # https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02978.html
         exec_sudo(["sync"])
-        exec_sudo(["fstrim", "--verbose",
+        if self.state['filesys'][self.base]['fstype'] != 'vfat':
-                   self.state['mount'][self.mount_point]['path']])
+            exec_sudo(["fstrim", "--verbose",
+                       self.state['mount'][self.mount_point]['path']])
         exec_sudo(["umount", self.state['mount'][self.mount_point]['path']])
 
     def delete(self):

diskimage_builder/block_device/tests/config/multiple_partitions_graph.yaml

@@ -21,6 +21,7 @@
     base: root
     name: mkfs_root
     type: xfs
+    uuid: root-uuid-1234
 
 - mount:
     base: mkfs_root
@@ -37,6 +38,7 @@
     base: var
     name: mkfs_var
     type: xfs
+    uuid: var-uuid-1234
 
 - mount:
     base: mkfs_var
@@ -52,7 +54,8 @@
 - mkfs:
     base: var_log
     name: mkfs_var_log
-    type: xfs
+    type: vfat
+    label: varlog
 
 - mount:
     base: mkfs_var_log

diskimage_builder/block_device/tests/config/multiple_partitions_tree.yaml

@@ -11,6 +11,7 @@
           size: 55%
           mkfs:
             type: xfs
+            uuid: root-uuid-1234
             mount:
               mount_point: /
               fstab:
@@ -20,6 +21,7 @@
           size: 40%
           mkfs:
             type: xfs
+            uuid: var-uuid-1234
             mount:
               mount_point: /var
               fstab:
@@ -28,7 +30,8 @@
         - name: var_log
           size: 5%
           mkfs:
-            type: xfs
+            type: vfat
+            label: varlog
             mount:
               mount_point: /var/log
               fstab:

diskimage_builder/block_device/tests/test_mount_order.py

@@ -19,6 +19,7 @@ import diskimage_builder.block_device.tests.test_config as tc
 
 from diskimage_builder.block_device.config import config_tree_to_graph
 from diskimage_builder.block_device.config import create_graph
+from diskimage_builder.block_device.level2.mkfs import FilesystemNode
 from diskimage_builder.block_device.level3.mount import cmp_mount_order
 from diskimage_builder.block_device.level3.mount import MountPointNode
 from diskimage_builder.block_device.tests.test_base import TestBase
@@ -59,7 +60,10 @@ class TestMountOrder(tc.TestGraphGeneration):
 
     @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo',
                 side_effect=_exec_sudo_log)
-    def test_mount_order(self, mock_exec_sudo):
+    @mock.patch('diskimage_builder.block_device.level2.mkfs.exec_sudo',
+                side_effect=_exec_sudo_log)
+    def test_mfks_and_mount_order(self, mock_exec_sudo_mkfs,
+                                  mock_exec_sudo_mount):
         # XXX: better mocking for the os.path.exists calls to avoid
         # failing if this exists.
         self.assertFalse(os.path.exists('/fake/'))
@@ -71,36 +75,50 @@ class TestMountOrder(tc.TestGraphGeneration):
         graph, call_order = create_graph(config, self.fake_default_config,
                                          state)
 
-        # build up some fake state so that we don't have to mock out
+        # Mocked block device state
-        # all the parent calls that would really make these values, as
+        state['blockdev'] = {}
-        # we just want to test MountPointNode
+        state['blockdev']['root'] = {'device': '/dev/loopXp1/root'}
-        state['filesys'] = {
+        state['blockdev']['var'] = {'device': '/dev/loopXp2/var'}
-            'mkfs_root': {'device': 'fake_root_device'},
+        state['blockdev']['var_log'] = {'device': '/dev/loopXp3/var_log'}
-            'mkfs_var': {'device': 'fake_var_device'},
-            'mkfs_var_log': {'device': 'fake_var_log_device'}
-        }
 
         for node in call_order:
-            if isinstance(node, MountPointNode):
+            if isinstance(node, (FilesystemNode, MountPointNode)):
                 node.create()
         for node in reversed(call_order):
-            if isinstance(node, MountPointNode):
+            if isinstance(node, (FilesystemNode, MountPointNode)):
                 node.umount()
 
-        # ensure that partitions are mounted in order root->var->var/log
+        # ensure that partitions were mounted in order root->var->var/log
         self.assertListEqual(state['mount_order'], ['/', '/var', '/var/log'])
 
+        # fs creation sequence (note we don't care about order of this
+        # as they're all independent)
+        cmd_sequence = [
+            mock.call(['mkfs', '-t', 'xfs', '-L', 'mkfs_root',
+                       '-m', 'uuid=root-uuid-1234',
+                       '-q', '/dev/loopXp1/root']),
+            mock.call(['mkfs', '-t', 'xfs', '-L', 'mkfs_var',
+                       '-m', 'uuid=var-uuid-1234',
+                       '-q', '/dev/loopXp2/var']),
+            mock.call(['mkfs', '-t', 'vfat', '-n', 'varlog',
+                       '/dev/loopXp3/var_log'])
+        ]
+        self.assertEqual(mock_exec_sudo_mkfs.call_count, len(cmd_sequence))
+        mock_exec_sudo_mkfs.assert_has_calls(cmd_sequence, any_order=True)
+
+        # Check mount sequence
         cmd_sequence = [
             # mount sequence
             mock.call(['mkdir', '-p', '/fake/']),
-            mock.call(['mount', 'fake_root_device', '/fake/']),
+            mock.call(['mount', '/dev/loopXp1/root', '/fake/']),
             mock.call(['mkdir', '-p', '/fake/var']),
-            mock.call(['mount', 'fake_var_device', '/fake/var']),
+            mock.call(['mount', '/dev/loopXp2/var', '/fake/var']),
             mock.call(['mkdir', '-p', '/fake/var/log']),
-            mock.call(['mount', 'fake_var_log_device', '/fake/var/log']),
+            mock.call(['mount', '/dev/loopXp3/var_log', '/fake/var/log']),
             # umount sequence
             mock.call(['sync']),
-            mock.call(['fstrim', '--verbose', '/fake/var/log']),
+            # note /fake/var/log is a vfs partition to make sure
+            # we don't try to fstrim it
             mock.call(['umount', '/fake/var/log']),
             mock.call(['sync']),
             mock.call(['fstrim', '--verbose', '/fake/var']),
@@ -109,13 +127,13 @@ class TestMountOrder(tc.TestGraphGeneration):
             mock.call(['fstrim', '--verbose', '/fake/']),
             mock.call(['umount', '/fake/'])
         ]
-        self.assertListEqual(mock_exec_sudo.call_args_list, cmd_sequence)
+        self.assertListEqual(mock_exec_sudo_mount.call_args_list, cmd_sequence)
 
     @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo',
                 side_effect=_exec_sudo_log)
     def test_mount_order_unsorted(self, mock_exec_sudo):
         # As above, but this is out of order and gets sorted
-        # so that root is mounted first.
+        # so that root is mounted first (and skips the mkfs testing).
         config = self.load_config_file('lvm_tree_partition_ordering.yaml')
         parsed_graph = config_tree_to_graph(config)
         state = {}
@@ -124,9 +142,18 @@ class TestMountOrder(tc.TestGraphGeneration):
                                          self.fake_default_config,
                                          state)
         state['filesys'] = {
-            'mkfs_root': {'device': 'fake_root_device'},
+            'mkfs_root': {
-            'mkfs_var': {'device': 'fake_var_device'},
+                'device': '/dev/loopXp1',
-            'mkfs_boot': {'device': 'fake_boot_device'}
+                'fstype': 'xfs'
+            },
+            'mkfs_var': {
+                'device': '/dev/loopXp2',
+                'fstype': 'xfs',
+            },
+            'mkfs_boot': {
+                'device': '/dev/loopXp3',
+                'fstype': 'vfat',
+            },
         }
 
         for node in call_order:
@@ -142,17 +169,17 @@ class TestMountOrder(tc.TestGraphGeneration):
         cmd_sequence = [
             # mount sequence
             mock.call(['mkdir', '-p', '/fake/']),
-            mock.call(['mount', 'fake_root_device', '/fake/']),
+            mock.call(['mount', '/dev/loopXp1', '/fake/']),
             mock.call(['mkdir', '-p', '/fake/boot']),
-            mock.call(['mount', 'fake_boot_device', '/fake/boot']),
+            mock.call(['mount', '/dev/loopXp3', '/fake/boot']),
             mock.call(['mkdir', '-p', '/fake/var']),
-            mock.call(['mount', 'fake_var_device', '/fake/var']),
+            mock.call(['mount', '/dev/loopXp2', '/fake/var']),
             # umount sequence
             mock.call(['sync']),
             mock.call(['fstrim', '--verbose', '/fake/var']),
             mock.call(['umount', '/fake/var']),
             mock.call(['sync']),
-            mock.call(['fstrim', '--verbose', '/fake/boot']),
+            # no trim on vfat /fake/boot
             mock.call(['umount', '/fake/boot']),
             mock.call(['sync']),
             mock.call(['fstrim', '--verbose', '/fake/']),