From ea06393fe853d6b54ab1e2f6d5c1f39e8c61907c Mon Sep 17 00:00:00 2001 From: Mario Villaplana Date: Thu, 19 Nov 2015 22:38:58 +0000 Subject: [PATCH] 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 --- doc/source/dev/drivers.rst | 7 ++-- doc/source/dev/vendor-passthru.rst | 10 ++++- ironic/conductor/manager.py | 25 +++++++----- ironic/conductor/task_manager.py | 3 ++ ironic/drivers/base.py | 15 ++++++-- ironic/drivers/fake.py | 3 +- ironic/drivers/modules/fake.py | 8 +++- ironic/tests/unit/conductor/test_manager.py | 38 ++++++++++++++++++- ironic/tests/unit/drivers/test_base.py | 16 ++++++++ ...passthru-shared-lock-6a9e32952ee6c2fe.yaml | 5 +++ 10 files changed, 108 insertions(+), 22 deletions(-) create mode 100644 releasenotes/notes/vendor-passthru-shared-lock-6a9e32952ee6c2fe.yaml diff --git a/doc/source/dev/drivers.rst b/doc/source/dev/drivers.rst index 01216a44a5..d2f4aee412 100644 --- a/doc/source/dev/drivers.rst +++ b/doc/source/dev/drivers.rst @@ -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 diff --git a/doc/source/dev/vendor-passthru.rst b/doc/source/dev/vendor-passthru.rst index 0a2b16440d..90eb044040 100644 --- a/doc/source/dev/vendor-passthru.rst +++ b/doc/source/dev/vendor-passthru.rst @@ -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 diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 7671d52c7d..092b9e5446 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -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) diff --git a/ironic/conductor/task_manager.py b/ironic/conductor/task_manager.py index 639643ad83..328396f429 100644 --- a/ironic/conductor/task_manager.py +++ b/ironic/conductor/task_manager.py @@ -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 ' diff --git a/ironic/drivers/base.py b/ironic/drivers/base.py index 62a9cd1794..fa7024bdb2 100644 --- a/ironic/drivers/base.py +++ b/ironic/drivers/base.py @@ -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, diff --git a/ironic/drivers/fake.py b/ironic/drivers/fake.py index 8fcc9f65e1..88ccefd61a 100644 --- a/ironic/drivers/fake.py +++ b/ironic/drivers/fake.py @@ -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() diff --git a/ironic/drivers/modules/fake.py b/ironic/drivers/modules/fake.py index e2a85c7180..2497cb8fc9 100644 --- a/ironic/drivers/modules/fake.py +++ b/ironic/drivers/modules/fake.py @@ -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.""" diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index a05d27e9bf..6e01c23ad6 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -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() diff --git a/ironic/tests/unit/drivers/test_base.py b/ironic/tests/unit/drivers/test_base.py index 55d647ed02..23d8766b9e 100644 --- a/ironic/tests/unit/drivers/test_base.py +++ b/ironic/tests/unit/drivers/test_base.py @@ -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() diff --git a/releasenotes/notes/vendor-passthru-shared-lock-6a9e32952ee6c2fe.yaml b/releasenotes/notes/vendor-passthru-shared-lock-6a9e32952ee6c2fe.yaml new file mode 100644 index 0000000000..df23d21410 --- /dev/null +++ b/releasenotes/notes/vendor-passthru-shared-lock-6a9e32952ee6c2fe.yaml @@ -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.