launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/remove-ppa-builder-limits into lp:launchpad.

Commit message:
Remove 80%-of-builders limitation on builds for any given public PPA and architecture.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #666308 in Launchpad itself: "public non-virtual ppas are unnecessarily limited on builders per architecture"
  https://bugs.launchpad.net/launchpad/+bug/666308

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/remove-ppa-builder-limits/+merge/183834

== Summary ==

Bug 666308 reports unnecessary limitations on certain types of PPAs, as well as a request for more flexibility.

== Proposed fix ==

This is an RFC, but my position is that the limitation on how many builders a given PPA may use at any one time does more harm than good at this point.  Now that we have reasonably reliable build cancellation, in the rare cases of abuse or needing something to jump the queue we can always cancel something and retry it later.  Other than that, all that this limitation really achieves at this point is to require us to occasionally play silly games with builders' architectures to circumvent it.

We could add more complexity to make this controllable per-PPA or per-owner, as the bug suggests, but I don't think it's worth it.

== Tests ==

bin/test -vvct buildmaster -t soyuz

== Demo and Q/A ==

I don't think we can currently QA this on dogfood since it only has a single working PPA builder at the moment; but since this is pretty much just deleting code I'm not too worried.
-- 
https://code.launchpad.net/~cjwatson/launchpad/remove-ppa-builder-limits/+merge/183834
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/remove-ppa-builder-limits into lp:launchpad.

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2013-09-02 07:30:53 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2013-09-04 09:02:03 +0000
@@ -252,20 +252,11 @@
 
 class TestFindBuildCandidatePPA(TestFindBuildCandidatePPABase):
 
-    def test_findBuildCandidate_first_build_started(self):
-        # A PPA cannot start a build if it would use 80% or more of the
-        # builders.
-        next_job = removeSecurityProxy(self.builder4)._findBuildCandidate()
-        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(next_job)
-        self.failIfEqual('joesppa', build.archive.name)
-
-    def test_findBuildCandidate_first_build_finished(self):
-        # When joe's first ppa build finishes, his fourth i386 build
-        # will be the next build candidate.
-        self.joe_builds[0].updateStatus(BuildStatus.FAILEDTOBUILD)
-        next_job = removeSecurityProxy(self.builder4)._findBuildCandidate()
-        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(next_job)
-        self.failUnlessEqual('joesppa', build.archive.name)
+    def test_findBuildCandidate(self):
+        # joe's fourth i386 build will be the next build candidate.
+        next_job = removeSecurityProxy(self.builder4)._findBuildCandidate()
+        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(next_job)
+        self.assertEqual('joesppa', build.archive.name)
 
     def test_findBuildCandidate_with_disabled_archive(self):
         # Disabled archives should not be considered for dispatching
@@ -283,11 +274,10 @@
     ppa_joe_private = True
 
     def test_findBuildCandidate_for_private_ppa(self):
-        # If a ppa is private it will be able to have parallel builds
-        # for the one architecture.
+        # joe's fourth i386 build will be the next build candidate.
         next_job = removeSecurityProxy(self.builder4)._findBuildCandidate()
         build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(next_job)
-        self.failUnlessEqual('joesppa', build.archive.name)
+        self.assertEqual('joesppa', build.archive.name)
 
         # If the source for the build is still pending, it won't be
         # dispatched because the builder has to fetch the source files

=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py	2013-09-02 06:34:15 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py	2013-09-04 09:02:03 +0000
@@ -16,17 +16,13 @@
 from zope.interface import implements
 
 from lp.buildmaster.enums import BuildStatus
-from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.buildmaster.model.buildfarmjob import BuildFarmJobOld
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.database.bulk import load_related
 from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import sqlvalues
-from lp.soyuz.enums import (
-    ArchivePurpose,
-    PackagePublishingStatus,
-    )
+from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.soyuz.interfaces.buildpackagejob import (
     COPY_ARCHIVE_SCORE_PENALTY,
@@ -129,7 +125,7 @@
             PackagePublishingStatus.SUPERSEDED,
             PackagePublishingStatus.DELETED,
             )
-        sub_query = """
+        return """
             SELECT TRUE FROM Archive, BinaryPackageBuild, BuildPackageJob,
                              DistroArchSeries
             WHERE
@@ -154,44 +150,6 @@
             BinaryPackageBuild.status = %s
         """ % sqlvalues(private_statuses, BuildStatus.NEEDSBUILD)
 
-        # 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 = getUtility(IBuilderSet).getBuildersForQueue(
-            processor, virtualized).count()
-        if num_arch_builders > 1:
-            sub_query += """
-            AND Archive.id NOT IN (
-                SELECT Archive.id
-                FROM Archive, BinaryPackageBuild, DistroArchSeries
-                WHERE
-                    BinaryPackageBuild.distro_arch_series = DistroArchSeries.id
-                    AND DistroArchSeries.processorfamily = %s
-                    AND BinaryPackageBuild.status = %s
-                    AND BinaryPackageBuild.archive = Archive.id
-                    AND Archive.purpose = %s
-                    AND Archive.private IS FALSE
-                GROUP BY Archive.id
-                HAVING (
-                    (count(*)+1) * 100.0 / %s
-                    ) >= 80
-                )
-            """ % sqlvalues(
-                processor.family, BuildStatus.BUILDING,
-                ArchivePurpose.PPA, num_arch_builders)
-
-        return sub_query
-
     @staticmethod
     def postprocessCandidate(job, logger):
         """See `IBuildFarmJob`."""