From 34b58f60241fe331fab809b711250d0fcd306353 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 1 Apr 2020 12:16:59 +0200 Subject: [PATCH] Only check for partitions on devices that are part of software RAID Now that an operator can pick the devices that participate in RAID, it no longer makes sense to verify all devices. Change-Id: Id5d8d539183f0db4ba3c4132ce6bc9919f9cd1ea Story: #2006369 --- ironic_python_agent/hardware.py | 28 +++---- .../tests/unit/test_hardware.py | 77 +++++++++++++++---- 2 files changed, 75 insertions(+), 30 deletions(-) diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 3d02df6ce..a4d805257 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -1527,21 +1527,21 @@ class GenericHardwareManager(HardwareManager): # Log the validated target_raid_configuration. LOG.debug("Target Software RAID configuration: %s", raid_config) - # Make sure there are no partitions yet (or left behind). - 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) - msg = "Partitions detected during RAID config: {}". format( - partitions) - raise errors.SoftwareRAIDError(msg) - block_devices, logical_disks = raid_utils.get_block_devices_for_raid( - block_devices, logical_disks) + self.list_block_devices(), logical_disks) + # Make sure there are no partitions yet (or left behind). + with_parts = [] + for dev_name in block_devices: + try: + if disk_utils.list_partitions(dev_name): + with_parts.append(dev_name) + except processutils.ProcessExecutionError: + # Presumably no partitions (or no partition table) + continue + if with_parts: + msg = ("Partitions detected on devices %s during RAID config" + % ', '.join(with_parts)) + raise errors.SoftwareRAIDError(msg) parted_start_dict = {} # Create an MBR partition table on each disk. diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 80140796e..c88809db8 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -2779,8 +2779,9 @@ class TestGenericHardwareManager(base.IronicAgentTest): self.hardware.validate_configuration, self.node, []) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True) @mock.patch.object(utils, 'execute', autospec=True) - def test_create_configuration(self, mocked_execute): + def test_create_configuration(self, mocked_execute, mock_list_parts): node = self.node raid_config = { "logical_disks": [ @@ -2801,6 +2802,10 @@ class TestGenericHardwareManager(base.IronicAgentTest): device2 = hardware.BlockDevice('/dev/sdb', 'sdb', 107374182400, True) self.hardware.list_block_devices = mock.Mock() self.hardware.list_block_devices.return_value = [device1, device2] + mock_list_parts.side_effect = [ + [], + processutils.ProcessExecutionError + ] mocked_execute.side_effect = [ None, # mklabel sda @@ -2843,8 +2848,16 @@ class TestGenericHardwareManager(base.IronicAgentTest): '/dev/sda2', '/dev/sdb2')]) self.assertEqual(raid_config, result) + self.assertEqual(2, mock_list_parts.call_count) + mock_list_parts.assert_has_calls([ + mock.call(x) for x in ['/dev/sda', '/dev/sdb'] + ]) + + @mock.patch.object(disk_utils, 'list_partitions', autospec=True, + return_value=[]) @mock.patch.object(utils, 'execute', autospec=True) - def test_create_configuration_raid_5(self, mocked_execute): + def test_create_configuration_raid_5(self, mocked_execute, + mock_list_parts): node = self.node raid_config = { "logical_disks": [ @@ -2922,8 +2935,11 @@ class TestGenericHardwareManager(base.IronicAgentTest): '/dev/sda2', '/dev/sdb2', '/dev/sdc2')]) self.assertEqual(raid_config, result) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True, + return_value=[]) @mock.patch.object(utils, 'execute', autospec=True) - def test_create_configuration_raid_6(self, mocked_execute): + def test_create_configuration_raid_6(self, mocked_execute, + mock_list_parts): node = self.node raid_config = { "logical_disks": [ @@ -3015,8 +3031,11 @@ class TestGenericHardwareManager(base.IronicAgentTest): '/dev/sda2', '/dev/sdb2', '/dev/sdc2', '/dev/sdd2')]) self.assertEqual(raid_config, result) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True, + return_value=[]) @mock.patch.object(utils, 'execute', autospec=True) - def test_create_configuration_no_max(self, mocked_execute): + def test_create_configuration_no_max(self, mocked_execute, + mock_list_parts): node = self.node raid_config = { "logical_disks": [ @@ -3078,8 +3097,11 @@ class TestGenericHardwareManager(base.IronicAgentTest): '/dev/sda2', '/dev/sdb2')]) self.assertEqual(raid_config, result) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True, + return_value=[]) @mock.patch.object(utils, 'execute', autospec=True) - def test_create_configuration_max_is_first_logical(self, mocked_execute): + def test_create_configuration_max_is_first_logical(self, mocked_execute, + mock_list_parts): node = self.node raid_config = { "logical_disks": [ @@ -3141,8 +3163,11 @@ class TestGenericHardwareManager(base.IronicAgentTest): '/dev/sda2', '/dev/sdb2')]) self.assertEqual(raid_config, result) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True, + return_value=[]) @mock.patch.object(utils, 'execute', autospec=True) - def test_create_configuration_with_hints(self, mocked_execute): + def test_create_configuration_with_hints(self, mocked_execute, + mock_list_parts): node = self.node raid_config = { "logical_disks": [ @@ -3216,6 +3241,11 @@ class TestGenericHardwareManager(base.IronicAgentTest): '/dev/sda2', '/dev/sdb2')]) self.assertEqual(raid_config, result) + self.assertEqual(2, mock_list_parts.call_count) + mock_list_parts.assert_has_calls([ + mock.call(x) for x in ['/dev/sda', '/dev/sdb'] + ]) + @mock.patch.object(utils, 'execute', autospec=True) def test_create_configuration_invalid_raid_config(self, mocked_execute): raid_config = { @@ -3291,8 +3321,10 @@ class TestGenericHardwareManager(base.IronicAgentTest): self.hardware.create_configuration, self.node, []) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True) @mock.patch.object(utils, 'execute', autospec=True) - def test_create_configuration_partitions_detected(self, mocked_execute): + def test_create_configuration_partitions_detected(self, mocked_execute, + mock_list_parts): raid_config = { "logical_disks": [ { @@ -3307,23 +3339,28 @@ class TestGenericHardwareManager(base.IronicAgentTest): }, ] } + mock_list_parts.side_effect = [ + [], + [{'partition_name': '/dev/sdb1'}], + ] self.node['target_raid_config'] = raid_config device1 = hardware.BlockDevice('/dev/sda', 'sda', 107374182400, True) device2 = hardware.BlockDevice('/dev/sdb', 'sdb', 107374182400, True) - partition1 = hardware.BlockDevice('/dev/sdb1', 'sdb1', 268435456, True) self.hardware.list_block_devices = mock.Mock() - self.hardware.list_block_devices.side_effect = [ - [device1, device2], # pre-flight validation call - [device1, device2], - [device1, device2, partition1]] + self.hardware.list_block_devices.return_value = [ + device1, device2 + ] self.assertRaises(errors.SoftwareRAIDError, self.hardware.create_configuration, self.node, []) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True, + return_value=[]) @mock.patch.object(utils, 'execute', autospec=True) def test_create_configuration_device_handling_failures(self, - mocked_execute): + mocked_execute, + mock_list_parts): raid_config = { "logical_disks": [ { @@ -3386,9 +3423,11 @@ class TestGenericHardwareManager(base.IronicAgentTest): self.hardware.create_configuration, self.node, []) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True, + return_value=[]) @mock.patch.object(utils, 'execute', autospec=True) def test_create_configuration_device_handling_failures_raid5( - self, mocked_execute): + self, mocked_execute, mock_list_parts): raid_config = { "logical_disks": [ { @@ -3459,8 +3498,11 @@ class TestGenericHardwareManager(base.IronicAgentTest): result = self.hardware.create_configuration(self.node, []) self.assertEqual(result, {}) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True, + return_value=[]) @mock.patch.object(utils, 'execute', autospec=True) - def test_create_configuration_with_nvme(self, mocked_execute): + def test_create_configuration_with_nvme(self, mocked_execute, + mock_list_parts): raid_config = { "logical_disks": [ { @@ -3524,8 +3566,11 @@ class TestGenericHardwareManager(base.IronicAgentTest): '/dev/nvme0n1p2', '/dev/nvme1n1p2')]) self.assertEqual(raid_config, result) + @mock.patch.object(disk_utils, 'list_partitions', autospec=True, + return_value=[]) @mock.patch.object(utils, 'execute', autospec=True) - def test_create_configuration_failure_with_nvme(self, mocked_execute): + def test_create_configuration_failure_with_nvme(self, mocked_execute, + mock_list_parts): raid_config = { "logical_disks": [ {