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,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.')),
 ]
 
 

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. "
+    'xclarity_manager_ip': _("IP address of the XClarity Controller."),
-                              "Required in driver_info."),
+    'xclarity_username': _("Username for the XClarity Controller "
-}
+                           "with administrator privileges."),
-
-COMMON_PROPERTIES = {
-    'xclarity_address': _("IP address of the XClarity node."),
-    'xclarity_username': _("Username for the XClarity 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,
+            ip=driver_info.get('xclarity_manager_ip'),
-            username=CONF.xclarity.username,
+            username=driver_info.get('xclarity_username'),
-            password=CONF.xclarity.password,
+            password=driver_info.get('xclarity_password'),
-            port=CONF.xclarity.port
+            port=driver_info.get('xclarity_port')
         )
     except xclarity_client_exceptions.XClarityError as exc:
-        msg = (_("Error getting connection to XClarity manager IP: %(ip)s. "
+        msg = (_("Error getting connection to XClarity address: %(ip)s. "
-                 "Error: %(exc)s"), {'ip': CONF.xclarity.manager_ip,
+                 "Error: %(exc)s"),
-                                     'exc': exc})
+               {'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

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,
+            client.set_node_boot_info(server_hardware_id,
-                                                    boot_info,
+                                      boot_info,
-                                                    xclarity_boot_device,
+                                      xclarity_boot_device,
-                                                    singleuse)
+                                      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)

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.parse_driver_info(task.node)
-        common.is_node_managed_by_xclarity(self.xclarity_client, 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(
+            power_state = client.get_node_power_status(server_hardware_id)
-                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,
+            client.set_node_power_status(server_hardware_id, power_state)
-                                                       power_state)
         except xclarity_client_exceptions.XClarityError as xclarity_exc:
             LOG.error(
                 "Error setting power state of node %(node_uuid)s to "

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',
     }
 

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(
+    def test_parse_driver_info_missing_driver_info_and_config(self):
-            self.context, driver='fake-xclarity',
+        del self.node.driver_info['xclarity_manager_ip']
-            properties=db_utils.get_test_xclarity_properties(),
+        del self.node.driver_info['xclarity_username']
-            driver_info=db_utils.get_test_xclarity_driver_info(),
+        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')

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.COMMON_PROPERTIES
-        expected = common.REQUIRED_ON_DRIVER_INFO
+        driver = management.XClarityManagement()
-        self.assertItemsEqual(expected,
+        self.assertEqual(expected, driver.get_properties())
-                              self.node.driver_info)
 
     @mock.patch.object(management.XClarityManagement, 'get_boot_device',
                        return_value='pxe')

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
+        expected = common.COMMON_PROPERTIES
-        self.assertItemsEqual(expected,
+        driver = power.XClarityPower()
-                              self.node.driver_info)
+        self.assertEqual(expected, driver.get_properties())
 
     @mock.patch.object(common, 'get_server_hardware_id',
                        spect_set=True, autospec=True)

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.