Allow specifying target devices for software RAID

This change adds support for the physical_disks RAID parameter in
a form of device hints (same as for root device selection).

Change-Id: I9751ab0f86ada41e3b668670dc112d58093b8099
Story: #2006369
Task: #39080

ironic_python_agent/hardware.py

@@ -37,6 +37,7 @@ import yaml
 from ironic_python_agent import encoding
 from ironic_python_agent import errors
 from ironic_python_agent import netutils
+from ironic_python_agent import raid_utils
 from ironic_python_agent import utils
 
 _global_managers = None
@@ -1530,6 +1531,8 @@ class GenericHardwareManager(HardwareManager):
         block_devices = self.list_block_devices()
         block_devices_partitions = self.list_block_devices(
             include_partitions=True)
+        # TODO(dtantsur): limit this validation to only the discs involved the
+        # software RAID.
         if len(block_devices) != len(block_devices_partitions):
             partitions = ' '.join(
                 partition.name for partition in block_devices_partitions)
@@ -1537,24 +1540,26 @@ class GenericHardwareManager(HardwareManager):
                   partitions)
             raise errors.SoftwareRAIDError(msg)
 
+        block_devices, logical_disks = raid_utils.get_block_devices_for_raid(
+            block_devices, logical_disks)
+
         parted_start_dict = {}
         # Create an MBR partition table on each disk.
         # TODO(arne_wiebalck): Check if GPT would work as well.
-        for block_device in block_devices:
+        for dev_name in block_devices:
-            LOG.info("Creating partition table on {}".format(
+            LOG.info("Creating partition table on {}".format(dev_name))
-                block_device.name))
             try:
-                utils.execute('parted', block_device.name, '-s', '--',
+                utils.execute('parted', dev_name, '-s', '--',
                               'mklabel', 'msdos')
             except processutils.ProcessExecutionError as e:
                 msg = "Failed to create partition table on {}: {}".format(
-                    block_device.name, e)
+                    dev_name, e)
                 raise errors.SoftwareRAIDError(msg)
 
-            out, _u = utils.execute('sgdisk', '-F', block_device.name)
+            out, _u = utils.execute('sgdisk', '-F', dev_name)
             # May differ from 2048s, according to device geometry (example:
             # 4k disks).
-            parted_start_dict[block_device.name] = "%ss" % out.splitlines()[-1]
+            parted_start_dict[dev_name] = "%ss" % out.splitlines()[-1]
 
         LOG.debug("First available sectors per devices %s", parted_start_dict)
 
@@ -1578,8 +1583,6 @@ class GenericHardwareManager(HardwareManager):
         # user-friendly to compute part boundaries automatically, instead of
         # parted, then convert back to mbr table if needed and possible.
 
-        default_physical_disks = [device.name for device in block_devices]
-
         for logical_disk in logical_disks:
             # Note: from the doc,
             # https://docs.openstack.org/ironic/latest/admin/raid.html#target-raid-configuration
@@ -1591,7 +1594,9 @@ class GenericHardwareManager(HardwareManager):
             else:
                 psize = int(psize)
 
-            for device in default_physical_disks:
+            # NOTE(dtantsur): populated in get_block_devices_for_raid
+            disk_names = logical_disk['block_devices']
+            for device in disk_names:
                 start = parted_start_dict[device]
 
                 if isinstance(start, int):
@@ -1633,11 +1638,10 @@ class GenericHardwareManager(HardwareManager):
                 parted_start_dict[device] = end
 
         # Create the RAID devices.
-        raid_device_count = len(block_devices)
         for index, logical_disk in enumerate(logical_disks):
             md_device = '/dev/md%d' % index
             component_devices = []
-            for device in default_physical_disks:
+            for device in logical_disk['block_devices']:
                 # The partition delimiter for all common harddrives (sd[a-z]+)
                 part_delimiter = ''
                 if 'nvme' in device:
@@ -1653,7 +1657,7 @@ class GenericHardwareManager(HardwareManager):
                           md_device, component_devices))
                 utils.execute('mdadm', '--create', md_device, '--force',
                               '--run', '--metadata=1', '--level', raid_level,
-                              '--raid-devices', raid_device_count,
+                              '--raid-devices', len(component_devices),
                               *component_devices)
             except processutils.ProcessExecutionError as e:
                 msg = "Failed to create md device {} on {}: {}".format(
@@ -1843,6 +1847,19 @@ class GenericHardwareManager(HardwareManager):
                        "disks to have 'controller'='software'")
                 raid_errors.append(msg)
 
+            physical_disks = logical_disk.get('physical_disks')
+            if physical_disks is not None:
+                if (not isinstance(physical_disks, list)
+                        or len(physical_disks) < 2):
+                    msg = ("The physical_disks parameter for software RAID "
+                           "must be a list with at least 2 items, each "
+                           "specifying a disk in the device hints format")
+                    raid_errors.append(msg)
+                if any(not isinstance(item, dict) for item in physical_disks):
+                    msg = ("The physical_disks parameter for software RAID "
+                           "must be a list of device hints (dictionaries)")
+                    raid_errors.append(msg)
+
         # The first RAID device needs to be RAID-1.
         if logical_disks[0]['raid_level'] != '1':
             msg = ("Software RAID Configuration requires RAID-1 for the "

ironic_python_agent/raid_utils.py

@@ -0,0 +1,65 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import copy
+
+from ironic_lib import utils as il_utils
+
+from ironic_python_agent import errors
+
+
+def get_block_devices_for_raid(block_devices, logical_disks):
+    """Get block devices that are involved in the RAID configuration.
+
+    This call does two things:
+    * Collect all block devices that are involved in RAID.
+    * Update each logical disks with suitable block devices.
+    """
+    serialized_devs = [dev.serialize() for dev in block_devices]
+    # NOTE(dtantsur): we're going to modify the structure, so make a copy
+    logical_disks = copy.deepcopy(logical_disks)
+    # NOTE(dtantsur): using a list here is less efficient than a set, but
+    # allows keeping the original ordering.
+    result = []
+    for logical_disk in logical_disks:
+        if logical_disk.get('physical_disks'):
+            matching = []
+            for phys_disk in logical_disk['physical_disks']:
+                candidates = [
+                    dev['name'] for dev in il_utils.find_devices_by_hints(
+                        serialized_devs, phys_disk)
+                ]
+                if not candidates:
+                    raise errors.SoftwareRAIDError(
+                        "No candidates for physical disk %(hints)s "
+                        "from the list %(devices)s"
+                        % {'hints': phys_disk, 'devices': serialized_devs})
+
+                try:
+                    matching.append(next(x for x in candidates
+                                         if x not in matching))
+                except StopIteration:
+                    raise errors.SoftwareRAIDError(
+                        "No candidates left for physical disk %(hints)s "
+                        "from the list %(candidates)s after picking "
+                        "%(matching)s for previous volumes"
+                        % {'hints': phys_disk, 'matching': matching,
+                           'candidates': candidates})
+        else:
+            # This RAID device spans all disks.
+            matching = [dev.name for dev in block_devices]
+
+        # Update the result keeping the ordering and avoiding duplicates.
+        result.extend(disk for disk in matching if disk not in result)
+        logical_disk['block_devices'] = matching
+
+    return result, logical_disks

ironic_python_agent/tests/unit/test_hardware.py

@@ -3141,6 +3141,81 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                       '/dev/sda2', '/dev/sdb2')])
         self.assertEqual(raid_config, result)
 
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_configuration_with_hints(self, mocked_execute):
+        node = self.node
+        raid_config = {
+            "logical_disks": [
+                {
+                    "size_gb": "10",
+                    "raid_level": "1",
+                    "controller": "software",
+                    "physical_disks": [
+                        {'size': '>= 50'}
+                    ] * 2,
+                },
+                {
+                    "size_gb": "MAX",
+                    "raid_level": "0",
+                    "controller": "software",
+                    "physical_disks": [
+                        {'rotational': True}
+                    ] * 2,
+                },
+            ]
+        }
+        node['target_raid_config'] = raid_config
+        device1 = hardware.BlockDevice('/dev/sda', 'sda', 107374182400, True)
+        device2 = hardware.BlockDevice('/dev/sdb', 'sdb', 107374182400, True)
+        self.hardware.list_block_devices = mock.Mock()
+        self.hardware.list_block_devices.return_value = [
+            device1,
+            hardware.BlockDevice('/dev/sdc', 'sdc', 21474836480, False),
+            device2,
+            hardware.BlockDevice('/dev/sdd', 'sdd', 21474836480, False),
+        ]
+
+        mocked_execute.side_effect = [
+            None,  # mklabel sda
+            ('42', None),  # sgdisk -F sda
+            None,  # mklabel sda
+            ('42', None),  # sgdisk -F sdb
+            None, None,  # parted + partx sda
+            None, None,  # parted + partx sdb
+            None, None,  # parted + partx sda
+            None, None,  # parted + partx sdb
+            None, None  # mdadms
+        ]
+
+        result = self.hardware.create_configuration(node, [])
+
+        mocked_execute.assert_has_calls([
+            mock.call('parted', '/dev/sda', '-s', '--', 'mklabel',
+                      'msdos'),
+            mock.call('sgdisk', '-F', '/dev/sda'),
+            mock.call('parted', '/dev/sdb', '-s', '--', 'mklabel',
+                      'msdos'),
+            mock.call('sgdisk', '-F', '/dev/sdb'),
+            mock.call('parted', '/dev/sda', '-s', '-a', 'optimal', '--',
+                      'mkpart', 'primary', '42s', '10GiB'),
+            mock.call('partx', '-u', '/dev/sda', check_exit_code=False),
+            mock.call('parted', '/dev/sdb', '-s', '-a', 'optimal', '--',
+                      'mkpart', 'primary', '42s', '10GiB'),
+            mock.call('partx', '-u', '/dev/sdb', check_exit_code=False),
+            mock.call('parted', '/dev/sda', '-s', '-a', 'optimal', '--',
+                      'mkpart', 'primary', '10GiB', '-1'),
+            mock.call('partx', '-u', '/dev/sda', check_exit_code=False),
+            mock.call('parted', '/dev/sdb', '-s', '-a', 'optimal', '--',
+                      'mkpart', 'primary', '10GiB', '-1'),
+            mock.call('partx', '-u', '/dev/sdb', check_exit_code=False),
+            mock.call('mdadm', '--create', '/dev/md0', '--force', '--run',
+                      '--metadata=1', '--level', '1', '--raid-devices', 2,
+                      '/dev/sda1', '/dev/sdb1'),
+            mock.call('mdadm', '--create', '/dev/md1', '--force', '--run',
+                      '--metadata=1', '--level', '0', '--raid-devices', 2,
+                      '/dev/sda2', '/dev/sdb2')])
+        self.assertEqual(raid_config, result)
+
     @mock.patch.object(utils, 'execute', autospec=True)
     def test_create_configuration_invalid_raid_config(self, mocked_execute):
         raid_config = {
@@ -3162,6 +3237,60 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                           self.hardware.create_configuration,
                           self.node, [])
 
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_configuration_invalid_hints(self, mocked_execute):
+        for hints in [
+                [],
+                [{'size': '>= 50'}],  # more than one disk required,
+                "size >= 50",
+                [{'size': '>= 50'}, "size >= 50"],
+        ]:
+            raid_config = {
+                "logical_disks": [
+                    {
+                        "size_gb": "MAX",
+                        "raid_level": "1",
+                        "controller": "software",
+                        "physical_disks": hints,
+                    }
+                ]
+            }
+            self.node['target_raid_config'] = raid_config
+            self.assertRaises(errors.SoftwareRAIDError,
+                              self.hardware.create_configuration,
+                              self.node, [])
+
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_configuration_mismatching_hints(self, mocked_execute):
+        device1 = hardware.BlockDevice('/dev/sda', 'sda', 107374182400, True)
+        device2 = hardware.BlockDevice('/dev/sdb', 'sdb', 107374182400, True)
+        self.hardware.list_block_devices = mock.Mock()
+        self.hardware.list_block_devices.return_value = [
+            device1,
+            hardware.BlockDevice('/dev/sdc', 'sdc', 21474836480, False),
+            device2,
+            hardware.BlockDevice('/dev/sdd', 'sdd', 21474836480, False),
+        ]
+        for hints in [
+                [{'size': '>= 150'}] * 2,
+                [{'name': '/dev/sda'}] * 2,
+        ]:
+            raid_config = {
+                "logical_disks": [
+                    {
+                        "size_gb": "MAX",
+                        "raid_level": "1",
+                        "controller": "software",
+                        "physical_disks": hints,
+                    }
+                ]
+            }
+            self.node['target_raid_config'] = raid_config
+            self.assertRaisesRegex(errors.SoftwareRAIDError,
+                                   'No candidates',
+                                   self.hardware.create_configuration,
+                                   self.node, [])
+
     @mock.patch.object(utils, 'execute', autospec=True)
     def test_create_configuration_partitions_detected(self, mocked_execute):
         raid_config = {

lower-constraints.txt

@@ -25,7 +25,7 @@ greenlet==0.4.13
 hacking==1.0.0
 idna==2.6
 imagesize==1.0.0
-ironic-lib==2.17.0
+ironic-lib==4.1.0
 Jinja2==2.10
 keystoneauth1==3.4.0
 linecache2==1.0.0

releasenotes/notes/raid-hints-604f9ffdd86432eb.yaml

@@ -0,0 +1,6 @@
+---
+features:
+  - |
+    Target devices for software RAID can now be specified in the form of
+    device hints (same as for root devices) in the ``physical_disks``
+    parameter of a logical disk configuration.

requirements.txt

@@ -16,5 +16,5 @@ pyudev>=0.18 # LGPLv2.1+
 requests>=2.14.2 # Apache-2.0
 rtslib-fb>=2.1.65 # Apache-2.0
 stevedore>=1.20.0 # Apache-2.0
-ironic-lib>=2.17.0 # Apache-2.0
+ironic-lib>=4.1.0 # Apache-2.0
 Werkzeug>=0.15.0 # BSD License