From 0dbd2ee458fb556edbb698562b2873ee601209ac Mon Sep 17 00:00:00 2001
From: "James E. Blair" <jim@acmegating.com>
Date: Tue, 28 May 2024 11:12:37 -0700
Subject: [PATCH] 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               |  5 +----
 nodepool/tests/unit/test_webapp.py |  1 +
 nodepool/tests/unit/test_zk.py     | 13 +++++++++++--
 nodepool/zk/zookeeper.py           |  4 ++--
 4 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/nodepool/launcher.py b/nodepool/launcher.py
index 9dc207958..031653b1a 100644
--- a/nodepool/launcher.py
+++ b/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:
diff --git a/nodepool/tests/unit/test_webapp.py b/nodepool/tests/unit/test_webapp.py
index 1e0b4dc70..25122ea0f 100644
--- a/nodepool/tests/unit/test_webapp.py
+++ b/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, },
diff --git a/nodepool/tests/unit/test_zk.py b/nodepool/tests/unit/test_zk.py
index d4f8c6e8b..5b0f448ca 100644
--- a/nodepool/tests/unit/test_zk.py
+++ b/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']))
diff --git a/nodepool/zk/zookeeper.py b/nodepool/zk/zookeeper.py
index 29293576f..3f4b00dbe 100644
--- a/nodepool/zk/zookeeper.py
+++ b/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: