From f5736f3178a69ceeb5eaa9f48aa37d13e41e00c4 Mon Sep 17 00:00:00 2001 From: Andreas Florath Date: Thu, 14 Sep 2017 05:14:46 +0000 Subject: [PATCH] block-device lvm: fix umount phase As described in blockdevice.py detachment and (most) resources release must be done in the umount phase of a block device module. Until now these jobs were done in the lvm cleanup() phase - which is too late - especially when using nested LVMs. This patch moves the functionality of the cleanup() phase to the umount() phase for the lvm module. It includes a test case that fails without applying the provided source code changes. Change-Id: I697bfbf042816c5ddf170bde9534cc4f0c7279ff Signed-off-by: Andreas Florath --- diskimage_builder/block_device/level1/lvm.py | 26 +++++++++---------- .../block_device/tests/test_lvm.py | 20 +++++++------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/diskimage_builder/block_device/level1/lvm.py b/diskimage_builder/block_device/level1/lvm.py index 1aceb4fd8..2da53d1f7 100644 --- a/diskimage_builder/block_device/level1/lvm.py +++ b/diskimage_builder/block_device/level1/lvm.py @@ -155,7 +155,7 @@ class VgsNode(NodeBase): 'devices': self.base, } - def _cleanup(self): + def _umount(self): exec_sudo(['vgchange', '-an', self.name]) def get_edges(self): @@ -214,7 +214,7 @@ class LvsNode(NodeBase): 'device': '/dev/mapper/%s-%s' % (self.base, self.name) } - def _cleanup(self): + def _umount(self): exec_sudo(['lvchange', '-an', '/dev/%s/%s' % (self.base, self.name)]) @@ -280,19 +280,19 @@ class LVMNode(NodeBase): for lvs in self.lvs: lvs._create() - def cleanup(self): + def umount(self): for lvs in self.lvs: - lvs._cleanup() + lvs._umount() for vgs in self.vgs: - vgs._cleanup() + vgs._umount() exec_sudo(['udevadm', 'settle']) -class LVMCleanupNode(NodeBase): +class LVMUmountNode(NodeBase): def __init__(self, name, state, pvs): - """Cleanup Node for LVM + """Umount Node for LVM Information about the PV, VG and LV is typically cached in lvmetad. Even after removing PV device and @@ -300,17 +300,17 @@ class LVMCleanupNode(NodeBase): which leads to a couple of problems. the 'pvscan --cache' scans the available disks and updates the cache. - This must be called after the cleanup of the + This must be called after the umount of the containing block device is done. """ - super(LVMCleanupNode, self).__init__(name, state) + super(LVMUmountNode, self).__init__(name, state) self.pvs = pvs def create(self): # This class is used for cleanup only pass - def cleanup(self): + def umount(self): try: exec_sudo(['pvscan', '--cache']) except subprocess.CalledProcessError as cpe: @@ -412,12 +412,12 @@ class LVMPlugin(PluginBase): # create the "driver" node self.lvm_node = LVMNode(config['name'], state, self.pvs, self.lvs, self.vgs) - self.lvm_cleanup_node = LVMCleanupNode( - config['name'] + "-CLEANUP", state, self.pvs) + self.lvm_umount_node = LVMUmountNode( + config['name'] + "-UMOUNT", state, self.pvs) def get_nodes(self): # the nodes for insertion into the graph are all of the pvs, # vgs and lvs nodes we have created above, the root node and # the cleanup node. return self.pvs + self.vgs + self.lvs \ - + [self.lvm_node, self.lvm_cleanup_node] + + [self.lvm_node, self.lvm_umount_node] diff --git a/diskimage_builder/block_device/tests/test_lvm.py b/diskimage_builder/block_device/tests/test_lvm.py index 14e1d4a9b..3ca37287c 100644 --- a/diskimage_builder/block_device/tests/test_lvm.py +++ b/diskimage_builder/block_device/tests/test_lvm.py @@ -21,9 +21,9 @@ 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.exception import \ BlockDeviceSetupException -from diskimage_builder.block_device.level1.lvm import LVMCleanupNode from diskimage_builder.block_device.level1.lvm import LVMNode from diskimage_builder.block_device.level1.lvm import LVMPlugin +from diskimage_builder.block_device.level1.lvm import LVMUmountNode from diskimage_builder.block_device.level1.lvm import LvsNode from diskimage_builder.block_device.level1.lvm import PvsNode from diskimage_builder.block_device.level1.lvm import VgsNode @@ -89,7 +89,7 @@ class TestLVM(tc.TestGraphGeneration): # XXX: This has not mocked out the "lower" layers of # creating the devices, which we're assuming works OK, nor # the upper layers. - if isinstance(node, (LVMNode, LVMCleanupNode, PvsNode, + if isinstance(node, (LVMNode, LVMUmountNode, PvsNode, VgsNode, LvsNode)): # only the LVMNode actually does anything here... node.create() @@ -198,7 +198,7 @@ class TestLVM(tc.TestGraphGeneration): # XXX: This has not mocked out the "lower" layers of # creating the devices, which we're assuming works OK, nor # the upper layers. - if isinstance(node, (LVMNode, LVMCleanupNode, PvsNode, + if isinstance(node, (LVMNode, LVMUmountNode, PvsNode, VgsNode, LvsNode)): # only the PvsNode actually does anything here... node.create() @@ -282,7 +282,7 @@ class TestLVM(tc.TestGraphGeneration): self.assertDictEqual(state['blockdev'], blockdev_state) # - # Cleanup test + # Umount test # with mock.patch(exec_sudo) as mock_exec_sudo, \ mock.patch('tempfile.NamedTemporaryFile') as mock_temp, \ @@ -308,9 +308,9 @@ class TestLVM(tc.TestGraphGeneration): reverse_order = reversed(call_order) for node in reverse_order: - if isinstance(node, (LVMNode, LVMCleanupNode, PvsNode, + if isinstance(node, (LVMNode, LVMUmountNode, PvsNode, VgsNode, LvsNode)): - node.cleanup() + node.umount() cmd_sequence = [ # delete the lv's @@ -365,7 +365,7 @@ class TestLVM(tc.TestGraphGeneration): # XXX: This has not mocked out the "lower" layers of # creating the devices, which we're assuming works OK, nor # the upper layers. - if isinstance(node, (LVMNode, LVMCleanupNode, + if isinstance(node, (LVMNode, LVMUmountNode, PvsNode, VgsNode, LvsNode)): # only the LVMNode actually does anything here... node.create() @@ -409,8 +409,9 @@ class TestLVM(tc.TestGraphGeneration): reverse_order = reversed(call_order) for node in reverse_order: - if isinstance(node, (LVMNode, PvsNode, VgsNode, LvsNode)): - node.cleanup() + if isinstance(node, (LVMNode, LVMUmountNode, + PvsNode, VgsNode, LvsNode)): + node.umount() cmd_sequence = [ # deactivate lv's @@ -422,6 +423,7 @@ class TestLVM(tc.TestGraphGeneration): mock.call(['vgchange', '-an', 'vg_data']), mock.call(['udevadm', 'settle']), + mock.call(['pvscan', '--cache']), ] self.assertListEqual(mock_exec_sudo.call_args_list, cmd_sequence)