Import fixes from the Nova driver reviews

Import fixes from the Nova driver review, based upon revision 7
of https://review.openstack.org/#/c/103167/

- better use of _L* helpers
- dynamically import python-ironicclient when the driver is initialized,
  so that Nova does not need to add it to requirements.txt
- remove unused CONF options pxe_bootfile_name
  and instance_type_extra_specs

Co-authored-by: Devananda van der Veen <devananda.vdv@gmail.com>

Change-Id: Iee1481632e9927766776c7d35264e8cf4c469c8a

4 files changed, 92 insertions, 78 deletions

diff --git a/ironic/nova/tests/scheduler/test_ironic_host_manager.py b/ironic/nova/tests/scheduler/test_ironic_host_manager.py
index 52b643333..d7bdb0955 100644
--- a/ironic/nova/tests/scheduler/test_ironic_host_manager.py
+++ b/ironic/nova/tests/scheduler/test_ironic_host_manager.py
@@ -84,9 +84,9 @@ class IronicHostManagerChangedNodesTestCase(test.NoDBTestCase):
         self.compute_node = dict(id=1, local_gb=10, memory_mb=1024, vcpus=1,
                             vcpus_used=0, local_gb_used=0, memory_mb_used=0,
                             updated_at=None, cpu_info='baremetal cpu',
-                                stats=jsonutils.dumps(dict(
-                                    ironic_driver=ironic_driver,
-                                    cpu_arch='i386')),
+                            stats=jsonutils.dumps(dict(
+                                        ironic_driver=ironic_driver,
+                                        cpu_arch='i386')),
                             supported_instances=supported_instances,
                             free_disk_gb=10, free_ram_mb=1024)
 
diff --git a/ironic/nova/tests/virt/ironic/test_driver.py b/ironic/nova/tests/virt/ironic/test_driver.py
index 2bdbaed41..503231ab4 100644
--- a/ironic/nova/tests/virt/ironic/test_driver.py
+++ b/ironic/nova/tests/virt/ironic/test_driver.py
@@ -46,7 +46,6 @@ from nova.virt import firewall
 CONF = cfg.CONF
 
 IRONIC_FLAGS = dict(
-    instance_type_extra_specs=['test_spec:test_value'],
     api_version=1,
     group='ironic',
 )
@@ -76,8 +75,7 @@ def _get_properties():
 def _get_stats():
     return {'cpu_arch': 'x86_64',
             'ironic_driver':
-                    'ironic.nova.virt.ironic.driver.IronicDriver',
-            'test_spec': 'test_value'}
+                    'ironic.nova.virt.ironic.driver.IronicDriver'}
 
 
 FAKE_CLIENT_WRAPPER = FakeClientWrapper()
@@ -107,7 +105,7 @@ class IronicDriverTestCase(test.NoDBTestCase):
         self.assertEqual(self.driver.get_hypervisor_version(), 1)
 
     @mock.patch.object(FAKE_CLIENT.node, 'get_by_instance_uuid')
-    def test_validate_instance_and_node(self, mock_gbiui):
+    def test__validate_instance_and_node(self, mock_gbiui):
         node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'
         instance_uuid = uuidutils.generate_uuid()
         node = ironic_utils.get_test_node(uuid=node_uuid,
@@ -117,18 +115,18 @@ class IronicDriverTestCase(test.NoDBTestCase):
         icli = cw.IronicClientWrapper()
 
         mock_gbiui.return_value = node
-        result = ironic_driver.validate_instance_and_node(icli, instance)
+        result = ironic_driver._validate_instance_and_node(icli, instance)
         self.assertEqual(result.uuid, node_uuid)
 
     @mock.patch.object(FAKE_CLIENT.node, 'get_by_instance_uuid')
-    def test_validate_instance_and_node_failed(self, mock_gbiui):
+    def test__validate_instance_and_node_failed(self, mock_gbiui):
         icli = cw.IronicClientWrapper()
         mock_gbiui.side_effect = ironic_exception.NotFound()
         instance_uuid = uuidutils.generate_uuid(),
         instance = fake_instance.fake_instance_obj(self.ctx,
                                                    uuid=instance_uuid)
         self.assertRaises(exception.InstanceNotFound,
-                          ironic_driver.validate_instance_and_node,
+                          ironic_driver._validate_instance_and_node,
                           icli, instance)
 
     def test__node_resource(self):
@@ -736,7 +734,7 @@ class IronicDriverTestCase(test.NoDBTestCase):
         mock_cleanup_deploy.assert_called_with(node, instance, network_info)
 
     @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state')
-    @mock.patch.object(ironic_driver, 'validate_instance_and_node')
+    @mock.patch.object(ironic_driver, '_validate_instance_and_node')
     def test_destroy_trigger_undeploy_fail(self, fake_validate, mock_sps):
         node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'
         node = ironic_utils.get_test_node(driver='fake', uuid=node_uuid,
@@ -780,7 +778,7 @@ class IronicDriverTestCase(test.NoDBTestCase):
         mock_node.get_by_instance_uuid.assert_called_with(instance.uuid)
 
     @mock.patch.object(FAKE_CLIENT.node, 'set_power_state')
-    @mock.patch.object(ironic_driver, 'validate_instance_and_node')
+    @mock.patch.object(ironic_driver, '_validate_instance_and_node')
     def test_reboot(self, mock_val_inst, mock_set_power):
         node = ironic_utils.get_test_node()
         mock_val_inst.return_value = node
@@ -789,7 +787,7 @@ class IronicDriverTestCase(test.NoDBTestCase):
         self.driver.reboot(self.ctx, instance, None, None)
         mock_set_power.assert_called_once_with(node.uuid, 'reboot')
 
-    @mock.patch.object(ironic_driver, 'validate_instance_and_node')
+    @mock.patch.object(ironic_driver, '_validate_instance_and_node')
     @mock.patch.object(FAKE_CLIENT.node, 'set_power_state')
     def test_power_off(self, mock_sp, fake_validate):
         node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'
@@ -803,7 +801,7 @@ class IronicDriverTestCase(test.NoDBTestCase):
         self.driver.power_off(instance)
         mock_sp.assert_called_once_with(node_uuid, 'off')
 
-    @mock.patch.object(ironic_driver, 'validate_instance_and_node')
+    @mock.patch.object(ironic_driver, '_validate_instance_and_node')
     @mock.patch.object(FAKE_CLIENT.node, 'set_power_state')
     def test_power_on(self, mock_sp, fake_validate):
         node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'
@@ -1013,7 +1011,6 @@ class IronicDriverTestCase(test.NoDBTestCase):
                                                    node=node_uuid,
                                                    instance_type_id=flavor_id)
 
-
         fake_looping_call = FakeLoopingCall()
         mock_looping.return_value = fake_looping_call
 
diff --git a/ironic/nova/virt/ironic/client_wrapper.py b/ironic/nova/virt/ironic/client_wrapper.py
index cfda09e40..7c5391867 100644
--- a/ironic/nova/virt/ironic/client_wrapper.py
+++ b/ironic/nova/virt/ironic/client_wrapper.py
@@ -17,14 +17,15 @@
 
 import time
 
-from ironicclient import client as ironic_client
-from ironicclient import exc as ironic_exception
-
 from nova import exception
 from nova.openstack.common.gettextutils import _
+from nova.openstack.common import importutils
 from nova.openstack.common import log as logging
 from oslo.config import cfg
 
+
+ironic = None
+
 LOG = logging.getLogger(__name__)
 CONF = cfg.CONF
 
@@ -32,6 +33,23 @@ CONF = cfg.CONF
 class IronicClientWrapper(object):
     """Ironic client wrapper class that encapsulates retry logic."""
 
+    def __init__(self):
+        """Initialise the IronicClientWrapper for use.
+
+        Initialise IronicClientWrapper by loading ironicclient
+        dynamically so that ironicclient is not a dependency for
+        Nova.
+        """
+        global ironic
+        if ironic is None:
+            ironic = importutils.import_module('ironicclient')
+            # NOTE(deva): work around a lack of symbols in the current version.
+            if not hasattr(ironic, 'exc'):
+                ironic.exc = importutils.import_module('ironicclient.exc')
+            if not hasattr(ironic, 'client'):
+                ironic.client = importutils.import_module(
+                                                    'ironicclient.client')
+
     def _get_client(self):
         # TODO(deva): save and reuse existing client & auth token
         #             until it expires or is no longer valid
@@ -48,9 +66,9 @@ class IronicClientWrapper(object):
                       'ironic_url': CONF.ironic.api_endpoint}
 
         try:
-            cli = ironic_client.get_client(CONF.ironic.api_version, **kwargs)
-        except ironic_exception.Unauthorized:
-            msg = (_("Unable to authenticate Ironic client."))
+            cli = ironic.client.get_client(CONF.ironic.api_version, **kwargs)
+        except ironic.exc.Unauthorized:
+            msg = _("Unable to authenticate Ironic client.")
             LOG.error(msg)
             raise exception.NovaException(msg)
 
@@ -78,9 +96,9 @@ class IronicClientWrapper(object):
 
         :raises: NovaException if all retries failed.
         """
-        retry_excs = (ironic_exception.ServiceUnavailable,
-                      ironic_exception.ConnectionRefused,
-                      ironic_exception.Conflict)
+        retry_excs = (ironic.exc.ServiceUnavailable,
+                      ironic.exc.ConnectionRefused,
+                      ironic.exc.Conflict)
         num_attempts = CONF.ironic.api_max_retries
 
         for attempt in range(1, num_attempts + 1):
diff --git a/ironic/nova/virt/ironic/driver.py b/ironic/nova/virt/ironic/driver.py
index 8f71f0a81..043bcaeaf 100644
--- a/ironic/nova/virt/ironic/driver.py
+++ b/ironic/nova/virt/ironic/driver.py
@@ -22,7 +22,6 @@ bare metal resources.
 """
 import logging as py_logging
 
-from ironicclient import exc as ironic_exception
 from oslo.config import cfg
 
 from ironic.nova.virt.ironic import client_wrapper
@@ -35,13 +34,17 @@ from nova import exception
 from nova.objects import flavor as flavor_obj
 from nova.objects import instance as instance_obj
 from nova.openstack.common import excutils
-from nova.openstack.common.gettextutils import _, _LW
+from nova.openstack.common.gettextutils import _, _LE, _LW
+from nova.openstack.common import importutils
 from nova.openstack.common import jsonutils
 from nova.openstack.common import log as logging
 from nova.openstack.common import loopingcall
 from nova.virt import driver as virt_driver
 from nova.virt import firewall
 
+
+ironic = None
+
 LOG = logging.getLogger(__name__)
 
 opts = [
@@ -62,18 +65,8 @@ opts = [
                help='Log level override for ironicclient. Set this in '
                     'order to override the global "default_log_levels", '
                     '"verbose", and "debug" settings.'),
-    cfg.StrOpt('pxe_bootfile_name',
-               help='This gets passed to Neutron as the bootfile dhcp '
-               'parameter when the dhcp_options_enabled is set.',
-               default='pxelinux.0'),
     cfg.StrOpt('admin_tenant_name',
                help='Ironic keystone tenant name.'),
-    cfg.ListOpt('instance_type_extra_specs',
-                default=[],
-                help='A list of additional capabilities corresponding to '
-                'instance_type_extra_specs for this compute '
-                'host to advertise. Valid entries are name=value, pairs '
-                'For example, "key1:val1, key2:val2"'),
     cfg.IntOpt('api_max_retries',
                default=60,
                help=('How many retries when a request does conflict.')),
@@ -104,15 +97,15 @@ def map_power_state(state):
     try:
         return _POWER_STATE_MAP[state]
     except KeyError:
-        LOG.warning(_("Power state %s not found.") % state)
+        LOG.warning(_LW("Power state %s not found."), state)
         return power_state.NOSTATE
 
 
-def validate_instance_and_node(icli, instance):
+def _validate_instance_and_node(icli, instance):
     """Get and validate a node's uuid out of a manager instance dict.
 
-    The compute manager is meant to know the node uuid, so missing uuid
-    a significant issue - it may mean we've been passed someone elses data.
+    The compute manager is meant to know the node uuid, so missing uuid is
+    a significant issue - it may mean we've been passed someone else's data.
 
     Check with the Ironic service that this node is still associated with
     this instance. This catches situations where Nova's instance dict
@@ -121,7 +114,7 @@ def validate_instance_and_node(icli, instance):
     """
     try:
         return icli.call("node.get_by_instance_uuid", instance['uuid'])
-    except ironic_exception.NotFound:
+    except ironic.exc.NotFound:
         raise exception.InstanceNotFound(instance_id=instance['uuid'])
 
 
@@ -152,17 +145,20 @@ class IronicDriver(virt_driver.ComputeDriver):
 
     def __init__(self, virtapi, read_only=False):
         super(IronicDriver, self).__init__(virtapi)
+        global ironic
+        if ironic is None:
+            ironic = importutils.import_module('ironicclient')
+            # NOTE(deva): work around a lack of symbols in the current version.
+            if not hasattr(ironic, 'exc'):
+                ironic.exc = importutils.import_module('ironicclient.exc')
+            if not hasattr(ironic, 'client'):
+                ironic.client = importutils.import_module(
+                                                    'ironicclient.client')
 
         self.firewall_driver = firewall.load_driver(default=_FIREWALL_DRIVER)
         extra_specs = {}
         extra_specs["ironic_driver"] = \
             "ironic.nova.virt.ironic.driver.IronicDriver"
-        for pair in CONF.ironic.instance_type_extra_specs:
-            keyval = pair.split(':', 1)
-            keyval[0] = keyval[0].strip()
-            keyval[1] = keyval[1].strip()
-            extra_specs[keyval[0]] = keyval[1]
-
         self.extra_specs = extra_specs
 
         icli_log_level = CONF.ironic.client_log_level
@@ -172,7 +168,9 @@ class IronicDriver(virt_driver.ComputeDriver):
             logger.setLevel(level)
 
     def _node_resources_unavailable(self, node_obj):
-        """Determines whether the node's resources should be presented
+        """Determine whether the node's resources are in an acceptable state.
+
+        Determines whether the node's resources should be presented
         to Nova for use based on the current power and maintenance state.
         """
         bad_states = [ironic_states.ERROR, ironic_states.NOSTATE]
@@ -275,7 +273,7 @@ class IronicDriver(virt_driver.ComputeDriver):
                       'value': instance['uuid']})
         try:
             icli.call('node.update', node.uuid, patch)
-        except ironic_exception.BadRequest:
+        except ironic.exc.BadRequest:
             msg = (_("Failed to add deploy parameters on node %(node)s "
                      "when provisioning the instance %(instance)s")
                    % {'node': node.uuid, 'instance': instance['uuid']})
@@ -294,12 +292,11 @@ class IronicDriver(virt_driver.ComputeDriver):
         patch.append({'op': 'remove', 'path': '/instance_uuid'})
         try:
             icli.call('node.update', node.uuid, patch)
-        except ironic_exception.BadRequest:
-            msg = (_("Failed clean up the parameters on node %(node)s "
-                     "when unprovisioning the instance %(instance)s")
-                   % {'node': node.uuid, 'instance': instance['uuid']})
-            LOG.error(msg)
-            reason = _("Fail to clean up node %s parameters") % node.uuid
+        except ironic.exc.BadRequest:
+            LOG.error(_LE("Failed to clean up the parameters on node %(node)s "
+                          "when unprovisioning the instance %(instance)s"),
+                         {'node': node.uuid, 'instance': instance['uuid']})
+            reason = (_("Fail to clean up node %s parameters") % node.uuid)
             raise exception.InstanceTerminationFailure(reason=reason)
 
         self._unplug_vifs(node, instance, network_info)
@@ -307,7 +304,7 @@ class IronicDriver(virt_driver.ComputeDriver):
 
     def _wait_for_active(self, icli, instance):
         """Wait for the node to be marked as ACTIVE in Ironic."""
-        node = validate_instance_and_node(icli, instance)
+        node = _validate_instance_and_node(icli, instance)
         if node.provision_state == ironic_states.ACTIVE:
             # job is done
             LOG.debug("Ironic node %(node)s is now ACTIVE",
@@ -358,7 +355,7 @@ class IronicDriver(virt_driver.ComputeDriver):
         """
         icli = client_wrapper.IronicClientWrapper()
         try:
-            validate_instance_and_node(icli, instance)
+            _validate_instance_and_node(icli, instance)
             return True
         except exception.InstanceNotFound:
             return False
@@ -397,7 +394,7 @@ class IronicDriver(virt_driver.ComputeDriver):
         try:
             icli.call("node.get", nodename)
             return True
-        except ironic_exception.NotFound:
+        except ironic.exc.NotFound:
             return False
 
     def get_available_nodes(self, refresh=False):
@@ -447,7 +444,7 @@ class IronicDriver(virt_driver.ComputeDriver):
         """
         icli = client_wrapper.IronicClientWrapper()
         try:
-            node = validate_instance_and_node(icli, instance)
+            node = _validate_instance_and_node(icli, instance)
         except exception.InstanceNotFound:
             return {'state': map_power_state(ironic_states.NOSTATE),
                     'max_mem': 0,
@@ -477,7 +474,7 @@ class IronicDriver(virt_driver.ComputeDriver):
         icli = client_wrapper.IronicClientWrapper()
         try:
             node = icli.call("node.get", instance['node'])
-        except ironic_exception.NotFound:
+        except ironic.exc.NotFound:
             return []
         ports = icli.call("node.list_ports", node.uuid)
         return [p.address for p in ports]
@@ -539,8 +536,8 @@ class IronicDriver(virt_driver.ComputeDriver):
             self._start_firewall(instance, network_info)
         except Exception:
             with excutils.save_and_reraise_exception():
-                LOG.error(_("Error preparing deploy for instance %(instance)s "
-                            "on baremetal node %(node)s.") %
+                LOG.error(_LE("Error preparing deploy for instance "
+                              "%(instance)s on baremetal node %(node)s."),
                           {'instance': instance['uuid'],
                            'node': node_uuid})
                 self._cleanup_deploy(node, instance, network_info)
@@ -564,10 +561,10 @@ class IronicDriver(virt_driver.ComputeDriver):
             timer.start(interval=CONF.ironic.api_retry_interval).wait()
         except Exception:
             with excutils.save_and_reraise_exception():
-                LOG.error(_("Error deploying instance %(instance)s on "
-                            "baremetal node %(node)s.") %
-                          {'instance': instance['uuid'],
-                           'node': node_uuid})
+                LOG.error(_LE("Error deploying instance %(instance)s on "
+                              "baremetal node %(node)s."),
+                             {'instance': instance['uuid'],
+                              'node': node_uuid})
                 self.destroy(context, instance, network_info)
 
     def _unprovision(self, icli, instance, node):
@@ -589,7 +586,7 @@ class IronicDriver(virt_driver.ComputeDriver):
         data = {'tries': 0}
 
         def _wait_for_provision_state():
-            node = validate_instance_and_node(icli, instance)
+            node = _validate_instance_and_node(icli, instance)
             if not node.provision_state:
                 LOG.debug("Ironic node %(node)s is now unprovisioned",
                           dict(node=node.uuid), instance=instance)
@@ -626,7 +623,7 @@ class IronicDriver(virt_driver.ComputeDriver):
         """
         icli = client_wrapper.IronicClientWrapper()
         try:
-            node = validate_instance_and_node(icli, instance)
+            node = _validate_instance_and_node(icli, instance)
         except exception.InstanceNotFound:
             LOG.warning(_LW("Destroy called on non-existing instance %s."),
                         instance['uuid'])
@@ -661,7 +658,7 @@ class IronicDriver(virt_driver.ComputeDriver):
 
         """
         icli = client_wrapper.IronicClientWrapper()
-        node = validate_instance_and_node(icli, instance)
+        node = _validate_instance_and_node(icli, instance)
         icli.call("node.set_power_state", node.uuid, 'reboot')
 
     def power_off(self, instance):
@@ -672,7 +669,7 @@ class IronicDriver(virt_driver.ComputeDriver):
         """
         # TODO(nobodycam): check the current power state first.
         icli = client_wrapper.IronicClientWrapper()
-        node = validate_instance_and_node(icli, instance)
+        node = _validate_instance_and_node(icli, instance)
         icli.call("node.set_power_state", node.uuid, 'off')
 
     def power_on(self, context, instance, network_info,
@@ -689,7 +686,7 @@ class IronicDriver(virt_driver.ComputeDriver):
         """
         # TODO(nobodycam): check the current power state first.
         icli = client_wrapper.IronicClientWrapper()
-        node = validate_instance_and_node(icli, instance)
+        node = _validate_instance_and_node(icli, instance)
         icli.call("node.set_power_state", node.uuid, 'on')
 
     def get_host_stats(self, refresh=False):
@@ -765,8 +762,9 @@ class IronicDriver(virt_driver.ComputeDriver):
         self.firewall_driver.unfilter_instance(instance, network_info)
 
     def _plug_vifs(self, node, instance, network_info):
-        LOG.debug("plug: instance_uuid=%(uuid)s vif=%(network_info)s"
-                  % {'uuid': instance['uuid'], 'network_info': network_info})
+        LOG.debug("plug: instance_uuid=%(uuid)s vif=%(network_info)s",
+                  {'uuid': instance['uuid'],
+                   'network_info': network_info})
         # start by ensuring the ports are clear
         self._unplug_vifs(node, instance, network_info)
 
@@ -793,8 +791,9 @@ class IronicDriver(virt_driver.ComputeDriver):
                 icli.call("port.update", pif.uuid, patch)
 
     def _unplug_vifs(self, node, instance, network_info):
-        LOG.debug("unplug: instance_uuid=%(uuid)s vif=%(network_info)s"
-                  % {'uuid': instance['uuid'], 'network_info': network_info})
+        LOG.debug("unplug: instance_uuid=%(uuid)s vif=%(network_info)s",
+                  {'uuid': instance['uuid'],
+                   'network_info': network_info})
         if network_info and len(network_info) > 0:
             icli = client_wrapper.IronicClientWrapper()
             ports = icli.call("node.list_ports", node.uuid)
@@ -805,7 +804,7 @@ class IronicDriver(virt_driver.ComputeDriver):
                 patch = [{'op': 'remove', 'path': '/extra/vif_port_id'}]
                 try:
                     icli.call("port.update", pif.uuid, patch)
-                except ironic_exception.BadRequest:
+                except ironic.exc.BadRequest:
                     pass
 
     def plug_vifs(self, instance, network_info):
@@ -890,8 +889,8 @@ class IronicDriver(virt_driver.ComputeDriver):
             icli.call("node.set_provision_state",
                       node_uuid, ironic_states.REBUILD)
         except (exception.NovaException,               # Retry failed
-                ironic_exception.InternalServerError,  # Validations
-                ironic_exception.BadRequest) as e:     # Maintenance
+                ironic.exc.InternalServerError,        # Validations
+                ironic.exc.BadRequest) as e:           # Maintenance
             msg = (_("Failed to request Ironic to rebuild instance "
                      "%(inst)s: %(reason)s") % {'inst': instance['uuid'],
                                                 'reason': str(e)})