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

@@ -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

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', ''

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...).