Merge "Allow vendor drivers to acquire shared locks"

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
+* can require an exclusive lock on the node. This only occurs if the method
-requests for the node will be delayed and may fail with an HTTP 409
+  doesn't specify require_exclusive_lock=False in the decorator. If an
-(Conflict) error code.
+  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

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

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,
+        Synchronously validate driver specific info, and if successful invoke
-        and if successful invokes the vendor method. If the method mode
+        the vendor method. If the method mode is 'async' the conductor will
-        is 'async' the conductor will start background worker to perform
+        start background worker to perform vendor action.
-        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
+        # NOTE(mariojv): Not all vendor passthru methods require an exclusive
-        # require an exclusive lock, we need to do so to guarantee that the
+        # lock on a node, so we acquire a shared lock initially. If a method
-        # state doesn't unexpectedly change between doing a vendor.validate
+        # requires an exclusive lock, we'll acquire one after checking
-        # and vendor.vendor_passthru.
+        # vendor_opts before starting validation.
-        with task_manager.acquire(context, node_id, shared=False,
+        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)
 

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 '

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,

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

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."""

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

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

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.