From f6dd50d78b114c5b56edb6a6b5d1b767cee762e5 Mon Sep 17 00:00:00 2001 From: jiapei Date: Sat, 14 Apr 2018 13:42:01 +0000 Subject: [PATCH] Fix XClarity parameters discrepancy The XClarity parameters required by driver_info are changed, so that the code is consistent with the document. Besides, we add driver information parse code to verify if it has all the required parameters. And unit tests pass. Add releasenotes for deprecate-xclarity-config. Change-Id: I4412c550ae4d1f506db0c1289920e5bd62f8f724 Story: 2001841 Task: 12607 --- ironic/conf/xclarity.py | 18 ++- ironic/drivers/modules/xclarity/common.py | 106 +++++++++++++----- ironic/drivers/modules/xclarity/management.py | 35 +++--- ironic/drivers/modules/xclarity/power.py | 30 ++--- ironic/tests/unit/db/utils.py | 4 + .../drivers/modules/xclarity/test_common.py | 90 +++++++++++---- .../modules/xclarity/test_management.py | 7 +- .../drivers/modules/xclarity/test_power.py | 6 +- ...cate-xclarity-config-af9b753f96779f42.yaml | 19 ++++ 9 files changed, 222 insertions(+), 93 deletions(-) create mode 100644 releasenotes/notes/deprecate-xclarity-config-af9b753f96779f42.yaml diff --git a/ironic/conf/xclarity.py b/ironic/conf/xclarity.py index a595126ba5..2bca905a7d 100644 --- a/ironic/conf/xclarity.py +++ b/ironic/conf/xclarity.py @@ -18,14 +18,24 @@ from ironic.common.i18n import _ opts = [ cfg.StrOpt('manager_ip', - help=_('IP address of XClarity controller.')), + help=_('IP address of the XClarity Controller. ' + 'Configuration here is deprecated and will be removed ' + 'in the Stein release. Please update the driver_info ' + 'field to use "xclarity_manager_ip" instead')), cfg.StrOpt('username', - help=_('Username to access the XClarity controller.')), + help=_('Username for the XClarity Controller. ' + 'Configuration here is deprecated and will be removed ' + 'in the Stein release. Please update the driver_info ' + 'field to use "xclarity_username" instead')), cfg.StrOpt('password', - help=_('Password for XClarity controller username.')), + help=_('Password for XClarity Controller username. ' + 'Configuration here is deprecated and will be removed ' + 'in the Stein release. Please update the driver_info ' + 'field to use "xclarity_password" instead')), cfg.PortOpt('port', default=443, - help=_('Port to be used for XClarity operations.')), + help=_('Port to be used for XClarity Controller ' + 'connection.')), ] diff --git a/ironic/drivers/modules/xclarity/common.py b/ironic/drivers/modules/xclarity/common.py index 1ffdfe248c..d216195148 100644 --- a/ironic/drivers/modules/xclarity/common.py +++ b/ironic/drivers/modules/xclarity/common.py @@ -18,6 +18,7 @@ from oslo_utils import importutils from ironic.common import exception from ironic.common.i18n import _ from ironic.common import states +from ironic.common import utils from ironic.conf import CONF LOG = logging.getLogger(__name__) @@ -28,45 +29,102 @@ xclarity_client_exceptions = importutils.try_import( 'xclarity_client.exceptions') REQUIRED_ON_DRIVER_INFO = { - 'xclarity_hardware_id': _("XClarity Server Hardware ID. " - "Required in driver_info."), -} - -COMMON_PROPERTIES = { - 'xclarity_address': _("IP address of the XClarity node."), - 'xclarity_username': _("Username for the XClarity with administrator " - "privileges."), + 'xclarity_manager_ip': _("IP address of the XClarity Controller."), + 'xclarity_username': _("Username for the XClarity Controller " + "with administrator privileges."), 'xclarity_password': _("Password for xclarity_username."), - 'xclarity_port': _("Port to be used for xclarity_username."), + 'xclarity_hardware_id': _("Server Hardware ID managed by XClarity."), } +OPTIONAL_ON_DRIVER_INFO = { + 'xclarity_port': _("Port to be used for XClarity Controller connection. " + "Optional"), +} + +COMMON_PROPERTIES = {} COMMON_PROPERTIES.update(REQUIRED_ON_DRIVER_INFO) +COMMON_PROPERTIES.update(OPTIONAL_ON_DRIVER_INFO) def get_properties(): return COMMON_PROPERTIES -def get_xclarity_client(): +def parse_driver_info(node): + """Parse a node's driver_info values. + + Parses the driver_info of the node, reads default values + and returns a dict containing the combination of both. + + :param node: an ironic node object to get informatin from. + :returns: a dict containing information parsed from driver_info. + :raises: InvalidParameterValue if some required information + is missing on the node or inputs is invalid. + """ + driver_info = node.driver_info + parsed_driver_info = {} + + error_msgs = [] + for param in REQUIRED_ON_DRIVER_INFO: + if param == "xclarity_hardware_id": + try: + parsed_driver_info[param] = str(driver_info[param]) + except KeyError: + error_msgs.append(_("'%s' not provided to XClarity.") % param) + except UnicodeEncodeError: + error_msgs.append(_("'%s' contains non-ASCII symbol.") % param) + else: + # corresponding config names don't have 'xclarity_' prefix + if param in driver_info: + parsed_driver_info[param] = str(driver_info[param]) + elif param not in driver_info and\ + CONF.xclarity.get(param[len('xclarity_'):]) is not None: + parsed_driver_info[param] = str( + CONF.xclarity.get(param[len('xclarity_'):])) + LOG.warning('The configuration [xclarity]/%(config)s ' + 'is deprecated and will be removed in the ' + 'Stein release. Please update the node ' + '%(node_uuid)s driver_info field to use ' + '"%(field)s" instead', + {'config': param[len('xclarity_'):], + 'node_uuid': node.uuid, 'field': param}) + else: + error_msgs.append(_("'%s' not provided to XClarity.") % param) + + port = driver_info.get('xclarity_port', CONF.xclarity.get('port')) + parsed_driver_info['xclarity_port'] = utils.validate_network_port( + port, 'xclarity_port') + + if error_msgs: + msg = (_('The following errors were encountered while parsing ' + 'driver_info:\n%s') % '\n'.join(error_msgs)) + raise exception.InvalidParameterValue(msg) + + return parsed_driver_info + + +def get_xclarity_client(node): """Generates an instance of the XClarity client. Generates an instance of the XClarity client using the imported xclarity_client library. + :param node: an ironic node object. :returns: an instance of the XClarity client :raises: XClarityError if can't get to the XClarity client """ + driver_info = parse_driver_info(node) try: xclarity_client = client.Client( - ip=CONF.xclarity.manager_ip, - username=CONF.xclarity.username, - password=CONF.xclarity.password, - port=CONF.xclarity.port + ip=driver_info.get('xclarity_manager_ip'), + username=driver_info.get('xclarity_username'), + password=driver_info.get('xclarity_password'), + port=driver_info.get('xclarity_port') ) except xclarity_client_exceptions.XClarityError as exc: - msg = (_("Error getting connection to XClarity manager IP: %(ip)s. " - "Error: %(exc)s"), {'ip': CONF.xclarity.manager_ip, - 'exc': exc}) + msg = (_("Error getting connection to XClarity address: %(ip)s. " + "Error: %(exc)s"), + {'ip': driver_info.get('xclarity_manager_ip'), 'exc': exc}) raise exception.XClarityError(error=msg) return xclarity_client @@ -118,17 +176,3 @@ def translate_xclarity_power_action(power_action): } return power_action_map[power_action] - - -def is_node_managed_by_xclarity(xclarity_client, node): - """Determines whether dynamic allocation is enabled for a specifc node. - - :param: xclarity_client: an instance of the XClarity client - :param: node: node object to get information from - :returns: Boolean depending on whether node is managed by XClarity - """ - try: - hardware_id = get_server_hardware_id(node) - return xclarity_client.is_node_managed(hardware_id) - except exception.MissingParameterValue: - return False diff --git a/ironic/drivers/modules/xclarity/management.py b/ironic/drivers/modules/xclarity/management.py index 2ae03f0be5..7a3b704259 100644 --- a/ironic/drivers/modules/xclarity/management.py +++ b/ironic/drivers/modules/xclarity/management.py @@ -47,20 +47,21 @@ SUPPORTED_BOOT_DEVICES = [ class XClarityManagement(base.ManagementInterface): - def __init__(self): - super(XClarityManagement, self).__init__() - self.xclarity_client = common.get_xclarity_client() def get_properties(self): return common.COMMON_PROPERTIES @METRICS.timer('XClarityManagement.validate') def validate(self, task): - """It validates if the node is being used by XClarity. + """Validate the driver-specific info supplied. + + This method validates if the 'driver_info' property of the supplied + task's node contains the required information for this driver to + manage the node. :param task: a task from TaskManager. """ - common.is_node_managed_by_xclarity(self.xclarity_client, task.node) + common.parse_driver_info(task.node) @METRICS.timer('XClarityManagement.get_supported_boot_devices') def get_supported_boot_devices(self, task): @@ -97,16 +98,18 @@ class XClarityManagement(base.ManagementInterface): :raises: InvalidParameterValue if the boot device is unknown :raises: XClarityError if the communication with XClarity fails """ - server_hardware_id = common.get_server_hardware_id(task.node) + node = task.node + client = common.get_xclarity_client(node) + server_hardware_id = common.get_server_hardware_id(node) try: boot_info = ( - self.xclarity_client.get_node_all_boot_info( + client.get_node_all_boot_info( server_hardware_id) ) except xclarity_client_exceptions.XClarityError as xclarity_exc: LOG.error( "Error getting boot device from XClarity for node %(node)s. " - "Error: %(error)s", {'node': task.node.uuid, + "Error: %(error)s", {'node': node.uuid, 'error': xclarity_exc}) raise exception.XClarityError(error=xclarity_exc) @@ -182,7 +185,9 @@ class XClarityManagement(base.ManagementInterface): :param task: a TaskManager instance. :param singleuse: if this device will be used only once at next boot """ - boot_info = self.xclarity_client.get_node_all_boot_info( + node = task.node + client = common.get_xclarity_client(node) + boot_info = client.get_node_all_boot_info( server_hardware_id) xclarity_boot_device = self._translate_ironic_to_xclarity( new_primary_boot_device) @@ -190,7 +195,7 @@ class XClarityManagement(base.ManagementInterface): LOG.debug( ("Setting boot device to %(device)s for XClarity " "node %(node)s"), - {'device': xclarity_boot_device, 'node': task.node.uuid} + {'device': xclarity_boot_device, 'node': node.uuid} ) for item in boot_info['bootOrder']['bootOrderList']: if singleuse and item['bootType'] == 'SingleUse': @@ -205,15 +210,15 @@ class XClarityManagement(base.ManagementInterface): item['currentBootOrderDevices'] = current try: - self.xclarity_client.set_node_boot_info(server_hardware_id, - boot_info, - xclarity_boot_device, - singleuse) + client.set_node_boot_info(server_hardware_id, + boot_info, + xclarity_boot_device, + singleuse) except xclarity_client_exceptions.XClarityError as xclarity_exc: LOG.error( ('Error setting boot device %(boot_device)s for the XClarity ' 'node %(node)s. Error: %(error)s'), - {'boot_device': xclarity_boot_device, 'node': task.node.uuid, + {'boot_device': xclarity_boot_device, 'node': node.uuid, 'error': xclarity_exc} ) raise exception.XClarityError(error=xclarity_exc) diff --git a/ironic/drivers/modules/xclarity/power.py b/ironic/drivers/modules/xclarity/power.py index eaaea67461..9eb6b110fc 100644 --- a/ironic/drivers/modules/xclarity/power.py +++ b/ironic/drivers/modules/xclarity/power.py @@ -31,21 +31,21 @@ xclarity_client_exceptions = importutils.try_import( class XClarityPower(base.PowerInterface): - def __init__(self): - super(XClarityPower, self).__init__() - self.xclarity_client = common.get_xclarity_client() def get_properties(self): return common.get_properties() @METRICS.timer('XClarityPower.validate') def validate(self, task): - """It validates if the node is being used by XClarity. + """Validate the driver-specific info supplied. + + This method validates if the 'driver_info' property of the supplied + task's node contains the required information for this driver to + manage the power state of the node. :param task: a task from TaskManager. """ - - common.is_node_managed_by_xclarity(self.xclarity_client, task.node) + common.parse_driver_info(task.node) @METRICS.timer('XClarityPower.get_power_state') def get_power_state(self, task): @@ -57,15 +57,16 @@ class XClarityPower(base.PowerInterface): :raises: XClarityError if fails to retrieve power state of XClarity resource """ - server_hardware_id = common.get_server_hardware_id(task.node) + node = task.node + client = common.get_xclarity_client(node) + server_hardware_id = common.get_server_hardware_id(node) try: - power_state = self.xclarity_client.get_node_power_status( - server_hardware_id) + power_state = client.get_node_power_status(server_hardware_id) except xclarity_client_exceptions.XClarityException as xclarity_exc: LOG.error( ("Error getting power state for node %(node)s. Error: " "%(error)s"), - {'node': task.node.uuid, 'error': xclarity_exc} + {'node': node.uuid, 'error': xclarity_exc} ) raise exception.XClarityError(error=xclarity_exc) return common.translate_xclarity_power_state(power_state) @@ -95,14 +96,15 @@ class XClarityPower(base.PowerInterface): if target_power_state == states.POWER_OFF: power_state = states.POWER_ON - server_hardware_id = common.get_server_hardware_id(task.node) + node = task.node + client = common.get_xclarity_client(node) + server_hardware_id = common.get_server_hardware_id(node) LOG.debug("Setting power state of node %(node_uuid)s to " "%(power_state)s", - {'node_uuid': task.node.uuid, 'power_state': power_state}) + {'node_uuid': node.uuid, 'power_state': power_state}) try: - self.xclarity_client.set_node_power_status(server_hardware_id, - power_state) + client.set_node_power_status(server_hardware_id, power_state) except xclarity_client_exceptions.XClarityError as xclarity_exc: LOG.error( "Error setting power state of node %(node_uuid)s to " diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index 5aea3fccd8..ca0eaab60b 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -506,6 +506,10 @@ def get_test_xclarity_properties(): def get_test_xclarity_driver_info(): return { + 'xclarity_manager_ip': "1.2.3.4", + 'xclarity_username': "USERID", + 'xclarity_password': "fake", + 'xclarity_port': 443, 'xclarity_hardware_id': 'fake_sh_id', } diff --git a/ironic/tests/unit/drivers/modules/xclarity/test_common.py b/ironic/tests/unit/drivers/modules/xclarity/test_common.py index 0b2c1bf682..a7253bf8dc 100644 --- a/ironic/tests/unit/drivers/modules/xclarity/test_common.py +++ b/ironic/tests/unit/drivers/modules/xclarity/test_common.py @@ -16,29 +16,91 @@ import mock from oslo_utils import importutils +from ironic.common import exception from ironic.drivers.modules.xclarity import common from ironic.tests.unit.db import base as db_base from ironic.tests.unit.db import utils as db_utils from ironic.tests.unit.objects import utils as obj_utils +xclarity_client = importutils.try_import('xclarity_client.client') xclarity_exceptions = importutils.try_import('xclarity_client.exceptions') xclarity_constants = importutils.try_import('xclarity_client.constants') +INFO_DICT = db_utils.get_test_xclarity_driver_info() + class XClarityCommonTestCase(db_base.DbTestCase): def setUp(self): super(XClarityCommonTestCase, self).setUp() + self.config(enabled_hardware_types=['xclarity'], + enabled_power_interfaces=['xclarity'], + enabled_management_interfaces=['xclarity']) + self.node = obj_utils.create_test_node( + self.context, driver='xclarity', + properties=db_utils.get_test_xclarity_properties(), + driver_info=INFO_DICT) - self.config(manager_ip='1.2.3.4', group='xclarity') + def test_parse_driver_info(self): + info = common.parse_driver_info(self.node) + self.assertEqual(INFO_DICT['xclarity_manager_ip'], + info['xclarity_manager_ip']) + self.assertEqual(INFO_DICT['xclarity_username'], + info['xclarity_username']) + self.assertEqual(INFO_DICT['xclarity_password'], + info['xclarity_password']) + self.assertEqual(INFO_DICT['xclarity_port'], info['xclarity_port']) + self.assertEqual(INFO_DICT['xclarity_hardware_id'], + info['xclarity_hardware_id']) + + def test_parse_driver_info_missing_hardware_id(self): + del self.node.driver_info['xclarity_hardware_id'] + self.assertRaises(exception.InvalidParameterValue, + common.parse_driver_info, self.node) + + def test_parse_driver_info_get_param_from_config(self): + del self.node.driver_info['xclarity_manager_ip'] + del self.node.driver_info['xclarity_username'] + del self.node.driver_info['xclarity_password'] + self.config(manager_ip='5.6.7.8', group='xclarity') self.config(username='user', group='xclarity') self.config(password='password', group='xclarity') + info = common.parse_driver_info(self.node) + self.assertEqual('5.6.7.8', info['xclarity_manager_ip']) + self.assertEqual('user', info['xclarity_username']) + self.assertEqual('password', info['xclarity_password']) - self.node = obj_utils.create_test_node( - self.context, driver='fake-xclarity', - properties=db_utils.get_test_xclarity_properties(), - driver_info=db_utils.get_test_xclarity_driver_info(), - ) + def test_parse_driver_info_missing_driver_info_and_config(self): + del self.node.driver_info['xclarity_manager_ip'] + del self.node.driver_info['xclarity_username'] + del self.node.driver_info['xclarity_password'] + e = self.assertRaises(exception.InvalidParameterValue, + common.parse_driver_info, self.node) + self.assertIn('xclarity_manager_ip', str(e)) + self.assertIn('xclarity_username', str(e)) + self.assertIn('xclarity_password', str(e)) + + def test_parse_driver_info_invalid_port(self): + self.node.driver_info['xclarity_port'] = 'asd' + self.assertRaises(exception.InvalidParameterValue, + common.parse_driver_info, self.node) + self.node.driver_info['xclarity_port'] = '65536' + self.assertRaises(exception.InvalidParameterValue, + common.parse_driver_info, self.node) + self.node.driver_info['xclarity_port'] = 'invalid' + self.assertRaises(exception.InvalidParameterValue, + common.parse_driver_info, self.node) + self.node.driver_info['xclarity_port'] = '-1' + self.assertRaises(exception.InvalidParameterValue, + common.parse_driver_info, self.node) + + @mock.patch.object(xclarity_client, 'Client', autospec=True) + def test_get_xclarity_client(self, mock_xclarityclient): + expected_call = mock.call(ip='1.2.3.4', password='fake', port=443, + username='USERID') + common.get_xclarity_client(self.node) + + self.assertEqual(mock_xclarityclient.mock_calls, [expected_call]) def test_get_server_hardware_id(self): driver_info = self.node.driver_info @@ -46,19 +108,3 @@ class XClarityCommonTestCase(db_base.DbTestCase): self.node.driver_info = driver_info result = common.get_server_hardware_id(self.node) self.assertEqual(result, 'test') - - @mock.patch.object(common, 'get_server_hardware_id', - spec_set=True, autospec=True) - @mock.patch.object(common, 'get_xclarity_client', - spec_set=True, autospec=True) - def test_check_node_managed_by_xclarity(self, mock_xc_client, - mock_validate_driver_info): - driver_info = self.node.driver_info - driver_info['xclarity_hardware_id'] = 'abcd' - self.node.driver_info = driver_info - - xclarity_client = mock_xc_client() - mock_validate_driver_info.return_value = '12345' - common.is_node_managed_by_xclarity(xclarity_client, - self.node) - xclarity_client.is_node_managed.assert_called_once_with('12345') diff --git a/ironic/tests/unit/drivers/modules/xclarity/test_management.py b/ironic/tests/unit/drivers/modules/xclarity/test_management.py index 8563ebad2f..36210fe382 100644 --- a/ironic/tests/unit/drivers/modules/xclarity/test_management.py +++ b/ironic/tests/unit/drivers/modules/xclarity/test_management.py @@ -59,10 +59,9 @@ class XClarityManagementDriverTestCase(db_base.DbTestCase): mock_validate.assert_called_with(task.node) def test_get_properties(self, mock_get_xc_client): - - expected = common.REQUIRED_ON_DRIVER_INFO - self.assertItemsEqual(expected, - self.node.driver_info) + expected = common.COMMON_PROPERTIES + driver = management.XClarityManagement() + self.assertEqual(expected, driver.get_properties()) @mock.patch.object(management.XClarityManagement, 'get_boot_device', return_value='pxe') diff --git a/ironic/tests/unit/drivers/modules/xclarity/test_power.py b/ironic/tests/unit/drivers/modules/xclarity/test_power.py index c459e34274..dcca3353d9 100644 --- a/ironic/tests/unit/drivers/modules/xclarity/test_power.py +++ b/ironic/tests/unit/drivers/modules/xclarity/test_power.py @@ -56,9 +56,9 @@ class XClarityPowerDriverTestCase(db_base.DbTestCase): driver_info=db_utils.get_test_xclarity_driver_info()) def test_get_properties(self, mock_get_xc_client): - expected = common.REQUIRED_ON_DRIVER_INFO - self.assertItemsEqual(expected, - self.node.driver_info) + expected = common.COMMON_PROPERTIES + driver = power.XClarityPower() + self.assertEqual(expected, driver.get_properties()) @mock.patch.object(common, 'get_server_hardware_id', spect_set=True, autospec=True) diff --git a/releasenotes/notes/deprecate-xclarity-config-af9b753f96779f42.yaml b/releasenotes/notes/deprecate-xclarity-config-af9b753f96779f42.yaml new file mode 100644 index 0000000000..d9d21e9cbb --- /dev/null +++ b/releasenotes/notes/deprecate-xclarity-config-af9b753f96779f42.yaml @@ -0,0 +1,19 @@ +--- +features: + - | + Adds new parameter fields to driver_info, which will become + mandatory in Stein release: + + * ``xclarity_manager_ip``: IP address of the XClarity Controller. + * ``xclarity_username``: Username for the XClarity Controller. + * ``xclarity_password``: Password for XClarity Controller username. + * ``xclarity_port``: Port to be used for XClarity Controller connection. +deprecations: + - | + Configuration options ``[xclarity]/manager_ip``, ``[xclarity]/username``, + and ``[xclarity]/password`` are deprecated and will be removed in + the Stein release. +fixes: + - | + Fixes an issue where parameters required in driver_info and + descriptions in documentation are different.