launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/ss-get-builder-as-needed into lp:launchpad.

Commit message:
Adjust SlaveScanner to hit the DB a bit less. We now cache the build cookie while the BuildQueue stays the same, and checkCancellation only retrieves a builder from the DB when it actually needs one.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/ss-get-builder-as-needed/+merge/188982

Adjust SlaveScanner to hit the DB a bit less. We now cache the build cookie while the BuildQueue stays the same, and checkCancellation only retrieves a builder from the DB when it actually needs one.
-- 
https://code.launchpad.net/~wgrant/launchpad/ss-get-builder-as-needed/+merge/188982
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/ss-get-builder-as-needed into lp:launchpad.

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2013-10-02 05:47:26 +0000
+++ lib/lp/buildmaster/manager.py	2013-10-03 04:20:42 +0000
@@ -159,6 +159,11 @@
         self._clock = clock
         self.date_cancel = None
 
+        # We cache the build cookie, keyed on the BuildQueue, to avoid
+        # hitting the DB on every scan.
+        self._cached_build_cookie = None
+        self._cached_build_queue = None
+
     def startCycle(self):
         """Scan the builder and dispatch to it or deal with failures."""
         self.loop = LoopingCall(self.singleCycle)
@@ -222,7 +227,7 @@
             transaction.abort()
 
     @defer.inlineCallbacks
-    def checkCancellation(self, vitals, builder, slave, interactor):
+    def checkCancellation(self, vitals, slave, interactor):
         """See if there is a pending cancellation request.
 
         If the current build is in status CANCELLING then terminate it
@@ -265,10 +270,28 @@
             vitals.build_queue.cancel()
             transaction.commit()
             value = yield interactor.resetOrFail(
-                vitals, slave, builder, self.logger, e)
+                vitals, slave, self.builders_cache[vitals.name], self.logger,
+                e)
             # value is not None if we resumed a slave host.
             defer.returnValue(value is not None)
 
+    def getExpectedCookie(self, vitals):
+        """Return the build cookie expected to be held by the slave.
+
+        Calculating this requires hitting the DB, so it's cached based
+        on the current BuildQueue.
+        """
+        if vitals.build_queue != self._cached_build_queue:
+            if vitals.build_queue is not None:
+                behavior = self.behavior_factory(
+                    vitals.build_queue, self.builders_cache[vitals.name],
+                    None)
+                self._cached_build_cookie = behavior.getBuildCookie()
+            else:
+                self._cached_build_cookie = None
+            self._cached_build_queue = vitals.build_queue
+        return self._cached_build_cookie
+
     @defer.inlineCallbacks
     def scan(self):
         """Probe the builder and update/dispatch/collect as appropriate.
@@ -289,16 +312,11 @@
         if not vitals.builderok:
             lost_reason = '%s is disabled' % vitals.name
         else:
-            builder = self.builders_cache[self.builder_name]
-            cancelled = yield self.checkCancellation(
-                vitals, builder, slave, interactor)
+            cancelled = yield self.checkCancellation(vitals, slave, interactor)
             if cancelled:
                 return
-            behavior = self.behavior_factory(
-                vitals.build_queue, builder, slave)
-            db_cookie = behavior.getBuildCookie() if behavior else None
             lost = yield interactor.rescueIfLost(
-                vitals, slave, db_cookie, self.logger)
+                vitals, slave, self.getExpectedCookie(vitals), self.logger)
             if lost:
                 lost_reason = '%s is lost' % vitals.name
 

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2013-09-30 08:12:17 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2013-10-03 04:20:42 +0000
@@ -453,13 +453,20 @@
 class MockBuildersCache:
 
     def __init__(self, builder, build_queue):
+        self.updateTestData(builder, build_queue)
+        self.get_call_count = 0
+        self.getVitals_call_count = 0
+
+    def updateTestData(self, builder, build_queue):
         self._builder = builder
         self._build_queue = build_queue
 
     def __getitem__(self, name):
+        self.get_call_count += 1
         return self._builder
 
     def getVitals(self, name):
+        self.getVitals_call_count += 1
         return extract_vitals_from_db(self._builder, self._build_queue)
 
 
@@ -540,6 +547,43 @@
         self.assertEqual(['status', 'abort'], slave.call_log)
         self.assertEqual(0, interactor.updateBuild.call_count)
 
+    def test_getExpectedCookie_caches(self):
+        bc = MockBuildersCache(MockBuilder(), FakeBuildQueue())
+        scanner = SlaveScanner(
+            'mock', bc, BufferLogger(), interactor_factory=FakeMethod(None),
+            slave_factory=FakeMethod(None),
+            behavior_factory=FakeMethod(TrivialBehavior()))
+
+        def assertCounts(expected):
+            self.assertEqual(
+                expected,
+                (scanner.interactor_factory.call_count,
+                 scanner.behavior_factory.call_count,
+                 scanner.builders_cache.get_call_count))
+
+        # The first call will get a Builder and a BuildFarmJobBehavior.
+        assertCounts((0, 0, 0))
+        cookie1 = scanner.getExpectedCookie(bc.getVitals('foo'))
+        self.assertEqual('trivial', cookie1)
+        assertCounts((0, 1, 1))
+
+        # A second call with the same BuildQueue will not reretrieve them.
+        cookie2 = scanner.getExpectedCookie(bc.getVitals('foo'))
+        self.assertEqual(cookie1, cookie2)
+        assertCounts((0, 1, 1))
+
+        # But a call with a new BuildQueue will regrab.
+        bc.updateTestData(bc._builder, FakeBuildQueue())
+        cookie3 = scanner.getExpectedCookie(bc.getVitals('foo'))
+        self.assertEqual(cookie1, cookie3)
+        assertCounts((0, 2, 2))
+
+        # And unsetting the BuildQueue returns None again.
+        bc.updateTestData(bc._builder, None)
+        cookie4 = scanner.getExpectedCookie(bc.getVitals('foo'))
+        self.assertIs(None, cookie4)
+        assertCounts((0, 2, 2))
+
 
 class TestCancellationChecking(TestCaseWithFactory):
     """Unit tests for the checkCancellation method."""
@@ -568,7 +612,7 @@
         # If the builder is nonvirtual make sure we return False.
         self.builder.virtualized = False
         d = self._getScanner().checkCancellation(
-            self.vitals, self.builder, None, self.interactor)
+            self.vitals, None, self.interactor)
         return d.addCallback(self.assertFalse)
 
     def test_ignores_no_buildqueue(self):
@@ -577,13 +621,13 @@
         buildqueue = self.builder.currentjob
         buildqueue.reset()
         d = self._getScanner().checkCancellation(
-            self.vitals, self.builder, None, self.interactor)
+            self.vitals, None, self.interactor)
         return d.addCallback(self.assertFalse)
 
     def test_ignores_build_not_cancelling(self):
         # If the active build is not in a CANCELLING state, ignore it.
         d = self._getScanner().checkCancellation(
-            self.vitals, self.builder, None, self.interactor)
+            self.vitals, None, self.interactor)
         return d.addCallback(self.assertFalse)
 
     @defer.inlineCallbacks
@@ -599,14 +643,14 @@
         scanner = self._getScanner(clock=clock)
 
         result = yield scanner.checkCancellation(
-            self.vitals, self.builder, slave, self.interactor)
+            self.vitals, slave, self.interactor)
         self.assertNotIn("resume", slave.call_log)
         self.assertFalse(result)
         self.assertEqual(BuildStatus.CANCELLING, build.status)
 
         clock.advance(SlaveScanner.CANCEL_TIMEOUT)
         result = yield scanner.checkCancellation(
-            self.vitals, self.builder, slave, self.interactor)
+            self.vitals, slave, self.interactor)
         self.assertEqual(1, slave.call_log.count("resume"))
         self.assertTrue(result)
         self.assertEqual(BuildStatus.CANCELLED, build.status)
@@ -621,7 +665,7 @@
         build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(buildqueue)
         build.updateStatus(BuildStatus.CANCELLING)
         result = yield self._getScanner().checkCancellation(
-            self.vitals, self.builder, slave, self.interactor)
+            self.vitals, slave, self.interactor)
         self.assertEqual(1, slave.call_log.count("resume"))
         self.assertTrue(result)
         self.assertEqual(BuildStatus.CANCELLED, build.status)