From 72ea2323e5969adc445d622e34b3b7455ea6c1c9 Mon Sep 17 00:00:00 2001 From: Christopher Dearborn Date: Wed, 15 Nov 2017 17:11:26 -0500 Subject: [PATCH] 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 | 60 +++++++++++++++++----- dracclient/constants.py | 16 ++++++ dracclient/resources/bios.py | 23 ++++++--- dracclient/resources/raid.py | 57 +++++++++++++++------ dracclient/tests/test_bios.py | 12 ++++- dracclient/tests/test_raid.py | 78 +++++++++++++++------------- dracclient/tests/test_utils.py | 7 +++ dracclient/utils.py | 94 ++++++++++++++++++++++++++++++++++ 8 files changed, 275 insertions(+), 72 deletions(-) diff --git a/dracclient/client.py b/dracclient/client.py index 4c68a9d..f325a24 100644 --- a/dracclient/client.py +++ b/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 - value indicating whether a config job must be created for the - values to be applied. + :returns: a dictionary containing: + - The commit_required key with a boolean value indicating + 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 - boolean value indicating whether a config job must be - created for the values to be applied. + :returns: a dictionary containing: + - The commit_required key with a boolean value indicating + 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 - value indicating whether a config job must be created for the - values to be applied. + :returns: a dictionary containing: + - The commit_required key with a boolean value indicating + 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 - value indicating whether a config job must be created for the - values to be applied. + :returns: a dictionary containing: + - The commit_required key with a boolean value indicating + 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 diff --git a/dracclient/constants.py b/dracclient/constants.py index a58bb09..a3b478a 100644 --- a/dracclient/constants.py +++ b/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] diff --git a/dracclient/resources/bios.py b/dracclient/resources/bios.py index 0a22c44..1090629 100644 --- a/dracclient/resources/bios.py +++ b/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 - value indicating whether a config job must be created for the - values to be applied. + :returns: a dictionary containing: + - The commit_required key with a boolean value indicating + 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( - doc, uris.DCIM_BIOSService)} + return utils.build_return_dict(doc, uris.DCIM_BIOSService) diff --git a/dracclient/resources/raid.py b/dracclient/resources/raid.py index 0469eab..b8fccc4 100644 --- a/dracclient/resources/raid.py +++ b/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 - value indicating whether a config job must be created for the - values to be applied. + :returns: a dictionary containing: + - The commit_required key with a boolean value indicating + 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': - utils.is_reboot_required(doc, uris.DCIM_RAIDService)} + return utils.build_return_dict(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 - value indicating whether a config job must be created for the - values to be applied. + :returns: a dictionary containing: + - The commit_required key with a boolean value indicating + 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( - doc, uris.DCIM_RAIDService)} + return utils.build_return_dict(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 - value indicating whether a config job must be created for the - values to be applied. + :returns: a dictionary containing: + - The commit_required key with a boolean value indicating + 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( - doc, uris.DCIM_RAIDService)} + return utils.build_return_dict(doc, uris.DCIM_RAIDService, + is_commit_required_value=True) diff --git a/dracclient/tests/test_bios.py b/dracclient/tests/test_bios.py index c19c06b..a1972b7 100644 --- a/dracclient/tests/test_bios.py +++ b/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): diff --git a/dracclient/tests/test_raid.py b/dracclient/tests/test_raid.py index 271a4d0..360dc2e 100644 --- a/dracclient/tests/test_raid.py +++ b/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) - mock_invoke.assert_called_once_with( - mock.ANY, uris.DCIM_RAIDService, expected_invocation, - expected_selectors, expected_properties, - 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) + 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, @@ -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, diff --git a/dracclient/tests/test_utils.py b/dracclient/tests/test_utils.py index cad2744..171d551 100644 --- a/dracclient/tests/test_utils.py +++ b/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') diff --git a/dracclient/utils.py b/dracclient/utils.py index c7ddc82..2737743 100644 --- a/dracclient/utils.py +++ b/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"""