launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:buildmaster-postprocess-in-dispatch into launchpad:master.

Commit message:
Move postprocessCandidate into build dispatch

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/388291

BuildQueueSet.findBuildCandidates had a bug in its postprocessing logic: it selected a prefix that was the *maximum* of twice the number of builders being considered and 10, rather than the *minimum*, so ended up doing an awful lot of unnecessary and often duplicated work on every scan cycle.

However, it makes more sense to do the postprocessing once we actually have an idle builder and are trying to dispatch something to it.  It's unlikely that many candidates will be removed by postprocessing, and even if that does happen then the worst case is that we spend a few cycles dealing with superseding them all, which will self-resolve.  This is better than spending more time every cycle doing database work.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:buildmaster-postprocess-in-dispatch into launchpad:master.

diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index c19e617..07088f0 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -598,8 +598,19 @@ class BuilderInteractor(object):
             found or None if no job was found.
         """
         logger = cls._getSlaveScannerLogger()
-        candidate = builder_factory.acquireBuildCandidate(vitals, builder)
-        if candidate is None:
+        # Try a few candidates so that we make reasonable progress if we
+        # have only a few idle builders but lots of candidates that fail
+        # postprocessing due to old source publications or similar.  The
+        # chance of a large prefix of the queue being bad candidates is
+        # negligible, and we want reasonably bounded per-cycle performance
+        # even if the prefix is large.
+        for _ in range(10):
+            candidate = builder_factory.acquireBuildCandidate(vitals, builder)
+            if candidate is not None:
+                if candidate.specific_source.postprocessCandidate(
+                        candidate, logger):
+                    break
+        else:
             logger.debug("No build candidates available for builder.")
             defer.returnValue(None)
 
diff --git a/lib/lp/buildmaster/interfaces/buildqueue.py b/lib/lp/buildmaster/interfaces/buildqueue.py
index c067428..ee35a7a 100644
--- a/lib/lp/buildmaster/interfaces/buildqueue.py
+++ b/lib/lp/buildmaster/interfaces/buildqueue.py
@@ -105,6 +105,8 @@ class IBuildQueue(Interface):
         Everyone else wants to use cancel().
         """
 
+    specific_source = Attribute("Type of concrete build farm job.")
+
     specific_build = Reference(
         IBuildFarmJob, title=_("Build farm job"),
         description=_("Concrete build farm job object."))
diff --git a/lib/lp/buildmaster/model/buildqueue.py b/lib/lp/buildmaster/model/buildqueue.py
index ded1382..cce2463 100644
--- a/lib/lp/buildmaster/model/buildqueue.py
+++ b/lib/lp/buildmaster/model/buildqueue.py
@@ -6,7 +6,6 @@ __metaclass__ = type
 __all__ = [
     'BuildQueue',
     'BuildQueueSet',
-    'specific_build_farm_job_sources',
     ]
 
 from datetime import datetime
@@ -121,12 +120,15 @@ class BuildQueue(StormBase):
     processor = Reference(processor_id, 'Processor.id')
     virtualized = Bool(name='virtualized')
 
+    @property
+    def specific_source(self):
+        """See `IBuildQueue`."""
+        return specific_build_farm_job_sources()[self._build_farm_job.job_type]
+
     @cachedproperty
     def specific_build(self):
         """See `IBuildQueue`."""
-        bfj = self._build_farm_job
-        specific_source = specific_build_farm_job_sources()[bfj.job_type]
-        return specific_source.getByBuildFarmJob(bfj)
+        return self.specific_source.getByBuildFarmJob(self._build_farm_job)
 
     @property
     def build_cookie(self):
@@ -340,8 +342,8 @@ class BuildQueueSet(object):
                 BuildQueue.lastscore >= max(minimum_scores))
 
         store = IStore(BuildQueue)
-        candidate_jobs = store.using(BuildQueue, BuildFarmJob).find(
-            (BuildQueue.id,),
+        return list(store.using(BuildQueue, BuildFarmJob).find(
+            BuildQueue,
             BuildFarmJob.id == BuildQueue._build_farm_job_id,
             BuildQueue.status == BuildQueueStatus.WAITING,
             BuildQueue.processor == processor,
@@ -350,22 +352,4 @@ class BuildQueueSet(object):
             And(*(job_type_conditions + score_conditions))
             # This must match the ordering used in
             # PrefetchedBuildCandidates._getSortKey.
-            ).order_by(Desc(BuildQueue.lastscore), BuildQueue.id)
-
-        # Only try a limited number of jobs. It's much easier on the
-        # database, the chance of a large prefix of the queue being
-        # bad candidates is negligible, and we want reasonably bounded
-        # per-cycle performance even if the prefix is large.
-        candidates = []
-        for (candidate_id,) in candidate_jobs[:max(limit * 2, 10)]:
-            candidate = getUtility(IBuildQueueSet).get(candidate_id)
-            job_source = job_sources[
-                removeSecurityProxy(candidate)._build_farm_job.job_type]
-            candidate_approved = job_source.postprocessCandidate(
-                candidate, logger)
-            if candidate_approved:
-                candidates.append(candidate)
-                if len(candidates) >= limit:
-                    break
-
-        return candidates
+            ).order_by(Desc(BuildQueue.lastscore), BuildQueue.id)[:limit])
diff --git a/lib/lp/buildmaster/tests/test_builder.py b/lib/lp/buildmaster/tests/test_builder.py
index 4200c7a..5d04e0d 100644
--- a/lib/lp/buildmaster/tests/test_builder.py
+++ b/lib/lp/buildmaster/tests/test_builder.py
@@ -172,34 +172,6 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase):
             [bq3, bq2],
             self.bq_set.findBuildCandidates(bq2.processor, True, 3))
 
-    def test_findBuildCandidates_supersedes_builds(self):
-        # BuildQueueSet.findBuildCandidates identifies if there are builds
-        # for superseded source package releases in the queue and marks the
-        # corresponding build record as SUPERSEDED.
-        archive = self.factory.makeArchive()
-        self.publisher.getPubSource(
-            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
-            archive=archive).createMissingBuilds()
-        old_candidates = self.bq_set.findBuildCandidates(
-            self.proc_386, True, 2)
-
-        # The candidate starts off as NEEDSBUILD:
-        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(
-            old_candidates[0])
-        self.assertEqual(BuildStatus.NEEDSBUILD, build.status)
-
-        # Now supersede the source package:
-        publication = build.current_source_publication
-        publication.status = PackagePublishingStatus.SUPERSEDED
-
-        # The list of candidates returned is now different:
-        new_candidates = self.bq_set.findBuildCandidates(
-            self.proc_386, True, 2)
-        self.assertNotEqual(new_candidates, old_candidates)
-
-        # And the old_candidate is superseded:
-        self.assertEqual(BuildStatus.SUPERSEDED, build.status)
-
     def test_findBuildCandidates_honours_limit(self):
         # BuildQueueSet.findBuildCandidates returns no more than the number
         # of candidates requested.
@@ -216,13 +188,6 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase):
         self.assertEqual(
             bqs, self.bq_set.findBuildCandidates(processor, True, 11))
 
-        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(bqs[0])
-        build.current_source_publication.status = (
-            PackagePublishingStatus.SUPERSEDED)
-
-        self.assertEqual(
-            bqs[1:6], self.bq_set.findBuildCandidates(processor, True, 5))
-
     def test_findBuildCandidates_honours_minimum_score(self):
         # Sometimes there's an emergency that requires us to lock down the
         # build farm except for certain whitelisted builds.  We do this by
diff --git a/lib/lp/buildmaster/tests/test_interactor.py b/lib/lp/buildmaster/tests/test_interactor.py
index 22d788c..0b8384d 100644
--- a/lib/lp/buildmaster/tests/test_interactor.py
+++ b/lib/lp/buildmaster/tests/test_interactor.py
@@ -60,7 +60,10 @@ from lp.buildmaster.interfaces.builder import (
     CannotFetchFile,
     CannotResumeHost,
     )
-from lp.buildmaster.manager import BaseBuilderFactory
+from lp.buildmaster.manager import (
+    BaseBuilderFactory,
+    PrefetchedBuilderFactory,
+    )
 from lp.buildmaster.tests.mock_slaves import (
     AbortingSlave,
     BuildingSlave,
@@ -75,6 +78,7 @@ from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
 from lp.services.twistedsupport.testing import TReqFixture
 from lp.services.twistedsupport.treq import check_status
+from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.model.binarypackagebuildbehaviour import (
     BinaryPackageBuildBehaviour,
     )
@@ -430,6 +434,37 @@ class TestBuilderInteractorDB(TestCaseWithFactory):
             vitals, builder, OkSlave(), builder_factory)
         return d.addCallback(self.assertEqual, candidate)
 
+    @defer.inlineCallbacks
+    def test_findAndStartJob_supersedes_builds(self):
+        # findAndStartJob checks whether queued jobs are for superseded
+        # source package releases and marks the corresponding build records
+        # as SUPERSEDED.
+        builder, distroseries, distroarchseries = self._setupBuilder()
+        builds = [
+            self.factory.makeBinaryPackageBuild(
+                distroarchseries=distroarchseries)
+            for _ in range(3)]
+        candidates = [build.queueBuild() for build in builds]
+        builder_factory = PrefetchedBuilderFactory()
+        candidates_iter = iter(candidates)
+        builder_factory.findBuildCandidate = lambda _: next(candidates_iter)
+        vitals = extract_vitals_from_db(builder)
+
+        # Supersede some of the builds' source packages.
+        for build in builds[:2]:
+            publication = build.current_source_publication
+            publication.status = PackagePublishingStatus.SUPERSEDED
+
+        # Starting a job selects a non-superseded candidate, and supersedes
+        # the candidates that have superseded source packages.
+        candidate = yield BuilderInteractor.findAndStartJob(
+            vitals, builder, OkSlave(), builder_factory)
+        self.assertEqual(candidates[2], candidate)
+        self.assertEqual(
+            [BuildStatus.SUPERSEDED, BuildStatus.SUPERSEDED,
+             BuildStatus.BUILDING],
+            [build.status for build in builds])
+
     def test_findAndStartJob_starts_job(self):
         # findAndStartJob finds the next queued job using findBuildCandidate
         # and then starts it.