openstack/ironic
Code Issues Proposed changes

Stop verifying updated driver in creating task

Browse Source 
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
This commit is contained in:
Hironori Shiina 2018-04-13 17:55:18 +09:00 committed by Dmitry Tantsur
parent 26694e0676
commit acdc372b5d
6 changed files with 34 additions and 83 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
ironic/common/driver_factory.py
View File

@ -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:

2
ironic/conductor/manager.py
View File

@ -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

14
ironic/conductor/task_manager.py
View File

@ -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():

15
ironic/tests/unit/conductor/test_manager.py
View File

@ -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):

73
ironic/tests/unit/conductor/test_task_manager.py
View File

@ -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,

7
releasenotes/notes/bug-2001832-62e244dc48c1f79e.yaml Normal file
View File

@ -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 <https://storyboard.openstack.org/#!/story/2001832>`_
for details.