Skip to content

Commit

Permalink
Rename NodeInfo._lock to avoid conflict with Mock._lock in tests
Browse files Browse the repository at this point in the history
Starting with python/cpython#98797, Python's
Mock has its own _lock. I hope they rename it to something really
private (e.g. __lock), but for now rename our attribute (and hope that
no downstream plugins relied on it, sigh).

Change-Id: I7ba858fb3f259b8e7a3becde94b7ba6b90615287
  • Loading branch information
dtantsur committed Jan 10, 2023
1 parent c0f9602 commit cb1e856
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 27 deletions.
16 changes: 8 additions & 8 deletions ironic_inspector/node_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,16 @@ def __init__(self, uuid, version_id=None, state=None, started_at=None,
# equivalent to True actually.
self._manage_boot = manage_boot if manage_boot is not None else True
# This is a lock on a node UUID, not on a NodeInfo object
self._lock = locking.get_lock(uuid)
self._node_lock = locking.get_lock(uuid)
# Whether lock was acquired using this NodeInfo object
self._fsm = None
self._options = None

def __del__(self):
if self._lock.is_locked():
if self._node_lock.is_locked():
LOG.warning('BUG: node lock was not released by the moment '
'node info object is deleted')
self._lock.release()
self._node_lock.release()

def __str__(self):
"""Self represented as an UUID and a state."""
Expand All @@ -103,13 +103,13 @@ def acquire_lock(self, blocking=True):
return immediately.
:returns: boolean value, whether lock was acquired successfully
"""
if self._lock.is_locked():
if self._node_lock.is_locked():
LOG.debug('Attempting to acquire lock already held',
node_info=self)
return True

LOG.debug('Attempting to acquire lock', node_info=self)
if self._lock.acquire(blocking):
if self._node_lock.acquire(blocking):
LOG.debug('Successfully acquired lock', node_info=self)
return True
else:
Expand All @@ -121,9 +121,9 @@ def release_lock(self):
Does nothing if lock was not acquired using this NodeInfo object.
"""
if self._lock.is_locked():
if self._node_lock.is_locked():
LOG.debug('Successfully released lock', node_info=self)
self._lock.release()
self._node_lock.release()

@property
def version_id(self):
Expand Down Expand Up @@ -640,7 +640,7 @@ def inner(node_info, *args, **kwargs):
finally:
# FIXME(milan) hacking the test cases to work
# with release_lock.assert_called_once...
if node_info._lock.is_locked():
if node_info._node_lock.is_locked():
node_info.release_lock()
return inner

Expand Down
1 change: 1 addition & 0 deletions ironic_inspector/test/unit/test_introspect.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def setUp(self):
self.node_info = mock.Mock(uuid=self.uuid, options={})
self.node_info.ports.return_value = self.ports_dict
self.node_info.node.return_value = self.node

driver_fixture = self.useFixture(fixtures.MockPatchObject(
pxe_filter, 'driver', autospec=True))
driver_mock = driver_fixture.mock.return_value
Expand Down
38 changes: 19 additions & 19 deletions ironic_inspector/test/unit/test_node_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def test_add_node(self):
(datetime.datetime.utcnow() - datetime.timedelta(seconds=60)
< node.started_at <
datetime.datetime.utcnow() + datetime.timedelta(seconds=60)))
self.assertFalse(node._lock.is_locked())
self.assertFalse(node._node_lock.is_locked())

res = set((r.uuid, r.started_at) for r in db.get_nodes())

Expand Down Expand Up @@ -196,7 +196,7 @@ def test_bmc(self):
datetime.datetime.utcnow() - datetime.timedelta(seconds=60)
< res.started_at <
datetime.datetime.utcnow() + datetime.timedelta(seconds=1))
self.assertTrue(res._lock.is_locked())
self.assertTrue(res._node_lock.is_locked())

def test_same_bmc_different_macs(self):
uuid2 = uuidutils.generate_uuid()
Expand Down Expand Up @@ -227,7 +227,7 @@ def test_macs(self):
datetime.datetime.utcnow() - datetime.timedelta(seconds=60)
< res.started_at <
datetime.datetime.utcnow() + datetime.timedelta(seconds=1))
self.assertTrue(res._lock.is_locked())
self.assertTrue(res._node_lock.is_locked())

def test_macs_not_found(self):
self.assertRaises(utils.Error, node_cache.find_node,
Expand All @@ -250,7 +250,7 @@ def test_both(self):
datetime.datetime.utcnow() - datetime.timedelta(seconds=60)
< res.started_at <
datetime.datetime.utcnow() + datetime.timedelta(seconds=1))
self.assertTrue(res._lock.is_locked())
self.assertTrue(res._node_lock.is_locked())

def test_inconsistency(self):
db.delete_node(uuid=self.uuid)
Expand Down Expand Up @@ -388,7 +388,7 @@ def test_ok(self):
self.assertEqual(started_at, info.started_at)
self.assertIsNone(info.finished_at)
self.assertIsNone(info.error)
self.assertFalse(info._lock.is_locked())
self.assertFalse(info._node_lock.is_locked())

def test_not_found(self):
self.assertRaises(utils.Error, node_cache.get_node,
Expand All @@ -410,7 +410,7 @@ def test_with_name(self):
self.assertEqual(started_at, info.started_at)
self.assertIsNone(info.finished_at)
self.assertIsNone(info.error)
self.assertFalse(info._lock.is_locked())
self.assertFalse(info._node_lock.is_locked())
ironic.get_node.assert_called_once_with('name')


Expand Down Expand Up @@ -448,7 +448,7 @@ def test_error(self):
def test_release_lock(self):
self.node_info.acquire_lock()
self.node_info.finished(istate.Events.finish)
self.assertFalse(self.node_info._lock.is_locked())
self.assertFalse(self.node_info._node_lock.is_locked())


class TestNodeInfoOptions(test_base.NodeTestBase):
Expand Down Expand Up @@ -812,40 +812,40 @@ class TestInternalLock(test_base.NodeTestBase):
def test_acquire(self, lock_mock):
node_info = node_cache.NodeInfo(self.uuid)
self.addCleanup(node_info.release_lock)
self.assertFalse(node_info._lock.is_locked())
self.assertFalse(node_info._node_lock.is_locked())
lock_mock.assert_called_once_with('node-{}'.format(self.uuid),
semaphores=mock.ANY)
self.assertFalse(lock_mock.return_value.acquire.called)

self.assertTrue(node_info.acquire_lock())
self.assertTrue(node_info._lock.is_locked())
self.assertTrue(node_info._node_lock.is_locked())
self.assertTrue(node_info.acquire_lock())
self.assertTrue(node_info._lock.is_locked())
self.assertTrue(node_info._node_lock.is_locked())
lock_mock.return_value.acquire.assert_called_once_with(blocking=True)

def test_release(self, lock_mock):
node_info = node_cache.NodeInfo(self.uuid)
node_info.acquire_lock()
self.assertTrue(node_info._lock.is_locked())
self.assertTrue(node_info._node_lock.is_locked())
node_info.release_lock()
self.assertFalse(node_info._lock.is_locked())
self.assertFalse(node_info._node_lock.is_locked())
node_info.release_lock()
self.assertFalse(node_info._lock.is_locked())
self.assertFalse(node_info._node_lock.is_locked())
lock_mock.return_value.acquire.assert_called_once_with(blocking=True)
lock_mock.return_value.release.assert_called_once_with()

def test_acquire_non_blocking(self, lock_mock):
node_info = node_cache.NodeInfo(self.uuid)
self.addCleanup(node_info.release_lock)
self.assertFalse(node_info._lock.is_locked())
self.assertFalse(node_info._node_lock.is_locked())
lock_mock.return_value.acquire.side_effect = iter([False, True])

self.assertFalse(node_info.acquire_lock(blocking=False))
self.assertFalse(node_info._lock.is_locked())
self.assertFalse(node_info._node_lock.is_locked())
self.assertTrue(node_info.acquire_lock(blocking=False))
self.assertTrue(node_info._lock.is_locked())
self.assertTrue(node_info._node_lock.is_locked())
self.assertTrue(node_info.acquire_lock(blocking=False))
self.assertTrue(node_info._lock.is_locked())
self.assertTrue(node_info._node_lock.is_locked())
lock_mock.return_value.acquire.assert_called_with(blocking=False)
self.assertEqual(2, lock_mock.return_value.acquire.call_count)

Expand Down Expand Up @@ -1123,7 +1123,7 @@ def function(node_info):
def test_unlock(self):
@node_cache.release_lock
def func(node_info):
self.assertTrue(node_info._lock.is_locked())
self.assertTrue(node_info._node_lock.is_locked())

self.node_info.acquire_lock(blocking=True)
with mock.patch.object(self.node_info, 'release_lock',
Expand All @@ -1134,7 +1134,7 @@ def func(node_info):
def test_unlock_unlocked(self):
@node_cache.release_lock
def func(node_info):
self.assertFalse(node_info._lock.is_locked())
self.assertFalse(node_info._node_lock.is_locked())

self.node_info.release_lock()
with mock.patch.object(self.node_info, 'release_lock',
Expand Down

0 comments on commit cb1e856

Please sign in to comment.