Separate reboot required from commit required

This patch separates indicators for reboot required and commit required.
It does this by adding two new keys (is_reboot_required and
is_commit_required) to the dictionary returned by set/create/modify
operations while continuing to return the commit_required key for
backwards compatibility.

The commit_required key should be considered deprecated at this time and
it will be removed in a future patch.

The is_reboot_required key has an enumerated value that can have three
possible values: true, optional, and false.  This allows the return of
optional for RAID operations that can return this value.

Change-Id: I70e52868f10bfafb30bbb38b85888bc4ec8e65ae
Closes-Bug: 1732549

dracclient/client.py

@@ -170,9 +170,18 @@ class DRACClient(object):
         :param settings: a dictionary containing the proposed values, with
                          each key being the name of attribute and the value
                          being the proposed value.
-        :returns: a dictionary containing the commit_needed key with a boolean
+        :returns: a dictionary containing:
-                  value indicating whether a config job must be created for the
+                 - The commit_required key with a boolean value indicating
-                  values to be applied.
+                   whether a config job must be created for the values to be
+                   applied.  This key actually has a value that indicates if
+                   a reboot is required.  This key has been deprecated and
+                   will be removed in a future release.
+                 - The is_commit_required key with a boolean value indicating
+                   whether a config job must be created for the values to be
+                   applied.
+                 - The is_reboot_required key with a RebootRequired enumerated
+                   value indicating whether the server must be rebooted for the
+                   values to be applied.  Possible values are true and false.
         :raises: WSManRequestFailure on request failures
         :raises: WSManInvalidResponse when receiving invalid response
         :raises: DRACOperationFailed on error reported back by the DRAC
@@ -404,9 +413,18 @@ class DRACClient(object):
         :param raid_enable: boolean flag, set to True if the disk is to
                become part of the RAID.  The same flag is applied to all
                listed disks
-        :returns: a dictionary containing the commit_required key with a
+        :returns: a dictionary containing:
-                  boolean value indicating whether a config job must be
+                 - The commit_required key with a boolean value indicating
-                  created for the values to be applied.
+                   whether a config job must be created for the values to be
+                   applied.  This key actually has a value that indicates if
+                   a reboot is required.  This key has been deprecated and
+                   will be removed in a future release.
+                 - 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.
         """
         return self._raid_mgmt.convert_physical_disks(
             physical_disks, raid_enable)
@@ -425,9 +443,18 @@ class DRACClient(object):
         :param disk_name: name of the virtual disk (optional)
         :param span_length: number of disks per span (optional)
         :param span_depth: number of spans in virtual disk (optional)
-        :returns: a dictionary containing the commit_needed key with a boolean
+        :returns: a dictionary containing:
-                  value indicating whether a config job must be created for the
+                 - The commit_required key with a boolean value indicating
-                  values to be applied.
+                   whether a config job must be created for the values to be
+                   applied.  This key actually has a value that indicates if
+                   a reboot is required.  This key has been deprecated and
+                   will be removed in a future release.
+                 - The is_commit_required key with the value always set to
+                   True indicating that a config job must be created to
+                   complete virtual disk creation.
+                 - The is_reboot_required key with a RebootRequired enumerated
+                   value indicating whether the server must be rebooted to
+                   complete virtual disk creation.
         :raises: WSManRequestFailure on request failures
         :raises: WSManInvalidResponse when receiving invalid response
         :raises: DRACOperationFailed on error reported back by the DRAC
@@ -446,9 +473,18 @@ class DRACClient(object):
         be applied, a config job must be created and the node must be rebooted.
 
         :param virtual_disk: id of the virtual disk
-        :returns: a dictionary containing the commit_needed key with a boolean
+        :returns: a dictionary containing:
-                  value indicating whether a config job must be created for the
+                 - The commit_required key with a boolean value indicating
-                  values to be applied.
+                   whether a config job must be created for the values to be
+                   applied.  This key actually has a value that indicates if
+                   a reboot is required.  This key has been deprecated and
+                   will be removed in a future release.
+                 - The is_commit_required key with the value always set to
+                   True indicating that a config job must be created to
+                   complete virtual disk deletion.
+                 - The is_reboot_required key with a RebootRequired enumerated
+                   value indicating whether the server must be rebooted to
+                   complete virtual disk deletion.
         :raises: WSManRequestFailure on request failures
         :raises: WSManInvalidResponse when receiving invalid response
         :raises: DRACOperationFailed on error reported back by the DRAC

dracclient/constants.py

@@ -34,3 +34,19 @@ PRIMARY_STATUS = {
 
 # binary unit constants
 UNITS_KI = 2 ** 10
+
+
+# Reboot required indicator
+# Note: When the iDRAC returns optional for this value, this indicates that
+# either a reboot may be performed to complete the operation or a real time
+# config job may be created to complete the operation without performing a
+# reboot.  This library does not currently support creating a real time config
+# job, so a reboot must be performed when a value of "optional" is returned.
+class RebootRequired(object):
+    true = 'true'
+    optional = 'optional'
+    false = 'false'
+
+    @classmethod
+    def all(cls):
+        return [cls.true, cls.optional, cls.false]

dracclient/resources/bios.py

@@ -522,9 +522,18 @@ class BIOSConfiguration(object):
         :param new_settings: a dictionary containing the proposed values, with
                              each key being the name of attribute and the
                              value being the proposed value.
-        :returns: a dictionary containing the commit_needed key with a boolean
+        :returns: a dictionary containing:
-                  value indicating whether a config job must be created for the
+                 - The commit_required key with a boolean value indicating
-                  values to be applied.
+                   whether a config job must be created for the values to be
+                   applied.  This key actually has a value that indicates if
+                   a reboot is required.  This key has been deprecated and
+                   will be removed in a future release.
+                 - The is_commit_required key with a boolean value indicating
+                   whether a config job must be created for the values to be
+                   applied.
+                 - The is_reboot_required key with a RebootRequired enumerated
+                   value indicating whether the server must be rebooted for the
+                   values to be applied.  Possible values are true and false.
         :raises: WSManRequestFailure on request failures
         :raises: WSManInvalidResponse when receiving invalid response
         :raises: DRACOperationFailed on error reported back by the DRAC
@@ -583,7 +592,10 @@ class BIOSConfiguration(object):
                 drac_messages=drac_messages)
 
         if not attrib_names:
-            return {'commit_required': False}
+            return utils.build_return_dict(
+                None, uris.DCIM_BIOSService, is_commit_required_value=False,
+                is_reboot_required_value=constants.RebootRequired.false,
+                commit_required_value=False)
 
         selectors = {'CreationClassName': 'DCIM_BIOSService',
                      'Name': 'DCIM:BIOSService',
@@ -596,5 +608,4 @@ class BIOSConfiguration(object):
         doc = self.client.invoke(uris.DCIM_BIOSService, 'SetAttributes',
                                  selectors, properties)
 
-        return {'commit_required': utils.is_reboot_required(
+        return utils.build_return_dict(doc, uris.DCIM_BIOSService)
-            doc, uris.DCIM_BIOSService)}

dracclient/resources/raid.py

@@ -318,9 +318,18 @@ class RAIDManagement(object):
         :param raid_enable: boolean flag, set to True if the disk is to
                become part of the RAID.  The same flag is applied to all
                listed disks
-        :returns: a dictionary containing the commit_needed key with a boolean
+        :returns: a dictionary containing:
-                  value indicating whether a config job must be created for the
+                 - The commit_required key with a boolean value indicating
-                  values to be applied.
+                   whether a config job must be created for the values to be
+                   applied.  This key actually has a value that indicates if
+                   a reboot is required.  This key has been deprecated and
+                   will be removed in a future release.
+                 - 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.
         """
         invocation = 'ConvertToRAID' if raid_enable else 'ConvertToNonRAID'
 
@@ -335,8 +344,8 @@ class RAIDManagement(object):
                                  selectors, properties,
                                  expected_return_value=utils.RET_SUCCESS)
 
-        return {'commit_required':
+        return utils.build_return_dict(doc, uris.DCIM_RAIDService,
-                utils.is_reboot_required(doc, uris.DCIM_RAIDService)}
+                                       is_commit_required_value=True)
 
     def create_virtual_disk(self, raid_controller, physical_disks, raid_level,
                             size_mb, disk_name=None, span_length=None,
@@ -353,9 +362,18 @@ class RAIDManagement(object):
         :param disk_name: name of the virtual disk (optional)
         :param span_length: number of disks per span (optional)
         :param span_depth: number of spans in virtual disk (optional)
-        :returns: a dictionary containing the commit_needed key with a boolean
+        :returns: a dictionary containing:
-                  value indicating whether a config job must be created for the
+                 - The commit_required key with a boolean value indicating
-                  values to be applied.
+                   whether a config job must be created for the values to be
+                   applied.  This key actually has a value that indicates if
+                   a reboot is required.  This key has been deprecated and
+                   will be removed in a future release.
+                 - The is_commit_required key with the value always set to
+                   True indicating that a config job must be created to
+                   complete virtual disk creation.
+                 - The is_reboot_required key with a RebootRequired enumerated
+                   value indicating whether the server must be rebooted to
+                   complete virtual disk creation.
         :raises: WSManRequestFailure on request failures
         :raises: WSManInvalidResponse when receiving invalid response
         :raises: DRACOperationFailed on error reported back by the DRAC
@@ -426,8 +444,8 @@ class RAIDManagement(object):
                                  selectors, properties,
                                  expected_return_value=utils.RET_SUCCESS)
 
-        return {'commit_required': utils.is_reboot_required(
+        return utils.build_return_dict(doc, uris.DCIM_RAIDService,
-            doc, uris.DCIM_RAIDService)}
+                                       is_commit_required_value=True)
 
     def delete_virtual_disk(self, virtual_disk):
         """Deletes a virtual disk
@@ -436,9 +454,18 @@ class RAIDManagement(object):
         be applied, a config job must be created and the node must be rebooted.
 
         :param virtual_disk: id of the virtual disk
-        :returns: a dictionary containing the commit_needed key with a boolean
+        :returns: a dictionary containing:
-                  value indicating whether a config job must be created for the
+                 - The commit_required key with a boolean value indicating
-                  values to be applied.
+                   whether a config job must be created for the values to be
+                   applied.  This key actually has a value that indicates if
+                   a reboot is required.  This key has been deprecated and
+                   will be removed in a future release.
+                 - The is_commit_required key with the value always set to
+                   True indicating that a config job must be created to
+                   complete virtual disk deletion.
+                 - The is_reboot_required key with a RebootRequired enumerated
+                   value indicating whether the server must be rebooted to
+                   complete virtual disk deletion.
         :raises: WSManRequestFailure on request failures
         :raises: WSManInvalidResponse when receiving invalid response
         :raises: DRACOperationFailed on error reported back by the DRAC
@@ -456,5 +483,5 @@ class RAIDManagement(object):
                                  selectors, properties,
                                  expected_return_value=utils.RET_SUCCESS)
 
-        return {'commit_required': utils.is_reboot_required(
+        return utils.build_return_dict(doc, uris.DCIM_RAIDService,
-            doc, uris.DCIM_RAIDService)}
+                                       is_commit_required_value=True)

dracclient/tests/test_bios.py

@@ -18,6 +18,7 @@ import mock
 import requests_mock
 
 import dracclient.client
+from dracclient import constants
 from dracclient import exceptions
 from dracclient.resources import bios
 import dracclient.resources.job
@@ -347,7 +348,10 @@ class ClientBIOSConfigurationTestCase(base.BaseTest):
         result = self.drac_client.set_bios_settings(
             {'ProcVirtualization': 'Disabled'})
 
-        self.assertEqual({'commit_required': True}, result)
+        self.assertEqual({'commit_required': True,
+                          'is_commit_required': True,
+                          'is_reboot_required': constants.RebootRequired.true},
+                         result)
         mock_invoke.assert_called_once_with(
             mock.ANY, uris.DCIM_BIOSService, 'SetAttributes',
             expected_selectors, expected_properties)
@@ -394,7 +398,11 @@ class ClientBIOSConfigurationTestCase(base.BaseTest):
         result = self.drac_client.set_bios_settings(
             {'ProcVirtualization': 'Enabled'})
 
-        self.assertEqual({'commit_required': False}, result)
+        self.assertEqual({'commit_required': False,
+                          'is_commit_required': False,
+                          'is_reboot_required':
+                          constants.RebootRequired.false},
+                         result)
 
     def test_set_bios_settings_with_readonly_attr(
             self, mock_requests, mock_wait_until_idrac_is_ready):

dracclient/tests/test_raid.py

@@ -17,6 +17,7 @@ import random
 import requests_mock
 
 import dracclient.client
+from dracclient import constants
 from dracclient import exceptions
 import dracclient.resources.job
 from dracclient.resources import raid
@@ -165,13 +166,19 @@ class ClientRAIDManagementTestCase(base.BaseTest):
                               'SystemName': 'DCIM:ComputerSystem',
                               'Name': 'DCIM:RAIDService'}
         expected_properties = {'PDArray': [device_fqdd]}
+        mock_invoke.return_value = lxml.etree.fromstring(
+            test_utils.RAIDInvocations[uris.DCIM_RAIDService][
+                'ConvertToRAID']['ok'])
 
         result = self.drac_client.convert_physical_disks(
             raid_controller='controller',
             physical_disks=[device_fqdd],
             raid_enable=True)
 
-        self.assertEqual({'commit_required': False}, result)
+        self.assertEqual({'commit_required': True,
+                          'is_commit_required': True,
+                          'is_reboot_required': constants.RebootRequired.true},
+                         result)
         mock_invoke.assert_called_once_with(
             mock.ANY, uris.DCIM_RAIDService, expected_invocation,
             expected_selectors, expected_properties,
@@ -191,13 +198,19 @@ class ClientRAIDManagementTestCase(base.BaseTest):
                               'SystemName': 'DCIM:ComputerSystem',
                               'Name': 'DCIM:RAIDService'}
         expected_properties = {'PDArray': device_list}
+        mock_invoke.return_value = lxml.etree.fromstring(
+            test_utils.RAIDInvocations[uris.DCIM_RAIDService][
+                'ConvertToRAID']['ok'])
 
         result = self.drac_client.convert_physical_disks(
             raid_controller='controller',
             physical_disks=device_list,
             raid_enable=True)
 
-        self.assertEqual({'commit_required': False}, result)
+        self.assertEqual({'commit_required': True,
+                          'is_commit_required': True,
+                          'is_reboot_required': constants.RebootRequired.true},
+                         result)
         mock_invoke.assert_called_once_with(
             mock.ANY, uris.DCIM_RAIDService, expected_invocation,
             expected_selectors, expected_properties,
@@ -215,13 +228,19 @@ class ClientRAIDManagementTestCase(base.BaseTest):
                               'SystemName': 'DCIM:ComputerSystem',
                               'Name': 'DCIM:RAIDService'}
         expected_properties = {'PDArray': [device_fqdd]}
+        mock_invoke.return_value = lxml.etree.fromstring(
+            test_utils.RAIDInvocations[uris.DCIM_RAIDService][
+                'ConvertToRAID']['ok'])
 
         result = self.drac_client.convert_physical_disks(
             raid_controller='controller',
             physical_disks=[device_fqdd],
             raid_enable=False)
 
-        self.assertEqual({'commit_required': False}, result)
+        self.assertEqual({'commit_required': True,
+                          'is_commit_required': True,
+                          'is_reboot_required': constants.RebootRequired.true},
+                         result)
         mock_invoke.assert_called_once_with(
             mock.ANY, uris.DCIM_RAIDService, expected_invocation,
             expected_selectors, expected_properties,
@@ -242,43 +261,19 @@ class ClientRAIDManagementTestCase(base.BaseTest):
                               'SystemName': 'DCIM:ComputerSystem',
                               'Name': 'DCIM:RAIDService'}
         expected_properties = {'PDArray': device_list}
+        mock_invoke.return_value = lxml.etree.fromstring(
+            test_utils.RAIDInvocations[uris.DCIM_RAIDService][
+                'ConvertToRAID']['ok'])
 
         result = self.drac_client.convert_physical_disks(
             raid_controller='controller',
             physical_disks=device_list,
             raid_enable=False)
 
-        self.assertEqual({'commit_required': False}, result)
+        self.assertEqual({'commit_required': True,
-        mock_invoke.assert_called_once_with(
+                          'is_commit_required': True,
-            mock.ANY, uris.DCIM_RAIDService, expected_invocation,
+                          'is_reboot_required': constants.RebootRequired.true},
-            expected_selectors, expected_properties,
+                         result)
-            expected_return_value=utils.RET_SUCCESS)
-
-    @mock.patch.object(dracclient.client.WSManClient, 'invoke',
-                       spec_set=True, autospec=True)
-    def test_convert_physical_disks_ok(self, mock_requests, mock_invoke):
-        '''Convert a number of disks to RAID mode and check the return value'''
-        device_list = []
-        for i in range(0, random.randint(2, 10)):
-            device_list += self._random_fqdd()
-
-        expected_invocation = 'ConvertToRAID'
-        expected_selectors = {'SystemCreationClassName': 'DCIM_ComputerSystem',
-                              'CreationClassName': 'DCIM_RAIDService',
-                              'SystemName': 'DCIM:ComputerSystem',
-                              'Name': 'DCIM:RAIDService'}
-        expected_properties = {'PDArray': device_list}
-
-        mock_invoke.return_value = lxml.etree.fromstring(
-            test_utils.RAIDInvocations[uris.DCIM_RAIDService][
-                expected_invocation]['ok'])
-
-        result = self.drac_client.convert_physical_disks(
-            raid_controller='controller',
-            physical_disks=device_list,
-            raid_enable=True)
-
-        self.assertEqual({'commit_required': True}, result)
         mock_invoke.assert_called_once_with(
             mock.ANY, uris.DCIM_RAIDService, expected_invocation,
             expected_selectors, expected_properties,
@@ -321,7 +316,10 @@ class ClientRAIDManagementTestCase(base.BaseTest):
             raid_controller='controller', physical_disks=['disk1', 'disk2'],
             raid_level='1', size_mb=42)
 
-        self.assertEqual({'commit_required': True}, result)
+        self.assertEqual({'commit_required': True,
+                          'is_commit_required': True,
+                          'is_reboot_required': constants.RebootRequired.true},
+                         result)
         mock_invoke.assert_called_once_with(
             mock.ANY, uris.DCIM_RAIDService, 'CreateVirtualDisk',
             expected_selectors, expected_properties,
@@ -351,7 +349,10 @@ class ClientRAIDManagementTestCase(base.BaseTest):
             raid_level='1', size_mb=42, disk_name='name', span_length=2,
             span_depth=3)
 
-        self.assertEqual({'commit_required': True}, result)
+        self.assertEqual({'commit_required': True,
+                          'is_commit_required': True,
+                          'is_reboot_required': constants.RebootRequired.true},
+                         result)
         mock_invoke.assert_called_once_with(
             mock.ANY, uris.DCIM_RAIDService, 'CreateVirtualDisk',
             expected_selectors, expected_properties,
@@ -443,7 +444,10 @@ class ClientRAIDManagementTestCase(base.BaseTest):
 
         result = self.drac_client.delete_virtual_disk('disk1')
 
-        self.assertEqual({'commit_required': True}, result)
+        self.assertEqual({'commit_required': True,
+                          'is_commit_required': True,
+                          'is_reboot_required': constants.RebootRequired.true},
+                         result)
         mock_invoke.assert_called_once_with(
             mock.ANY, uris.DCIM_RAIDService, 'DeleteVirtualDisk',
             expected_selectors, expected_properties,

dracclient/tests/test_utils.py

@@ -166,3 +166,10 @@ class UtilsTestCase(base.BaseTest):
             controllers[0], uris.DCIM_ControllerView, 'DriverVersion',
             nullable=True)
         self.assertEqual(result, [])
+
+    def test_build_return_dict_fail(self):
+        self.assertRaises(exceptions.InvalidParameterValue,
+                          utils.build_return_dict,
+                          doc=None,
+                          resource_uri=None,
+                          is_reboot_required_value='foo')

dracclient/utils.py

@@ -15,6 +15,7 @@
 Common functionalities shared between different DRAC modules.
 """
 
+from dracclient import constants
 from dracclient import exceptions
 
 NS_XMLSchema_Instance = 'http://www.w3.org/2001/XMLSchema-instance'
@@ -24,6 +25,12 @@ RET_SUCCESS = '0'
 RET_ERROR = '2'
 RET_CREATED = '4096'
 
+REBOOT_REQUIRED = {
+    'yes': constants.RebootRequired.true,
+    'no': constants.RebootRequired.false,
+    'optional': constants.RebootRequired.optional
+}
+
 
 def find_xml(doc, item, namespace, find_all=False):
     """Find the first or all elements in an ElementTree object.
@@ -118,6 +125,77 @@ def get_all_wsman_resource_attrs(doc, resource_uri, attr_name, nullable=False):
         return [item.text.strip() for item in items if _is_attr_non_nil(item)]
 
 
+def build_return_dict(doc, resource_uri,
+                      is_commit_required_value=None,
+                      is_reboot_required_value=None,
+                      commit_required_value=None):
+    """Builds a dictionary to be returned
+
+       Build a dictionary to be returned from WSMAN operations that are not
+       read-only.
+
+    :param doc: the element tree object.
+    :param resource_uri: the resource URI of the namespace.
+    :param is_commit_required_value: The value to be returned for
+           is_commit_required, or None if the value should be determined
+           from the doc.
+    :param is_reboot_required_value: The value to be returned for
+           is_reboot_required, or None if the value should be determined
+           from the doc.
+    :param commit_required_value: The value to be returned for
+           commit_required, or None if the value should be determined
+           from the doc.
+    :returns: a dictionary containing:
+             - is_commit_required: indicates if a commit is required.
+             - is_reboot_required: indicates if a reboot is required.
+             - commit_required: a deprecated key indicating if a commit is
+               required.  This key actually has a value that indicates if a
+               reboot is required.
+    """
+
+    if is_reboot_required_value is not None and \
+            is_reboot_required_value not in constants.RebootRequired.all():
+        msg = ("is_reboot_required_value must be a member of the "
+               "RebootRequired enumeration or None.  The passed value was "
+               "%(is_reboot_required_value)s" % {
+                   'is_reboot_required_value': is_reboot_required_value})
+        raise exceptions.InvalidParameterValue(reason=msg)
+
+    result = {}
+    if is_commit_required_value is None:
+        is_commit_required_value = is_commit_required(doc, resource_uri)
+
+    result['is_commit_required'] = is_commit_required_value
+
+    if is_reboot_required_value is None:
+        is_reboot_required_value = reboot_required(doc, resource_uri)
+
+    result['is_reboot_required'] = is_reboot_required_value
+
+    # Include commit_required in the response for backwards compatibility
+    # TBD: Remove this parameter in the future
+    if commit_required_value is None:
+        commit_required_value = is_reboot_required(doc, resource_uri)
+
+    result['commit_required'] = commit_required_value
+
+    return result
+
+
+def is_commit_required(doc, resource_uri):
+    """Check the response document if commit is required.
+
+    If SetResult contains "pending" in the response then a commit is required.
+
+    :param doc: the element tree object.
+    :param resource_uri: the resource URI of the namespace.
+    :returns: a boolean value indicating commit is required or not.
+    """
+
+    commit_required = find_xml(doc, 'SetResult', resource_uri)
+    return "pendingvalue" in commit_required.text.lower()
+
+
 def is_reboot_required(doc, resource_uri):
     """Check the response document if reboot is requested.
 
@@ -134,6 +212,22 @@ def is_reboot_required(doc, resource_uri):
     return reboot_required.text.lower() == 'yes'
 
 
+def reboot_required(doc, resource_uri):
+    """Check the response document if reboot is requested.
+
+    RebootRequired attribute in the response indicates whether node needs to
+    be rebooted, so that the pending changes can be committed.
+
+    :param doc: the element tree object.
+    :param resource_uri: the resource URI of the namespace.
+    :returns: True if reboot is required, False if it is not, and the string
+              "optional" if reboot is optional.
+    """
+
+    reboot_required_value = find_xml(doc, 'RebootRequired', resource_uri)
+    return REBOOT_REQUIRED[reboot_required_value.text.lower()]
+
+
 def validate_integer_value(value, attr_name, error_msgs):
     """Validate integer value"""