zuul
/
nodepool
Code Issues Proposed changes

Clarify terminology around node request locks 

We had NodeRequestLock objects which are not the same type or data as
the request.lock objects. This is confusing. NodeRequestLock objects
hold znode state info for a lock object in zk and request.lock objects
are the actual zk Lock() for the earlier znode.

Make this more clear by renaming the NodeRequestLock object
NodeRequestLockStats.

Similarly there are many places where we pass 'lock' as a parameter
when we don't mean the actual request.lock object but instead the
request.lock.id object. Update these to be more clearly 'lock_id'
instead of 'lock'.

Finally we also have cases where we want the 'lock_id' but pass 'id'
which is a reserved word in python so lets just avoid it and call these
things 'lock_id'.

Change-Id: Id5c4fe71266efa7f8362fa9c8be7ab27c60059fd
This commit is contained in:
Clark Boylan 2017-12-06 16:53:14 -08:00
parent ee37c7937e
commit 92451cdb73
4 changed files with 46 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
nodepool/launcher.py
View File

@ -432,11 +432,11 @@ class CleanupWorker(BaseCleanupWorker):
zk = self._nodepool.getZK()
requests = zk.getNodeRequests()
now = time.time()
for lock in zk.nodeRequestLockIterator():
if lock.id in requests:
for lock_stat in zk.nodeRequestLockStatsIterator():
if lock_stat.lock_id in requests:
continue
if (now - lock.stat.mtime/1000) > LOCK_CLEANUP:
zk.deleteNodeRequestLock(lock.id)
if (now - lock_stat.stat.mtime/1000) > LOCK_CLEANUP:
zk.deleteNodeRequestLock(lock_stat.lock_id)
def _cleanupLeakedInstances(self):
'''

2
nodepool/tests/__init__.py
View File

@ -425,7 +425,7 @@ class DBTestCase(BaseTestCase):
def waitForNodeRequestLockDeletion(self, request_id):
while True:
exists = False
for lock_id in self.zk.getNodeRequestLocks():
for lock_id in self.zk.getNodeRequestLockIDs():
if request_id == lock_id:
exists = True
break

32
nodepool/tests/test_zk.py
View File

@ -589,29 +589,29 @@ class TestZooKeeper(tests.DBTestCase):
with testtools.ExpectedException(StopIteration):
next(i)
def test_getNodeRequestLocks(self):
def test_getNodeRequestLockIDs(self):
req = self._create_node_request()
self.zk.lockNodeRequest(req, blocking=False)
locks = self.zk.getNodeRequestLocks()
self.assertEqual(1, len(locks))
self.assertEqual(req.id, locks[0])
lock_ids = self.zk.getNodeRequestLockIDs()
self.assertEqual(1, len(lock_ids))
self.assertEqual(req.id, lock_ids[0])
self.zk.unlockNodeRequest(req)
self.zk.deleteNodeRequest(req)
def test_getNodeRequestLock(self):
def test_getNodeRequestLockStats(self):
req = self._create_node_request()
self.zk.lockNodeRequest(req, blocking=False)
lock = self.zk.getNodeRequestLock(req.id)
self.assertEqual(lock.id, req.id)
self.assertIsNotNone(lock.stat)
lock_stats = self.zk.getNodeRequestLockStats(req.id)
self.assertEqual(lock_stats.lock_id, req.id)
self.assertIsNotNone(lock_stats.stat)
self.zk.unlockNodeRequest(req)
self.zk.deleteNodeRequest(req)
def test_nodeRequestLockIterator(self):
def test_nodeRequestLockStatsIterator(self):
req = self._create_node_request()
self.zk.lockNodeRequest(req, blocking=False)
i = self.zk.nodeRequestLockIterator()
self.assertEqual(zk.NodeRequestLock(req.id), next(i))
i = self.zk.nodeRequestLockStatsIterator()
self.assertEqual(zk.NodeRequestLockStats(req.id), next(i))
with testtools.ExpectedException(StopIteration):
next(i)
self.zk.unlockNodeRequest(req)
@ -634,11 +634,11 @@ class TestZooKeeper(tests.DBTestCase):
self.zk.deleteNodeRequest(req)
# We expect the lock to linger even after the request is deleted
locks = self.zk.getNodeRequestLocks()
self.assertEqual(1, len(locks))
self.assertEqual(req.id, locks[0])
self.zk.deleteNodeRequestLock(locks[0])
self.assertEqual([], self.zk.getNodeRequestLocks())
lock_ids = self.zk.getNodeRequestLockIDs()
self.assertEqual(1, len(lock_ids))
self.assertEqual(req.id, lock_ids[0])
self.zk.deleteNodeRequestLock(lock_ids[0])
self.assertEqual([], self.zk.getNodeRequestLockIDs())
class TestZKModel(tests.BaseTestCase):

50
nodepool/zk.py
View File

@ -330,25 +330,25 @@ class ImageUpload(BaseModel):
return o
class NodeRequestLock(object):
class NodeRequestLockStats(object):
'''
Class representing a node request lock.
Class holding the stats of a node request lock znode.
This doesn't need to derive from BaseModel since this class exists only
to associate the znode stats with the lock.
'''
def __init__(self, id=None):
self.id = id
def __init__(self, lock_id=None):
self.lock_id = lock_id
self.stat = None
def __eq__(self, other):
if isinstance(other, NodeRequestLock):
return (self.id == other.id)
if isinstance(other, NodeRequestLockStats):
return (self.lock_id == other.lock_id)
else:
return False
def __repr__(self):
return '<NodeRequestLock %s>' % self.id
return '<NodeRequestLockStats %s>' % self.lock_id
class NodeRequest(BaseModel):
@ -1331,17 +1331,17 @@ class ZooKeeper(object):
return sorted(requests)
def getNodeRequestLocks(self):
def getNodeRequestLockIDs(self):
'''
Get the current list of all node request locks.
Get the current list of all node request lock ids.
'''
try:
locks = self.client.get_children(self.REQUEST_LOCK_ROOT)
lock_ids = self.client.get_children(self.REQUEST_LOCK_ROOT)
except kze.NoNodeError:
return []
return locks
return lock_ids
def getNodeRequestLock(self, lock):
def getNodeRequestLockStats(self, lock_id):
'''
Get the data for a specific node request lock.
@ -1349,26 +1349,26 @@ class ZooKeeper(object):
main purpose for this method is to get the ZK stat data for the lock
so we can inspect it and use it for lock deletion.
:param str lock: The node request lock ID.
:param str lock_id: The node request lock ID.
:returns: A NodeRequestLock object.
:returns: A NodeRequestLockStats object.
'''
path = self._requestLockPath(lock)
path = self._requestLockPath(lock_id)
try:
data, stat = self.client.get(path)
except kze.NoNodeError:
return None
d = NodeRequestLock(lock)
d = NodeRequestLockStats(lock_id)
d.stat = stat
return d
def deleteNodeRequestLock(self, lock):
def deleteNodeRequestLock(self, lock_id):
'''
Delete the znode for a node request lock.
Delete the znode for a node request lock id.
:param str lock: The lock ID.
:param str lock_id: The lock ID.
'''
path = self._requestLockPath(lock)
path = self._requestLockPath(lock_id)
try:
self.client.delete(path, recursive=True)
except kze.NoNodeError:
@ -1639,14 +1639,14 @@ class ZooKeeper(object):
if node:
yield node
def nodeRequestLockIterator(self):
def nodeRequestLockStatsIterator(self):
'''
Utility generator method for iterating through all nodes request locks.
'''
for lock_id in self.getNodeRequestLocks():
lock = self.getNodeRequestLock(lock_id)
if lock:
yield lock
for lock_id in self.getNodeRequestLockIDs():
lock_stats = self.getNodeRequestLockStats(lock_id)
if lock_stats:
yield lock_stats
def nodeRequestIterator(self):
'''