Use cached node list in createMinReady

We previously avoided using the cached node list when deciding
whether to create a min-ready node request.  This is because if
the cache is out of date, we might end up issuing a min-ready
request when it isn't necessary.

However, in systems with large numbers of nodes, this can be a
very expensive operation.  Meanwhile, by introducing the tree
cache, we have attempted to make the cache as fast as possible
and therefore more likely to be up-to-date (especially in a
smaller system where this would matter more).

Let's try using the cache for this instead so that we get optimal
performance in small and large systems.

Since this is the last call site for this method which did not use
the cache, the method argument indicating whether to use the cache
is removed and unit tests adjusted accordingly.

Change-Id: I203206413265fa5216fccec0f7579e24f0aee48e

nodepool/launcher.py

@@ -1282,10 +1282,7 @@ class NodePool(threading.Thread):
         label_names = list(self.config.labels.keys())
         requested_labels = list(self._submittedRequests.keys())
         needed_labels = list(set(label_names) - set(requested_labels))
-
-        # Note we explicitly don't use the cache here because otherwise we can
-        # end up creating more min-ready nodes than we want.
-        ready_nodes = self.zk.getReadyNodesOfTypes(needed_labels, cached=False)
+        ready_nodes = self.zk.getReadyNodesOfTypes(needed_labels)
 
         for label in self.config.labels.values():
             if label.name not in needed_labels:

nodepool/tests/unit/test_webapp.py

@@ -298,6 +298,7 @@ class TestWebApp(tests.DBTestCase):
                          'application/json')
         data = f.read()
         objs = json.loads(data.decode('utf8'))
+        objs = [o for o in objs if 'min-ready' not in o['requestor']]
         self.assertDictContainsSubset({'node_types': ['fake-label'],
                                        'requestor': 'test_request_list',
                                        'event_id': req.event_id, },

nodepool/tests/unit/test_zk.py

@@ -776,7 +776,11 @@ class TestZooKeeper(tests.DBTestCase):
         n3.type = 'label2'
         self.zk.storeNode(n3)
 
-        r = self.zk.getReadyNodesOfTypes(['label1'], cached=False)
+        for _ in iterate_timeout(10, Exception, "wait for cache"):
+            r = self.zk.getReadyNodesOfTypes(['label1'])
+            if len(r.get('label1')) == 2:
+                break
+
         self.assertIn('label1', r)
         self.assertEqual(2, len(r['label1']))
         self.assertIn(n1, r['label1'])
@@ -796,7 +800,12 @@ class TestZooKeeper(tests.DBTestCase):
         n3.type = 'label2'
         self.zk.storeNode(n3)
 
-        r = self.zk.getReadyNodesOfTypes(['label1', 'label3'], cached=False)
+        for _ in iterate_timeout(10, Exception, "wait for cache"):
+            r = self.zk.getReadyNodesOfTypes(['label1', 'label3'])
+            if (len(r.get('label1')) == 2 and
+                len(r.get('label3')) == 1):
+                break
+
         self.assertIn('label1', r)
         self.assertIn('label3', r)
         self.assertEqual(2, len(r['label1']))

nodepool/zk/zookeeper.py

@@ -2701,7 +2701,7 @@ class ZooKeeper(ZooKeeperBase):
         if node._thread_lock.locked():
             node._thread_lock.release()
 
-    def getReadyNodesOfTypes(self, labels, cached=True):
+    def getReadyNodesOfTypes(self, labels):
         '''
         Query ZooKeeper for unused/ready nodes.
 
@@ -2713,7 +2713,7 @@ class ZooKeeper(ZooKeeperBase):
             those labels.
         '''
         ret = {}
-        for node in self.nodeIterator(cached=cached, cached_ids=cached):
+        for node in self.nodeIterator(cached=True, cached_ids=True):
             if node.state != READY or node.allocated_to:
                 continue
             for label in labels: