Move wait_for_baremetal_node_lock to the baremetal proxy

It is a quite low-level method that regular users should not use. This patch
deprecates it in favor of new baremetal.wait_for_node_reservation.

Change-Id: I071bdea02898cf911c2c018d014895c8a856524c

doc/source/user/proxies/baremetal.rst

@@ -24,6 +24,7 @@ Node Operations
    .. automethod:: openstack.baremetal.v1._proxy.Proxy.nodes
    .. automethod:: openstack.baremetal.v1._proxy.Proxy.set_node_provision_state
    .. automethod:: openstack.baremetal.v1._proxy.Proxy.wait_for_nodes_provision_state
+   .. automethod:: openstack.baremetal.v1._proxy.Proxy.wait_for_node_reservation
    .. automethod:: openstack.baremetal.v1._proxy.Proxy.validate_node
 
 Port Operations

openstack/baremetal/v1/_proxy.py

@@ -332,6 +332,32 @@ class Proxy(proxy.Proxy):
                           {'nodes': ', '.join(n.id for n in remaining),
                            'target': expected_state})
 
+    def wait_for_node_reservation(self, node, timeout=None):
+        """Wait for a lock on the node to be released.
+
+        Bare metal nodes in ironic have a reservation lock that
+        is used to represent that a conductor has locked the node
+        while performing some sort of action, such as changing
+        configuration as a result of a machine state change.
+
+        This lock can occur during power syncronization, and prevents
+        updates to objects attached to the node, such as ports.
+
+        Note that nothing prevents a conductor from acquiring the lock again
+        after this call returns, so it should be treated as best effort.
+
+        Returns immediately if there is no reservation on the node.
+
+        :param node: The value can be the name or ID of a node or a
+            :class:`~openstack.baremetal.v1.node.Node` instance.
+        :param timeout: How much (in seconds) to wait for the lock to be
+            released. The value of ``None`` (the default) means no timeout.
+
+        :returns: The updated :class:`~openstack.baremetal.v1.node.Node`
+        """
+        res = self._get_resource(_node.Node, node)
+        return res.wait_for_reservation(self, timeout=timeout)
+
     def validate_node(self, node, required=('boot', 'deploy', 'power')):
         """Validate required information on a node.
 

openstack/baremetal/v1/node.py

@@ -367,6 +367,44 @@ class Node(resource.Resource):
                           {'node': self.id, 'target': expected_state,
                            'state': self.provision_state})
 
+    def wait_for_reservation(self, session, timeout=None):
+        """Wait for a lock on the node to be released.
+
+        Bare metal nodes in ironic have a reservation lock that
+        is used to represent that a conductor has locked the node
+        while performing some sort of action, such as changing
+        configuration as a result of a machine state change.
+
+        This lock can occur during power syncronization, and prevents
+        updates to objects attached to the node, such as ports.
+
+        Note that nothing prevents a conductor from acquiring the lock again
+        after this call returns, so it should be treated as best effort.
+
+        Returns immediately if there is no reservation on the node.
+
+        :param session: The session to use for making this request.
+        :type session: :class:`~keystoneauth1.adapter.Adapter`
+        :param timeout: How much (in seconds) to wait for the lock to be
+            released. The value of ``None`` (the default) means no timeout.
+
+        :return: This :class:`Node` instance.
+        """
+        if self.reservation is None:
+            return self
+
+        for count in utils.iterate_timeout(
+                timeout,
+                "Timeout waiting for the lock to be released on node %s" %
+                self.id):
+            self.fetch(session)
+            if self.reservation is None:
+                return self
+
+            _logger.debug('Still waiting for the lock to be released on node '
+                          '%(node)s, currently locked by conductor %(host)s',
+                          {'node': self.id, 'host': self.reservation})
+
     def _check_state_reached(self, session, expected_state,
                              abort_on_failed_state=True):
         """Wait for the node to reach the expected state.

openstack/cloud/openstackcloud.py

@@ -10149,49 +10149,15 @@ class OpenStackCloud(_normalize.Normalizer):
     def wait_for_baremetal_node_lock(self, node, timeout=30):
         """Wait for a baremetal node to have no lock.
 
-        Baremetal nodes in ironic have a reservation lock that
-        is used to represent that a conductor has locked the node
-        while performing some sort of action, such as changing
-        configuration as a result of a machine state change.
-
-        This lock can occur during power syncronization, and prevents
-        updates to objects attached to the node, such as ports.
-
-        In the vast majority of cases, locks should clear in a few
-        seconds, and as such this method will only wait for 30 seconds.
-        The default wait is two seconds between checking if the lock
-        has cleared.
-
-        This method is intended for use by methods that need to
-        gracefully block without genreating errors, however this
-        method does prevent another client or a timer from
-        triggering a lock immediately after we see the lock as
-        having cleared.
-
-        :param node: The json representation of the node,
-                     specificially looking for the node
-                     'uuid' and 'reservation' fields.
-        :param timeout: Integer in seconds to wait for the
-                        lock to clear. Default: 30
+        DEPRECATED, use ``wait_for_node_reservation`` on the `baremetal` proxy.
 
         :raises: OpenStackCloudException upon client failure.
         :returns: None
         """
-        # TODO(TheJulia): This _can_ still fail with a race
-        # condition in that between us checking the status,
-        # a conductor where the conductor could still obtain
-        # a lock before we are able to obtain a lock.
-        # This means we should handle this with such conections
-
-        if node['reservation'] is None:
-            return
-        else:
-            msg = 'Waiting for lock to be released for node {node}'.format(
-                node=node['uuid'])
-            for count in utils.iterate_timeout(timeout, msg, 2):
-                current_node = self.get_machine(node['uuid'])
-                if current_node['reservation'] is None:
-                    return
+        warnings.warn("The wait_for_baremetal_node_lock call is deprecated "
+                      "in favor of wait_for_node_reservation on the baremetal "
+                      "proxy", DeprecationWarning)
+        self.baremetal.wait_for_node_reservation(node, timeout)
 
     @_utils.valid_kwargs('type', 'service_type', 'description')
     def create_service(self, name, enabled=True, **kwargs):

openstack/tests/unit/baremetal/v1/test_node.py

@@ -492,3 +492,42 @@ class TestNodeValidate(base.TestCase):
         # Reason can be empty
         self.assertFalse(result['boot'].result)
         self.assertIsNone(result['boot'].reason)
+
+
+@mock.patch('time.sleep', lambda _t: None)
+@mock.patch.object(node.Node, 'fetch', autospec=True)
+class TestNodeWaitForReservation(base.TestCase):
+
+    def setUp(self):
+        super(TestNodeWaitForReservation, self).setUp()
+        self.session = mock.Mock(spec=adapter.Adapter)
+        self.session.default_microversion = '1.6'
+        self.node = node.Node(**FAKE)
+
+    def test_no_reservation(self, mock_fetch):
+        self.node.reservation = None
+        node = self.node.wait_for_reservation(None)
+        self.assertIs(node, self.node)
+        self.assertFalse(mock_fetch.called)
+
+    def test_reservation(self, mock_fetch):
+        self.node.reservation = 'example.com'
+
+        def _side_effect(node, session):
+            if self.node.reservation == 'example.com':
+                self.node.reservation = 'example2.com'
+            else:
+                self.node.reservation = None
+
+        mock_fetch.side_effect = _side_effect
+        node = self.node.wait_for_reservation(self.session)
+        self.assertIs(node, self.node)
+        self.assertEqual(2, mock_fetch.call_count)
+
+    def test_timeout(self, mock_fetch):
+        self.node.reservation = 'example.com'
+
+        self.assertRaises(exceptions.ResourceTimeout,
+                          self.node.wait_for_reservation,
+                          self.session, timeout=0.001)
+        mock_fetch.assert_called_with(self.node, self.session)

openstack/tests/unit/cloud/test_baremetal_node.py

@@ -912,7 +912,8 @@ class TestBaremetalNode(base.IronicTestCase):
                 self.fake_baremetal_node,
                 timeout=1))
 
-        self.assertEqual(0, len(self.adapter.request_history))
+        # NOTE(dtantsur): service discovery apparently requires 3 calls
+        self.assertEqual(3, len(self.adapter.request_history))
 
     def test_wait_for_baremetal_node_lock_timeout(self):
         self.fake_baremetal_node['reservation'] = 'conductor0'

releasenotes/notes/baremetal-reservation-40327923092e9647.yaml

@@ -0,0 +1,10 @@
+---
+features:
+  - |
+    Added ``wait_for_node_reservation`` to the baremetal proxy.
+deprecations:
+  - |
+    The `OpenStackCloud` ``wait_for_baremetal_node_lock`` call is deprecated.
+    Generally, users should not have to call it. The new
+    ``wait_for_node_reservation`` from the baremetal proxy can be used when
+    needed.