Merge lp:~julian-edwards/launchpad/build-sched-bug-478691 into lp:launchpad

[Julian Edwards]

[Julian Edwards (julian-edwards)]

= Summary =
Fix findBuildCandidate so that instead of only allowing one builder per arch
for each PPA, it allows up to 80% of the available builders for that arch.

== Proposed fix ==
As per https://bugs.edge.launchpad.net/soyuz/+bug/478691

To alleviate the problem of daily build PPAs from monopolising the build darm,
a change was made last cycle to allow PPAs' builds to only occupy at most one
builder of each architecture type. Unfortunately, this has proven to not only
prevent the daily builds from completing within 24 hours, it also leaves the
build farm under-utilised.

== Pre-implementation notes ==
I've spoken to some of the daily PPA owners (see the bug comments) and we've
decided to change the calculation slightly so that instead of only being able
to occupy one builder of each architecture at a time, it now only dispatches
the build if it would take the usage to 80% or more of available builders of
that arch.

== Implementation details ==
This was just a change to the big SQL query done in findBuildCandidate. I
also made the tests a little more generic in case we need to tweak this again
(we *will* need to tweak it again!).

== Tests ==
bin/test -cvv test_builder

== Demo and Q/A ==
Can't really Q/A this as we don't have multiple builders available in dogfood.
:(

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/model/builder.py
  lib/lp/soyuz/tests/test_builder.py

[Brad Crittenden (bac)]

This looks good. Please make the change we discussed:

for build in builds_list[:num_builds]

review: Approve (code)

[Francis J. Lacoste (flacoste)]

review: Approve (release-critical)

=== modified file 'lib/lp/soyuz/model/builder.py'
--- lib/lp/soyuz/model/builder.py	2009-10-14 09:43:42 +0000
+++ lib/lp/soyuz/model/builder.py	2009-11-11 10:46:12 +0000
@@ -569,22 +569,39 @@
             archive.require_virtualized = %s
         """ % sqlvalues(self.virtualized))
 
-        # Ensure that if a currently-building build exists for the same
-        # public ppa archive and architecture currently building then
-        # we don't consider another as a candidate.
-        clauses.append("""
-            NOT EXISTS (
-                SELECT Build.id
-                FROM Build build2, DistroArchSeries distroarchseries2
-                WHERE
-                    build2.archive = build.archive AND
-                    archive.purpose = %s AND
-                    archive.private IS FALSE AND
-                    build2.distroarchseries = distroarchseries2.id AND
-                    distroarchseries2.processorfamily = %s AND
-                    build2.buildstate = %s)
-        """ % sqlvalues(
-            ArchivePurpose.PPA, self.processor.family, BuildStatus.BUILDING))
+        # Ensure that if BUILDING builds exist for the same
+        # public ppa archive and architecture and another would not
+        # leave at least 20% of them free, then we don't consider
+        # another as a candidate.
+        #
+        # This clause selects the count of currently building builds on
+        # the arch in question, then adds one to that total before
+        # deriving a percentage of the total available builders on that
+        # arch.  It then makes sure that percentage is under 80.
+        #
+        # The extra clause is only used if the number of available
+        # builders is greater than one, or nothing would get dispatched
+        # at all.
+        num_arch_builders = Builder.selectBy(
+            processor=self.processor, manual=False, builderok=True).count()
+        if num_arch_builders > 1:
+            clauses.append("""
+                EXISTS (SELECT true
+                WHERE ((
+                    SELECT COUNT(build2.id)
+                    FROM Build build2, DistroArchSeries distroarchseries2
+                    WHERE
+                        build2.archive = build.archive AND
+                        archive.purpose = %s AND
+                        archive.private IS FALSE AND
+                        build2.distroarchseries = distroarchseries2.id AND
+                        distroarchseries2.processorfamily = %s AND
+                        build2.buildstate = %s) + 1::numeric)
+                    *100 / %s
+                    < 80)
+            """ % sqlvalues(
+                ArchivePurpose.PPA, self.processor.family,
+                BuildStatus.BUILDING, num_arch_builders))
 
         query = " AND ".join(clauses)
         candidate = BuildQueue.selectFirst(

=== modified file 'lib/lp/soyuz/tests/test_builder.py'
--- lib/lp/soyuz/tests/test_builder.py	2009-10-14 14:25:19 +0000
+++ lib/lp/soyuz/tests/test_builder.py	2009-11-11 10:46:12 +0000
@@ -26,14 +26,83 @@
         self.publisher = SoyuzTestPublisher()
         self.publisher.prepareBreezyAutotest()
 
-        # Create two i386 builders ready to build PPA builds.
-        builder_set = getUtility(IBuilderSet)
-        self.builder1 = self.factory.makeBuilder(name='bob2')
-        self.builder2 = self.factory.makeBuilder(name='frog2')
+        # Create some i386 builders ready to build PPA builds.  Two
+        # already exist in sampledata so we'll use those first.
+        self.builder1 = getUtility(IBuilderSet)['bob']
+        self.builder2 = getUtility(IBuilderSet)['frog']
+        self.builder3 = self.factory.makeBuilder(name='builder3')
+        self.builder4 = self.factory.makeBuilder(name='builder4')
+        self.builder5 = self.factory.makeBuilder(name='builder5')
+        self.builders = [
+            self.builder1,
+            self.builder2,
+            self.builder3,
+            self.builder4,
+            self.builder5,
+            ]
+
+        # Ensure all builders are operational.
+        for builder in self.builders:
+            builder.builderok = True
+            builder.manual = False
+
+
+class TestFindBuildCandidatePPAWithSingleBuilder(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestFindBuildCandidatePPAWithSingleBuilder, self).setUp()
+        self.publisher = SoyuzTestPublisher()
+        self.publisher.prepareBreezyAutotest()
+
+        self.bob_builder = getUtility(IBuilderSet)['bob']
+        self.frog_builder = getUtility(IBuilderSet)['frog']
+
+        # Disable bob so only frog is available.
+        self.bob_builder.manual = True
+        self.bob_builder.builderok = True
+        self.frog_builder.manual = False
+        self.frog_builder.builderok = True
+
+        # Make a new PPA and give it some builds.
+        self.ppa_joe = self.factory.makeArchive(name="joesppa")
+        builds = self.publisher.getPubSource(
+            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
+            archive=self.ppa_joe).createMissingBuilds()
+
+    def test_findBuildCandidate_first_build_started(self):
+        # The allocation rule for PPA dispatching doesn't apply when
+        # there's only one builder available.
+
+        # Asking frog to find a candidate should give us the joesppa build.
+        next_job = self.frog_builder.findBuildCandidate()
+        self.assertEqual('joesppa', next_job.build.archive.name)
+
+        # If bob is in a failed state the joesppa build is still
+        # returned.
+        self.bob_builder.builderok = False
+        self.bob_builder.manual = False
+        next_job = self.frog_builder.findBuildCandidate()
+        self.assertEqual('joesppa', next_job.build.archive.name)
 
 
 class TestFindBuildCandidatePPA(TestFindBuildCandidateBase):
 
+    def _setBuildsBuildingForArch(self, builds_list, num_builds,
+                                  archtag="i386"):
+        """Helper function.
+
+        Set the first `num_builds` in `builds_list` with `archtag` as
+        BUILDING.
+        """
+        count = 0
+        for build in builds_list[:num_builds]:
+            if build.distroarchseries.architecturetag == archtag:
+                build.buildstate = BuildStatus.BUILDING
+                build.builder = self.builders[count]
+            count += 1
+
     def setUp(self):
         """Publish some builds for the test archive."""
         super(TestFindBuildCandidatePPA, self).setUp()
@@ -42,41 +111,58 @@
         self.ppa_joe = self.factory.makeArchive(name="joesppa")
         self.ppa_jim = self.factory.makeArchive(name="jimsppa")
 
-        self.publisher.getPubSource(
-            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
-            archive=self.ppa_joe).createMissingBuilds()
-        self.publisher.getPubSource(
-            sourcename="firefox", status=PackagePublishingStatus.PUBLISHED,
-            archive=self.ppa_joe).createMissingBuilds()
-
-        self.publisher.getPubSource(
-            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
-            archive=self.ppa_jim).createMissingBuilds()
-        self.publisher.getPubSource(
-            sourcename="firefox", status=PackagePublishingStatus.PUBLISHED,
-            archive=self.ppa_jim).createMissingBuilds()
-
-        # Grab the first build, ensure that it is what we expect
-        # (ie. the first build from joesppa) and set it building.
-        self.first_job = self.builder1.findBuildCandidate()
-        self.failUnlessEqual('joesppa', self.first_job.build.archive.name)
-        self.failUnlessEqual(
-            u'i386 build of gedit 666 in ubuntutest breezy-autotest RELEASE',
-            self.first_job.build.title)
-        self.first_job.build.buildstate = BuildStatus.BUILDING
-        self.first_job.build.builder = self.builder1
+        self.joe_builds = []
+        self.joe_builds.extend(
+            self.publisher.getPubSource(
+                sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
+                archive=self.ppa_joe).createMissingBuilds())
+        self.joe_builds.extend(
+            self.publisher.getPubSource(
+                sourcename="firefox",
+                status=PackagePublishingStatus.PUBLISHED,
+                archive=self.ppa_joe).createMissingBuilds())
+        self.joe_builds.extend(
+            self.publisher.getPubSource(
+                sourcename="cobblers",
+                status=PackagePublishingStatus.PUBLISHED,
+                archive=self.ppa_joe).createMissingBuilds())
+        self.joe_builds.extend(
+            self.publisher.getPubSource(
+                sourcename="thunderpants",
+                status=PackagePublishingStatus.PUBLISHED,
+                archive=self.ppa_joe).createMissingBuilds())
+
+        self.jim_builds = []
+        self.jim_builds.extend(
+            self.publisher.getPubSource(
+                sourcename="gedit",
+                status=PackagePublishingStatus.PUBLISHED,
+                archive=self.ppa_jim).createMissingBuilds())
+        self.jim_builds.extend(
+            self.publisher.getPubSource(
+                sourcename="firefox",
+                status=PackagePublishingStatus.PUBLISHED,
+                archive=self.ppa_jim).createMissingBuilds())
+
+        # Set the first three builds in joe's PPA as building, which
+        # leaves two builders free.
+        self._setBuildsBuildingForArch(self.joe_builds, 3)
+        num_active_builders = len(
+            [build for build in self.joe_builds if build.builder is not None])
+        num_free_builders = len(self.builders) - num_active_builders
+        self.assertEqual(num_free_builders, 2)
 
     def test_findBuildCandidate_first_build_started(self):
-        # Once a build for an ppa+arch has started, a second one for the
-        # same ppa+arch will not be a candidate.
-        next_job = self.builder2.findBuildCandidate()
+        # A PPA cannot start a build if it would use 80% or more of the
+        # builders.
+        next_job = self.builder4.findBuildCandidate()
         self.failIfEqual('joesppa', next_job.build.archive.name)
 
     def test_findBuildCandidate_first_build_finished(self):
-        # When joe's first ppa build finishes, his second i386 build
+        # When joe's first ppa build finishes, his fourth i386 build
         # will be the next build candidate.
-        self.first_job.build.buildstate = BuildStatus.FAILEDTOBUILD
-        next_job = self.builder2.findBuildCandidate()
+        self.joe_builds[0].buildstate = BuildStatus.FAILEDTOBUILD
+        next_job = self.builder4.findBuildCandidate()
         self.failUnlessEqual('joesppa', next_job.build.archive.name)
 
     def test_findBuildCandidate_for_private_ppa(self):
@@ -84,7 +170,7 @@
         # for the one architecture.
         self.ppa_joe.private = True
         self.ppa_joe.buildd_secret = 'sekrit'
-        next_job = self.builder2.findBuildCandidate()
+        next_job = self.builder4.findBuildCandidate()
         self.failUnlessEqual('joesppa', next_job.build.archive.name)