From acdc372b5dd2abfeaf12ca657c3edbf6d0f5df3a Mon Sep 17 00:00:00 2001 From: Hironori Shiina Date: Fri, 13 Apr 2018 17:55:18 +0900 Subject: [PATCH] Stop verifying updated driver in creating task When a driver of a node is updated, the new driver is passed for creating a task. If a new driver is a hardware type, the new hardware type is validated with existing interfaces stored in database even if requesting to update interfaces together. This causes an error if the new hardware type doesn't support an existing interface. Now that a hardware type and interfaces are validated before creating a task for locking a node when the node is updated, it is not necessary to verify an updated driver in the task creation. Change-Id: I3445d6c33660be535babcf3e300ac4ba1b86ae4f Story: #2001832 Task: #12595 --- ironic/common/driver_factory.py | 6 +- ironic/conductor/manager.py | 2 - ironic/conductor/task_manager.py | 14 +--- ironic/tests/unit/conductor/test_manager.py | 15 ++++ .../tests/unit/conductor/test_task_manager.py | 73 ++----------------- .../notes/bug-2001832-62e244dc48c1f79e.yaml | 7 ++ 6 files changed, 34 insertions(+), 83 deletions(-) create mode 100644 releasenotes/notes/bug-2001832-62e244dc48c1f79e.yaml diff --git a/ironic/common/driver_factory.py b/ironic/common/driver_factory.py index 5c69c58748..2800c304b0 100644 --- a/ironic/common/driver_factory.py +++ b/ironic/common/driver_factory.py @@ -33,7 +33,7 @@ LOG = log.getLogger(__name__) EM_SEMAPHORE = 'extension_manager' -def build_driver_for_task(task, driver_name=None): +def build_driver_for_task(task): """Builds a composable driver for a given task. Starts with a `BareDriver` object, and attaches implementations of the @@ -42,8 +42,6 @@ def build_driver_for_task(task, driver_name=None): driver factories and are configurable via the database. :param task: The task containing the node to build a driver for. - :param driver_name: The name of the classic driver or hardware type to use - as a base, if different than task.node.driver. :returns: A driver object for the task. :raises: DriverNotFound if node.driver could not be found in either "ironic.drivers" or "ironic.hardware.types" namespaces. @@ -53,7 +51,7 @@ def build_driver_for_task(task, driver_name=None): the requested implementation is not compatible with it. """ node = task.node - driver_name = driver_name or node.driver + driver_name = node.driver driver_or_hw_type = get_driver_or_hardware_type(driver_name) try: diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 7e2c1273eb..8ad217dd71 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -189,9 +189,7 @@ class ConductorManager(base_manager.BaseConductorManager): 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, - driver_name=driver_name, purpose='node update') as task: # Prevent instance_uuid overwriting if ('instance_uuid' in delta and node_obj.instance_uuid and diff --git a/ironic/conductor/task_manager.py b/ironic/conductor/task_manager.py index f19a736170..19f7633d4f 100644 --- a/ironic/conductor/task_manager.py +++ b/ironic/conductor/task_manager.py @@ -149,23 +149,20 @@ def require_exclusive_lock(f): return wrapper -def acquire(context, node_id, shared=False, driver_name=None, - purpose='unspecified action'): +def acquire(context, node_id, shared=False, purpose='unspecified action'): """Shortcut for acquiring a lock on a Node. :param context: Request context. :param node_id: ID or UUID of node to lock. :param shared: Boolean indicating whether to take a shared or exclusive lock. Default: False. - :param driver_name: Name of Driver. Default: None. :param purpose: human-readable purpose to put to debug logs. :returns: An instance of :class:`TaskManager`. """ # NOTE(lintan): This is a workaround to set the context of periodic tasks. context.ensure_thread_contain_context() - return TaskManager(context, node_id, shared=shared, - driver_name=driver_name, purpose=purpose) + return TaskManager(context, node_id, shared=shared, purpose=purpose) class TaskManager(object): @@ -176,7 +173,7 @@ class TaskManager(object): """ - def __init__(self, context, node_id, shared=False, driver_name=None, + def __init__(self, context, node_id, shared=False, purpose='unspecified action'): """Create a new TaskManager. @@ -189,8 +186,6 @@ class TaskManager(object): :param node_id: ID or UUID of node to lock. :param shared: Boolean indicating whether to take a shared or exclusive lock. Default: False. - :param driver_name: The name of the driver to load, if different - from the Node's current driver. :param purpose: human-readable purpose to put to debug logs. :raises: DriverNotFound :raises: InterfaceNotFoundInEntrypoint @@ -236,8 +231,7 @@ class TaskManager(object): context, self.node.id) self.volume_targets = objects.VolumeTarget.list_by_node_id( context, self.node.id) - self.driver = driver_factory.build_driver_for_task( - self, driver_name=driver_name) + self.driver = driver_factory.build_driver_for_task(self) except Exception: with excutils.save_and_reraise_exception(): diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 06662cf4a2..ca2a322c84 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -706,6 +706,21 @@ class UpdateNodeTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): states.ACTIVE, resource_class='old', new_resource_class=None, expect_error=True) + def test_update_node_hardware_type(self): + existing_hardware = 'fake-hardware' + existing_interface = 'fake' + new_hardware = 'manual-management' + new_interface = 'pxe' + node = obj_utils.create_test_node(self.context, + driver=existing_hardware, + boot_interface=existing_interface) + node.driver = new_hardware + node.boot_interface = new_interface + self.service.update_node(self.context, node) + node.refresh() + self.assertEqual(new_hardware, node.driver) + self.assertEqual(new_interface, node.boot_interface) + @mgr_utils.mock_record_keepalive class VendorPassthruTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): diff --git a/ironic/tests/unit/conductor/test_task_manager.py b/ironic/tests/unit/conductor/test_task_manager.py index 732db381ac..ced54195bd 100644 --- a/ironic/tests/unit/conductor/test_task_manager.py +++ b/ironic/tests/unit/conductor/test_task_manager.py @@ -67,36 +67,7 @@ class TaskManagerTestCase(db_base.DbTestCase): self.assertEqual(get_voltgt_mock.return_value, task.volume_targets) self.assertEqual(build_driver_mock.return_value, task.driver) self.assertFalse(task.shared) - build_driver_mock.assert_called_once_with(task, driver_name=None) - - node_get_mock.assert_called_once_with(self.context, 'fake-node-id') - reserve_mock.assert_called_once_with(self.context, self.host, - 'fake-node-id') - get_ports_mock.assert_called_once_with(self.context, self.node.id) - get_portgroups_mock.assert_called_once_with(self.context, self.node.id) - get_volconn_mock.assert_called_once_with(self.context, self.node.id) - get_voltgt_mock.assert_called_once_with(self.context, self.node.id) - release_mock.assert_called_once_with(self.context, self.host, - self.node.id) - - def test_excl_lock_with_driver( - self, get_voltgt_mock, get_volconn_mock, get_portgroups_mock, - get_ports_mock, build_driver_mock, - reserve_mock, release_mock, node_get_mock): - reserve_mock.return_value = self.node - with task_manager.TaskManager(self.context, 'fake-node-id', - driver_name='fake-driver') as task: - self.assertEqual(self.context, task.context) - self.assertEqual(self.node, task.node) - self.assertEqual(get_ports_mock.return_value, task.ports) - self.assertEqual(get_portgroups_mock.return_value, task.portgroups) - self.assertEqual(get_volconn_mock.return_value, - task.volume_connectors) - self.assertEqual(get_voltgt_mock.return_value, task.volume_targets) - self.assertEqual(build_driver_mock.return_value, task.driver) - self.assertFalse(task.shared) - build_driver_mock.assert_called_once_with( - task, driver_name='fake-driver') + build_driver_mock.assert_called_once_with(task) node_get_mock.assert_called_once_with(self.context, 'fake-node-id') reserve_mock.assert_called_once_with(self.context, self.host, @@ -150,8 +121,7 @@ class TaskManagerTestCase(db_base.DbTestCase): self.assertEqual(mock.sentinel.driver2, task2.driver) self.assertFalse(task2.shared) - self.assertEqual([mock.call(task, driver_name=None), - mock.call(task2, driver_name=None)], + self.assertEqual([mock.call(task), mock.call(task2)], build_driver_mock.call_args_list) self.assertEqual([mock.call(self.context, 'node-id1'), @@ -312,7 +282,7 @@ class TaskManagerTestCase(db_base.DbTestCase): 'fake-node-id') get_ports_mock.assert_called_once_with(self.context, self.node.id) get_portgroups_mock.assert_called_once_with(self.context, self.node.id) - build_driver_mock.assert_called_once_with(mock.ANY, driver_name=None) + build_driver_mock.assert_called_once_with(mock.ANY) release_mock.assert_called_once_with(self.context, self.host, self.node.id) @@ -333,37 +303,7 @@ class TaskManagerTestCase(db_base.DbTestCase): self.assertEqual(build_driver_mock.return_value, task.driver) self.assertTrue(task.shared) - build_driver_mock.assert_called_once_with(task, driver_name=None) - - self.assertFalse(reserve_mock.called) - self.assertFalse(release_mock.called) - node_get_mock.assert_called_once_with(self.context, 'fake-node-id') - get_ports_mock.assert_called_once_with(self.context, self.node.id) - get_portgroups_mock.assert_called_once_with(self.context, self.node.id) - get_volconn_mock.assert_called_once_with(self.context, self.node.id) - get_voltgt_mock.assert_called_once_with(self.context, self.node.id) - - def test_shared_lock_with_driver( - self, get_voltgt_mock, get_volconn_mock, get_portgroups_mock, - get_ports_mock, build_driver_mock, - reserve_mock, release_mock, node_get_mock): - node_get_mock.return_value = self.node - with task_manager.TaskManager(self.context, - 'fake-node-id', - shared=True, - driver_name='fake-driver') as task: - self.assertEqual(self.context, task.context) - self.assertEqual(self.node, task.node) - self.assertEqual(get_ports_mock.return_value, task.ports) - self.assertEqual(get_portgroups_mock.return_value, task.portgroups) - self.assertEqual(get_volconn_mock.return_value, - task.volume_connectors) - self.assertEqual(get_voltgt_mock.return_value, task.volume_targets) - self.assertEqual(build_driver_mock.return_value, task.driver) - self.assertTrue(task.shared) - - build_driver_mock.assert_called_once_with( - task, driver_name='fake-driver') + build_driver_mock.assert_called_once_with(task) self.assertFalse(reserve_mock.called) self.assertFalse(release_mock.called) @@ -491,7 +431,7 @@ class TaskManagerTestCase(db_base.DbTestCase): get_portgroups_mock.assert_called_once_with(self.context, self.node.id) get_volconn_mock.assert_called_once_with(self.context, self.node.id) get_voltgt_mock.assert_called_once_with(self.context, self.node.id) - build_driver_mock.assert_called_once_with(mock.ANY, driver_name=None) + build_driver_mock.assert_called_once_with(mock.ANY) def test_upgrade_lock( self, get_voltgt_mock, get_volconn_mock, get_portgroups_mock, @@ -520,8 +460,7 @@ class TaskManagerTestCase(db_base.DbTestCase): self.assertFalse(task.shared) self.assertEqual('spam', task._purpose) - build_driver_mock.assert_called_once_with(mock.ANY, - driver_name=None) + build_driver_mock.assert_called_once_with(mock.ANY) # make sure reserve() was called only once reserve_mock.assert_called_once_with(self.context, self.host, diff --git a/releasenotes/notes/bug-2001832-62e244dc48c1f79e.yaml b/releasenotes/notes/bug-2001832-62e244dc48c1f79e.yaml new file mode 100644 index 0000000000..7f5eb75599 --- /dev/null +++ b/releasenotes/notes/bug-2001832-62e244dc48c1f79e.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes a bug where a node's hardware type cannot be changed to another + hardware type which doesn't support any hardware interface currently used. + See `bug 2001832 `_ + for details.