Allow erasing metadata from disk partitions

Modify the metadata erasing call chain to retrieve a list of devices
that includes partitions in addition to disks so it can erase metadata
from all of them, otherwise incidentally recreating disk partitions
causes the Linux kernel to discover and automatically recreate some
types of storage entities (eg LVM PVs, VGs, & LVs, RAID members &
devices).

Change-Id: If8f47a083966051856439e3291a6872929b93e3b
Story: #2003673
Task: #26192

ironic_python_agent/hardware.py

@@ -363,7 +363,12 @@ class HardwareManager(object):
     def get_cpus(self):
         raise errors.IncompatibleHardwareMethodError
 
-    def list_block_devices(self):
+    def list_block_devices(self, include_partitions=False):
+        """List physical block devices
+
+        :param include_partitions: If to include partitions
+        :return: A list of BlockDevices
+        """
         raise errors.IncompatibleHardwareMethodError
 
     def get_memory(self):
@@ -762,8 +767,14 @@ class GenericHardwareManager(HardwareManager):
 
         return Memory(total=total, physical_mb=physical)
 
-    def list_block_devices(self):
+    def list_block_devices(self, include_partitions=False):
-        return list_all_block_devices()
+        block_devices = list_all_block_devices()
+        if include_partitions:
+            block_devices.extend(
+                list_all_block_devices(block_type='part',
+                                       ignore_raid=True)
+            )
+        return block_devices
 
     def get_os_install_device(self):
         cached_node = get_cached_node()
@@ -866,7 +877,11 @@ class GenericHardwareManager(HardwareManager):
         :raises BlockDeviceEraseError: when there's an error erasing the
                 block device
         """
-        block_devices = self.list_block_devices()
+        block_devices = self.list_block_devices(include_partitions=True)
+        # NOTE(coreywright): Reverse sort by device name so a partition (eg
+        # sda1) is processed before it disappears when its associated disk (eg
+        # sda) has its partition table erased and the kernel notified.
+        block_devices.sort(key=lambda dev: dev.name, reverse=True)
         erase_errors = {}
         for dev in block_devices:
             if self._is_virtual_media_device(dev):

ironic_python_agent/tests/unit/test_hardware.py

@@ -1148,6 +1148,19 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         list_mock.assert_called_once_with()
 
+    @mock.patch.object(hardware, 'list_all_block_devices', autospec=True)
+    def test_list_block_devices_including_partitions(self, list_mock):
+        device = hardware.BlockDevice('/dev/hdaa', 'small', 65535, False)
+        partition = hardware.BlockDevice('/dev/hdaa1', '', 32767, False)
+        list_mock.side_effect = [[device], [partition]]
+        devices = self.hardware.list_block_devices(include_partitions=True)
+
+        self.assertEqual([device, partition], devices)
+
+        self.assertEqual([mock.call(), mock.call(block_type='part',
+                                                 ignore_raid=True)],
+                         list_mock.call_args_list)
+
     @mock.patch.object(os, 'readlink', autospec=True)
     @mock.patch.object(os, 'listdir', autospec=True)
     @mock.patch.object(hardware, '_get_device_info', autospec=True)
@@ -1965,18 +1978,24 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         block_devices = [
             hardware.BlockDevice('/dev/sr0', 'vmedia', 12345, True),
             hardware.BlockDevice('/dev/sda', 'small', 65535, False),
+            hardware.BlockDevice('/dev/sda1', '', 32767, False),
         ]
-        mock_list_devs.return_value = block_devices
+        # NOTE(coreywright): Don't return the list, but a copy of it, because
-        mock__is_vmedia.side_effect = (True, False)
+        # we depend on its elements' order when referencing it later during
+        # verification, but the method under test sorts the list changing it.
+        mock_list_devs.return_value = list(block_devices)
+        mock__is_vmedia.side_effect = lambda _, dev: dev.name == '/dev/sr0'
 
         self.hardware.erase_devices_metadata(self.node, [])
-        mock_metadata.assert_called_once_with(
+        self.assertEqual([mock.call('/dev/sda1', self.node['uuid']),
-            '/dev/sda', self.node['uuid'])
+                          mock.call('/dev/sda', self.node['uuid'])],
-        mock_list_devs.assert_called_once_with(mock.ANY)
+                         mock_metadata.call_args_list)
-        mock__is_vmedia.assert_has_calls([
+        mock_list_devs.assert_called_once_with(self.hardware,
-            mock.call(mock.ANY, block_devices[0]),
+                                               include_partitions=True)
-            mock.call(mock.ANY, block_devices[1])
+        self.assertEqual([mock.call(self.hardware, block_devices[0]),
-        ])
+                          mock.call(self.hardware, block_devices[2]),
+                          mock.call(self.hardware, block_devices[1])],
+                         mock__is_vmedia.call_args_list)
 
     @mock.patch.object(hardware.GenericHardwareManager,
                        '_is_virtual_media_device', autospec=True)
@@ -1990,28 +2009,33 @@ class TestGenericHardwareManager(base.IronicAgentTest):
             hardware.BlockDevice('/dev/sdb', 'big', 10737418240, True),
         ]
         mock__is_vmedia.return_value = False
-        mock_list_devs.return_value = block_devices
+        # NOTE(coreywright): Don't return the list, but a copy of it, because
-        # Simulate /dev/sda failing and /dev/sdb succeeding
+        # we depend on its elements' order when referencing it later during
+        # verification, but the method under test sorts the list changing it.
+        mock_list_devs.return_value = list(block_devices)
+        # Simulate first call to destroy_disk_metadata() failing, which is for
+        # /dev/sdb due to erase_devices_metadata() reverse sorting block
+        # devices by name, and second call succeeding, which is for /dev/sda
         error_output = 'Booo00000ooommmmm'
+        error_regex = '(?s)/dev/sdb.*' + error_output
         mock_metadata.side_effect = (
             processutils.ProcessExecutionError(error_output),
             None,
         )
 
-        self.assertRaisesRegex(errors.BlockDeviceEraseError, error_output,
+        self.assertRaisesRegex(errors.BlockDeviceEraseError, error_regex,
                                self.hardware.erase_devices_metadata,
                                self.node, [])
         # Assert all devices are erased independent if one of them
         # failed previously
-        mock_metadata.assert_has_calls([
+        self.assertEqual([mock.call('/dev/sdb', self.node['uuid']),
-            mock.call('/dev/sda', self.node['uuid']),
+                          mock.call('/dev/sda', self.node['uuid'])],
-            mock.call('/dev/sdb', self.node['uuid']),
+                         mock_metadata.call_args_list)
-        ])
+        mock_list_devs.assert_called_once_with(self.hardware,
-        mock_list_devs.assert_called_once_with(mock.ANY)
+                                               include_partitions=True)
-        mock__is_vmedia.assert_has_calls([
+        self.assertEqual([mock.call(self.hardware, block_devices[1]),
-            mock.call(mock.ANY, block_devices[0]),
+                          mock.call(self.hardware, block_devices[0])],
-            mock.call(mock.ANY, block_devices[1])
+                         mock__is_vmedia.call_args_list)
-        ])
 
     @mock.patch.object(utils, 'execute', autospec=True)
     def test_get_bmc_address(self, mocked_execute):

releasenotes/notes/erase_metadata_from_partitions-4f1902533d530b8f.yaml

@@ -0,0 +1,8 @@
+---
+features:
+  - |
+    Erase metadata on disk partitions to prevent the Linux kernel from
+    automatically recreating storage entities (eg LVM, RAID) described by the
+    metadata. The Hardware Manager API was correspondingly modified to
+    optionally include partitions when listing block devices. See `story
+    2003673 <https://storyboard.openstack.org/#!/story/2003673>`_ for details.