Merge lp:~michael.nelson/launchpad/450124-findBuildCandidate_improvements into lp:launchpad

[Michael Nelson]

[Michael Nelson (michael.nelson)]

= Summary =

Ensures that private PPAs are able to have builds for the same
architecture building in parallel.

== Pre-implementation notes ==

Chatted with Julian about other options - see bug 450124 for
documentation of those discussions.

== Implementation details ==

I had to create a unittest as the documentation is getting unreadable. I
also reverted the additional specific test that I had added to the
documentation now that it is covered by the unittest.

== Tests ==

bin/test -vvt TestFindBuildCandidate

== Demo and Q/A ==

= 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/testing/factory.py
  lib/lp/soyuz/doc/buildd-dispatching.txt
  lib/lp/soyuz/tests/test_builder.py
  lib/lp/soyuz/interfaces/builder.py

--
Michael

[Gavin Panella (allenap)]

Hi Michael,

I have a few comments about the tests which you can take or
leave. Other than that, all good as far as I can tell :)

Gavin.

[...]
> === added file 'lib/lp/soyuz/tests/test_builder.py'
> --- lib/lp/soyuz/tests/test_builder.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/tests/test_builder.py 2009-10-14 10:55:53 +0000
> @@ -0,0 +1,116 @@
> +# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Test Builder features."""
> +
> +import unittest
> +
> +from zope.component import getUtility
> +
> +from canonical.testing import LaunchpadZopelessLayer
> +from lp.soyuz.interfaces.archive import ArchivePurpose
> +from lp.soyuz.interfaces.builder import IBuilderSet
> +from lp.soyuz.interfaces.build import BuildStatus
> +from lp.soyuz.interfaces.publishing import PackagePublishingStatus
> +from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
> +from lp.testing import TestCaseWithFactory
> +
> +
> +class TestFindBuildCandidate(TestCaseWithFactory):
> +
> + layer = LaunchpadZopelessLayer
> +
> + def setUp(self):
> + """Publish some builds for the test archive."""
> + super(TestFindBuildCandidate, self).setUp()
> + self.publisher = SoyuzTestPublisher()
> + self.publisher.prepareBreezyAutotest()
> +
> + # Create two PPAs and add some builds to each.
> + 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()
> +
> + # 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')
> +
> + # 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

That looks like a lot of set-up. If there's anything there that's
specific to only one test_* method, consider moving it o...

review: Approve

[Michael Nelson (michael.nelson)]

Gavin Panella wrote:
> Review: Approve
> Hi Michael,
>
> I have a few comments about the tests which you can take or
> leave. Other than that, all good as far as I can tell :)
>
> Gavin.

Thanks Gavin.

>
>
> [...]
>> === added file 'lib/lp/soyuz/tests/test_builder.py'
>> --- lib/lp/soyuz/tests/test_builder.py 1970-01-01 00:00:00 +0000
>> +++ lib/lp/soyuz/tests/test_builder.py 2009-10-14 10:55:53 +0000
>> @@ -0,0 +1,116 @@
>> +# Copyright 2009 Canonical Ltd. This software is licensed under the
>> +# GNU Affero General Public License version 3 (see the file LICENSE).
>> +
>> +"""Test Builder features."""
>> +
>> +import unittest
>> +
>> +from zope.component import getUtility
>> +
>> +from canonical.testing import LaunchpadZopelessLayer
>> +from lp.soyuz.interfaces.archive import ArchivePurpose
>> +from lp.soyuz.interfaces.builder import IBuilderSet
>> +from lp.soyuz.interfaces.build import BuildStatus
>> +from lp.soyuz.interfaces.publishing import PackagePublishingStatus
>> +from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
>> +from lp.testing import TestCaseWithFactory
>> +
>> +
>> +class TestFindBuildCandidate(TestCaseWithFactory):
>> +
>> + layer = LaunchpadZopelessLayer
>> +
>> + def setUp(self):
>> + """Publish some builds for the test archive."""
>> + super(TestFindBuildCandidate, self).setUp()
>> + self.publisher = SoyuzTestPublisher()
>> + self.publisher.prepareBreezyAutotest()
>> +
>> + # Create two PPAs and add some builds to each.
>> + 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()
>> +
>> + # 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')
>> +
>> + # 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 = ...

[Gavin Panella (allenap)]

That's a beauty :)

=== modified file 'lib/lp/soyuz/doc/buildd-dispatching.txt'
--- lib/lp/soyuz/doc/buildd-dispatching.txt	2009-10-12 11:46:43 +0000
+++ lib/lp/soyuz/doc/buildd-dispatching.txt	2009-10-14 14:28:13 +0000
@@ -202,20 +202,15 @@
 
 == PPA build dispatching ==
 
-Create two Build records of the same source targeted for a PPA archive:
+Create a new Build record of the same source targeted for a PPA archive:
 
     >>> from lp.registry.interfaces.person import IPersonSet
     >>> cprov = getUtility(IPersonSet).getByName('cprov')
-    >>> warty = getUtility(IDistributionSet)['ubuntu']['warty']
-    >>> warty_i386 = warty['i386']
 
-    # One for hoary and one for warty.
     >>> ppa_build = sprf.sourcepackagerelease.createBuild(
     ...     hoary_i386, PackagePublishingPocket.RELEASE, cprov.archive)
-    >>> ppa_build_2 = sprf.sourcepackagerelease.createBuild(
-    ...     warty_i386, PackagePublishingPocket.RELEASE, cprov.archive)
 
-Create BuildQueue records and inspect some parameters:
+Create BuildQueue record and inspect some parameters:
 
     >>> ppa_job = ppa_build.createBuildQueueEntry()
     >>> ppa_job.id
@@ -224,9 +219,6 @@
     True
     >>> ppa_job.buildstart == None
     True
-    >>> ppa_job_2 = ppa_build_2.createBuildQueueEntry()
-    >>> ppa_job_2.id
-    4
 
 The build job's archive requires virtualized builds.
 
@@ -248,10 +240,10 @@
     >>> print job
     None
 
-In order to enable 'bob' to find and build the PPA jobs, we have to
+In order to enable 'bob' to find and build the PPA job, we have to
 change it to virtualized.  This is because PPA builds will only build
-on virtualized builders.  We also need to make sure the builds' sources
-are published, or they will also be ignored (by superseding it).  We can
+on virtualized builders.  We also need to make sure this build's source
+is published, or it will also be ignored (by superseding it).  We can
 do this by copying the existing publication in Ubuntu.
 
     >>> from lp.soyuz.model.publishing import (
@@ -261,11 +253,6 @@
     ...    sourcepackagerelease=ppa_job.build.sourcepackagerelease)
     >>> new_pub = old_pub.copyTo(
     ...     old_pub.distroseries, old_pub.pocket, ppa_job.build.archive)
-    >>> [old_pub_2] = SourcePackagePublishingHistory.selectBy(
-    ...    distroseries=ppa_job_2.build.distroseries,
-    ...    sourcepackagerelease=ppa_job_2.build.sourcepackagerelease)
-    >>> new_pub_2 = old_pub_2.copyTo(
-    ...     old_pub_2.distroseries, old_pub_2.pocket, ppa_job_2.build.archive)
 
     >>> bob_builder.virtualized = True
     >>> syncUpdate(bob_builder)
@@ -274,6 +261,9 @@
     >>> ppa_job.id == job.id
     True
 
+For further details regarding IBuilder.findBuildCandidate() please see
+lib/lp/soyuz/tests/test_builder.py.
+
 Start buildd-slave to be able to dispatch jobs.
 
     >>> BuilddSlaveTestSetup().setUp()
@@ -309,30 +299,14 @@
     >>> ppa_job.buildstart == get_transaction_timestamp()
     True
 
-At this point, the second build job - for the same archive and architecture -
-will not be found as a build candidate for other builders. This is to
-avoid nightly builds using all the builders for an arch at once.
-
-    >>> print frog_builder.findBuildCandidate()
-    None
-
-But once the first build (for the same archive and architecture) finishes,
-the second build will be available as a build candidate for both builders.
-
-    >>> ppa_job.build.buildstate = BuildStatus.FAILEDTOBUILD
-    >>> ppa_job.destroySelf()
-    >>> print frog_builder.findBuildCandidate().id
-    4
-    >>> print bob_builder.findBuildCandidate().id
-    4
-
 Shutdown builder slave, mark the ppa build record as failed, remove the
 buildqueue record and make 'bob' builder non-virtual again,  so the
 environment is back to the initial state.
 
     >>> BuilddSlaveTestSetup().tearDown()
-    >>> ppa_job_2.build.buildstate = BuildStatus.FAILEDTOBUILD
-    >>> ppa_job_2.destroySelf()
+
+    >>> ppa_job.build.buildstate = BuildStatus.FAILEDTOBUILD
+    >>> ppa_job.destroySelf()
     >>> bob_builder.virtualized = False
     >>> flush_database_updates()
 
@@ -355,7 +329,7 @@
 
     >>> sec_job = sec_build.createBuildQueueEntry()
     >>> sec_job.id
-    5
+    4
     >>> print sec_job.builder
     None
     >>> print sec_job.buildstart

=== modified file 'lib/lp/soyuz/interfaces/builder.py'
--- lib/lp/soyuz/interfaces/builder.py	2009-08-27 14:48:38 +0000
+++ lib/lp/soyuz/interfaces/builder.py	2009-10-14 14:28:13 +0000
@@ -262,6 +262,10 @@
 
         The pending BuildQueue item with the highest score for this builder
         ProcessorFamily or None if no candidate is available.
+
+        For public PPA builds, subsequent builds for a given ppa and
+        architecture will not be returned until the current build for
+        the ppa and architecture is finished.
         """
 
     def dispatchBuildCandidate(candidate):

=== modified file 'lib/lp/soyuz/model/builder.py'
--- lib/lp/soyuz/model/builder.py	2009-10-12 15:56:25 +0000
+++ lib/lp/soyuz/model/builder.py	2009-10-14 14:28:13 +0000
@@ -570,8 +570,8 @@
         """ % sqlvalues(self.virtualized))
 
         # Ensure that if a currently-building build exists for the same
-        # ppa archive and architecture currently building then we don't
-        # consider another as a candidate.
+        # public ppa archive and architecture currently building then
+        # we don't consider another as a candidate.
         clauses.append("""
             NOT EXISTS (
                 SELECT Build.id
@@ -579,6 +579,7 @@
                 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)
@@ -586,7 +587,6 @@
             ArchivePurpose.PPA, self.processor.family, BuildStatus.BUILDING))
 
         query = " AND ".join(clauses)
-
         candidate = BuildQueue.selectFirst(
             query, clauseTables=clauseTables, prejoins=['build'],
             orderBy=['-buildqueue.lastscore', 'build.id'])

=== added file 'lib/lp/soyuz/tests/test_builder.py'
--- lib/lp/soyuz/tests/test_builder.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_builder.py	2009-10-14 14:28:13 +0000
@@ -0,0 +1,127 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test Builder features."""
+
+import unittest
+
+from zope.component import getUtility
+
+from canonical.testing import LaunchpadZopelessLayer
+from lp.soyuz.interfaces.archive import ArchivePurpose
+from lp.soyuz.interfaces.builder import IBuilderSet
+from lp.soyuz.interfaces.build import BuildStatus
+from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
+from lp.testing import TestCaseWithFactory
+
+
+class TestFindBuildCandidateBase(TestCaseWithFactory):
+    """Setup the test publisher and some builders."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestFindBuildCandidateBase, self).setUp()
+        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')
+
+
+class TestFindBuildCandidatePPA(TestFindBuildCandidateBase):
+
+    def setUp(self):
+        """Publish some builds for the test archive."""
+        super(TestFindBuildCandidatePPA, self).setUp()
+
+        # Create two PPAs and add some builds to each.
+        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
+
+    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()
+        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
+        # will be the next build candidate.
+        self.first_job.build.buildstate = BuildStatus.FAILEDTOBUILD
+        next_job = self.builder2.findBuildCandidate()
+        self.failUnlessEqual('joesppa', next_job.build.archive.name)
+
+    def test_findBuildCandidate_for_private_ppa(self):
+        # If a ppa is private it will be able to have parallel builds
+        # for the one architecture.
+        self.ppa_joe.private = True
+        self.ppa_joe.buildd_secret = 'sekrit'
+        next_job = self.builder2.findBuildCandidate()
+        self.failUnlessEqual('joesppa', next_job.build.archive.name)
+
+
+class TestFindBuildCandidateDistroArchive(TestFindBuildCandidateBase):
+
+    def setUp(self):
+        """Publish some builds for the test archive."""
+        super(TestFindBuildCandidateDistroArchive, self).setUp()
+        # Create a primary archive and publish some builds for the
+        # queue.
+        non_ppa = self.factory.makeArchive(
+            name="primary", purpose=ArchivePurpose.PRIMARY)
+
+        gedit_build = self.publisher.getPubSource(
+            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
+            archive=non_ppa).createMissingBuilds()[0]
+        firefox_build = self.publisher.getPubSource(
+            sourcename="firefox", status=PackagePublishingStatus.PUBLISHED,
+            archive=non_ppa).createMissingBuilds()[0]
+
+    def test_findBuildCandidate_for_non_ppa(self):
+        # Normal archives are not restricted to serial builds per
+        # arch.
+
+        next_job = self.builder2.findBuildCandidate()
+        self.failUnlessEqual('primary', next_job.build.archive.name)
+        self.failUnlessEqual(
+            'gedit', next_job.build.sourcepackagerelease.name)
+
+        # Now even if we set the build building, we'll still get the
+        # second non-ppa build for the same archive as the next candidate.
+        next_job.build.buildstate = BuildStatus.BUILDING
+        next_job.build.builder = self.builder2
+        next_job = self.builder2.findBuildCandidate()
+        self.failUnlessEqual('primary', next_job.build.archive.name)
+        self.failUnlessEqual(
+            'firefox', next_job.build.sourcepackagerelease.name)
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2009-10-06 00:01:08 +0000
+++ lib/lp/testing/factory.py	2009-10-14 14:28:13 +0000
@@ -106,6 +106,7 @@
     ISourcePackageNameSet)
 from lp.registry.interfaces.ssh import ISSHKeySet, SSHKeyType
 from lp.services.worlddata.interfaces.language import ILanguageSet
+from lp.soyuz.interfaces.builder import IBuilderSet
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.packageset import IPackagesetSet
 from lp.testing import run_with_login, time_counter
@@ -1485,6 +1486,34 @@
             owner=owner, purpose=purpose,
             distribution=distribution, name=name)
 
+    def makeBuilder(self, processor=None, url=None, name=None, title=None,
+                    description=None, owner=None, active=True,
+                    virtualized=True, vm_host=None):
+        """Make a new builder for i386 virtualized builds by default.
+
+        Note: the builder returned will not be able to actually build -
+        we currently have a build slave setup for 'bob' only in the
+        test environment.
+        See lib/canonical/buildd/tests/buildd-slave-test.conf
+        """
+        if processor is None:
+            processor_fam = ProcessorFamilySet().getByName('x86')
+            processor = processor_fam.processors[0]
+        if url is None:
+            url = 'http://%s:8221/' % self.getUniqueString()
+        if name is None:
+            name = self.getUniqueString()
+        if title is None:
+            title = self.getUniqueString()
+        if description is None:
+            description = self.getUniqueString()
+        if owner is None:
+            owner = self.makePerson()
+
+        return getUtility(IBuilderSet).new(
+            processor, url, name, title, description, owner, active,
+            virtualized, vm_host)
+
     def makePOTemplate(self, productseries=None, distroseries=None,
                        sourcepackagename=None, owner=None, name=None,
                        translation_domain=None, path=None):