From 6e981156c83c4d7217e985c88553548d5924c601 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 23 Nov 2016 16:17:58 +0100 Subject: [PATCH] Move interface validation from API to conductor side This change moves the check on network interface to driver_factory.py and moves node creation from API to conductor side. A new exception InterfaceNotFoundInEntrypoint (similar to DriverNotFoundInEntrypoint) was introduced for the case when invalid interface is requested either during creation or during updating. The current approach already duplicates the check in two places. With the driver composition in place it will not be quite possible: different conductors may have different interfaces enabled as long as no hardware type ends up with different interfaces on different conductors. Also when calculating the defaults, we'll need access to a real hardware type object, which may not be possible on API side. Also there is a demand for driver-side validations on creation (e.g. detecting duplicating IPMI addresses), which also requires creation to happen on conductor side. Such features, however, are out of scope for this change. A side effect of this change is that objects.Node.network_interface is now defined the same way as other interfaces (i.e. nullable string). Also added more clean ups to base unit test class, as otherwise newly added tests randomly break other tests. Change-Id: Id1da20ccd5bb50e61a82449ef3d2ffce91822d71 Partial-Bug: #1524745 --- ironic/api/controllers/v1/node.py | 31 +---------- ironic/api/controllers/v1/utils.py | 15 ----- ironic/common/driver_factory.py | 37 ++++++++++++- ironic/common/exception.py | 6 ++ ironic/conductor/manager.py | 34 ++++++++---- ironic/conductor/rpcapi.py | 18 +++++- ironic/conductor/task_manager.py | 1 + ironic/objects/node.py | 29 +++++----- ironic/tests/base.py | 8 ++- ironic/tests/unit/api/v1/test_nodes.py | 54 +++--------------- .../tests/unit/common/test_driver_factory.py | 55 +++++++++++++------ ironic/tests/unit/conductor/test_manager.py | 35 +++++++++++- .../unit/drivers/modules/network/test_flat.py | 4 +- ironic/tests/unit/objects/test_node.py | 20 ------- ironic/tests/unit/objects/test_objects.py | 10 ++-- .../create-on-conductor-c1c52a1f022c4048.yaml | 11 ++++ 16 files changed, 205 insertions(+), 163 deletions(-) create mode 100644 releasenotes/notes/create-on-conductor-c1c52a1f022c4048.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index b6b120a4d8..cb371f2526 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1410,21 +1410,6 @@ class NodesController(rest.RestController): n_interface is not wtypes.Unset): raise exception.NotAcceptable() - # NOTE(vsaienko) The validation is performed on API side, - # all conductors and api should have the same list of - # enabled_network_interfaces. - # TODO(vsaienko) remove it once driver-composition-reform - # is implemented. - if (n_interface is not wtypes.Unset and - not api_utils.is_valid_network_interface(n_interface)): - error_msg = _("Cannot create node with the invalid network " - "interface '%(n_interface)s'. Enabled network " - "interfaces are: %(enabled_int)s") - raise wsme.exc.ClientSideError( - error_msg % {'n_interface': n_interface, - 'enabled_int': CONF.enabled_network_interfaces}, - status_code=http_client.BAD_REQUEST) - # NOTE(deva): get_topic_for checks if node.driver is in the hash ring # and raises NoValidHost if it is not. # We need to ensure that node has a UUID before it can @@ -1433,7 +1418,7 @@ class NodesController(rest.RestController): node.uuid = uuidutils.generate_uuid() try: - pecan.request.rpcapi.get_topic_for(node) + topic = pecan.request.rpcapi.get_topic_for(node) except exception.NoValidHost as e: # NOTE(deva): convert from 404 to 400 because client can see # list of available drivers and shouldn't request @@ -1448,7 +1433,8 @@ class NodesController(rest.RestController): new_node = objects.Node(pecan.request.context, **node.as_dict()) - new_node.create() + new_node = pecan.request.rpcapi.create_node( + pecan.request.context, new_node, topic) # Set the HTTP Location Header pecan.response.location = link.build_url('nodes', new_node.uuid) return Node.convert_with_links(new_node) @@ -1476,17 +1462,6 @@ class NodesController(rest.RestController): if n_interfaces and not api_utils.allow_network_interface(): raise exception.NotAcceptable() - for n_interface in n_interfaces: - if (n_interface is not None and - not api_utils.is_valid_network_interface(n_interface)): - error_msg = _("Node %(node)s: Cannot change " - "network_interface to invalid value: " - "%(n_interface)s") - raise wsme.exc.ClientSideError( - error_msg % {'node': node_ident, - 'n_interface': n_interface}, - status_code=http_client.BAD_REQUEST) - rpc_node = api_utils.get_rpc_node(node_ident) remove_inst_uuid_patch = [{'op': 'remove', 'path': '/instance_uuid'}] diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 832f7f8a95..5f11f6bb6e 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -276,21 +276,6 @@ def check_allowed_fields(fields): raise exception.NotAcceptable() -# NOTE(vsaienko) The validation is performed on API side, all conductors -# and api should have the same list of enabled_network_interfaces. -# TODO(vsaienko) remove it once driver-composition-reform is implemented. -def is_valid_network_interface(network_interface): - """Determine if the provided network_interface is valid. - - Check to see that the provided network_interface is in the enabled - network interfaces list. - - :param: network_interface: the node network interface to check. - :returns: True if the network_interface is valid, False otherwise. - """ - return network_interface in CONF.enabled_network_interfaces - - def check_allow_management_verbs(verb): min_version = MIN_VERB_VERSIONS.get(verb) if min_version is not None and pecan.request.version.minor < min_version: diff --git a/ironic/common/driver_factory.py b/ironic/common/driver_factory.py index 90040b7d85..94d51ed3e1 100644 --- a/ironic/common/driver_factory.py +++ b/ironic/common/driver_factory.py @@ -44,8 +44,11 @@ def build_driver_for_task(task, driver_name=None): :returns: A driver object for the task. :raises: DriverNotFound if node.driver could not be found in the "ironic.drivers" namespace. + :raises: InterfaceNotFoundInEntrypoint if some node interfaces are set + to invalid or unsupported values. """ node = task.node + check_and_update_node_interfaces(node) driver = driver_base.BareDriver() _attach_interfaces_to_driver(driver, node, driver_name=driver_name) return driver @@ -62,12 +65,40 @@ def _attach_interfaces_to_driver(driver, node, driver_name=None): try: net_driver = network_factory.get_driver(network_iface) except KeyError: - raise exception.DriverNotFoundInEntrypoint( - driver_name=network_iface, - entrypoint=network_factory._entrypoint_name) + raise exception.InterfaceNotFoundInEntrypoint( + iface=network_iface, + entrypoint=network_factory._entrypoint_name, + valid=network_factory.names) driver.network = net_driver +def check_and_update_node_interfaces(node): + """Ensure that node interfaces (e.g. for creation or updating) are valid. + + Updates interfaces with calculated defaults, if they are not provided. + + :param node: node object to check and potentially update + :raises: InterfaceNotFoundInEntrypoint on validation failure + :returns: True if any changes were made to the node, otherwise False + """ + # NOTE(dtantsur): objects raise NotImplementedError on accessing fields + # that are known, but missing from an object. Thus, we cannot just use + # getattr(node, 'network_interface', None) here. + if 'network_interface' in node and node.network_interface is not None: + if node.network_interface not in CONF.enabled_network_interfaces: + raise exception.InterfaceNotFoundInEntrypoint( + iface=node.network_interface, + entrypoint=NetworkInterfaceFactory._entrypoint_name, + valid=NetworkInterfaceFactory().names) + else: + node.network_interface = ( + CONF.default_network_interface or + ('flat' if CONF.dhcp.dhcp_provider == 'neutron' else 'noop')) + return True + + return False + + def get_driver(driver_name): """Simple method to get a ref to an instance of a driver. diff --git a/ironic/common/exception.py b/ironic/common/exception.py index ee73f5f25f..3d69ea428c 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -250,6 +250,12 @@ class DriverNotFoundInEntrypoint(DriverNotFound): "'%(entrypoint)s' entrypoint: %(driver_name)s.") +class InterfaceNotFoundInEntrypoint(InvalidParameterValue): + _msg_fmt = _("Could not find the following interface in the " + "'%(entrypoint)s' entrypoint: %(iface)s. Valid interfaces " + "are %(valid)s.") + + class ImageNotFound(NotFound): _msg_fmt = _("Image %(image_id)s could not be found.") diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 03daab5b3b..31c2d42b50 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -82,7 +82,7 @@ class ConductorManager(base_manager.BaseConductorManager): """Ironic Conductor manager main class.""" # NOTE(rloo): This must be in sync with rpcapi.ConductorAPI's. - RPC_API_VERSION = '1.35' + RPC_API_VERSION = '1.36' target = messaging.Target(version=RPC_API_VERSION) @@ -90,10 +90,29 @@ class ConductorManager(base_manager.BaseConductorManager): super(ConductorManager, self).__init__(host, topic) self.power_state_sync_count = collections.defaultdict(int) + @METRICS.timer('ConductorManager.create_node') + @messaging.expected_exceptions(exception.InvalidParameterValue, + exception.InterfaceNotFoundInEntrypoint) + def create_node(self, context, node_obj): + """Create a node in database. + + :param context: an admin context + :param node_obj: a created (but not saved to the database) node object. + :returns: created node object. + :raises: InterfaceNotFoundInEntrypoint if validation fails for any + dynamic interfaces (e.g. network_interface). + :raises: InvalidParameterValue if some fields fail validation. + """ + LOG.debug("RPC create_node called for node %s.", node_obj.uuid) + driver_factory.check_and_update_node_interfaces(node_obj) + node_obj.create() + return node_obj + @METRICS.timer('ConductorManager.update_node') @messaging.expected_exceptions(exception.InvalidParameterValue, exception.NodeLocked, - exception.InvalidState) + exception.InvalidState, + exception.InterfaceNotFoundInEntrypoint) def update_node(self, context, node_obj): """Update a node with the supplied data. @@ -126,15 +145,8 @@ class ConductorManager(base_manager.BaseConductorManager): raise exception.InvalidState( action % {'node': node_obj.uuid, 'allowed': ', '.join(allowed_update_states)}) - net_iface = node_obj.network_interface - if net_iface not in CONF.enabled_network_interfaces: - raise exception.InvalidParameterValue( - _("Cannot change network_interface to invalid value " - "%(n_interface)s for node %(node)s, valid interfaces " - "are: %(valid_choices)s.") % { - 'n_interface': net_iface, 'node': node_obj.uuid, - 'valid_choices': CONF.enabled_network_interfaces, - }) + + driver_factory.check_and_update_node_interfaces(node_obj) driver_name = node_obj.driver if 'driver' in delta else None with task_manager.acquire(context, node_id, shared=False, diff --git a/ironic/conductor/rpcapi.py b/ironic/conductor/rpcapi.py index a61e41fa2d..2c411e934f 100644 --- a/ironic/conductor/rpcapi.py +++ b/ironic/conductor/rpcapi.py @@ -82,11 +82,12 @@ class ConductorAPI(object): | 1.33 - Added update and destroy portgroup. | 1.34 - Added heartbeat | 1.35 - Added destroy_volume_connector and update_volume_connector + | 1.36 - Added create_node """ # NOTE(rloo): This must be in sync with manager.ConductorManager's. - RPC_API_VERSION = '1.35' + RPC_API_VERSION = '1.36' def __init__(self, topic=None): super(ConductorAPI, self).__init__() @@ -140,6 +141,21 @@ class ConductorAPI(object): host = random.choice(list(hash_ring.hosts)) return self.topic + "." + host + def create_node(self, context, node_obj, topic=None): + """Synchronously, have a conductor validate and create a node. + + Create the node's information in the database and return a node object. + + :param context: request context. + :param node_obj: a created (but not saved) node object. + :param topic: RPC topic. Defaults to self.topic. + :returns: created node object. + :raises: InterfaceNotFoundInEntrypoint if validation fails for any + dynamic interfaces (e.g. network_interface). + """ + cctxt = self.client.prepare(topic=topic or self.topic, version='1.36') + return cctxt.call(context, 'create_node', node_obj=node_obj) + def update_node(self, context, node_obj, topic=None): """Synchronously, have a conductor update the node's information. diff --git a/ironic/conductor/task_manager.py b/ironic/conductor/task_manager.py index e3f9c6d943..cd5b25fd8b 100644 --- a/ironic/conductor/task_manager.py +++ b/ironic/conductor/task_manager.py @@ -187,6 +187,7 @@ class TaskManager(object): from the Node's current driver. :param purpose: human-readable purpose to put to debug logs. :raises: DriverNotFound + :raises: InterfaceNotFoundInEntrypoint :raises: NodeNotFound :raises: NodeLocked diff --git a/ironic/objects/node.py b/ironic/objects/node.py index 46a72c4e33..1fcef73e8f 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -19,7 +19,6 @@ from oslo_versionedobjects import base as object_base from ironic.common import exception from ironic.common.i18n import _ -from ironic.conf import CONF from ironic.db import api as db_api from ironic.objects import base from ironic.objects import fields as object_fields @@ -28,13 +27,6 @@ from ironic.objects import notification REQUIRED_INT_PROPERTIES = ['local_gb', 'cpus', 'memory_mb'] -def _default_network_interface(): - network_iface = (CONF.default_network_interface or - ('flat' if CONF.dhcp.dhcp_provider == 'neutron' - else 'noop')) - return network_iface - - @base.IronicObjectRegistry.register class Node(base.IronicObject, object_base.VersionedObjectDictCompat): # Version 1.0: Initial version @@ -61,7 +53,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): # Version 1.19: Add fields: boot_interface, console_interface, # deploy_interface, inspect_interface, management_interface, # power_interface, raid_interface, vendor_interface - VERSION = '1.19' + # Version 1.20: Type of network_interface changed to just nullable string + VERSION = '1.20' dbapi = db_api.get_instance() @@ -126,8 +119,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): 'deploy_interface': object_fields.StringField(nullable=True), 'inspect_interface': object_fields.StringField(nullable=True), 'management_interface': object_fields.StringField(nullable=True), - 'network_interface': object_fields.StringFieldThatAcceptsCallable( - nullable=False, default=_default_network_interface), + 'network_interface': object_fields.StringField(nullable=True), 'power_interface': object_fields.StringField(nullable=True), 'raid_interface': object_fields.StringField(nullable=True), 'vendor_interface': object_fields.StringField(nullable=True), @@ -445,7 +437,9 @@ class NodePayload(notification.NotificationPayloadBase): 'uuid': ('node', 'uuid') } # Version 1.0: Initial version, based off of Node version 1.18. - VERSION = '1.0' + # Version 1.1: Type of network_interface changed to just nullable string + # similar to version 1.20 of Node. + VERSION = '1.1' fields = { 'clean_step': object_fields.FlexibleDictField(nullable=True), 'console_enabled': object_fields.BooleanField(), @@ -458,7 +452,7 @@ class NodePayload(notification.NotificationPayloadBase): 'last_error': object_fields.StringField(nullable=True), 'maintenance': object_fields.BooleanField(), 'maintenance_reason': object_fields.StringField(nullable=True), - 'network_interface': object_fields.StringFieldThatAcceptsCallable(), + 'network_interface': object_fields.StringField(nullable=True), 'name': object_fields.StringField(nullable=True), 'power_state': object_fields.StringField(nullable=True), 'properties': object_fields.FlexibleDictField(nullable=True), @@ -491,7 +485,8 @@ class NodeSetPowerStateNotification(notification.NotificationBase): class NodeSetPowerStatePayload(NodePayload): """Payload schema for when ironic changes a node's power state.""" # Version 1.0: Initial version - VERSION = '1.0' + # Version 1.1: Parent NodePayload version 1.1 + VERSION = '1.1' fields = { # "to_power" indicates the future target_power_state of the node. A @@ -532,7 +527,8 @@ class NodeCorrectedPowerStatePayload(NodePayload): before the node was updated. """ # Version 1.0: Initial version - VERSION = '1.0' + # Version 1.1: Parent NodePayload version 1.1 + VERSION = '1.1' fields = { 'from_power': object_fields.StringField(nullable=True) @@ -558,7 +554,8 @@ class NodeSetProvisionStateNotification(notification.NotificationBase): class NodeSetProvisionStatePayload(NodePayload): """Payload schema for when ironic changes a node provision state.""" # Version 1.0: Initial version - VERSION = '1.0' + # Version 1.1: Parent NodePayload version 1.1 + VERSION = '1.1' SCHEMA = dict(NodePayload.SCHEMA, **{'instance_info': ('node', 'instance_info')}) diff --git a/ironic/tests/base.py b/ironic/tests/base.py index 565923ac01..50a4a6ee3b 100644 --- a/ironic/tests/base.py +++ b/ironic/tests/base.py @@ -37,6 +37,7 @@ import testtools from ironic.common import config as ironic_config from ironic.common import context as ironic_context +from ironic.common import driver_factory from ironic.common import hash_ring from ironic.conf import CONF from ironic.objects import base as objects_base @@ -110,6 +111,9 @@ class TestCase(testtools.TestCase): self.useFixture(fixtures.EnvironmentVariable('http_proxy')) self.policy = self.useFixture(policy_fixture.PolicyFixture()) + driver_factory.DriverFactory._extension_manager = None + driver_factory.NetworkInterfaceFactory._extension_manager = None + def _set_config(self): self.cfg_fixture = self.useFixture(config_fixture.Config(CONF)) self.config(use_stderr=False, @@ -119,7 +123,9 @@ class TestCase(testtools.TestCase): group='neutron') self.config(provisioning_network_uuid=uuidutils.generate_uuid(), group='neutron') - self.config(enabled_network_interfaces=['flat', 'noop', 'neutron']) + self.config(enabled_drivers=['fake']) + self.config(enabled_network_interfaces=['flat', 'noop', 'neutron'], + default_network_interface=None) self.set_defaults(host='fake-mini', debug=True) self.set_defaults(connection="sqlite://", diff --git a/ironic/tests/unit/api/v1/test_nodes.py b/ironic/tests/unit/api/v1/test_nodes.py index a3af99e7cb..c7a1fe71eb 100644 --- a/ironic/tests/unit/api/v1/test_nodes.py +++ b/ironic/tests/unit/api/v1/test_nodes.py @@ -34,10 +34,10 @@ from ironic.api.controllers.v1 import node as api_node from ironic.api.controllers.v1 import utils as api_utils from ironic.api.controllers.v1 import versions from ironic.common import boot_devices +from ironic.common import driver_factory from ironic.common import exception from ironic.common import states from ironic.conductor import rpcapi -from ironic.conf import CONF from ironic import objects from ironic.tests import base from ironic.tests.unit.api import base as test_api_base @@ -1646,43 +1646,6 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.OK, response.status_code) - def test_update_network_interface_null_sets_default(self): - CONF.set_override('default_network_interface', 'neutron') - node = obj_utils.create_test_node(self.context, - uuid=uuidutils.generate_uuid(), - network_interface='flat') - self.mock_update_node.return_value = node - network_interface = 'neutron' - headers = {api_base.Version.string: str(api_v1.MAX_VER)} - response = self.patch_json('/nodes/%s' % node.uuid, - [{'path': '/network_interface', - 'value': None, - 'op': 'add'}], - headers=headers) - self.assertEqual('application/json', response.content_type) - self.assertEqual(http_client.OK, response.status_code) - # check the node we pass to updated_node - node_arg = self.mock_update_node.call_args[0][1] - self.assertEqual(network_interface, node_arg['network_interface']) - - def test_update_network_interface_remove_sets_default(self): - CONF.set_override('default_network_interface', 'neutron') - node = obj_utils.create_test_node(self.context, - uuid=uuidutils.generate_uuid(), - network_interface='flat') - self.mock_update_node.return_value = node - network_interface = 'neutron' - headers = {api_base.Version.string: str(api_v1.MAX_VER)} - response = self.patch_json('/nodes/%s' % node.uuid, - [{'path': '/network_interface', - 'op': 'remove'}], - headers=headers) - self.assertEqual('application/json', response.content_type) - self.assertEqual(http_client.OK, response.status_code) - # check the node we pass to updated_node - node_arg = self.mock_update_node.call_args[0][1] - self.assertEqual(network_interface, node_arg['network_interface']) - def test_update_network_interface_old_api(self): node = obj_utils.create_test_node(self.context, uuid=uuidutils.generate_uuid()) @@ -1757,6 +1720,14 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual(http_client.BAD_REQUEST, response.status_code) +def _create_node_locally(node): + driver_factory.check_and_update_node_interfaces(node) + node.create() + return node + + +@mock.patch.object(rpcapi.ConductorAPI, 'create_node', + lambda _api, _ctx, node, _topic: _create_node_locally(node)) class TestPost(test_api_base.BaseApiTest): def setUp(self): @@ -1816,13 +1787,6 @@ class TestPost(test_api_base.BaseApiTest): network_interface='neutron') self.assertEqual('neutron', result['network_interface']) - def test_create_node_default_network_interface(self): - CONF.set_override('default_network_interface', 'neutron') - CONF.set_override('enabled_network_interfaces', 'flat,noop,neutron') - headers = {api_base.Version.string: '1.20'} - result = self._test_create_node(headers=headers) - self.assertEqual('neutron', result['network_interface']) - def test_create_node_name_empty_invalid(self): ndict = test_api_utils.post_get_test_node(name='') response = self.post_json('/nodes', ndict, diff --git a/ironic/tests/unit/common/test_driver_factory.py b/ironic/tests/unit/common/test_driver_factory.py index 64a66a65b8..f2b2e890b3 100644 --- a/ironic/tests/unit/common/test_driver_factory.py +++ b/ironic/tests/unit/common/test_driver_factory.py @@ -30,10 +30,6 @@ class FakeEp(object): class DriverLoadTestCase(base.TestCase): - def setUp(self): - super(DriverLoadTestCase, self).setUp() - driver_factory.DriverFactory._extension_manager = None - def _fake_init_name_err(self, *args, **kwargs): kwargs['on_load_failure_callback'](None, FakeEp, NameError('aaa')) @@ -104,11 +100,6 @@ class WarnUnsupportedDriversTestCase(base.TestCase): class GetDriverTestCase(base.TestCase): - def setUp(self): - super(GetDriverTestCase, self).setUp() - driver_factory.DriverFactory._extension_manager = None - self.config(enabled_drivers=['fake']) - def test_get_driver_known(self): driver = driver_factory.get_driver('fake') self.assertIsInstance(driver, drivers_base.BaseDriver) @@ -119,12 +110,6 @@ class GetDriverTestCase(base.TestCase): class NetworkInterfaceFactoryTestCase(db_base.DbTestCase): - def setUp(self): - super(NetworkInterfaceFactoryTestCase, self).setUp() - driver_factory.DriverFactory._extension_manager = None - driver_factory.NetworkInterfaceFactory._extension_manager = None - self.config(enabled_drivers=['fake']) - @mock.patch.object(driver_factory, '_warn_if_unsupported') def test_build_driver_for_task(self, mock_warn): # flat, neutron, and noop network interfaces are enabled in base test @@ -188,7 +173,7 @@ class NetworkInterfaceFactoryTestCase(db_base.DbTestCase): def test_build_driver_for_task_unknown_network_interface(self): node = obj_utils.create_test_node(self.context, driver='fake', network_interface='meow') - self.assertRaises(exception.DriverNotFoundInEntrypoint, + self.assertRaises(exception.InterfaceNotFoundInEntrypoint, task_manager.acquire, self.context, node.id) @@ -201,3 +186,41 @@ class NewFactoryTestCase(db_base.DbTestCase): factory = NewDriverFactory() self.assertEqual('woof', factory._entrypoint_name) self.assertEqual([], factory._enabled_driver_list) + + +class CheckAndUpdateNodeInterfacesTestCase(db_base.DbTestCase): + def test_no_network_interface(self): + node = obj_utils.get_test_node(self.context, driver='fake') + self.assertTrue(driver_factory.check_and_update_node_interfaces(node)) + self.assertEqual('flat', node.network_interface) + + def test_none_network_interface(self): + node = obj_utils.get_test_node(self.context, driver='fake', + network_interface=None) + self.assertTrue(driver_factory.check_and_update_node_interfaces(node)) + self.assertEqual('flat', node.network_interface) + + def test_no_network_interface_default_from_conf(self): + self.config(default_network_interface='noop') + node = obj_utils.get_test_node(self.context, driver='fake') + self.assertTrue(driver_factory.check_and_update_node_interfaces(node)) + self.assertEqual('noop', node.network_interface) + + def test_no_network_interface_default_from_dhcp(self): + self.config(dhcp_provider='none', group='dhcp') + node = obj_utils.get_test_node(self.context, driver='fake') + self.assertTrue(driver_factory.check_and_update_node_interfaces(node)) + # "none" dhcp provider corresponds to "noop" network_interface + self.assertEqual('noop', node.network_interface) + + def test_valid_network_interface(self): + node = obj_utils.get_test_node(self.context, driver='fake', + network_interface='noop') + self.assertFalse(driver_factory.check_and_update_node_interfaces(node)) + + def test_invalid_network_interface(self): + node = obj_utils.get_test_node(self.context, driver='fake', + network_interface='banana') + self.assertRaises(exception.InterfaceNotFoundInEntrypoint, + driver_factory.check_and_update_node_interfaces, + node) diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 47f08975c8..8efd9d521f 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -380,6 +380,38 @@ class ChangeNodePowerStateTestCase(mgr_utils.ServiceSetUpMixin, obj_fields.NotificationLevel.INFO) +@mgr_utils.mock_record_keepalive +class CreateNodeTestCase(mgr_utils.ServiceSetUpMixin, + tests_db_base.DbTestCase): + def test_create_node(self): + node = obj_utils.get_test_node(self.context, driver='fake', + extra={'test': 'one'}) + + res = self.service.create_node(self.context, node) + + self.assertEqual({'test': 'one'}, res['extra']) + res = objects.Node.get_by_uuid(self.context, node['uuid']) + self.assertEqual({'test': 'one'}, res['extra']) + + @mock.patch.object(driver_factory, 'check_and_update_node_interfaces', + autospec=True) + def test_create_node_validation_fails(self, mock_validate): + node = obj_utils.get_test_node(self.context, driver='fake', + extra={'test': 'one'}) + mock_validate.side_effect = exception.InterfaceNotFoundInEntrypoint( + 'boom') + + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.create_node, + self.context, node) + # Compare true exception hidden by @messaging.expected_exceptions + self.assertEqual(exception.InterfaceNotFoundInEntrypoint, + exc.exc_info[0]) + + self.assertRaises(exception.NotFound, + objects.Node.get_by_uuid, self.context, node['uuid']) + + @mgr_utils.mock_record_keepalive class UpdateNodeTestCase(mgr_utils.ServiceSetUpMixin, tests_db_base.DbTestCase): @@ -503,7 +535,8 @@ class UpdateNodeTestCase(mgr_utils.ServiceSetUpMixin, exc = self.assertRaises(messaging.rpc.ExpectedException, self.service.update_node, self.context, node) - self.assertEqual(exception.InvalidParameterValue, exc.exc_info[0]) + self.assertEqual(exception.InterfaceNotFoundInEntrypoint, + exc.exc_info[0]) node.refresh() self.assertEqual(old_iface, node.network_interface) diff --git a/ironic/tests/unit/drivers/modules/network/test_flat.py b/ironic/tests/unit/drivers/modules/network/test_flat.py index bf4b36b353..19d3cb053a 100644 --- a/ironic/tests/unit/drivers/modules/network/test_flat.py +++ b/ironic/tests/unit/drivers/modules/network/test_flat.py @@ -62,8 +62,10 @@ class TestFlatInterface(db_base.DbTestCase): @mock.patch.object(neutron, 'rollback_ports') def test_add_cleaning_network_no_cleaning_net_uuid(self, rollback_mock, add_mock): - self.config(cleaning_network_uuid='abc', group='neutron') with task_manager.acquire(self.context, self.node.id) as task: + # This has to go after acquire, or acquire will raise + # DriverLoadError. + self.config(cleaning_network_uuid='abc', group='neutron') self.assertRaises(exception.InvalidParameterValue, self.interface.add_cleaning_network, task) self.assertFalse(rollback_mock.called) diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index 5fcaeb9194..467dcea8a4 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/ironic/tests/unit/objects/test_node.py @@ -18,7 +18,6 @@ from testtools import matchers from ironic.common import context from ironic.common import exception -from ironic.conf import CONF from ironic import objects from ironic.tests.unit.db import base from ironic.tests.unit.db import utils @@ -196,22 +195,3 @@ class TestNodeObject(base.DbTestCase): } node._validate_property_values(values['properties']) self.assertEqual(expect, values['properties']) - - def test_get_network_interface_use_field(self): - CONF.set_override('default_network_interface', None) - for nif in ('neutron', 'flat', 'noop'): - self.node.network_interface = nif - self.assertEqual(nif, self.node.network_interface) - - def test_get_network_interface_use_conf(self): - for nif in ('neutron', 'flat', 'noop'): - CONF.set_override('default_network_interface', nif) - self.node = obj_utils.get_test_node(self.ctxt, **self.fake_node) - self.assertEqual(nif, self.node.network_interface) - - def test_get_network_interface_use_dhcp_provider(self): - CONF.set_override('default_network_interface', None) - for dhcp, nif in (('neutron', 'flat'), ('none', 'noop')): - CONF.set_override('dhcp_provider', dhcp, 'dhcp') - self.node = obj_utils.get_test_node(self.ctxt, **self.fake_node) - self.assertEqual(nif, self.node.network_interface) diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 43e2b2f447..2d24fb6319 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -404,7 +404,7 @@ class TestObject(_LocalTest, _TestObject): # version bump. It is md5 hash of object fields and remotable methods. # The fingerprint values should only be changed if there is a version bump. expected_object_fingerprints = { - 'Node': '1.19-e8b294016d8d5b322df813f790d092b4', + 'Node': '1.20-b6a13eb50f9d64fa6c9d614c61dbec31', 'MyObj': '1.5-4f5efe8f0fcaf182bbe1c7fe3ba858db', 'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905', 'Port': '1.6-609504503d68982a10f495659990084b', @@ -412,15 +412,15 @@ expected_object_fingerprints = { 'Conductor': '1.1-5091f249719d4a465062a1b3dc7f860d', 'EventType': '1.1-aa2ba1afd38553e3880c267404e8d370', 'NotificationPublisher': '1.0-51a09397d6c0687771fb5be9a999605d', - 'NodePayload': '1.0-ccb491ab5cd247e2ba3f21af4c12eb7c', + 'NodePayload': '1.1-d895cf6411ac666f9e982f85ea0a9499', 'NodeSetPowerStateNotification': '1.0-59acc533c11d306f149846f922739c15', - 'NodeSetPowerStatePayload': '1.0-80986cc6a099cccd481fe3e288157a07', + 'NodeSetPowerStatePayload': '1.1-b8fab1bea5a2da5900445ab515e41715', 'NodeCorrectedPowerStateNotification': '1.0-59acc533c11d306f149846f922739' 'c15', - 'NodeCorrectedPowerStatePayload': '1.0-2a484d7c342caa9fe488de16dc5f1f1e', + 'NodeCorrectedPowerStatePayload': '1.1-5d1544defc858ae8a722f4cadd511bac', 'NodeSetProvisionStateNotification': '1.0-59acc533c11d306f149846f922739c15', - 'NodeSetProvisionStatePayload': '1.0-91be7439b9b6b04931c9b99b8e1ea87a', + 'NodeSetProvisionStatePayload': '1.1-743be1f5748f346e3da33390983172b1', 'VolumeConnector': '1.0-3e0252c0ab6e6b9d158d09238a577d97' } diff --git a/releasenotes/notes/create-on-conductor-c1c52a1f022c4048.yaml b/releasenotes/notes/create-on-conductor-c1c52a1f022c4048.yaml new file mode 100644 index 0000000000..82d02642a1 --- /dev/null +++ b/releasenotes/notes/create-on-conductor-c1c52a1f022c4048.yaml @@ -0,0 +1,11 @@ +--- +upgrade: + - In this release node creation logic was moved from the API service to + the conductor service. This is more consistent with other node operations + and opens opportunities for conductor-side validations on nodes. + However, with this change, node creation may take longer, and this may + limit the number of nodes that can be enrolled in parallel. + - The "[DEFAULT]default_network_interface" and "[dhcp]dhcp_provider" + configuration options were previously required for the ironic-api service + to calculate the correct "network_interface" default. Now these options + are only read by the ironic-conductor service.