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.