Merge "DB: Only re-query for a lock holder if we cannot lock"

ironic/db/sqlalchemy/api.py

@@ -707,14 +707,17 @@ class Connection(api.Connection):
                 values(reservation=tag).
                 execution_options(synchronize_session=False))
             session.flush()
-        node = self._get_node_reservation(node.id)
         # NOTE(TheJulia): In SQLAlchemy 2.0 style, we don't
         # magically get a changed node as they moved from the
         # many ways to do things to singular ways to do things.
         if res.rowcount != 1:
             # Nothing updated and node exists. Must already be
-            # locked.
-            raise exception.NodeLocked(node=node.uuid, host=node.reservation)
+            # locked. Identify who holds it and log.
+            if utils.is_ironic_using_sqlite():
+                lock_holder = CONF.hostname
+            else:
+                lock_holder = self._get_node_reservation(node.id).reservation
+            raise exception.NodeLocked(node=node.uuid, host=lock_holder)
 
     @oslo_db_api.retry_on_deadlock
     def reserve_node(self, tag, node_id):

ironic/tests/unit/db/test_nodes.py

@@ -15,6 +15,7 @@
 
 """Tests for manipulating Nodes via the DB API"""
 
+import copy
 import datetime
 from unittest import mock
 
@@ -29,6 +30,7 @@ from sqlalchemy.orm import exc as sa_orm_exc
 
 from ironic.common import exception
 from ironic.common import states
+from ironic.common import utils as common_utils
 from ironic.db.sqlalchemy import api as dbapi
 from ironic.db.sqlalchemy.api import Connection as db_conn
 from ironic.db.sqlalchemy.models import NodeInventory
@@ -1037,6 +1039,39 @@ class DbNodeTestCase(base.DbTestCase):
         res = self.dbapi.get_node_by_uuid(uuid)
         self.assertEqual(r1, res.reservation)
 
+    def test_reserve_node_reads_reservation_once_sqlite(self):
+        node = utils.create_test_node()
+        uuid = node.uuid
+
+        r1 = 'fake-reservation'
+
+        with mock.patch.object(db_conn, '_get_node_reservation',
+                               autospec=True) as mock_get_res:
+            mock_get_res.return_value = node
+            self.dbapi.reserve_node(r1, uuid)
+            mock_get_res.assert_called_once_with(mock.ANY, node.uuid)
+
+    @mock.patch.object(common_utils, 'is_ironic_using_sqlite', autospec=True)
+    def test_reserve_node_reads_reservation_twice(self, is_sqlite_mock):
+        # Ensure we re-query for who holds the reservation *when* lock fails
+        # to trigger.
+        node = utils.create_test_node()
+        uuid = node.uuid
+        is_sqlite_mock.return_value = False
+        r1 = 'fake-reservation'
+        self.dbapi.update_node(node.id, {'reservation': r1})
+        locked_node = copy.copy(node)
+        locked_node.reservation = r1
+        with mock.patch.object(db_conn, '_get_node_reservation',
+                               autospec=True) as mock_get_res:
+            mock_get_res.side_effect = [node, locked_node]
+            self.assertRaisesRegex(exception.NodeLocked,
+                                   'locked by host fake-reservation',
+                                   self.dbapi.reserve_node, r1, uuid)
+            mock_get_res.assert_has_calls([
+                mock.call(mock.ANY, node.uuid),
+                mock.call(mock.ANY, node.id)])
+
     def test_release_reservation(self):
         node = utils.create_test_node()
         uuid = node.uuid