From fd9a8acecde925e2bd55a5e611fbbc1c809df523 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Thu, 18 Jan 2018 08:31:31 +1100 Subject: [PATCH] Don't fstrim vfat partitions This small change avoids running fstrim on vfat partitions. The mount order test-case has been updated to also test the mkfs creation components, and the input config modified to have a vfat partition to cover this path. Change-Id: I8952e748d4bdc12a5769706de9057c1e97d95e37 --- .../block_device/level3/mount.py | 17 ++-- .../config/multiple_partitions_graph.yaml | 5 +- .../config/multiple_partitions_tree.yaml | 5 +- .../block_device/tests/test_mount_order.py | 77 +++++++++++++------ 4 files changed, 70 insertions(+), 34 deletions(-) diff --git a/diskimage_builder/block_device/level3/mount.py b/diskimage_builder/block_device/level3/mount.py index 1d653d02..963f12b5 100644 --- a/diskimage_builder/block_device/level3/mount.py +++ b/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 - # system. This discards unused blocks from the mounted + # Before calling umount, call 'fstrim' on suitable 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. - # Calling sync before fstrim is a workaround for this behaviour. + # + # A race condition can occur when trying to fstrim immediately + # 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", - self.state['mount'][self.mount_point]['path']]) + if self.state['filesys'][self.base]['fstype'] != 'vfat': + exec_sudo(["fstrim", "--verbose", + self.state['mount'][self.mount_point]['path']]) exec_sudo(["umount", self.state['mount'][self.mount_point]['path']]) def delete(self): diff --git a/diskimage_builder/block_device/tests/config/multiple_partitions_graph.yaml b/diskimage_builder/block_device/tests/config/multiple_partitions_graph.yaml index 8a46086a..fe262a2b 100644 --- a/diskimage_builder/block_device/tests/config/multiple_partitions_graph.yaml +++ b/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 diff --git a/diskimage_builder/block_device/tests/config/multiple_partitions_tree.yaml b/diskimage_builder/block_device/tests/config/multiple_partitions_tree.yaml index 4c931f65..d60d5a8e 100644 --- a/diskimage_builder/block_device/tests/config/multiple_partitions_tree.yaml +++ b/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: diff --git a/diskimage_builder/block_device/tests/test_mount_order.py b/diskimage_builder/block_device/tests/test_mount_order.py index 20403941..65a09ce8 100644 --- a/diskimage_builder/block_device/tests/test_mount_order.py +++ b/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 - # all the parent calls that would really make these values, as - # we just want to test MountPointNode - state['filesys'] = { - 'mkfs_root': {'device': 'fake_root_device'}, - 'mkfs_var': {'device': 'fake_var_device'}, - 'mkfs_var_log': {'device': 'fake_var_log_device'} - } + # Mocked block device state + state['blockdev'] = {} + state['blockdev']['root'] = {'device': '/dev/loopXp1/root'} + state['blockdev']['var'] = {'device': '/dev/loopXp2/var'} + state['blockdev']['var_log'] = {'device': '/dev/loopXp3/var_log'} 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_var': {'device': 'fake_var_device'}, - 'mkfs_boot': {'device': 'fake_boot_device'} + 'mkfs_root': { + 'device': '/dev/loopXp1', + '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/']),