From 96961070ee7b3c005d846d89e0010d2a03bdbb88 Mon Sep 17 00:00:00 2001 From: Corey Wright Date: Wed, 29 Aug 2018 22:57:02 -0500 Subject: [PATCH] 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 | 23 +++++-- .../tests/unit/test_hardware.py | 66 +++++++++++++------ ...data_from_partitions-4f1902533d530b8f.yaml | 8 +++ 3 files changed, 72 insertions(+), 25 deletions(-) create mode 100644 releasenotes/notes/erase_metadata_from_partitions-4f1902533d530b8f.yaml diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index ee54315ce..92a9db543 100644 --- a/ironic_python_agent/hardware.py +++ b/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): - return list_all_block_devices() + def list_block_devices(self, include_partitions=False): + 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): diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 4e28d8a1f..6b7873a34 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/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 - mock__is_vmedia.side_effect = (True, False) + # NOTE(coreywright): Don't return the list, but a copy of it, because + # 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( - '/dev/sda', self.node['uuid']) - mock_list_devs.assert_called_once_with(mock.ANY) - mock__is_vmedia.assert_has_calls([ - mock.call(mock.ANY, block_devices[0]), - mock.call(mock.ANY, block_devices[1]) - ]) + self.assertEqual([mock.call('/dev/sda1', self.node['uuid']), + mock.call('/dev/sda', self.node['uuid'])], + mock_metadata.call_args_list) + mock_list_devs.assert_called_once_with(self.hardware, + include_partitions=True) + 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 - # Simulate /dev/sda failing and /dev/sdb succeeding + # NOTE(coreywright): Don't return the list, but a copy of it, because + # 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([ - mock.call('/dev/sda', self.node['uuid']), - mock.call('/dev/sdb', self.node['uuid']), - ]) - mock_list_devs.assert_called_once_with(mock.ANY) - mock__is_vmedia.assert_has_calls([ - mock.call(mock.ANY, block_devices[0]), - mock.call(mock.ANY, block_devices[1]) - ]) + self.assertEqual([mock.call('/dev/sdb', self.node['uuid']), + mock.call('/dev/sda', self.node['uuid'])], + mock_metadata.call_args_list) + mock_list_devs.assert_called_once_with(self.hardware, + include_partitions=True) + self.assertEqual([mock.call(self.hardware, block_devices[1]), + mock.call(self.hardware, block_devices[0])], + mock__is_vmedia.call_args_list) @mock.patch.object(utils, 'execute', autospec=True) def test_get_bmc_address(self, mocked_execute): diff --git a/releasenotes/notes/erase_metadata_from_partitions-4f1902533d530b8f.yaml b/releasenotes/notes/erase_metadata_from_partitions-4f1902533d530b8f.yaml new file mode 100644 index 000000000..9306e853c --- /dev/null +++ b/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 `_ for details.