Merge "Fix software RAID creation on different physical devices"

ironic_python_agent/hardware.py

@@ -3101,9 +3101,11 @@ class GenericHardwareManager(HardwareManager):
 
                 parted_start_dict[device] = end
 
-        # Create the RAID devices.
+        # Create the RAID devices. The indices mapping tracks the last used
+        # partition index for each physical device.
+        indices = {}
         for index, logical_disk in enumerate(logical_disks):
-            raid_utils.create_raid_device(index, logical_disk)
+            raid_utils.create_raid_device(index, logical_disk, indices)
 
         LOG.info("Successfully created Software RAID")
 
@@ -3398,6 +3400,8 @@ class GenericHardwareManager(HardwareManager):
             size2 = logical_disks[1]['size_gb']
 
             # Only one logical disk is allowed to span the whole device.
+            # FIXME(dtantsur): this logic is not correct when logical disks use
+            # different physical devices.
             if size1 == 'MAX' and size2 == 'MAX':
                 msg = ("Software RAID can have only one RAID device with "
                        "size 'MAX'")

ironic_python_agent/raid_utils.py

@@ -202,24 +202,31 @@ def _get_actual_component_devices(raid_device):
     return component_devices
 
 
-def create_raid_device(index, logical_disk):
+def create_raid_device(index, logical_disk, indices=None):
     """Create a raid device.
 
     :param index: the index of the resulting md device.
     :param logical_disk: the logical disk containing the devices used to
         crete the raid.
+    :param indices: Mapping to track the last used partition index for each
+        physical device across calls to create_raid_device.
     :raise: errors.SoftwareRAIDError if not able to create the raid device
         or fails to re-add a device to a raid.
     """
     md_device = '/dev/md%d' % index
     component_devices = []
+    if indices is None:
+        # Backward compatibility with custom hardware managers
+        indices = {dev: index for dev in logical_disk['block_devices']}
     for device in logical_disk['block_devices']:
         # The partition delimiter for all common harddrives (sd[a-z]+)
         part_delimiter = ''
         if 'nvme' in device:
             part_delimiter = 'p'
+        index_on_device = indices.get(device, 0)
         component_devices.append(
-            device + part_delimiter + str(index + RAID_PARTITION))
+            device + part_delimiter + str(index_on_device + RAID_PARTITION))
+        indices[device] = index_on_device + 1
     raid_level = logical_disk['raid_level']
     # The schema check allows '1+0', but mdadm knows it as '10'.
     if raid_level == '1+0':

ironic_python_agent/tests/unit/test_hardware.py

@@ -4341,6 +4341,117 @@ class TestGenericHardwareManager(base.IronicAgentTest):
             mock.call(x) for x in ['/dev/sda', '/dev/sdb']
         ])
 
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
+                       autospec=True)
+    @mock.patch.object(utils, 'get_node_boot_mode', lambda node: 'bios')
+    @mock.patch.object(disk_utils, 'list_partitions', autospec=True,
+                       return_value=[])
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_configuration_with_different_disks(self, mocked_execute,
+                                                       mock_list_parts,
+                                                       mocked_actual_comp):
+        node = self.node
+        raid_config = {
+            "logical_disks": [
+                {
+                    "size_gb": "10",
+                    "raid_level": "1",
+                    "controller": "software",
+                    "physical_disks": [{'name': '/dev/sda'},
+                                       {'name': '/dev/sdb'}],
+                },
+                {
+                    "size_gb": "MAX",
+                    "raid_level": "0",
+                    "controller": "software",
+                    "physical_disks": [{'name': '/dev/sdc'},
+                                       {'name': '/dev/sdd'}],
+                },
+            ]
+        }
+        node['target_raid_config'] = raid_config
+        device1 = hardware.BlockDevice('/dev/sda', 'sda', 107374182400, True)
+        device2 = hardware.BlockDevice('/dev/sdb', 'sdb', 107374182400, True)
+        device3 = hardware.BlockDevice('/dev/sdc', 'sdc', 107374182400, True)
+        device4 = hardware.BlockDevice('/dev/sdd', 'sdd', 107374182400, True)
+        self.hardware.list_block_devices = mock.Mock()
+        self.hardware.list_block_devices.return_value = [
+            device1,
+            device2,
+            device3,
+            device4,
+        ]
+
+        mocked_execute.side_effect = [
+            None,  # mklabel sda
+            ('42', None),  # sgdisk -F sda
+            None,  # mklabel sda
+            ('42', None),  # sgdisk -F sdb
+            None,  # mklabel sdc
+            ('42', None),  # sgdisk -F sdc
+            None,  # mklabel sdd
+            ('42', None),  # sgdisk -F sdd
+            None, None, None,  # parted + partx + udevadm_settle sda
+            None, None, None,  # parted + partx + udevadm_settle sdb
+            None, None, None,  # parted + partx + udevadm_settle sdc
+            None, None, None,  # parted + partx + udevadm_settle sdd
+            None, None, None,  # parted + partx + udevadm_settle sda
+            None, None, None,  # parted + partx + udevadm_settle sdb
+            None, None, None,  # parted + partx + udevadm_settle sdc
+            None, None, None,  # parted + partx + udevadm_settle sdd
+            None, None  # mdadms
+        ]
+
+        mocked_actual_comp.side_effect = [
+            ('/dev/sda1', '/dev/sdb1'),
+            ('/dev/sdc1', '/dev/sdd1'),
+        ]
+
+        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/sdc', '-s', '--', 'mklabel', 'msdos'),
+            mock.call('sgdisk', '-F', '/dev/sdc'),
+            mock.call('parted', '/dev/sdd', '-s', '--', 'mklabel', 'msdos'),
+            mock.call('sgdisk', '-F', '/dev/sdd'),
+            mock.call('parted', '/dev/sda', '-s', '-a', 'optimal', '--',
+                      'mkpart', 'primary', '42s', '10GiB'),
+            mock.call('partx', '-av', '/dev/sda', attempts=3,
+                      delay_on_retry=True),
+            mock.call('udevadm', 'settle'),
+            mock.call('parted', '/dev/sdb', '-s', '-a', 'optimal', '--',
+                      'mkpart', 'primary', '42s', '10GiB'),
+            mock.call('partx', '-av', '/dev/sdb', attempts=3,
+                      delay_on_retry=True),
+            mock.call('udevadm', 'settle'),
+            mock.call('parted', '/dev/sdc', '-s', '-a', 'optimal', '--',
+                      'mkpart', 'primary', '42s', '-1'),
+            mock.call('partx', '-av', '/dev/sdc', attempts=3,
+                      delay_on_retry=True),
+            mock.call('udevadm', 'settle'),
+            mock.call('parted', '/dev/sdd', '-s', '-a', 'optimal', '--',
+                      'mkpart', 'primary', '42s', '-1'),
+            mock.call('partx', '-av', '/dev/sdd', attempts=3,
+                      delay_on_retry=True),
+            mock.call('udevadm', 'settle'),
+            mock.call('mdadm', '--create', '/dev/md0', '--force', '--run',
+                      '--metadata=1', '--level', '1', '--name', 'md0',
+                      '--raid-devices', 2, '/dev/sda1', '/dev/sdb1'),
+            mock.call('mdadm', '--create', '/dev/md1', '--force', '--run',
+                      '--metadata=1', '--level', '0', '--name', 'md1',
+                      '--raid-devices', 2, '/dev/sdc1', '/dev/sdd1')])
+        self.assertEqual(raid_config, result)
+
+        self.assertEqual(4, mock_list_parts.call_count)
+        mock_list_parts.assert_has_calls([
+            mock.call(x) for x in ['/dev/sda', '/dev/sdb',
+                                   '/dev/sdc', '/dev/sdd']
+        ])
+
     @mock.patch.object(utils, 'execute', autospec=True)
     @mock.patch.object(os.path, 'isdir', autospec=True, return_value=False)
     def test_create_configuration_invalid_raid_config(self,

ironic_python_agent/tests/unit/test_raid_utils.py

@@ -52,7 +52,7 @@ class TestRaidUtils(base.IronicAgentTest):
                                           '/dev/sdb1',
                                           '/dev/sdc1']
 
-        raid_utils.create_raid_device(0, logical_disk)
+        raid_utils.create_raid_device(0, logical_disk, {})
 
         mock_execute.assert_called_once_with(
             'mdadm', '--create', '/dev/md0', '--force', '--run',
@@ -73,7 +73,7 @@ class TestRaidUtils(base.IronicAgentTest):
                                           '/dev/sdb1',
                                           '/dev/sdc1']
 
-        raid_utils.create_raid_device(0, logical_disk)
+        raid_utils.create_raid_device(0, logical_disk, {})
 
         mock_execute.assert_called_once_with(
             'mdadm', '--create', '/dev/md0', '--force', '--run',
@@ -92,7 +92,7 @@ class TestRaidUtils(base.IronicAgentTest):
         mocked_components.return_value = ['/dev/sda1',
                                           '/dev/sdc1']
 
-        raid_utils.create_raid_device(0, logical_disk)
+        raid_utils.create_raid_device(0, logical_disk, {})
 
         expected_calls = [
             mock.call('mdadm', '--create', '/dev/md0', '--force', '--run',
@@ -105,6 +105,29 @@ class TestRaidUtils(base.IronicAgentTest):
         self.assertEqual(mock_execute.call_count, 2)
         mock_execute.assert_has_calls(expected_calls)
 
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
+                       autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_raid_device_with_indices(self, mock_execute,
+                                             mocked_components):
+        indices = {'/dev/sda': 1, '/dev/sdb': 2}
+        logical_disk = {
+            "block_devices": ['/dev/sda', '/dev/sdb', '/dev/sdc'],
+            "raid_level": "1",
+        }
+        mocked_components.return_value = ['/dev/sda2',
+                                          '/dev/sdb3',
+                                          '/dev/sdc1']
+
+        raid_utils.create_raid_device(0, logical_disk, indices)
+
+        mock_execute.assert_called_once_with(
+            'mdadm', '--create', '/dev/md0', '--force', '--run',
+            '--metadata=1', '--level', '1', '--name', 'md0',
+            '--raid-devices', 3, '/dev/sda2', '/dev/sdb3', '/dev/sdc1')
+        self.assertEqual(
+            {'/dev/sda': 2, '/dev/sdb': 3, '/dev/sdc': 1}, indices)
+
     @mock.patch.object(utils, 'execute', autospec=True)
     def test_create_raid_device_fail_create_device(self, mock_execute):
         logical_disk = {

releasenotes/notes/fix-software-raid-different-devices-3298f735fdbd3a05.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Software RAID creation now correctly handles configurations where
+    logical disks use different sets of physical devices. Previously,
+    partition indices were incorrectly shared across all devices, causing
+    failures when creating multiple RAID arrays on different disks.