From ed3c5d971123ac2962ad93c64ebf7aed6d7a5c90 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Mon, 18 Sep 2017 11:50:54 +1000 Subject: [PATCH] Actually sort mount-point list We intended to do an in-place sort of the mount-point list, but sorted() returns a new list that wasn't captured. Move to the .sort() function. It seems the existing unit-test missed this. Add a new test taken from the bug which does exhibit a sorting issue. Also added a unit-test of just the comparitor for sanity. Closes-Bug: 1699437 Change-Id: I8101e4a1804a4af7dbda20d48bf362c3f4ad2742 --- .../block_device/level3/mount.py | 4 +- .../config/lvm_tree_partition_ordering.yaml | 64 +++++++++++++++++++ .../block_device/tests/test_mount_order.py | 57 ++++++++++++++++- 3 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 diskimage_builder/block_device/tests/config/lvm_tree_partition_ordering.yaml diff --git a/diskimage_builder/block_device/level3/mount.py b/diskimage_builder/block_device/level3/mount.py index 43b42fa6..47fa785d 100644 --- a/diskimage_builder/block_device/level3/mount.py +++ b/diskimage_builder/block_device/level3/mount.py @@ -149,8 +149,8 @@ class Mount(PluginBase): "Mount point [%s] specified more than once" % self.node.mount_point) sorted_mount_points.append((self.node.mount_point, self.node.name)) - sorted(sorted_mount_points, key=functools.cmp_to_key(cmp_mount_order)) - # reset the state key to the new list + sorted_mount_points.sort(key=functools.cmp_to_key(cmp_mount_order)) + # Save the state if it's new (otherwise this is idempotent update) state['sorted_mount_points'] = sorted_mount_points logger.debug("Ordered mounts now: %s", sorted_mount_points) diff --git a/diskimage_builder/block_device/tests/config/lvm_tree_partition_ordering.yaml b/diskimage_builder/block_device/tests/config/lvm_tree_partition_ordering.yaml new file mode 100644 index 00000000..06526b8a --- /dev/null +++ b/diskimage_builder/block_device/tests/config/lvm_tree_partition_ordering.yaml @@ -0,0 +1,64 @@ +- local_loop: + name: image0 + size: 5G + +- partitioning: + base: image0 + label: mbr + partitions: + - name: boot + flags: [ boot,primary ] + size: 1G + - name: root + flags: [ primary ] + size: 3G + +- lvm: + name: lvm + base: [root] + pvs: + - name: pv + base: root + + vgs: + - name: vg + base: ["pv"] + + lvs: + - name: lv_root + base: vg + size: 2000M + + - name: lv_var + base: vg + size: 500M + +- mkfs: + base: boot + type: ext3 + mount: + mount_point: /boot + fstab: + options: "nodev,nosuid" + fsck-passno: 2 + +- mkfs: + name: mkfs_root + base: lv_root + label: "img-rootfs" + type: "ext4" + mount: + mount_point: / + fstab: + options: "noacl,errors=remount-ro" + fsck-passno: 1 + +- mkfs: + name: mkfs_var + base: lv_var + type: "ext4" + mount: + mount_point: /var + fstab: + options: "noacl" + fsck-passno: 2 diff --git a/diskimage_builder/block_device/tests/test_mount_order.py b/diskimage_builder/block_device/tests/test_mount_order.py index e2838ea8..bd7c97ae 100644 --- a/diskimage_builder/block_device/tests/test_mount_order.py +++ b/diskimage_builder/block_device/tests/test_mount_order.py @@ -10,22 +10,51 @@ # License for the specific language governing permissions and limitations # under the License. +import functools import logging import mock 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.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 logger = logging.getLogger(__name__) +class TestMountComparator(TestBase): + + def test_mount_comparator(self): + # This tests cmp_mount_order to ensure it sorts in the + # expected order. The comparator takes a tuple of + # (mount_point, node_name) but we can ignore the name + partitions = [ + ('/var/log', 'fake_log'), + ('/boot', 'fake_boot'), + ('/', 'fake_name'), + ('/var', 'fake_name')] + partitions.sort(key=functools.cmp_to_key(cmp_mount_order)) + + res = list(x[0] for x in partitions) + + # "/" must be first + self.assertEqual(res[0], '/') + + # /var before /var/log + var_pos = res.index('/var') + var_log_pos = res.index('/var/log') + self.assertGreater(var_log_pos, var_pos) + + class TestMountOrder(tc.TestGraphGeneration): @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo') def test_mount_order(self, mock_exec_sudo): - + # This is probably in order after graph creation, so ensure it + # remains stable config = self.load_config_file('multiple_partitions_graph.yaml') state = {} @@ -53,3 +82,29 @@ class TestMountOrder(tc.TestGraphGeneration): # ensure that partitions are mounted in order root->var->var/log self.assertListEqual(state['mount_order'], ['/', '/var', '/var/log']) + + @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo') + 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. + config = self.load_config_file('lvm_tree_partition_ordering.yaml') + parsed_graph = config_tree_to_graph(config) + state = {} + + graph, call_order = create_graph(parsed_graph, + self.fake_default_config, + state) + state['filesys'] = {} + state['filesys']['mkfs_root'] = {} + state['filesys']['mkfs_root']['device'] = 'fake' + state['filesys']['mkfs_var'] = {} + state['filesys']['mkfs_var']['device'] = 'fake' + state['filesys']['mkfs_boot'] = {} + state['filesys']['mkfs_boot']['device'] = 'fake' + + for node in call_order: + if isinstance(node, MountPointNode): + node.create() + + # ensure that partitions are mounted in order / -> /boot -> /var + self.assertListEqual(state['mount_order'], ['/', '/boot', '/var'])