Fix multi-device behavior

ATARAID is functionally a version of software RAID where the setup is managed by the controller and the Operating System takes over managing the RAID after boot. Most commonly this is found for mirrored boot devices. Prior to this patch, we were looking for non-dependent items (i.e. base block devices), with a type of disk. Now we will permit the "disk" to be added to the list if lsblk indicates that it is a type containing "raid". The lsblk results should not change as we explicitly look for disk objects. Change-Id: Ia4a03b33cc06ce42e1bc33026683c28b31901cb7 Story: #2003445 Task: #24647
2018-08-16 10:57:59 -07:00 · 2018-08-16 10:57:59 -07:00 · c540731aee
parent 667589bb00
commit c540731aee
3 changed files with 127 additions and 13 deletions
--- a/ironic_python_agent/hardware.py
+++ b/ironic_python_agent/hardware.py
@ -111,19 +111,31 @@ def _check_for_iscsi():
                    "Error: %s", e)


-def list_all_block_devices(block_type='disk'):
+def list_all_block_devices(block_type='disk',
+                           ignore_raid=False):
    """List all physical block devices

    The switches we use for lsblk: P for KEY="value" output, b for size output
-    in bytes, d to exclude dependent devices (like md or dm devices), i to
-    ensure ascii characters only, and o to specify the fields/columns we need.
+    in bytes, i to ensure ascii characters only, and o to specify the
+    fields/columns we need.

    Broken out as its own function to facilitate custom hardware managers that
    don't need to subclass GenericHardwareManager.

    :param block_type: Type of block device to find
+    :param ignore_raid: Ignore auto-identified raid devices, example: md0
+                        Defaults to false as these are generally disk
+                        devices and should be treated as such if encountered.
    :return: A list of BlockDevices
    """
+
+    def _is_known_device(existing, new_device_name):
+        """Return true if device name is already known."""
+        for known_dev in existing:
+            if os.path.join('/dev', new_device_name) == known_dev.name:
+                return True
+        return False
+
    _udev_settle()

    # map device names to /dev/disk/by-path symbolic links that points to it
@ -143,14 +155,16 @@ def list_all_block_devices(block_type='disk'):
            by_path_mapping[devname] = path

    except OSError as e:
+        # NOTE(TheJulia): This is for multipath detection, and will raise
+        # some warning logs with unrelated tests.
        LOG.warning("Path %(path)s is inaccessible, /dev/disk/by-path/* "
                    "version of block device name is unavailable "
                    "Cause: %(error)s", {'path': disk_by_path_dir, 'error': e})

    columns = ['KNAME', 'MODEL', 'SIZE', 'ROTA', 'TYPE']
-    report = utils.execute('lsblk', '-Pbdi', '-o{}'.format(','.join(columns)),
+    report = utils.execute('lsblk', '-Pbi', '-o{}'.format(','.join(columns)),
                           check_exit_code=[0])[0]
-    lines = report.split('\n')
+    lines = report.splitlines()
    context = pyudev.Context()

    devices = []
@ -161,12 +175,27 @@ def list_all_block_devices(block_type='disk'):
        for key, val in (v.split('=', 1) for v in vals):
            device[key] = val.strip()
        # Ignore block types not specified
-        if device.get('TYPE') != block_type:
-            LOG.debug(
-                "TYPE did not match. Wanted: {!r} but found: {!r}".format(
-                    block_type, line))
+        devtype = device.get('TYPE')
+
+        # We already have devices, we should ensure we don't store duplicates.
+        if _is_known_device(devices, device.get('KNAME')):
            continue

+        # Search for raid in the reply type, as RAID is a
+        # disk device, and we should honor it if is present.
+        # Other possible type values, which we skip recording:
+        #   lvm, part, rom, loop
+        if devtype != block_type:
+            if devtype is not None and 'raid' in devtype and not ignore_raid:
+                LOG.debug(
+                    "TYPE detected to contain 'raid', signifying a RAID "
+                    "volume. Found: {!r}".format(line))
+            else:
+                LOG.debug(
+                    "TYPE did not match. Wanted: {!r} but found: {!r}".format(
+                        block_type, line))
+                continue
+
        # Ensure all required columns are at least present, even if blank
        missing = set(columns) - set(device)
        if missing:
--- a/ironic_python_agent/tests/unit/test_hardware.py
+++ b/ironic_python_agent/tests/unit/test_hardware.py
@ -154,6 +154,32 @@ BLK_DEVICE_TEMPLATE_SMALL_DEVICES = [
                         vendor="FooTastic"),
 ]

+# NOTE(TheJulia): This list intentionally contains duplicates
+# as the code filters them out by kernel device name.
+RAID_BLK_DEVICE_TEMPLATE = (
+    'KNAME="sda" MODEL="DRIVE 0" SIZE="1765517033472" '
+    'ROTA="1" TYPE="disk"\n'
+    'KNAME="sdb" MODEL="DRIVE 1" SIZE="1765517033472" '
+    'ROTA="1" TYPE="disk"\n'
+    'KNAME="sdb" MODEL="DRIVE 1" SIZE="1765517033472" '
+    'ROTA="1" TYPE="disk"\n'
+    'KNAME="md0" MODEL="RAID" SIZE="1765517033470" '
+    'ROTA="0" TYPE="raid1"\n'
+    'KNAME="md0" MODEL="RAID" SIZE="1765517033470" '
+    'ROTA="0" TYPE="raid1"'
+)
+RAID_BLK_DEVICE_TEMPLATE_DEVICES = [
+    hardware.BlockDevice(name='/dev/sda', model='DRIVE 0',
+                         size=1765517033472, rotational=True,
+                         vendor="FooTastic"),
+    hardware.BlockDevice(name='/dev/sdb', model='DRIVE 1',
+                         size=1765517033472, rotational=True,
+                         vendor="FooTastic"),
+    hardware.BlockDevice(name='/dev/md0', model='RAID',
+                         size=1765517033470, rotational=False,
+                         vendor="FooTastic"),
+]
+
 SHRED_OUTPUT_0_ITERATIONS_ZERO_FALSE = ()

 SHRED_OUTPUT_1_ITERATION_ZERO_TRUE = (
@ -813,7 +839,29 @@ class TestGenericHardwareManager(base.IronicAgentTest):
        mocked_execute.return_value = (BLK_DEVICE_TEMPLATE, '')
        self.assertEqual('/dev/sdb', self.hardware.get_os_install_device())
        mocked_execute.assert_called_once_with(
-            'lsblk', '-Pbdi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
+            'lsblk', '-Pbi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
+            check_exit_code=[0])
+        mock_cached_node.assert_called_once_with()
+
+    @mock.patch.object(os, 'readlink', autospec=True)
+    @mock.patch.object(os, 'listdir', autospec=True)
+    @mock.patch.object(hardware, 'get_cached_node', autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_get_os_install_device_raid(self, mocked_execute,
+                                        mock_cached_node, mocked_listdir,
+                                        mocked_readlink):
+        # NOTE(TheJulia): The readlink and listdir mocks are just to satisfy
+        # what is functionally an available path check and that information
+        # is stored in the returned result for use by root device hints.
+        mocked_readlink.side_effect = '../../sda'
+        mocked_listdir.return_value = ['1:0:0:0']
+        mock_cached_node.return_value = None
+        mocked_execute.return_value = (RAID_BLK_DEVICE_TEMPLATE, '')
+        # This should ideally select the smallest device and in theory raid
+        # should always be smaller
+        self.assertEqual('/dev/md0', self.hardware.get_os_install_device())
+        mocked_execute.assert_called_once_with(
+            'lsblk', '-Pbi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
            check_exit_code=[0])
        mock_cached_node.assert_called_once_with()

@ -832,7 +880,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
        ex = self.assertRaises(errors.DeviceNotFound,
                               self.hardware.get_os_install_device)
        mocked_execute.assert_called_once_with(
-            'lsblk', '-Pbdi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
+            'lsblk', '-Pbi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
            check_exit_code=[0])
        self.assertIn(str(4 * units.Gi), ex.details)
        mock_cached_node.assert_called_once_with()
@ -2137,11 +2185,30 @@ class TestModuleFunctions(base.IronicAgentTest):
        mocked_execute.return_value = (BLK_DEVICE_TEMPLATE_SMALL, '')
        result = hardware.list_all_block_devices()
        mocked_execute.assert_called_once_with(
-            'lsblk', '-Pbdi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
+            'lsblk', '-Pbi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
            check_exit_code=[0])
        self.assertEqual(BLK_DEVICE_TEMPLATE_SMALL_DEVICES, result)
        mocked_udev.assert_called_once_with()

+    @mock.patch.object(os, 'readlink', autospec=True)
+    @mock.patch.object(hardware, '_get_device_info',
+                       lambda x, y, z: 'FooTastic')
+    @mock.patch.object(hardware, '_udev_settle', autospec=True)
+    @mock.patch.object(hardware.pyudev.Device, "from_device_file",
+                       autospec=False)
+    def test_list_all_block_devices_success_raid(self, mocked_fromdevfile,
+                                                 mocked_udev, mocked_readlink,
+                                                 mocked_execute):
+        mocked_readlink.return_value = '../../sda'
+        mocked_fromdevfile.return_value = {}
+        mocked_execute.return_value = (RAID_BLK_DEVICE_TEMPLATE, '')
+        result = hardware.list_all_block_devices()
+        mocked_execute.assert_called_once_with(
+            'lsblk', '-Pbi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
+            check_exit_code=[0])
+        self.assertEqual(RAID_BLK_DEVICE_TEMPLATE_DEVICES, result)
+        mocked_udev.assert_called_once_with()
+
    @mock.patch.object(hardware, '_get_device_info',
                       lambda x, y: "FooTastic")
    @mock.patch.object(hardware, '_udev_settle', autospec=True)
@ -2150,7 +2217,7 @@ class TestModuleFunctions(base.IronicAgentTest):
        mocked_execute.return_value = ('TYPE="foo" MODEL="model"', '')
        result = hardware.list_all_block_devices()
        mocked_execute.assert_called_once_with(
-            'lsblk', '-Pbdi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
+            'lsblk', '-Pbi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
            check_exit_code=[0])
        self.assertEqual([], result)
        mocked_udev.assert_called_once_with()
--- a/releasenotes/notes/ataraid_does_not_appear_as_disk-8a260e66b3496bf6.yaml
+++ b/releasenotes/notes/ataraid_does_not_appear_as_disk-8a260e66b3496bf6.yaml
@ -0,0 +1,18 @@
+---
+fixes:
+  - |
+    Fixes an issue where devices offered via ATARAID
+    (or even software RAID setup by a hardware manager)
+    were excluded from the list as they are not returned
+    as type disk, even though they should be considered
+    both disks and block devices.
+    See `story <https://storyboard.openstack.org/#!/story/2003445>`_
+    for more details.
+upgrade:
+  - |
+    Operators with custom hardware managers to support software RAID
+    may wish to ensure that they have removed the software RAID prior
+    to any stock cleaning step for erasing block devices executes.
+    As a result of a bug fix, such devices may now be picked up in
+    the list of possible devices to delete, which may extend cleaning
+    depending on the deployment configuration.