Move some raid specific functions to raid_utils

To reduce size of the hardware module and separate the raid specific
code in raid_utils, we move some functions and adapt the tests.

Change-Id: I73f6cf118575b627e66727d88d5567377c1999a0

ironic_python_agent/hardware.py

@@ -209,35 +209,6 @@ def _get_component_devices(raid_device):
     return component_devices
 
 
-def _get_actual_component_devices(raid_device):
-    """Get the component devices of a Software RAID device.
-
-    Examine an md device and return its constituent devices.
-
-    :param raid_device: A Software RAID block device name.
-    :returns: A list of the component devices.
-    """
-    if not raid_device:
-        return []
-
-    try:
-        out, _ = utils.execute('mdadm', '--detail', raid_device,
-                               use_standard_locale=True)
-    except processutils.ProcessExecutionError as e:
-        LOG.warning('Could not get component devices of %(dev)s: %(err)s',
-                    {'dev': raid_device, 'err': e})
-        return []
-
-    component_devices = []
-    lines = out.splitlines()
-    # the first line contains the md device itself
-    for line in lines[1:]:
-        device = re.findall(r'/dev/\w+', line)
-        component_devices += device
-
-    return component_devices
-
-
 def _calc_memory(sys_dict):
     physical = 0
     for sys_child in sys_dict['children']:
@@ -1834,12 +1805,6 @@ class GenericHardwareManager(HardwareManager):
         return self._do_create_configuration(node, ports, raid_config)
 
     def _do_create_configuration(self, node, ports, raid_config):
-        # incr starts to 1
-        # It means md0 is on the partition 1, md1 on 2...
-        # incr could be incremented if we ever decide, for example to create
-        # some additional partitions here (boot partitions)
-        incr = 1
-
         # No 'software' controller: do nothing. If 'controller' is
         # set to 'software' on only one of the drives, the validation
         # code will catch it.
@@ -1948,45 +1913,7 @@ class GenericHardwareManager(HardwareManager):
 
         # Create the RAID devices.
         for index, logical_disk in enumerate(logical_disks):
-            md_device = '/dev/md%d' % index
+            raid_utils.create_raid_device(index, logical_disk)
-            component_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'
-                component_devices.append(
-                    device + part_delimiter + str(index + incr))
-            raid_level = logical_disk['raid_level']
-            # The schema check allows '1+0', but mdadm knows it as '10'.
-            if raid_level == '1+0':
-                raid_level = '10'
-            try:
-                LOG.debug("Creating md device %(dev)s on %(comp)s",
-                          {'dev': md_device, 'comp': component_devices})
-                utils.execute('mdadm', '--create', md_device, '--force',
-                              '--run', '--metadata=1', '--level', raid_level,
-                              '--raid-devices', len(component_devices),
-                              *component_devices)
-            except processutils.ProcessExecutionError as e:
-                msg = "Failed to create md device {} on {}: {}".format(
-                    md_device, ' '.join(component_devices), e)
-                raise errors.SoftwareRAIDError(msg)
-
-            # check for missing devices and re-add them
-            actual_components = _get_actual_component_devices(md_device)
-            missing = set(component_devices) - set(actual_components)
-            for dev in missing:
-                try:
-                    LOG.warning('Found %(device)s to be missing from %(md)s '
-                                '... re-adding!',
-                                {'device': dev, 'md': md_device})
-                    utils.execute('mdadm', '--add', md_device, dev,
-                                  attempts=3, delay_on_retry=True)
-                except processutils.ProcessExecutionError as e:
-                    msg = "Failed re-add {} to {}: {}".format(
-                        dev, md_device, e)
-                    raise errors.SoftwareRAIDError(msg)
 
         LOG.info("Successfully created Software RAID")
 
@@ -2380,7 +2307,7 @@ def dispatch_to_managers(method, *args, **kwargs):
         if getattr(manager, method, None):
             try:
                 return getattr(manager, method)(*args, **kwargs)
-            except(errors.IncompatibleHardwareMethodError):
+            except errors.IncompatibleHardwareMethodError:
                 LOG.debug('HardwareManager %(manager)s does not '
                           'support %(method)s',
                           {'manager': manager, 'method': method})
@@ -2424,10 +2351,6 @@ def cache_node(node):
     expected root device to appear.
 
     :param node: Ironic node object
-    :param wait_for_disks: Default True switch to wait for disk setup to be
-                           completed so the node information can be aligned
-                           with the physical storage devices of the host.
-                           This is likely to be used in unit testing.
     """
     global NODE
     new_node = NODE is None or NODE['uuid'] != node['uuid']

ironic_python_agent/raid_utils.py

@@ -11,8 +11,10 @@
 # limitations under the License.
 
 import copy
+import re
 
 from ironic_lib import utils as il_utils
+from oslo_concurrency import processutils
 from oslo_log import log as logging
 
 from ironic_python_agent import errors
@@ -26,6 +28,12 @@ LOG = logging.getLogger(__name__)
 # https://www.rodsbooks.com/efi-bootloaders/principles.html
 ESP_SIZE_MIB = 550
 
+# NOTE(rpittau) The partition number used to create a raid device.
+# Could be changed to variable if we ever decide, for example to create
+# some additional partitions (e.g. boot partitions), so md0 is on the
+# partition 1, md1 on the partition 2, and so on.
+RAID_PARTITION = 1
+
 
 def get_block_devices_for_raid(block_devices, logical_disks):
     """Get block devices that are involved in the RAID configuration.
@@ -161,3 +169,82 @@ def create_raid_partition_tables(block_devices, partition_table_type,
         parted_start_dict[dev_name] = calculate_raid_start(
             target_boot_mode, partition_table_type, dev_name)
     return parted_start_dict
+
+
+def _get_actual_component_devices(raid_device):
+    """Get the component devices of a Software RAID device.
+
+    Examine an md device and return its constituent devices.
+
+    :param raid_device: A Software RAID block device name.
+    :returns: A list of the component devices.
+    """
+    if not raid_device:
+        return []
+
+    try:
+        out, _ = utils.execute('mdadm', '--detail', raid_device,
+                               use_standard_locale=True)
+    except processutils.ProcessExecutionError as e:
+        LOG.warning('Could not get component devices of %(dev)s: %(err)s',
+                    {'dev': raid_device, 'err': e})
+        return []
+
+    component_devices = []
+    lines = out.splitlines()
+    # the first line contains the md device itself
+    for line in lines[1:]:
+        device = re.findall(r'/dev/\w+', line)
+        component_devices += device
+
+    return component_devices
+
+
+def create_raid_device(index, logical_disk):
+    """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.
+    :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 = []
+    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'
+        component_devices.append(
+            device + part_delimiter + str(index + RAID_PARTITION))
+    raid_level = logical_disk['raid_level']
+    # The schema check allows '1+0', but mdadm knows it as '10'.
+    if raid_level == '1+0':
+        raid_level = '10'
+    try:
+        LOG.debug("Creating md device %(dev)s on %(comp)s",
+                  {'dev': md_device, 'comp': component_devices})
+        utils.execute('mdadm', '--create', md_device, '--force',
+                      '--run', '--metadata=1', '--level', raid_level,
+                      '--raid-devices', len(component_devices),
+                      *component_devices)
+    except processutils.ProcessExecutionError as e:
+        msg = "Failed to create md device {} on {}: {}".format(
+            md_device, ' '.join(component_devices), e)
+        raise errors.SoftwareRAIDError(msg)
+
+    # check for missing devices and re-add them
+    actual_components = _get_actual_component_devices(md_device)
+    missing = set(component_devices) - set(actual_components)
+    for dev in missing:
+        try:
+            LOG.warning('Found %(device)s to be missing from %(md)s '
+                        '... re-adding!',
+                        {'device': dev, 'md': md_device})
+            utils.execute('mdadm', '--add', md_device, dev,
+                          attempts=3, delay_on_retry=True)
+        except processutils.ProcessExecutionError as e:
+            msg = "Failed re-add {} to {}: {}".format(
+                dev, md_device, e)
+            raise errors.SoftwareRAIDError(msg)

ironic_python_agent/tests/unit/test_hardware.py

@@ -29,6 +29,7 @@ from stevedore import extension
 from ironic_python_agent import errors
 from ironic_python_agent import hardware
 from ironic_python_agent import netutils
+from ironic_python_agent import raid_utils
 from ironic_python_agent.tests.unit import base
 from ironic_python_agent.tests.unit.samples import hardware_samples as hws
 from ironic_python_agent import utils
@@ -2400,7 +2401,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         mocked_create.assert_called_once_with(self.hardware, self.node, [],
                                               raid_config)
 
-    @mock.patch.object(hardware, '_get_actual_component_devices',
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
                        autospec=True)
     @mock.patch.object(disk_utils, 'list_partitions', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
@@ -2484,7 +2485,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
             mock.call(x) for x in ['/dev/sda', '/dev/sdb']
         ])
 
-    @mock.patch.object(hardware, '_get_actual_component_devices',
+    @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,
@@ -2574,7 +2575,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                       '/dev/sda2', '/dev/sdb2', '/dev/sdc2')])
         self.assertEqual(raid_config, result)
 
-    @mock.patch.object(hardware, '_get_actual_component_devices',
+    @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,
@@ -2678,7 +2679,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                       '/dev/sda2', '/dev/sdb2', '/dev/sdc2', '/dev/sdd2')])
         self.assertEqual(raid_config, result)
 
-    @mock.patch.object(hardware, '_get_actual_component_devices',
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
                        autospec=True)
     @mock.patch.object(disk_utils, 'list_partitions', autospec=True,
                        return_value=[])
@@ -2751,7 +2752,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                       '/dev/sda2', '/dev/sdb2')])
         self.assertEqual(raid_config, result)
 
-    @mock.patch.object(hardware, '_get_actual_component_devices',
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
                        autospec=True)
     @mock.patch.object(disk_utils, 'list_partitions', autospec=True,
                        return_value=[])
@@ -2830,7 +2831,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                       '/dev/sda2', '/dev/sdb2')])
         self.assertEqual(raid_config, result)
 
-    @mock.patch.object(hardware, '_get_actual_component_devices',
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
                        autospec=True)
     @mock.patch.object(disk_utils, 'list_partitions', autospec=True,
                        return_value=[])
@@ -2904,7 +2905,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                       '/dev/sda2', '/dev/sdb2')])
         self.assertEqual(raid_config, result)
 
-    @mock.patch.object(hardware, '_get_actual_component_devices',
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
                        autospec=True)
     @mock.patch.object(disk_utils, 'list_partitions', autospec=True,
                        return_value=[])
@@ -2980,7 +2981,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                       '/dev/sda2', '/dev/sdb2')])
         self.assertEqual(raid_config, result)
 
-    @mock.patch.object(hardware, '_get_actual_component_devices',
+    @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,
@@ -3334,7 +3335,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         result = self.hardware.create_configuration(self.node, [])
         self.assertEqual(result, {})
 
-    @mock.patch.object(hardware, '_get_actual_component_devices',
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
                        autospec=True)
     @mock.patch.object(disk_utils, 'list_partitions', autospec=True,
                        return_value=[])
@@ -3475,21 +3476,6 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                                self.hardware.create_configuration,
                                self.node, [])
 
-    @mock.patch.object(utils, 'execute', autospec=True)
-    def test__get_actual_component_devices(self, mocked_execute):
-        mocked_execute.side_effect = [(hws.MDADM_DETAIL_OUTPUT, '')]
-        component_devices = hardware._get_actual_component_devices(
-            '/dev/md0')
-        self.assertEqual(['/dev/vde1', '/dev/vdf1'], component_devices)
-
-    @mock.patch.object(utils, 'execute', autospec=True)
-    def test__get_actual_component_devices_broken_raid0(self, mocked_execute):
-        mocked_execute.side_effect = [(hws.MDADM_DETAIL_OUTPUT_BROKEN_RAID0,
-                                       '')]
-        component_devices = hardware._get_actual_component_devices(
-            '/dev/md126')
-        self.assertEqual(['/dev/sda2'], component_devices)
-
     @mock.patch.object(utils, 'execute', autospec=True)
     def test__get_md_uuid(self, mocked_execute):
         mocked_execute.side_effect = [(hws.MDADM_DETAIL_OUTPUT, '')]

ironic_python_agent/tests/unit/test_raid_utils.py

@@ -0,0 +1,114 @@
+# 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.
+
+from unittest import mock
+
+from oslo_concurrency import processutils
+
+from ironic_python_agent import errors
+from ironic_python_agent import raid_utils
+from ironic_python_agent.tests.unit import base
+from ironic_python_agent.tests.unit.samples import hardware_samples as hws
+from ironic_python_agent import utils
+
+
+class TestRaidUtils(base.IronicAgentTest):
+
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test__get_actual_component_devices(self, mock_execute):
+        mock_execute.side_effect = [(hws.MDADM_DETAIL_OUTPUT, '')]
+        component_devices = raid_utils._get_actual_component_devices(
+            '/dev/md0')
+        self.assertEqual(['/dev/vde1', '/dev/vdf1'], component_devices)
+
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test__get_actual_component_devices_broken_raid0(self, mock_execute):
+        mock_execute.side_effect = [(hws.MDADM_DETAIL_OUTPUT_BROKEN_RAID0, '')]
+        component_devices = raid_utils._get_actual_component_devices(
+            '/dev/md126')
+        self.assertEqual(['/dev/sda2'], component_devices)
+
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
+                       autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_raid_device(self, mock_execute, mocked_components):
+        logical_disk = {
+            "block_devices": ['/dev/sda', '/dev/sdb', '/dev/sdc'],
+            "raid_level": "1",
+        }
+        mocked_components.return_value = ['/dev/sda1',
+                                          '/dev/sdb1',
+                                          '/dev/sdc1']
+
+        raid_utils.create_raid_device(0, logical_disk)
+
+        mock_execute.assert_called_once_with(
+            'mdadm', '--create', '/dev/md0', '--force', '--run',
+            '--metadata=1', '--level', '1', '--raid-devices', 3,
+            '/dev/sda1', '/dev/sdb1', '/dev/sdc1')
+
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
+                       autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_raid_device_missing_device(self, mock_execute,
+                                               mocked_components):
+        logical_disk = {
+            "block_devices": ['/dev/sda', '/dev/sdb', '/dev/sdc'],
+            "raid_level": "1",
+        }
+        mocked_components.return_value = ['/dev/sda1',
+                                          '/dev/sdc1']
+
+        raid_utils.create_raid_device(0, logical_disk)
+
+        expected_calls = [
+            mock.call('mdadm', '--create', '/dev/md0', '--force', '--run',
+                      '--metadata=1', '--level', '1', '--raid-devices', 3,
+                      '/dev/sda1', '/dev/sdb1', '/dev/sdc1'),
+            mock.call('mdadm', '--add', '/dev/md0', '/dev/sdb1',
+                      attempts=3, delay_on_retry=True)
+        ]
+        self.assertEqual(mock_execute.call_count, 2)
+        mock_execute.assert_has_calls(expected_calls)
+
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_raid_device_fail_create_device(self, mock_execute):
+        logical_disk = {
+            "block_devices": ['/dev/sda', '/dev/sdb', '/dev/sdc'],
+            "raid_level": "1",
+        }
+        mock_execute.side_effect = processutils.ProcessExecutionError()
+
+        self.assertRaisesRegex(errors.SoftwareRAIDError,
+                               "Failed to create md device /dev/md0",
+                               raid_utils.create_raid_device, 0,
+                               logical_disk)
+
+    @mock.patch.object(raid_utils, '_get_actual_component_devices',
+                       autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_raid_device_fail_read_device(self, mock_execute,
+                                                 mocked_components):
+        logical_disk = {
+            "block_devices": ['/dev/sda', '/dev/sdb', '/dev/sdc'],
+            "raid_level": "1",
+        }
+        mock_execute.side_effect = [mock.Mock,
+                                    processutils.ProcessExecutionError()]
+
+        mocked_components.return_value = ['/dev/sda1',
+                                          '/dev/sdc1']
+
+        self.assertRaisesRegex(errors.SoftwareRAIDError,
+                               "Failed re-add /dev/sdb1 to /dev/md0",
+                               raid_utils.create_raid_device, 0,
+                               logical_disk)