Allow vendor drivers to acquire shared locks
Previously, all node vendor passthru methods required an exclusive lock on the node to validate and start the task. This allows node vendor passthru methods to specify require_exclusive_lock=False in their passthru decorators to let the conductor acquire a shared lock on a node. Change-Id: I43cf43bc5c17f44a735e16c3c5cb744cf6911d27 Closes-Bug: #1481665
This commit is contained in:
Mario Villaplana
parent
6cd2f21ab0
commit
ea06393fe8
|
|
@ -53,9 +53,10 @@ A method:
|
|||
+ For synchronous methods, a 200 (OK) HTTP status code is returned to
|
||||
indicate that the request was fulfilled. The response may include a body.
|
||||
|
||||
While performing the request, a lock is held on the node, and other
|
||||
requests for the node will be delayed and may fail with an HTTP 409
|
||||
(Conflict) error code.
|
||||
* can require an exclusive lock on the node. This only occurs if the method
|
||||
doesn't specify require_exclusive_lock=False in the decorator. If an
|
||||
exclusive lock is held on the node, other requests for the node will be
|
||||
delayed and may fail with an HTTP 409 (Conflict) error code.
|
||||
|
||||
This endpoint exposes a node's driver directly, and as such, it is
|
||||
expressly not part of Ironic's standard REST API. There is only a
|
||||
|
|
|
|||
|
|
@ -95,7 +95,7 @@ parameter of the method (ignoring self). A method decorated with the
|
|||
a method decorated with the `@driver_passthru` decorator should expect
|
||||
a Context object as first parameter.
|
||||
|
||||
Both decorators accepts the same parameters:
|
||||
Both decorators accept these parameters:
|
||||
|
||||
* http_methods: A list of what the HTTP methods supported by that vendor
|
||||
function. To know what HTTP method that function was invoked with, a
|
||||
|
|
@ -120,6 +120,14 @@ Both decorators accepts the same parameters:
|
|||
* async: A boolean value to determine whether this method should run
|
||||
asynchronously or synchronously. Defaults to True (Asynchronously).
|
||||
|
||||
The node vendor passthru decorator (`@passthru`) also accepts the following
|
||||
parameter:
|
||||
|
||||
* require_exclusive_lock: A boolean value determining whether this method
|
||||
should require an exclusive lock on a node between validate() and the
|
||||
beginning of method execution. For synchronous methods, the lock on the node
|
||||
would also be kept for the duration of method execution. Defaults to True.
|
||||
|
||||
.. WARNING::
|
||||
Please avoid having a synchronous method for slow/long-running
|
||||
operations **or** if the method does talk to a BMC; BMCs are flaky
|
||||
|
|
|
|||
|
|
@ -279,10 +279,9 @@ class ConductorManager(base_manager.BaseConductorManager):
|
|||
http_method, info):
|
||||
"""RPC method to encapsulate vendor action.
|
||||
|
||||
Synchronously validate driver specific info or get driver status,
|
||||
and if successful invokes the vendor method. If the method mode
|
||||
is 'async' the conductor will start background worker to perform
|
||||
vendor action.
|
||||
Synchronously validate driver specific info, and if successful invoke
|
||||
the vendor method. If the method mode is 'async' the conductor will
|
||||
start background worker to perform vendor action.
|
||||
|
||||
:param context: an admin context.
|
||||
:param node_id: the id or uuid of a node.
|
||||
|
|
@ -295,7 +294,8 @@ class ConductorManager(base_manager.BaseConductorManager):
|
|||
vendor interface or method is unsupported.
|
||||
:raises: NoFreeConductorWorker when there is no free worker to start
|
||||
async task.
|
||||
:raises: NodeLocked if node is locked by another conductor.
|
||||
:raises: NodeLocked if the vendor passthru method requires an exclusive
|
||||
lock but the node is locked by another conductor
|
||||
:returns: A dictionary containing:
|
||||
|
||||
:return: The response of the invoked vendor method
|
||||
|
|
@ -308,11 +308,11 @@ class ConductorManager(base_manager.BaseConductorManager):
|
|||
|
||||
"""
|
||||
LOG.debug("RPC vendor_passthru called for node %s." % node_id)
|
||||
# NOTE(max_lobur): Even though not all vendor_passthru calls may
|
||||
# require an exclusive lock, we need to do so to guarantee that the
|
||||
# state doesn't unexpectedly change between doing a vendor.validate
|
||||
# and vendor.vendor_passthru.
|
||||
with task_manager.acquire(context, node_id, shared=False,
|
||||
# NOTE(mariojv): Not all vendor passthru methods require an exclusive
|
||||
# lock on a node, so we acquire a shared lock initially. If a method
|
||||
# requires an exclusive lock, we'll acquire one after checking
|
||||
# vendor_opts before starting validation.
|
||||
with task_manager.acquire(context, node_id, shared=True,
|
||||
purpose='calling vendor passthru') as task:
|
||||
if not getattr(task.driver, 'vendor', None):
|
||||
raise exception.UnsupportedDriverExtension(
|
||||
|
|
@ -334,6 +334,11 @@ class ConductorManager(base_manager.BaseConductorManager):
|
|||
_('The method %(method)s does not support HTTP %(http)s') %
|
||||
{'method': driver_method, 'http': http_method})
|
||||
|
||||
# Change shared lock to exclusive if a vendor method requires
|
||||
# it. Vendor methods default to requiring an exclusive lock.
|
||||
if vendor_opts['require_exclusive_lock']:
|
||||
task.upgrade_lock()
|
||||
|
||||
vendor_iface.validate(task, method=driver_method,
|
||||
http_method=http_method, **info)
|
||||
|
||||
|
|
|
|||
|
|
@ -258,6 +258,9 @@ class TaskManager(object):
|
|||
|
||||
Also reloads node object from the database.
|
||||
Does nothing if lock is already exclusive.
|
||||
|
||||
:raises: NodeLocked if an exclusive lock remains on the node after
|
||||
"node_locked_retry_attempts"
|
||||
"""
|
||||
if self.shared:
|
||||
LOG.debug('Upgrading shared lock on node %(uuid)s for %(purpose)s '
|
||||
|
|
|
|||
|
|
@ -611,7 +611,7 @@ VendorMetadata = collections.namedtuple('VendorMetadata', ['method',
|
|||
|
||||
|
||||
def _passthru(http_methods, method=None, async=True, driver_passthru=False,
|
||||
description=None, attach=False):
|
||||
description=None, attach=False, require_exclusive_lock=True):
|
||||
"""A decorator for registering a function as a passthru function.
|
||||
|
||||
Decorator ensures function is ready to catch any ironic exceptions
|
||||
|
|
@ -637,7 +637,12 @@ def _passthru(http_methods, method=None, async=True, driver_passthru=False,
|
|||
value should be returned in the response body.
|
||||
Defaults to False.
|
||||
:param description: a string shortly describing what the method does.
|
||||
|
||||
:param require_exclusive_lock: Boolean value. Only valid for node passthru
|
||||
methods. If True, lock the node before
|
||||
validate() and invoking the vendor method.
|
||||
The node remains locked during execution
|
||||
for a synchronous passthru method. If False,
|
||||
don't lock the node. Defaults to True.
|
||||
"""
|
||||
def handle_passthru(func):
|
||||
api_method = method
|
||||
|
|
@ -653,6 +658,7 @@ def _passthru(http_methods, method=None, async=True, driver_passthru=False,
|
|||
if driver_passthru:
|
||||
func._driver_metadata = metadata
|
||||
else:
|
||||
metadata[1]['require_exclusive_lock'] = require_exclusive_lock
|
||||
func._vendor_metadata = metadata
|
||||
|
||||
passthru_logmessage = _LE('vendor_passthru failed with method %s')
|
||||
|
|
@ -673,9 +679,10 @@ def _passthru(http_methods, method=None, async=True, driver_passthru=False,
|
|||
|
||||
|
||||
def passthru(http_methods, method=None, async=True, description=None,
|
||||
attach=False):
|
||||
attach=False, require_exclusive_lock=True):
|
||||
return _passthru(http_methods, method, async, driver_passthru=False,
|
||||
description=description, attach=attach)
|
||||
description=description, attach=attach,
|
||||
require_exclusive_lock=require_exclusive_lock)
|
||||
|
||||
|
||||
def driver_passthru(http_methods, method=None, async=True, description=None,
|
||||
|
|
|
|||
|
|
@ -70,7 +70,8 @@ class FakeDriver(base.BaseDriver):
|
|||
self.b = fake.FakeVendorB()
|
||||
self.mapping = {'first_method': self.a,
|
||||
'second_method': self.b,
|
||||
'third_method_sync': self.b}
|
||||
'third_method_sync': self.b,
|
||||
'fourth_method_shared_lock': self.b}
|
||||
self.vendor = utils.MixinVendorInterface(self.mapping)
|
||||
self.console = fake.FakeConsole()
|
||||
self.management = fake.FakeManagement()
|
||||
|
|
|
|||
|
|
@ -133,7 +133,8 @@ class FakeVendorB(base.VendorInterface):
|
|||
'B2': 'B2 description. Required.'}
|
||||
|
||||
def validate(self, task, method, **kwargs):
|
||||
if method in ('second_method', 'third_method_sync'):
|
||||
if method in ('second_method', 'third_method_sync',
|
||||
'fourth_method_shared_lock'):
|
||||
bar = kwargs.get('bar')
|
||||
if not bar:
|
||||
raise exception.MissingParameterValue(_(
|
||||
|
|
@ -149,6 +150,11 @@ class FakeVendorB(base.VendorInterface):
|
|||
def third_method_sync(self, task, http_method, bar):
|
||||
return True if bar == 'meow' else False
|
||||
|
||||
@base.passthru(['POST'], require_exclusive_lock=False,
|
||||
description=_("Test if the value of bar is woof"))
|
||||
def fourth_method_shared_lock(self, task, http_method, bar):
|
||||
return True if bar == 'woof' else False
|
||||
|
||||
|
||||
class FakeConsole(base.ConsoleInterface):
|
||||
"""Example implementation of a simple console interface."""
|
||||
|
|
|
|||
|
|
@ -290,8 +290,9 @@ class UpdateNodeTestCase(mgr_utils.ServiceSetUpMixin,
|
|||
class VendorPassthruTestCase(mgr_utils.ServiceSetUpMixin,
|
||||
tests_db_base.DbTestCase):
|
||||
|
||||
@mock.patch.object(task_manager.TaskManager, 'upgrade_lock')
|
||||
@mock.patch.object(task_manager.TaskManager, 'spawn_after')
|
||||
def test_vendor_passthru_async(self, mock_spawn):
|
||||
def test_vendor_passthru_async(self, mock_spawn, mock_upgrade):
|
||||
node = obj_utils.create_test_node(self.context, driver='fake')
|
||||
info = {'bar': 'baz'}
|
||||
self._start_service()
|
||||
|
|
@ -307,13 +308,17 @@ class VendorPassthruTestCase(mgr_utils.ServiceSetUpMixin,
|
|||
self.assertIsNone(response['return'])
|
||||
self.assertTrue(response['async'])
|
||||
|
||||
# Assert lock was upgraded to an exclusive one
|
||||
self.assertEqual(1, mock_upgrade.call_count)
|
||||
|
||||
node.refresh()
|
||||
self.assertIsNone(node.last_error)
|
||||
# Verify reservation has been cleared.
|
||||
self.assertIsNone(node.reservation)
|
||||
|
||||
@mock.patch.object(task_manager.TaskManager, 'upgrade_lock')
|
||||
@mock.patch.object(task_manager.TaskManager, 'spawn_after')
|
||||
def test_vendor_passthru_sync(self, mock_spawn):
|
||||
def test_vendor_passthru_sync(self, mock_spawn, mock_upgrade):
|
||||
node = obj_utils.create_test_node(self.context, driver='fake')
|
||||
info = {'bar': 'meow'}
|
||||
self._start_service()
|
||||
|
|
@ -329,11 +334,40 @@ class VendorPassthruTestCase(mgr_utils.ServiceSetUpMixin,
|
|||
self.assertTrue(response['return'])
|
||||
self.assertFalse(response['async'])
|
||||
|
||||
# Assert lock was upgraded to an exclusive one
|
||||
self.assertEqual(1, mock_upgrade.call_count)
|
||||
|
||||
node.refresh()
|
||||
self.assertIsNone(node.last_error)
|
||||
# Verify reservation has been cleared.
|
||||
self.assertIsNone(node.reservation)
|
||||
|
||||
@mock.patch.object(task_manager.TaskManager, 'upgrade_lock')
|
||||
@mock.patch.object(task_manager.TaskManager, 'spawn_after')
|
||||
def test_vendor_passthru_shared_lock(self, mock_spawn, mock_upgrade):
|
||||
node = obj_utils.create_test_node(self.context, driver='fake')
|
||||
info = {'bar': 'woof'}
|
||||
self._start_service()
|
||||
|
||||
response = self.service.vendor_passthru(self.context, node.uuid,
|
||||
'fourth_method_shared_lock',
|
||||
'POST', info)
|
||||
# Waiting to make sure the below assertions are valid.
|
||||
self._stop_service()
|
||||
|
||||
# Assert spawn_after was called
|
||||
self.assertTrue(mock_spawn.called)
|
||||
self.assertIsNone(response['return'])
|
||||
self.assertTrue(response['async'])
|
||||
|
||||
# Assert lock was never upgraded to an exclusive one
|
||||
self.assertFalse(mock_upgrade.called)
|
||||
|
||||
node.refresh()
|
||||
self.assertIsNone(node.last_error)
|
||||
# Verify there's no reservation on the node
|
||||
self.assertIsNone(node.reservation)
|
||||
|
||||
def test_vendor_passthru_http_method_not_supported(self):
|
||||
node = obj_utils.create_test_node(self.context, driver='fake')
|
||||
self._start_service()
|
||||
|
|
|
|||
|
|
@ -44,6 +44,10 @@ class FakeVendorInterface(driver_base.VendorInterface):
|
|||
def normalexception(self):
|
||||
raise Exception("Fake!")
|
||||
|
||||
@driver_base.passthru(['POST'], require_exclusive_lock=False)
|
||||
def shared_task(self):
|
||||
return "shared fake"
|
||||
|
||||
def validate(self, task, **kwargs):
|
||||
pass
|
||||
|
||||
|
|
@ -75,6 +79,18 @@ class PassthruDecoratorTestCase(base.TestCase):
|
|||
mock_log.exception.assert_called_with(
|
||||
mock.ANY, 'normalexception')
|
||||
|
||||
def test_passthru_shared_task_metadata(self):
|
||||
self.assertIn('require_exclusive_lock',
|
||||
self.fvi.shared_task._vendor_metadata[1])
|
||||
self.assertFalse(
|
||||
self.fvi.shared_task._vendor_metadata[1]['require_exclusive_lock'])
|
||||
|
||||
def test_passthru_exclusive_task_metadata(self):
|
||||
self.assertIn('require_exclusive_lock',
|
||||
self.fvi.noexception._vendor_metadata[1])
|
||||
self.assertTrue(
|
||||
self.fvi.noexception._vendor_metadata[1]['require_exclusive_lock'])
|
||||
|
||||
def test_passthru_check_func_references(self):
|
||||
inst1 = FakeVendorInterface()
|
||||
inst2 = FakeVendorInterface()
|
||||
|
|
|
|||
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
features:
|
||||
- Adds the ability for node vendor passthru methods to use shared locks.
|
||||
Default behavior of always acquiring an exclusive lock for node vendor
|
||||
passthru methods is unchanged.
|
||||
Loading…
Reference in New Issue