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 ff312640d8)

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
-
-        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.
-
-        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.
+        """Convert disks RAID status
+
+        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.
+
+        :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.

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
-
-        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.
-
-        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.
+        """Convert disks RAID status
+
+        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.
+
+        :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)
-
-        return {'is_reboot_required': is_reboot_required,
+                    controllers_to_results[controller] = conversion_results
+
+                    # 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):

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)