From 2f8918cb3e4782e9c0625adcf5de9dda441d7d7d Mon Sep 17 00:00:00 2001 From: Christopher Dearborn Date: Fri, 19 Apr 2019 14:34:33 -0400 Subject: [PATCH] Add realtime support to drive conversion This patch updates change_physical_disk_state() so that it returns the actual results of drive conversion on each controller. This allows the caller to use the returned information for realtime drive conversion. This patch also deprecates returning the is_reboot_required and commit_required_ids keys in the dictionary. Change-Id: I10f4a44660e70f0cd8efd0ca9e8e96cb46751a61 (cherry picked from commit ff312640d8af2b2b18ab08bf725a4ee8f2e99bcf) --- dracclient/client.py | 43 ++++----- dracclient/resources/raid.py | 106 +++++++++++++--------- dracclient/tests/test_raid.py | 160 +++++++++++++--------------------- 3 files changed, 150 insertions(+), 159 deletions(-) diff --git a/dracclient/client.py b/dracclient/client.py index fa6016a..422ec37 100644 --- a/dracclient/client.py +++ b/dracclient/client.py @@ -1059,32 +1059,33 @@ class DRACClient(object): def change_physical_disk_state(self, mode, controllers_to_physical_disk_ids=None): - """Convert disks RAID status and return a list of controller IDs + """Convert disks RAID status - Builds a list of controller ids that have had disks converted to the - specified RAID status by: - - Examining all the disks in the system and filtering out any that are - not attached to a RAID/BOSS controller. - - Inspect the controllers' disks to see if there are any that need to - be converted, if so convert them. If a disk is already in the desired - status the disk is ignored. Also check for failed or unknown disk - statuses and raise an exception where appropriate. - - Return a list of controller IDs for controllers whom have had any of - their disks converted, and whether a reboot is required. + This method intelligently converts the requested physical disks from + RAID to JBOD or vice versa. It does this by only converting the + disks that are not already in the correct state. - The caller typically should then create a config job for the list of - controllers returned to finalize the RAID configuration. - - :param mode: constants.RaidStatus enumeration used to determine what - raid status to check for. + :param mode: constants.RaidStatus enumeration that indicates the mode + to change the disks to. :param controllers_to_physical_disk_ids: Dictionary of controllers and - corresponding disk ids we are inspecting and creating jobs for - when needed. - :returns: a dict containing the following key/values: + corresponding disk ids to convert to the requested mode. + :returns: a dictionary containing: + - conversion_results, a dictionary that maps controller ids + to the conversion results for that controller. The + conversion results are a dict that contains: + - The is_commit_required key with the value always set to + True indicating that a config job must be created to + complete disk conversion. + - The is_reboot_required key with a RebootRequired + enumerated value indicating whether the server must be + rebooted to complete disk conversion. + Also contained in the main dict are the following key/values, + which are deprecated, should not be used, and will be removed + in a future release: - is_reboot_required, a boolean stating whether a reboot is - required or not. + required or not. - commit_required_ids, a list of controller ids that will - need to commit their pending RAID changes via a config job. + need to commit their pending RAID changes via a config job. :raises: DRACOperationFailed on error reported back by the DRAC and the exception message does not contain NOT_SUPPORTED_MSG constant. :raises: Exception on unknown error. diff --git a/dracclient/resources/raid.py b/dracclient/resources/raid.py index 30aae6c..ef8e5ca 100644 --- a/dracclient/resources/raid.py +++ b/dracclient/resources/raid.py @@ -12,6 +12,7 @@ # under the License. import collections +import copy import logging from dracclient import constants @@ -635,15 +636,17 @@ class RAIDManagement(object): :param mode: constants.RaidStatus enumeration used to determine what raid status to check for. :param physical_disks: all physical disks - :param controllers_to_physical_disk_ids: Dictionary of controllers - we are inspecting and creating jobs for when needed. If - needed modify this dict so that only drives that need to - be changed to RAID or JBOD are in the list of disk keys - for corresponding controller. + :param controllers_to_physical_disk_ids: Dictionary of controllers and + corresponding disk ids to convert to the requested mode. + :returns: a dictionary mapping controller FQDDs to the list of + physical disks that need to be converted for that controller. :raises: ValueError: Exception message will list failed drives and drives whose state cannot be changed at this time, drive state is not "ready" or "non-RAID". """ + controllers_to_physical_disk_ids = copy.deepcopy( + controllers_to_physical_disk_ids) + p_disk_id_to_status = {} for physical_disk in physical_disks: p_disk_id_to_status[physical_disk.id] = physical_disk.raid_status @@ -702,34 +705,37 @@ class RAIDManagement(object): raise ValueError(error_msg) + return controllers_to_physical_disk_ids + def change_physical_disk_state(self, mode, controllers_to_physical_disk_ids=None): - """Convert disks RAID status and return a list of controller IDs + """Convert disks RAID status - Builds a list of controller ids that have had disks converted to the - specified RAID status by: - - Examining all the disks in the system and filtering out any that are - not attached to a RAID/BOSS controller. - - Inspect the controllers' disks to see if there are any that need to - be converted, if so convert them. If a disk is already in the desired - status the disk is ignored. Also check for failed or unknown disk - statuses and raise an exception where appropriate. - - Return a list of controller IDs for controllers whom have had any of - their disks converted, and whether a reboot is required. + This method intelligently converts the requested physical disks from + RAID to JBOD or vice versa. It does this by only converting the + disks that are not already in the correct state. - The caller typically should then create a config job for the list of - controllers returned to finalize the RAID configuration. - - :param mode: constants.RaidStatus enumeration used to determine what - raid status to check for. + :param mode: constants.RaidStatus enumeration that indicates the mode + to change the disks to. :param controllers_to_physical_disk_ids: Dictionary of controllers and - corresponding disk ids we are inspecting and creating jobs for - when needed. - :returns: a dict containing the following key/values: + corresponding disk ids to convert to the requested mode. + :returns: a dictionary containing: + - conversion_results, a dictionary that maps controller ids + to the conversion results for that controller. The + conversion results are a dict that contains: + - The is_commit_required key with the value always set to + True indicating that a config job must be created to + complete disk conversion. + - The is_reboot_required key with a RebootRequired + enumerated value indicating whether the server must be + rebooted to complete disk conversion. + Also contained in the main dict are the following key/values, + which are deprecated, should not be used, and will be removed + in a future release: - is_reboot_required, a boolean stating whether a reboot is - required or not. + required or not. - commit_required_ids, a list of controller ids that will - need to commit their pending RAID changes via a config job. + need to commit their pending RAID changes via a config job. :raises: DRACOperationFailed on error reported back by the DRAC and the exception message does not contain NOT_SUPPORTED_MSG constant. :raises: Exception on unknown error. @@ -756,13 +762,14 @@ class RAIDManagement(object): Raise exception if there are any failed drives or drives not in status 'ready' or 'non-RAID' ''' - self._check_disks_status(mode, physical_disks, - controllers_to_physical_disk_ids) + final_ctls_to_phys_disk_ids = self._check_disks_status( + mode, physical_disks, controllers_to_physical_disk_ids) is_reboot_required = False controllers = [] + controllers_to_results = {} for controller, physical_disk_ids \ - in controllers_to_physical_disk_ids.items(): + in final_ctls_to_phys_disk_ids.items(): if physical_disk_ids: LOG.debug("Converting the following disks to {} on RAID " "controller {}: {}".format( @@ -775,22 +782,39 @@ class RAIDManagement(object): if constants.NOT_SUPPORTED_MSG in str(ex): LOG.debug("Controller {} does not support " "JBOD mode".format(controller)) - pass + controllers_to_results[controller] = \ + utils.build_return_dict( + doc=None, + resource_uri=None, + is_commit_required_value=False, + is_reboot_required_value=constants. + RebootRequired.false) else: raise else: - if conversion_results: - reboot_true = constants.RebootRequired.true - reboot_optional = constants.RebootRequired.optional - _is_reboot_required = \ - conversion_results["is_reboot_required"] - is_reboot_required = is_reboot_required \ - or (_is_reboot_required - in [reboot_true, reboot_optional]) - if conversion_results["is_commit_required"]: - controllers.append(controller) + controllers_to_results[controller] = conversion_results - return {'is_reboot_required': is_reboot_required, + # Remove the code below when is_reboot_required and + # commit_required_ids are deprecated + reboot_true = constants.RebootRequired.true + reboot_optional = constants.RebootRequired.optional + _is_reboot_required = \ + conversion_results["is_reboot_required"] + is_reboot_required = is_reboot_required \ + or (_is_reboot_required + in [reboot_true, reboot_optional]) + controllers.append(controller) + else: + controllers_to_results[controller] = \ + utils.build_return_dict( + doc=None, + resource_uri=None, + is_commit_required_value=False, + is_reboot_required_value=constants. + RebootRequired.false) + + return {'conversion_results': controllers_to_results, + 'is_reboot_required': is_reboot_required, 'commit_required_ids': controllers} def is_realtime_supported(self, raid_controller_fqdd): diff --git a/dracclient/tests/test_raid.py b/dracclient/tests/test_raid.py index 7d9aeb9..0057d5c 100644 --- a/dracclient/tests/test_raid.py +++ b/dracclient/tests/test_raid.py @@ -998,13 +998,13 @@ class ClientRAIDManagementTestCase(base.BaseTest): raid_mgt._check_disks_status, mode, physical_disks, - self.controllers_to_physical_disk_ids.copy()) + self.controllers_to_physical_disk_ids) mode = constants.RaidStatus.jbod self.assertRaises(ValueError, raid_mgt._check_disks_status, mode, physical_disks, - self.controllers_to_physical_disk_ids.copy()) + self.controllers_to_physical_disk_ids) def test_check_disks_status_fail(self, mock_requests): mode = constants.RaidStatus.raid @@ -1016,13 +1016,13 @@ class ClientRAIDManagementTestCase(base.BaseTest): raid_mgt._check_disks_status, mode, physical_disks, - self.controllers_to_physical_disk_ids.copy()) + self.controllers_to_physical_disk_ids) mode = constants.RaidStatus.jbod self.assertRaises(ValueError, raid_mgt._check_disks_status, mode, physical_disks, - self.controllers_to_physical_disk_ids.copy()) + self.controllers_to_physical_disk_ids) def test_check_disks_status_no_change(self, mock_requests): raid_mgt = self.drac_client._raid_mgmt @@ -1030,11 +1030,8 @@ class ClientRAIDManagementTestCase(base.BaseTest): physical_disks = [self.disk_1, self.disk_2, self.disk_3, self.disk_4] - raid_cntl_to_phys_disk_ids = (self.controllers_to_physical_disk_ids. - copy()) - - raid_mgt._check_disks_status(mode, physical_disks, - raid_cntl_to_phys_disk_ids) + raid_cntl_to_phys_disk_ids = raid_mgt._check_disks_status( + mode, physical_disks, self.controllers_to_physical_disk_ids) raid_len = len(raid_cntl_to_phys_disk_ids['RAID.Integrated.1-1']) self.assertEqual(raid_len, 0) @@ -1044,10 +1041,8 @@ class ClientRAIDManagementTestCase(base.BaseTest): physical_disks = [disk_1_non_raid, disk_2_non_raid, self.disk_3, self.disk_4] - jbod_cntl_to_phys_disk_ids = (self.controllers_to_physical_disk_ids. - copy()) - raid_mgt._check_disks_status(mode, physical_disks, - jbod_cntl_to_phys_disk_ids) + jbod_cntl_to_phys_disk_ids = raid_mgt._check_disks_status( + mode, physical_disks, self.controllers_to_physical_disk_ids) jbod_len = len(jbod_cntl_to_phys_disk_ids['RAID.Integrated.1-1']) self.assertEqual(jbod_len, 0) @@ -1056,11 +1051,8 @@ class ClientRAIDManagementTestCase(base.BaseTest): mode = constants.RaidStatus.jbod physical_disks = [self.disk_1, self.disk_2, self.disk_3, self.disk_4] - jbod_cntl_to_phys_disk_ids = (self.controllers_to_physical_disk_ids. - copy()) - - raid_mgt._check_disks_status(mode, physical_disks, - jbod_cntl_to_phys_disk_ids) + jbod_cntl_to_phys_disk_ids = raid_mgt._check_disks_status( + mode, physical_disks, self.controllers_to_physical_disk_ids) jbod_len = len(jbod_cntl_to_phys_disk_ids['RAID.Integrated.1-1']) self.assertEqual(jbod_len, 2) @@ -1069,10 +1061,8 @@ class ClientRAIDManagementTestCase(base.BaseTest): disk_2_non_raid = self.disk_2._replace(raid_status='non-RAID') physical_disks = [disk_1_non_raid, disk_2_non_raid, self.disk_3, self.disk_4] - raid_cntl_to_phys_disk_ids = (self.controllers_to_physical_disk_ids. - copy()) - raid_mgt._check_disks_status(mode, physical_disks, - raid_cntl_to_phys_disk_ids) + raid_cntl_to_phys_disk_ids = raid_mgt._check_disks_status( + mode, physical_disks, self.controllers_to_physical_disk_ids) raid_len = len(raid_cntl_to_phys_disk_ids['RAID.Integrated.1-1']) self.assertEqual(raid_len, 2) @@ -1087,13 +1077,13 @@ class ClientRAIDManagementTestCase(base.BaseTest): raid_mgt._check_disks_status, mode, physical_disks, - self.controllers_to_physical_disk_ids.copy()) + self.controllers_to_physical_disk_ids) mode = constants.RaidStatus.jbod self.assertRaises(ValueError, raid_mgt._check_disks_status, mode, physical_disks, - self.controllers_to_physical_disk_ids.copy()) + self.controllers_to_physical_disk_ids) @mock.patch.object(dracclient.client.WSManClient, 'wait_until_idrac_is_ready', spec_set=True, @@ -1109,16 +1099,22 @@ class ClientRAIDManagementTestCase(base.BaseTest): mock_requests.post( 'https://1.2.3.4:443/wsman', text=test_utils.RAIDEnumerations[uris.DCIM_PhysicalDiskView]['ok']) - mock_convert_physical_disks.return_value = {'commit_required': True, - 'is_commit_required': True, - 'is_reboot_required': - constants.RebootRequired - .true} - cntl_to_phys_d_ids = self.controllers_to_physical_disk_ids + cvt_phys_disks_return_value = {'commit_required': True, + 'is_commit_required': True, + 'is_reboot_required': constants. + RebootRequired.true} + mock_convert_physical_disks.return_value = cvt_phys_disks_return_value + + expected_return_value = {'RAID.Integrated.1-1': + cvt_phys_disks_return_value, + 'AHCI.Integrated.1-1': + cvt_phys_disks_return_value} results = self.drac_client.change_physical_disk_state( - mode, cntl_to_phys_d_ids) + mode, self.controllers_to_physical_disk_ids) self.assertTrue(results["is_reboot_required"]) self.assertEqual(len(results["commit_required_ids"]), 2) + self.assertEqual(results['conversion_results'], + expected_return_value) @mock.patch.object(dracclient.resources.raid.RAIDManagement, 'list_physical_disks', spec_set=True, @@ -1136,40 +1132,44 @@ class ClientRAIDManagementTestCase(base.BaseTest): physical_disks = [disk_1_non_raid, disk_2_non_raid, self.disk_3, self.disk_4] mock_list_physical_disks.return_value = physical_disks - mock_convert_physical_disks.return_value = {'commit_required': True, - 'is_commit_required': True, - 'is_reboot_required': - constants.RebootRequired - .true} - cntl_to_phys_d_ids = self.controllers_to_physical_disk_ids + boss_return_value = {'is_commit_required': False, + 'is_reboot_required': + constants.RebootRequired.false} + raid_return_value = {'is_commit_required': True, + 'is_reboot_required': + constants.RebootRequired.true} + mock_convert_physical_disks.return_value = raid_return_value + results = self.drac_client.change_physical_disk_state( - mode, cntl_to_phys_d_ids) + mode, self.controllers_to_physical_disk_ids) self.assertTrue(results["is_reboot_required"]) self.assertEqual(len(results["commit_required_ids"]), 1) + self.assertEqual(len(results['conversion_results']), 2) + self.assertEqual(results['conversion_results']['AHCI.Integrated.1-1'], + boss_return_value) + self.assertEqual(results['conversion_results']['RAID.Integrated.1-1'], + raid_return_value) @mock.patch.object(dracclient.resources.raid.RAIDManagement, 'list_physical_disks', spec_set=True, autospec=True) - @mock.patch.object(dracclient.resources.raid.RAIDManagement, - 'convert_physical_disks', spec_set=True, - autospec=True) def test_change_physical_disk_state_none( self, mock_requests, - mock_convert_physical_disks, mock_list_physical_disks): mode = constants.RaidStatus.raid physical_disks = [self.disk_1, self.disk_2, self.disk_3, self.disk_4] - mock_convert_physical_disks.return_value = {'commit_required': True, - 'is_commit_required': True, - 'is_reboot_required': - constants.RebootRequired - .true} mock_list_physical_disks.return_value = physical_disks - cntl_to_phys_d_ids = self.controllers_to_physical_disk_ids + expected_return_value = {'is_commit_required': False, + 'is_reboot_required': + constants.RebootRequired.false} results = self.drac_client.change_physical_disk_state( - mode, cntl_to_phys_d_ids) + mode, self.controllers_to_physical_disk_ids) self.assertFalse(results["is_reboot_required"]) self.assertEqual(len(results["commit_required_ids"]), 0) + self.assertEqual(results['conversion_results']['RAID.Integrated.1-1'], + expected_return_value) + self.assertEqual(results['conversion_results']['AHCI.Integrated.1-1'], + expected_return_value) @mock.patch.object(dracclient.resources.raid.RAIDManagement, 'list_physical_disks', spec_set=True, @@ -1189,11 +1189,17 @@ class ClientRAIDManagementTestCase(base.BaseTest): physical_disks = [disk_1_non_raid, disk_2_non_raid, self.disk_3, self.disk_4] mock_list_physical_disks.return_value = physical_disks - cntl_to_phys_d_ids = self.controllers_to_physical_disk_ids + expected_return_value = {'is_commit_required': False, + 'is_reboot_required': + constants.RebootRequired.false} results = self.drac_client.change_physical_disk_state( - mode, cntl_to_phys_d_ids) + mode, self.controllers_to_physical_disk_ids) self.assertFalse(results["is_reboot_required"]) self.assertEqual(len(results["commit_required_ids"]), 0) + self.assertEqual(results['conversion_results']['RAID.Integrated.1-1'], + expected_return_value) + self.assertEqual(results['conversion_results']['AHCI.Integrated.1-1'], + expected_return_value) @mock.patch.object(dracclient.resources.raid.RAIDManagement, 'list_physical_disks', spec_set=True, @@ -1213,12 +1219,12 @@ class ClientRAIDManagementTestCase(base.BaseTest): physical_disks = [disk_1_non_raid, disk_2_non_raid, self.disk_3, self.disk_4] mock_list_physical_disks.return_value = physical_disks - cntl_to_phys_d_ids = self.controllers_to_physical_disk_ids self.assertRaisesRegexp( exceptions.DRACOperationFailed, "OTHER_MESSAGE", self.drac_client.change_physical_disk_state, - mode, cntl_to_phys_d_ids) + mode, + self.controllers_to_physical_disk_ids) @mock.patch.object(dracclient.resources.raid.RAIDManagement, 'list_physical_disks', spec_set=True, @@ -1237,11 +1243,12 @@ class ClientRAIDManagementTestCase(base.BaseTest): physical_disks = [disk_1_non_raid, disk_2_non_raid, self.disk_3, self.disk_4] mock_list_physical_disks.return_value = physical_disks - cntl_to_phys_d_ids = self.controllers_to_physical_disk_ids self.assertRaisesRegexp( - Exception, "SOMETHING_BAD_HAPPENED", + Exception, + "SOMETHING_BAD_HAPPENED", self.drac_client.change_physical_disk_state, - mode, cntl_to_phys_d_ids) + mode, + self.controllers_to_physical_disk_ids) @mock.patch.object(dracclient.client.WSManClient, 'wait_until_idrac_is_ready', spec_set=True, @@ -1296,47 +1303,6 @@ class ClientRAIDManagementTestCase(base.BaseTest): self.assertFalse(results["is_reboot_required"]) self.assertEqual(len(results["commit_required_ids"]), 0) - @mock.patch.object(dracclient.client.WSManClient, - 'wait_until_idrac_is_ready', spec_set=True, - autospec=True) - @mock.patch.object(dracclient.resources.raid.RAIDManagement, - 'list_physical_disks', spec_set=True, - autospec=True) - @mock.patch.object(dracclient.resources.raid.RAIDManagement, - 'convert_physical_disks', spec_set=True, - autospec=True) - def test_change_physical_disk_state_conversion_return_values( - self, mock_requests, - mock_convert_physical_disks, - mock_list_physical_disks, - mock_wait_until_idrac_is_ready): - mock_requests.post( - 'https://1.2.3.4:443/wsman', - text=test_utils.RAIDEnumerations[uris.DCIM_ControllerView]['ok']) - mode = constants.RaidStatus.jbod - physical_disks = [self.disk_1, self.disk_2, self.disk_3, self.disk_4] - '''Test all logic branches for 100% coverage, it is unlikely - convert_physical_disks() will return empty dict but we do check - for this case in change_physical_disk_state()''' - mock_convert_physical_disks.return_value = {} - mock_list_physical_disks.return_value = physical_disks - results = self.drac_client.change_physical_disk_state(mode) - self.assertFalse(results["is_reboot_required"]) - self.assertEqual(len(results["commit_required_ids"]), 0) - '''Where convert_physical_disks() does not require a commit after - executing, unlikely case but provides 100% code coverage of all - logic branches.''' - mock_convert_physical_disks.return_value = {'commit_required': - True, - 'is_commit_required': - False, - 'is_reboot_required': - constants.RebootRequired - .false} - results = self.drac_client.change_physical_disk_state(mode) - self.assertFalse(results["is_reboot_required"]) - self.assertEqual(len(results["commit_required_ids"]), 0) - @mock.patch.object(dracclient.client.WSManClient, 'wait_until_idrac_is_ready', spec_set=True, autospec=True)