From d52872809044c43325e1642ea03f642c200d1f0c Mon Sep 17 00:00:00 2001 From: Lucas Alvares Gomes Date: Mon, 18 Jul 2016 17:18:14 +0100 Subject: [PATCH] Add erase_devices_metadata cleaning step This patch is adding a new cleaning step called "erase_devices_metadata" to the GenericHardwareManager. This step is responsible for erasing the metadata of the disks present in the node (partition tables, signatures, filesystem identifiers etc...). It's important to note that the "erase_devices" cleaning step will also remove all these metadatas (because it will zero/shred the whole disk) but, it takes a lot of time to run and for some usages of Ironic only cleaning the device metadata and leaving the data from previous tenants on the disk after the machine is recycled is fine. That's the use case for systems using Ironic just to install the same base image onto many nodes which will run another cloud on top afterwards (TripleO). The new cleaning step has a default priority of 99, so it should run before the "erase_devices" cleaning step so that we can guarantee that the metadata was removed even in case of a failure when cleaning the disks. The version of the GenericHardwareManager was bumped to "1.1" with the addition of this new clean step. This patch make use of the "destroy_disk_metadata" method from ironic-lib to get rid of the metadata. Closes-Bug: #1603411 Change-Id: I3d7b39d5ee3e03ce63185e4168b1ac954a896c93 --- ironic_python_agent/hardware.py | 62 ++++++++++++--- .../tests/unit/test_hardware.py | 79 +++++++++++++++++++ ...-metadata-clean-step-31b4a615c0ff7f18.yaml | 6 ++ 3 files changed, 136 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/erase-device-metadata-clean-step-31b4a615c0ff7f18.yaml diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 1248eb047..bb5cf5176 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -19,7 +19,7 @@ import os import shlex import time - +from ironic_lib import disk_utils import netifaces from oslo_concurrency import processutils from oslo_config import cfg @@ -399,15 +399,7 @@ class HardwareManager(object): dict as defined above """ - return [ - { - 'step': 'erase_devices', - 'priority': 10, - 'interface': 'deploy', - 'reboot_requested': False, - 'abortable': True - } - ] + return [] def get_version(self): """Get a name and version for this hardware manager. @@ -437,7 +429,8 @@ class HardwareManager(object): class GenericHardwareManager(HardwareManager): HARDWARE_MANAGER_NAME = 'generic_hardware_manager' - HARDWARE_MANAGER_VERSION = '1.0' + # 1.1 - Added new clean step called erase_devices_metadata + HARDWARE_MANAGER_VERSION = '1.1' def __init__(self): self.sys_path = '/sys' @@ -753,6 +746,35 @@ class GenericHardwareManager(HardwareManager): LOG.error(msg) raise errors.IncompatibleHardwareMethodError(msg) + def erase_devices_metadata(self, node, ports): + """Attempt to erase the disk devices metadata. + + :param node: Ironic node object + :param ports: list of Ironic port objects + :raises BlockDeviceEraseError when there's an error erasing the + block device + """ + block_devices = self.list_block_devices() + erase_errors = {} + for dev in block_devices: + if self._is_virtual_media_device(dev): + LOG.info("Skipping the erase of virtual media device %s", + dev.name) + continue + + try: + disk_utils.destroy_disk_metadata(dev.name, node['uuid']) + except processutils.ProcessExecutionError as e: + LOG.error('Failed to erase the metadata on device "%(dev)s". ' + 'Error: %(error)s', {'dev': dev.name, 'error': e}) + erase_errors[dev.name] = e + + if erase_errors: + excpt_msg = ('Failed to erase the metadata on the device(s): %s' % + '; '.join(['"%s": %s' % (k, v) + for k, v in erase_errors.items()])) + raise errors.BlockDeviceEraseError(excpt_msg) + def _shred_block_device(self, node, block_device): """Erase a block device using shred. @@ -897,6 +919,24 @@ class GenericHardwareManager(HardwareManager): return out.strip() + def get_clean_steps(self, node, ports): + return [ + { + 'step': 'erase_devices', + 'priority': 10, + 'interface': 'deploy', + 'reboot_requested': False, + 'abortable': True + }, + { + 'step': 'erase_devices_metadata', + 'priority': 99, + 'interface': 'deploy', + 'reboot_requested': False, + 'abortable': True + } + ] + def _compare_extensions(ext1, ext2): mgr1 = ext1.obj diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 9beb48e0e..d18a838de 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -15,6 +15,7 @@ import os import time +from ironic_lib import disk_utils import mock import netifaces from oslo_concurrency import processutils @@ -284,6 +285,26 @@ class TestGenericHardwareManager(test_base.BaseTestCase): CONF.clear_override('disk_wait_attempts') CONF.clear_override('disk_wait_delay') + def test_get_clean_steps(self): + expected_clean_steps = [ + { + 'step': 'erase_devices', + 'priority': 10, + 'interface': 'deploy', + 'reboot_requested': False, + 'abortable': True + }, + { + 'step': 'erase_devices_metadata', + 'priority': 99, + 'interface': 'deploy', + 'reboot_requested': False, + 'abortable': True + } + ] + clean_steps = self.hardware.get_clean_steps(self.node, []) + self.assertEqual(expected_clean_steps, clean_steps) + @mock.patch('netifaces.ifaddresses') @mock.patch('os.listdir') @mock.patch('os.path.exists') @@ -1194,6 +1215,64 @@ class TestGenericHardwareManager(test_base.BaseTestCase): test_security_erase_option( self, False, '--security-erase') + @mock.patch.object(hardware.GenericHardwareManager, + '_is_virtual_media_device', autospec=True) + @mock.patch.object(hardware.GenericHardwareManager, + 'list_block_devices', autospec=True) + @mock.patch.object(disk_utils, 'destroy_disk_metadata', autospec=True) + def test_erase_devices_metadata( + self, mock_metadata, mock_list_devs, mock__is_vmedia): + block_devices = [ + hardware.BlockDevice('/dev/sr0', 'vmedia', 12345, True), + hardware.BlockDevice('/dev/sda', 'small', 65535, False), + ] + mock_list_devs.return_value = block_devices + mock__is_vmedia.side_effect = (True, False) + + 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]) + ]) + + @mock.patch.object(hardware.GenericHardwareManager, + '_is_virtual_media_device', autospec=True) + @mock.patch.object(hardware.GenericHardwareManager, + 'list_block_devices', autospec=True) + @mock.patch.object(disk_utils, 'destroy_disk_metadata', autospec=True) + def test_erase_devices_metadata_error( + self, mock_metadata, mock_list_devs, mock__is_vmedia): + block_devices = [ + hardware.BlockDevice('/dev/sda', 'small', 65535, False), + 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 + error_output = 'Booo00000ooommmmm' + mock_metadata.side_effect = ( + processutils.ProcessExecutionError(error_output), + None, + ) + + self.assertRaisesRegex(errors.BlockDeviceEraseError, error_output, + 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]) + ]) + @mock.patch.object(utils, 'execute') def test_get_bmc_address(self, mocked_execute): mocked_execute.return_value = '192.1.2.3\n', '' diff --git a/releasenotes/notes/erase-device-metadata-clean-step-31b4a615c0ff7f18.yaml b/releasenotes/notes/erase-device-metadata-clean-step-31b4a615c0ff7f18.yaml new file mode 100644 index 000000000..11ec17b34 --- /dev/null +++ b/releasenotes/notes/erase-device-metadata-clean-step-31b4a615c0ff7f18.yaml @@ -0,0 +1,6 @@ +--- +features: + - Add a new cleaning step called "erase_devices_metadata" to the generic + hardware manager which is responsible for destroying the metadata + on the disk devices (partition tables, signatures, file-system + identifications, etc...).