diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 23e494158d..a6ab523e01 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/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): diff --git a/ironic/tests/unit/db/test_nodes.py b/ironic/tests/unit/db/test_nodes.py index 3dee718eb1..b10d367514 100644 --- a/ironic/tests/unit/db/test_nodes.py +++ b/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