From d8bdbda03067842c13deda504d68670b4b955b66 Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Tue, 7 Feb 2017 20:44:18 +0000 Subject: [PATCH] No node interface settings for classic drivers Classic drivers have their interfaces (except for network and storage) pre-determined. Unlike dynamic drivers, it is not possible to change these interfaces for nodes with a classic driver. If that is attempted (when creating or updating a node), an exception MustBeNone is raised (and HTTP status 400 is returned). So that we don't break existing ironic clusters that have nodes with classic drivers and interfaces specified, a warning is logged (instead of raising an exception) when a TaskManager is created. Change-Id: I290b10f735d0da9710d1ee3b50c3252f73956428 Partial-Bug: #1524745 --- ironic/common/driver_factory.py | 33 +++++++++++++- ironic/common/exception.py | 5 +++ ironic/conductor/manager.py | 21 ++++++--- ironic/tests/unit/api/v1/test_nodes.py | 36 ++++++++++++++- .../tests/unit/common/test_driver_factory.py | 45 ++++++++++++++++++- ...annot-set-interfaces-620b37c4e5c88b80.yaml | 5 +++ 6 files changed, 135 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/nodes-classic-drivers-cannot-set-interfaces-620b37c4e5c88b80.yaml diff --git a/ironic/common/driver_factory.py b/ironic/common/driver_factory.py index d8331da3d3..a45da9386c 100644 --- a/ironic/common/driver_factory.py +++ b/ironic/common/driver_factory.py @@ -55,7 +55,17 @@ def build_driver_for_task(task, driver_name=None): driver_name = driver_name or node.driver driver_or_hw_type = get_driver_or_hardware_type(driver_name) - check_and_update_node_interfaces(node, driver_or_hw_type=driver_or_hw_type) + try: + check_and_update_node_interfaces( + node, driver_or_hw_type=driver_or_hw_type) + except exception.MustBeNone as e: + # NOTE(rloo). This was raised because nodes with classic drivers + # cannot have any interfaces (except for network and + # storage) set. However, there was a small window + # where this was possible so instead of breaking those + # users totally, we'll spam them with warnings instead. + LOG.warning(_LW('%s They will be ignored. To avoid this warning, ' + 'please set them to None.'), e) bare_driver = driver_base.BareDriver() _attach_interfaces_to_driver(bare_driver, node, driver_or_hw_type) @@ -225,6 +235,8 @@ def check_and_update_node_interfaces(node, driver_or_hw_type=None): :raises: NoValidDefaultForInterface if the default value cannot be calculated and is not provided in the configuration :raises: DriverNotFound if the node's driver or hardware type is not found + :raises: MustBeNone if one or more of the node's interface + fields were specified when they should not be. """ if driver_or_hw_type is None: driver_or_hw_type = get_driver_or_hardware_type(node.driver) @@ -237,6 +249,25 @@ def check_and_update_node_interfaces(node, driver_or_hw_type=None): # Only network and storage interfaces are dynamic for classic drivers factories = ['network', 'storage'] + # These are interfaces that cannot be specified via the node. E.g., + # for classic drivers, none are allowed except for network & storage. + not_allowed_ifaces = driver_base.ALL_INTERFACES - set(factories) + + bad_interface_fields = [] + for iface in not_allowed_ifaces: + field_name = '%s_interface' % iface + # NOTE(dtantsur): objects raise NotImplementedError on accessing fields + # that are known, but missing from an object. Thus, we cannot just use + # getattr(node, field_name, None) here. + if field_name in node: + impl_name = getattr(node, field_name) + if impl_name is not None: + bad_interface_fields.append(field_name) + + if bad_interface_fields: + raise exception.MustBeNone(node=node.uuid, driver=node.driver, + node_fields=','.join(bad_interface_fields)) + # Result - whether the node object was modified result = False diff --git a/ironic/common/exception.py b/ironic/common/exception.py index bc1e5faccc..347e029fbc 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -350,6 +350,11 @@ class NoValidDefaultForInterface(InvalidParameterValue): "value found for %(interface_type)s interface.") +class MustBeNone(InvalidParameterValue): + _msg_fmt = _("For node %(node)s with driver %(driver)s, these node " + "fields must be set to None: %(node_fields)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 827244ed1d..eb1bdbbf2a 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -96,10 +96,12 @@ class ConductorManager(base_manager.BaseConductorManager): self.power_state_sync_count = collections.defaultdict(int) @METRICS.timer('ConductorManager.create_node') + # No need to add these since they are subclasses of InvalidParameterValue: + # InterfaceNotFoundInEntrypoint + # IncompatibleInterface, + # NoValidDefaultForInterface + # MustBeNone @messaging.expected_exceptions(exception.InvalidParameterValue, - exception.InterfaceNotFoundInEntrypoint, - exception.IncompatibleInterface, - exception.NoValidDefaultForInterface, exception.DriverNotFound) def create_node(self, context, node_obj): """Create a node in database. @@ -114,6 +116,8 @@ class ConductorManager(base_manager.BaseConductorManager): :raises: NoValidDefaultForInterface if no default can be calculated for some interfaces, and explicit values must be provided. :raises: InvalidParameterValue if some fields fail validation. + :raises: MustBeNone if one or more of the node's interface + fields were specified when they should not be. :raises: DriverNotFound if the driver or hardware type is not found. """ LOG.debug("RPC create_node called for node %s.", node_obj.uuid) @@ -122,12 +126,14 @@ class ConductorManager(base_manager.BaseConductorManager): return node_obj @METRICS.timer('ConductorManager.update_node') + # No need to add these since they are subclasses of InvalidParameterValue: + # InterfaceNotFoundInEntrypoint + # IncompatibleInterface, + # NoValidDefaultForInterface + # MustBeNone @messaging.expected_exceptions(exception.InvalidParameterValue, exception.NodeLocked, exception.InvalidState, - exception.InterfaceNotFoundInEntrypoint, - exception.IncompatibleInterface, - exception.NoValidDefaultForInterface, exception.DriverNotFound) def update_node(self, context, node_obj): """Update a node with the supplied data. @@ -140,7 +146,8 @@ class ConductorManager(base_manager.BaseConductorManager): :param node_obj: a changed (but not saved) node object. :raises: NoValidDefaultForInterface if no default can be calculated for some interfaces, and explicit values must be provided. - + :raises: MustBeNone if one or more of the node's interface + fields were specified when they should not be. """ node_id = node_obj.uuid LOG.debug("RPC update_node called for node %s.", node_id) diff --git a/ironic/tests/unit/api/v1/test_nodes.py b/ironic/tests/unit/api/v1/test_nodes.py index e7db92f376..f9ce20c900 100644 --- a/ironic/tests/unit/api/v1/test_nodes.py +++ b/ironic/tests/unit/api/v1/test_nodes.py @@ -1815,6 +1815,22 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_code) + def test_update_classic_driver_interface_fields(self): + headers = {api_base.Version.string: '1.31'} + self.mock_update_node.side_effect = ( + exception.MustBeNone('error')) + for field in api_utils.V31_FIELDS: + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid()) + response = self.patch_json('/nodes/%s' % node.uuid, + [{'path': '/%s' % field, + 'value': 'fake', + 'op': 'add'}], + headers=headers, + expect_errors=True) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + self.assertEqual('application/json', response.content_type) + def _create_node_locally(node): driver_factory.check_and_update_node_interfaces(node) @@ -1890,10 +1906,13 @@ class TestPost(test_api_base.BaseApiTest): def test_create_node_specify_interfaces(self): headers = {api_base.Version.string: '1.31'} + for field in api_utils.V31_FIELDS: + cfg.CONF.set_override('enabled_%ss' % field, ['fake']) for field in api_utils.V31_FIELDS: node = { 'uuid': uuidutils.generate_uuid(), - field: 'fake' + field: 'fake', + 'driver': 'fake-hardware' } result = self._test_create_node(headers=headers, **node) self.assertEqual('fake', result[field]) @@ -1906,6 +1925,21 @@ class TestPost(test_api_base.BaseApiTest): expect_errors=True) self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_int) + def test_create_node_classic_driver_specify_interface(self): + headers = {api_base.Version.string: '1.31'} + for field in api_utils.V31_FIELDS: + node = { + 'uuid': uuidutils.generate_uuid(), + field: 'fake', + } + ndict = test_api_utils.post_get_test_node(**node) + response = self.post_json('/nodes', ndict, + headers=headers, + expect_errors=True) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + self.assertEqual('application/json', response.content_type) + self.assertTrue(response.json['error_message']) + 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 e0cc2eec5c..b4e3705139 100644 --- a/ironic/tests/unit/common/test_driver_factory.py +++ b/ironic/tests/unit/common/test_driver_factory.py @@ -13,6 +13,7 @@ # under the License. import mock +from oslo_utils import uuidutils from stevedore import dispatch from ironic.common import driver_factory @@ -32,7 +33,7 @@ class FakeEp(object): name = 'fake' -class DriverLoadTestCase(base.TestCase): +class DriverLoadTestCase(db_base.DbTestCase): def _fake_init_name_err(self, *args, **kwargs): kwargs['on_load_failure_callback'](None, FakeEp, NameError('aaa')) @@ -91,6 +92,33 @@ class DriverLoadTestCase(base.TestCase): ['fake'], driver_factory.DriverFactory._extension_manager.names()) self.assertTrue(mock_warn.called) + def test_build_driver_for_task(self): + node = obj_utils.create_test_node(self.context, driver='fake') + with task_manager.acquire(self.context, node.id) as task: + for iface in drivers_base.ALL_INTERFACES: + impl = getattr(task.driver, iface) + self.assertIsNotNone(impl) + + @mock.patch.object(driver_factory, '_attach_interfaces_to_driver', + autospec=True) + @mock.patch.object(driver_factory.LOG, 'warning', autospec=True) + def test_build_driver_for_task_incorrect(self, mock_warn, mock_attach): + # Cannot set these node interfaces for classic driver + no_set_interfaces = (drivers_base.ALL_INTERFACES - + set(['network', 'storage'])) + for iface in no_set_interfaces: + iface_name = '%s_interface' % iface + node_kwargs = {'uuid': uuidutils.generate_uuid(), + iface_name: 'fake'} + node = obj_utils.create_test_node(self.context, driver='fake', + **node_kwargs) + with task_manager.acquire(self.context, node.id) as task: + mock_warn.assert_called_once_with(mock.ANY, mock.ANY) + mock_warn.reset_mock() + mock_attach.assert_called_once_with(mock.ANY, task.node, + mock.ANY) + mock_attach.reset_mock() + class WarnUnsupportedDriversTestCase(base.TestCase): @mock.patch.object(driver_factory.LOG, 'warning', autospec=True) @@ -258,6 +286,21 @@ class CheckAndUpdateNodeInterfacesTestCase(db_base.DbTestCase): driver_factory.check_and_update_node_interfaces, node) + def test_classic_setting_interfaces(self): + # Cannot set these node interfaces for classic driver + no_set_interfaces = (drivers_base.ALL_INTERFACES - + set(['network', 'storage'])) + for iface in no_set_interfaces: + iface_name = '%s_interface' % iface + node_kwargs = {'uuid': uuidutils.generate_uuid(), + iface_name: 'fake'} + node = obj_utils.create_test_node(self.context, driver='fake', + **node_kwargs) + self.assertRaisesRegex( + exception.InvalidParameterValue, + 'driver fake.*%s' % iface_name, + driver_factory.check_and_update_node_interfaces, node) + class DefaultInterfaceTestCase(db_base.DbTestCase): def setUp(self): diff --git a/releasenotes/notes/nodes-classic-drivers-cannot-set-interfaces-620b37c4e5c88b80.yaml b/releasenotes/notes/nodes-classic-drivers-cannot-set-interfaces-620b37c4e5c88b80.yaml new file mode 100644 index 0000000000..29b2fe5262 --- /dev/null +++ b/releasenotes/notes/nodes-classic-drivers-cannot-set-interfaces-620b37c4e5c88b80.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Nodes with classic drivers cannot have any interfaces (except for network + and storage) specified. HTTP status 400 is returned in these cases.