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 <andreas@florath.net>

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(
+        self.lvm_umount_node = LVMUmountNode(
-            config['name'] + "-CLEANUP", state, self.pvs)
+            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]

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)):
+                if isinstance(node, (LVMNode, LVMUmountNode,
-                    node.cleanup()
+                                     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)