diff options
author | Michael Davies <michael@the-davies.net> | 2014-07-18 02:37:11 +0930 |
---|---|---|
committer | Michael Davies <michael@the-davies.net> | 2014-07-25 07:21:38 +0930 |
commit | ea98e03d2be5cf9a5797d23b1fb0a76a71a2d38b (patch) | |
tree | a426fa713c2078df20aea66b1cd7f387548b619c /ironic/nova | |
parent | b3e4e4fb7172cd2db04d9a711712da93c6160eef (diff) | |
download | ironic-ea98e03d2be5cf9a5797d23b1fb0a76a71a2d38b.tar.gz |
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
Diffstat (limited to 'ironic/nova')
-rw-r--r-- | ironic/nova/tests/scheduler/test_ironic_host_manager.py | 6 | ||||
-rw-r--r-- | ironic/nova/tests/virt/ironic/test_driver.py | 21 | ||||
-rw-r--r-- | ironic/nova/virt/ironic/client_wrapper.py | 36 | ||||
-rw-r--r-- | ironic/nova/virt/ironic/driver.py | 107 |
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)}) |