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
This commit is contained in:
Ruby Loo
parent
2b6ff862c1
commit
d8bdbda030
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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.")
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
Loading…
Reference in New Issue