← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~flacoste/launchpad/bug-805634 into lp:launchpad

 

Francis J. Lacoste has proposed merging lp:~flacoste/launchpad/bug-805634 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #805634 in Launchpad itself: "Raise priority of time-sensitive copy archives"
  https://bugs.launchpad.net/launchpad/+bug/805634

For more details, see:
https://code.launchpad.net/~flacoste/launchpad/bug-805634/+merge/67077

= Summary =

Ubuntu is relying more and more on rebuilds for their QA. Currently, rebuilds
have the lowest priority in the build farm which means that it can take for up
to 24 days for the rebuild to complete. That's too long to be useful to do
anything with the results as part of the regular development schedule.

== Proposed fix ==

For QA-related rebuilds which are time-sensitive, add a --raise-priority
option to populate-archive which will give the same priorities to these builds
than other regular PPA builds.

That will significant impact on other PPA users as no regular PPA builds will
be processed until the rebuild of main is completed.  We intend to increase
the build farm size to limit the impact. Other higher priority builds (OEM,
security, etc.) will continue to be processed first.

== Pre-implementation notes ==

When --raise-priority is selected, we add a relative_build_score to the
created archive copy that will offset the penalty received for COPY archive.

== Implementation details ==

ALl rebuilds were scored at -10, and the archive relative_build_score wasn't
applied. I modified the score computation to use the regular scoring mechanism
(score depends on component, urgency, and pocket) and then apply a hefty
penalty to bring the score below zero. But the result will be that the rebuild
will start with all of main before doing universe.

I've made some clean-up to some factory helpers to have better consistency on the archive selected.

== Tests ==

./bin/test -vvt test_populatearchive|TestBuildPackageJobScore

== Demo and Q/A ==

Run populate-archive with --raise-priority and look at the relate build score
on the web.

Should we do more?

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/model/buildpackagejob.py
  lib/lp/testing/factory.py
  lib/lp/soyuz/tests/test_buildpackagejob.py
  lib/lp/soyuz/interfaces/buildpackagejob.py
  lib/lp/soyuz/scripts/tests/test_populatearchive.py
  lib/lp/soyuz/scripts/populate_archive.py
-- 
https://code.launchpad.net/~flacoste/launchpad/bug-805634/+merge/67077
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~flacoste/launchpad/bug-805634 into lp:launchpad.
=== modified file 'lib/lp/soyuz/interfaces/buildpackagejob.py'
--- lib/lp/soyuz/interfaces/buildpackagejob.py	2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/interfaces/buildpackagejob.py	2011-07-06 19:43:45 +0000
@@ -9,6 +9,11 @@
 
 __all__ = [
     'IBuildPackageJob',
+    'COPY_ARCHIVE_SCORE_PENALTY',
+    'PRIVATE_ARCHIVE_SCORE_BONUS',
+    'SCORE_BY_COMPONENT',
+    'SCORE_BY_POCKET',
+    'SCORE_BY_URGENCY',
     ]
 
 from lazr.restful.fields import Reference
@@ -16,10 +21,48 @@
 
 from canonical.launchpad import _
 from lp.services.job.interfaces.job import IJob
+from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.sourcepackage import SourcePackageUrgency
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuild
 from lp.soyuz.interfaces.buildfarmbuildjob import IBuildFarmBuildJob
 
 
+SCORE_BY_POCKET = {
+    PackagePublishingPocket.BACKPORTS: 0,
+    PackagePublishingPocket.RELEASE: 1500,
+    PackagePublishingPocket.PROPOSED: 3000,
+    PackagePublishingPocket.UPDATES: 3000,
+    PackagePublishingPocket.SECURITY: 4500,
+}
+
+
+SCORE_BY_COMPONENT = {
+    'multiverse': 0,
+    'universe': 250,
+    'restricted': 750,
+    'main': 1000,
+    'partner': 1250,
+}
+
+
+SCORE_BY_URGENCY = {
+    SourcePackageUrgency.LOW: 5,
+    SourcePackageUrgency.MEDIUM: 10,
+    SourcePackageUrgency.HIGH: 15,
+    SourcePackageUrgency.EMERGENCY: 20,
+}
+
+
+PRIVATE_ARCHIVE_SCORE_BONUS = 10000
+
+
+# Rebuilds have usually a lower priority than other builds.
+# This will be subtracted from the final score, usually taking it
+# below 0, ensuring they are built only when nothing else is waiting
+# in the build farm.
+COPY_ARCHIVE_SCORE_PENALTY = 2600
+
+
 class IBuildPackageJob(IBuildFarmBuildJob):
     """A read-only interface for build package jobs."""
 

=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py	2010-11-26 17:05:59 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py	2011-07-06 19:43:45 +0000
@@ -22,14 +22,21 @@
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.model.buildfarmjob import BuildFarmJobOldDerived
 from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.registry.interfaces.sourcepackage import SourcePackageUrgency
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.soyuz.enums import (
     ArchivePurpose,
     PackagePublishingStatus,
     )
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
-from lp.soyuz.interfaces.buildpackagejob import IBuildPackageJob
+from lp.soyuz.interfaces.buildpackagejob import (
+    COPY_ARCHIVE_SCORE_PENALTY,
+    IBuildPackageJob,
+    PRIVATE_ARCHIVE_SCORE_BONUS,
+    SCORE_BY_COMPONENT,
+    SCORE_BY_POCKET,
+    SCORE_BY_URGENCY,
+    )
+
 from lp.soyuz.model.buildfarmbuildjob import BuildFarmBuildJob
 
 
@@ -58,29 +65,6 @@
 
     def score(self):
         """See `IBuildPackageJob`."""
-        score_pocketname = {
-            PackagePublishingPocket.BACKPORTS: 0,
-            PackagePublishingPocket.RELEASE: 1500,
-            PackagePublishingPocket.PROPOSED: 3000,
-            PackagePublishingPocket.UPDATES: 3000,
-            PackagePublishingPocket.SECURITY: 4500,
-            }
-
-        score_componentname = {
-            'multiverse': 0,
-            'universe': 250,
-            'restricted': 750,
-            'main': 1000,
-            'partner': 1250,
-            }
-
-        score_urgency = {
-            SourcePackageUrgency.LOW: 5,
-            SourcePackageUrgency.MEDIUM: 10,
-            SourcePackageUrgency.HIGH: 15,
-            SourcePackageUrgency.EMERGENCY: 20,
-            }
-
         # Define a table we'll use to calculate the score based on the time
         # in the build queue.  The table is a sorted list of (upper time
         # limit in seconds, score) tuples.
@@ -93,52 +77,46 @@
             (300, 5),
         ]
 
-        private_archive_increment = 10000
-
-        # For build jobs in rebuild archives a score value of -1
-        # was chosen because their priority is lower than build retries
-        # or language-packs. They should be built only when there is
-        # nothing else to build.
-        rebuild_archive_score = -10
-
-        score = 0
-
         # Please note: the score for language packs is to be zero because
         # they unduly delay the building of packages in the main component
         # otherwise.
         if self.build.source_package_release.section.name == 'translations':
-            pass
-        elif self.build.archive.is_copy:
-            score = rebuild_archive_score
-        else:
-            # Calculates the urgency-related part of the score.
-            urgency = score_urgency[self.build.source_package_release.urgency]
-            score += urgency
-
-            # Calculates the pocket-related part of the score.
-            score_pocket = score_pocketname[self.build.pocket]
-            score += score_pocket
-
-            # Calculates the component-related part of the score.
-            score += score_componentname.get(
-                self.build.current_component.name, 0)
-
-            # Calculates the build queue time component of the score.
-            right_now = datetime.now(pytz.timezone('UTC'))
-            eta = right_now - self.job.date_created
-            for limit, dep_score in queue_time_scores:
-                if eta.seconds > limit:
-                    score += dep_score
-                    break
-
-            # Private builds get uber score.
-            if self.build.archive.private:
-                score += private_archive_increment
-
-            # Lastly, apply the archive score delta.  This is to boost
-            # or retard build scores for any build in a particular
-            # archive.
-            score += self.build.archive.relative_build_score
+            return 0
+
+        score = 0
+
+        # Calculates the urgency-related part of the score.
+        urgency = SCORE_BY_URGENCY[
+            self.build.source_package_release.urgency]
+        score += urgency
+
+        # Calculates the pocket-related part of the score.
+        score_pocket = SCORE_BY_POCKET[self.build.pocket]
+        score += score_pocket
+
+        # Calculates the component-related part of the score.
+        score += SCORE_BY_COMPONENT.get(
+            self.build.current_component.name, 0)
+
+        # Calculates the build queue time component of the score.
+        right_now = datetime.now(pytz.timezone('UTC'))
+        eta = right_now - self.job.date_created
+        for limit, dep_score in queue_time_scores:
+            if eta.seconds > limit:
+                score += dep_score
+                break
+
+        # Private builds get uber score.
+        if self.build.archive.private:
+            score += PRIVATE_ARCHIVE_SCORE_BONUS
+
+        if self.build.archive.is_copy:
+            score -= COPY_ARCHIVE_SCORE_PENALTY
+
+        # Lastly, apply the archive score delta.  This is to boost
+        # or retard build scores for any build in a particular
+        # archive.
+        score += self.build.archive.relative_build_score
 
         return score
 
@@ -182,9 +160,6 @@
     @staticmethod
     def addCandidateSelectionCriteria(processor, virtualized):
         """See `IBuildFarmJob`."""
-        # Avoiding circular import.
-        from lp.buildmaster.model.builder import Builder
-
         private_statuses = (
             PackagePublishingStatus.PUBLISHED,
             PackagePublishingStatus.SUPERSEDED,

=== modified file 'lib/lp/soyuz/scripts/populate_archive.py'
--- lib/lp/soyuz/scripts/populate_archive.py	2011-02-24 15:30:54 +0000
+++ lib/lp/soyuz/scripts/populate_archive.py	2011-07-06 19:43:45 +0000
@@ -17,6 +17,7 @@
 from lp.app.validators.name import valid_name
 from lp.soyuz.adapters.packagelocation import build_package_location
 from lp.soyuz.enums import ArchivePurpose
+from lp.soyuz.interfaces.buildpackagejob import COPY_ARCHIVE_SCORE_PENALTY
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.packagecloner import IPackageCloner
 from lp.soyuz.interfaces.processor import IProcessorFamilySet
@@ -110,7 +111,7 @@
             """Associate the archive with the processor families."""
             aa_set = getUtility(IArchiveArchSet)
             for proc_family in proc_families:
-                ignore_this = aa_set.new(archive, proc_family)
+                aa_set.new(archive, proc_family)
 
         def build_location(distro, suite, component, packageset_names=None):
             """Build and return package location."""
@@ -223,6 +224,10 @@
             # Associate the newly created copy archive with the processor
             # families specified by the user.
             set_archive_architectures(copy_archive, proc_families)
+
+            # If --raise-priority was specified, offset the penalty
+            # normally assigned to copy builds.
+            copy_archive.relative_build_score = COPY_ARCHIVE_SCORE_PENALTY
         else:
             # Archive name clash! Creation requested for existing archive with
             # the same name and distribution.
@@ -279,7 +284,7 @@
         changed.
         """
         pkg_cloner = getUtility(IPackageCloner)
-        ignore_result = pkg_cloner.packageSetDiff(
+        pkg_cloner.packageSetDiff(
             origin, destination, self.logger)
 
     def mainTask(self):
@@ -392,3 +397,9 @@
             "--nonvirtualized", dest="nonvirtualized", default=False,
             action="store_true",
             help='Create the archive as nonvirtual if specified.')
+
+        self.parser.add_option(
+            "--raise-priority", dest="priority", default=False,
+            action="store_true",
+            help='The builds from this copy archive will use the regular '
+            'score instead of the lower COPY score.')

=== modified file 'lib/lp/soyuz/scripts/tests/test_populatearchive.py'
--- lib/lp/soyuz/scripts/tests/test_populatearchive.py	2010-12-23 01:02:00 +0000
+++ lib/lp/soyuz/scripts/tests/test_populatearchive.py	2011-07-06 19:43:45 +0000
@@ -26,6 +26,7 @@
     )
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
+from lp.soyuz.interfaces.buildpackagejob import COPY_ARCHIVE_SCORE_PENALTY
 from lp.soyuz.scripts.ftpmaster import (
     PackageLocationError,
     SoyuzScriptError,
@@ -701,3 +702,27 @@
         # Make sure the source to be copied are the ones we expect (this
         # should break in case of a sample data change/corruption).
         self.assertEqual(src_names, self.expected_src_names)
+
+    def testRaisePriority(self):
+        # The --raise-priority option should create a copy archive
+        # with a relative_build_score of 10 to offset the -10 of copy
+        # archives.
+        series = self.factory.makeDistroSeries()
+        archive_name = self.factory.getUniqueString('copy-archive')
+        script = self.getScript([
+            '--from-distribution', series.distribution.name,
+            '--from-suite', series.name,
+            '--to-distribution', series.distribution.name,
+            '--to-suite', series.name,
+            '--to-archive', archive_name,
+            '--to-user', series.registrant.name,
+            '--reason', 'testing!',
+            '--architecture', '386',
+            '--raise-priority'])
+        script.mainTask()
+
+        copy_archive = getUtility(IArchiveSet).getByDistroPurpose(
+            series.distribution, ArchivePurpose.COPY, archive_name)
+        self.assertIsNotNone(copy_archive, "COPY archive wasn't created")
+        self.assertEquals(
+            COPY_ARCHIVE_SCORE_PENALTY, copy_archive.relative_build_score)

=== modified file 'lib/lp/soyuz/tests/test_buildpackagejob.py'
--- lib/lp/soyuz/tests/test_buildpackagejob.py	2010-10-04 19:50:45 +0000
+++ lib/lp/soyuz/tests/test_buildpackagejob.py	2011-07-06 19:43:45 +0000
@@ -50,7 +50,7 @@
         builder = None
         builders = test.builders.get(builder_key(build), [])
         try:
-            builder = builders[n-1]
+            builder = builders[n - 1]
         except IndexError:
             pass
         return builder
@@ -256,16 +256,41 @@
     layer = DatabaseFunctionalLayer
 
     def test_score_unusual_component(self):
-        unusual_component = self.factory.makeComponent(name="unusual")
-        source_package_release = self.factory.makeSourcePackageRelease()
-        self.factory.makeSourcePackagePublishingHistory(
-            sourcepackagerelease=source_package_release,
-            component=unusual_component,
-            archive=source_package_release.upload_archive,
-            distroseries=source_package_release.upload_distroseries)
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            component='unusual')
         build = self.factory.makeBinaryPackageBuild(
-            source_package_release=source_package_release)
+            source_package_release=spph.sourcepackagerelease)
         build.queueBuild()
         job = build.buildqueue_record.specific_job
         # For now just test that it doesn't raise an Exception
         job.score()
+
+    def test_main_release_low_score(self):
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            component='main', urgency='low')
+        build = self.factory.makeBinaryPackageBuild(
+            source_package_release=spph.sourcepackagerelease,
+            pocket='RELEASE')
+        job = build.makeJob()
+        self.assertEquals(2505, job.score())
+
+    def test_copy_archive_main_release_low_score(self):
+        copy_archive = self.factory.makeArchive(purpose='COPY')
+        spph = self.factory.makeSourcePackagePublishingHistory(
+           archive=copy_archive, component='main', urgency='low')
+        build = self.factory.makeBinaryPackageBuild(
+            source_package_release=spph.sourcepackagerelease,
+            pocket='RELEASE')
+        job = build.makeJob()
+        self.assertEquals(-95, job.score())
+
+    def test_copy_archive_relative_score_is_applied(self):
+        copy_archive = self.factory.makeArchive(purpose='COPY')
+        removeSecurityProxy(copy_archive).relative_build_score = 2600
+        spph = self.factory.makeSourcePackagePublishingHistory(
+           archive=copy_archive, component='main', urgency='low')
+        build = self.factory.makeBinaryPackageBuild(
+            source_package_release=spph.sourcepackagerelease,
+            pocket='RELEASE')
+        job = build.makeJob()
+        self.assertEquals(2505, job.score())

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-07-01 13:57:52 +0000
+++ lib/lp/testing/factory.py	2011-07-06 19:43:45 +0000
@@ -2553,6 +2553,9 @@
         """
         if purpose is None:
             purpose = ArchivePurpose.PPA
+        elif isinstance(purpose, basestring):
+            purpose = ArchivePurpose.items[purpose.upper()]
+
         if distribution is None:
             # See bug #568769
             if purpose == ArchivePurpose.PPA:
@@ -3485,20 +3488,20 @@
                     distribution=distribution)
 
         if archive is None:
-            archive = self.makeArchive(
-                distribution=distroseries.distribution,
-                purpose=ArchivePurpose.PRIMARY)
+            archive = distroseries.main_archive
 
         if (sourcepackagename is None or
             isinstance(sourcepackagename, basestring)):
             sourcepackagename = self.getOrMakeSourcePackageName(
                 sourcepackagename)
 
-        if component is None:
-            component = self.makeComponent()
+        if (component is None or isinstance(component, basestring)):
+            component = self.makeComponent(component)
 
         if urgency is None:
             urgency = self.getAnySourcePackageUrgency()
+        elif isinstance(urgency, basestring):
+            urgency = SourcePackageUrgency.items[urgency.upper()]
 
         section = self.makeSection(name=section_name)
 
@@ -3591,6 +3594,9 @@
                 processorfamily=processor.family)
         if pocket is None:
             pocket = PackagePublishingPocket.RELEASE
+        elif isinstance(pocket, basestring):
+            pocket = PackagePublishingPocket.items[pocket.upper()]
+
         if source_package_release is None:
             multiverse = self.makeComponent(name='multiverse')
             source_package_release = self.makeSourcePackageRelease(
@@ -3618,66 +3624,66 @@
         IStore(binary_package_build).flush()
         return binary_package_build
 
-    def makeSourcePackagePublishingHistory(self, sourcepackagename=None,
-                                           distroseries=None, maintainer=None,
-                                           creator=None, component=None,
-                                           section_name=None,
-                                           urgency=None, version=None,
+    def makeSourcePackagePublishingHistory(self,
+                                           distroseries=None,
                                            archive=None,
-                                           builddepends=None,
-                                           builddependsindep=None,
-                                           build_conflicts=None,
-                                           build_conflicts_indep=None,
-                                           architecturehintlist='all',
-                                           dateremoved=None,
-                                           date_uploaded=UTC_NOW,
+                                           sourcepackagerelease=None,
                                            pocket=None,
                                            status=None,
+                                           dateremoved=None,
+                                           date_uploaded=UTC_NOW,
                                            scheduleddeletiondate=None,
-                                           dsc_standards_version='3.6.2',
-                                           dsc_format='1.0',
-                                           dsc_binaries='foo-bin',
-                                           sourcepackagerelease=None,
                                            ancestor=None,
+                                           **kwargs
                                            ):
-        """Make a `SourcePackagePublishingHistory`."""
+        """Make a `SourcePackagePublishingHistory`.
+
+        :param sourcepackagerelease: The source package release to publish
+            If not provided, a new one will be created.
+        :param distroseries: The distro series in which to publish.
+            Default to the source package release one, or a new one will
+            be created when not provided.
+        :param archive: The archive to publish into. Default to the
+            initial source package release  upload archive, or to the
+            distro series main archive.
+        :param pocket: The pocket to publish into. Can be specified as a
+            string. Defaults to the RELEASE pocket.
+        :param status: The publication status. Defaults to PENDING. If
+            set to PUBLISHED, the publisheddate will be set to now.
+        :param dateremoved: The removal date.
+        :param date_uploaded: The upload date. Defaults to now.
+        :param scheduleddeletiondate: The date where the publication
+            is scheduled to be removed.
+        :param ancestor: The publication ancestor parameter.
+        :param **kwargs: All other parameters are passed through to the
+            makeSourcePackageRelease call if needed.
+        """
+        if sourcepackagerelease is not None:
+            distroseries = sourcepackagerelease.upload_distroseries
+            archive = distroseries.main_archive
+
         if distroseries is None:
             if archive is None:
                 distribution = None
             else:
                 distribution = archive.distribution
             distroseries = self.makeDistroRelease(distribution=distribution)
-
         if archive is None:
-            archive = self.makeArchive(
-                distribution=distroseries.distribution,
-                purpose=ArchivePurpose.PRIMARY)
+            archive = distroseries.main_archive
 
         if pocket is None:
             pocket = self.getAnyPocket()
+        elif isinstance(pocket, basestring):
+            pocket = PackagePublishingPocket.items[pocket.upper()]
 
         if status is None:
             status = PackagePublishingStatus.PENDING
+        elif isinstance(status, basestring):
+            status = PackagePublishingStatus.items[status.upper()]
 
         if sourcepackagerelease is None:
             sourcepackagerelease = self.makeSourcePackageRelease(
-                archive=archive,
-                sourcepackagename=sourcepackagename,
-                distroseries=distroseries,
-                maintainer=maintainer,
-                creator=creator, component=component,
-                section_name=section_name,
-                urgency=urgency,
-                version=version,
-                builddepends=builddepends,
-                builddependsindep=builddependsindep,
-                build_conflicts=build_conflicts,
-                build_conflicts_indep=build_conflicts_indep,
-                architecturehintlist=architecturehintlist,
-                dsc_standards_version=dsc_standards_version,
-                dsc_format=dsc_format,
-                dsc_binaries=dsc_binaries,
-                date_uploaded=date_uploaded)
+                archive=archive, distroseries=distroseries, **kwargs)
 
         spph = getUtility(IPublishingSet).newSourcePublication(
             archive, sourcepackagerelease, distroseries,


Follow ups