From ddbba070216233e695f90095334ab7ddaef563f2 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 16 Mar 2020 13:16:05 +0100 Subject: [PATCH] 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 | 43 ++++-- ironic_python_agent/raid_utils.py | 65 +++++++++ .../tests/unit/test_hardware.py | 129 ++++++++++++++++++ lower-constraints.txt | 2 +- .../notes/raid-hints-604f9ffdd86432eb.yaml | 6 + requirements.txt | 2 +- 6 files changed, 232 insertions(+), 15 deletions(-) create mode 100644 ironic_python_agent/raid_utils.py create mode 100644 releasenotes/notes/raid-hints-604f9ffdd86432eb.yaml diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 48104f389..3d02df6ce 100644 --- a/ironic_python_agent/hardware.py +++ b/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: - LOG.info("Creating partition table on {}".format( - block_device.name)) + for dev_name in block_devices: + LOG.info("Creating partition table on {}".format(dev_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 " diff --git a/ironic_python_agent/raid_utils.py b/ironic_python_agent/raid_utils.py new file mode 100644 index 000000000..7f718ac4e --- /dev/null +++ b/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 diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 18407c4f0..80140796e 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/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 = { diff --git a/lower-constraints.txt b/lower-constraints.txt index 530867035..54e0ee2aa 100644 --- a/lower-constraints.txt +++ b/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 diff --git a/releasenotes/notes/raid-hints-604f9ffdd86432eb.yaml b/releasenotes/notes/raid-hints-604f9ffdd86432eb.yaml new file mode 100644 index 000000000..bf4ec695f --- /dev/null +++ b/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. diff --git a/requirements.txt b/requirements.txt index 49052372d..28ea8ace7 100644 --- a/requirements.txt +++ b/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