Merge lp:~al-maisan/launchpad/disabled-copyarch-519665 into lp:launchpad

[Muharem Hrnjadovic]

[Muharem Hrnjadovic (al-maisan)]

Hello there,

The new generalized build farm candidate job selection logic ignores jobs with status JobStatus.SUSPENDED.

This branch tweaks the binary build creation process so that build jobs are created in suspended mode for disabled archives. That prevents them from getting dispatched to builders "by accident".

Tests to run:

    bin/test -vv -t build

No pertinent "make lint" errors or warnings.

[Gavin Panella (allenap)]

Looks great :)

My only comment is about the following function you use in the test:

> def build_not_pending_and_suspended(build):
> """True if the given build is not pending and suspended."""
> return (
> build.buildstate != BuildStatus.NEEDSBUILD or
> build.buildqueue_record.job.status != JobStatus.SUSPENDED)

It absolutely does not need to change, but consider writing it like
you described it:

  def build_not_pending_and_suspended(build):
      """True if the given build is not pending and suspended."""
      return not (
          build.buildstate == BuildStatus.NEEDSBUILD and
          build.buildqueue_record.job.status == JobStatus.SUSPENDED)

It just took me a few extra mental leaps - which meant a few minutes
in my current state :) - to verify that the description matched the
implementation.

review: Approve

[Jeroen T. Vermeulen (jtv)]

I have a few additional notes that Gavin thought were worth adding:

 * What did the pre-imp conclude about the problem of re-enabling archives? Is it not going to be a problem to un-suspend jobs when that happens?

 * In build_not_pending_and_suspended, the "not" is ambiguous in scope. Can't you replace this with two separate checks?

 * In production code we check "len(foo) == 0" to check for empty lists. But in tests it's nicer to compare "foo == []" so that a failure will give more information.

 * With assertEqual, pass expected value first and actual value second! Again, better failure messages.

Jeroen

review: Needs Fixing (code)

[Muharem Hrnjadovic (al-maisan)]

On 02/10/2010 06:00 PM, Jeroen T. Vermeulen wrote:
> Review: Needs Fixing code
> I have a few additional notes that Gavin thought were worth adding:
Hello,

thanks for posing these questions, they are all pertinent and good.

> * What did the pre-imp conclude about the problem of re-enabling
> archives? Is it not going to be a problem to un-suspend jobs when
> that happens?
We already have a mechanism that takes care of suspending/un-suspending
binary build jobs when the associated archive is disabled and enabled
respectively.
Please see Archive.enable() and Archive.disable() for more detail.

The problem in this specific case is that the so called copy archives
are instantiated in a disabled state. Hence there is no

    "enabled -> disabled"

state transition for these and their binary build jobs are created in an
unsuspended state which is a bug.

Please note also that the branch at hand enforces the constraint the
binary builds for disabled archives should be (created) suspended
irrespective of archive type (although the problem manifested itself in
a copy archive context).

> * In build_not_pending_and_suspended, the "not" is ambiguous in
> scope. Can't you replace this with two separate checks?
Hmm .. maybe I can name it better. How about

    - build_in_wrong_state() plus
    - an explanation what "wrong" means in the case at hand

> * In production code we check "len(foo) == 0" to check for empty
> lists. But in tests it's nicer to compare "foo == []" so that a
> failure will give more information.
OK. Done.

> * With assertEqual, pass expected value first and actual value
> second! Again, better failure messages.
Oh, yes! Fixed that as well.

Please see also the enclosed incremental diff.

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

[Jeroen T. Vermeulen (jtv)]

All good. Thanks for adding the final polish!

review: Approve (code)

=== modified file 'lib/lp/buildmaster/interfaces/buildbase.py'
--- lib/lp/buildmaster/interfaces/buildbase.py	2010-02-02 16:44:53 +0000
+++ lib/lp/buildmaster/interfaces/buildbase.py	2010-02-11 06:25:26 +0000
@@ -129,8 +129,12 @@
         Invoke getFileFromSlave method with 'buildlog' identifier.
         """
 
-    def queueBuild():
-        """Create a BuildQueue entry for this build."""
+    def queueBuild(suspended=False):
+        """Create a BuildQueue entry for this build.
+
+        :param suspended: Whether the associated `Job` instance should be
+            created in a suspended state.
+        """
 
     def estimateDuration():
         """Estimate the build duration."""

=== modified file 'lib/lp/buildmaster/model/buildbase.py'
--- lib/lp/buildmaster/model/buildbase.py	2010-02-03 16:21:57 +0000
+++ lib/lp/buildmaster/model/buildbase.py	2010-02-11 06:25:26 +0000
@@ -356,9 +356,14 @@
         self.buildduration = RIGHT_NOW - self.buildqueue_record.date_started
         self.dependencies = slave_status.get('dependencies')
 
-    def queueBuild(self):
+    def queueBuild(self, suspended=False):
         """See `IBuildBase`"""
         specific_job = self.makeJob()
+
+        # This build queue job is to be created in a suspended state.
+        if suspended:
+            specific_job.job.suspend()
+
         duration_estimate = self.estimateDuration()
         queue_entry = BuildQueue(
             estimated_duration=duration_estimate,

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2010-01-31 19:26:34 +0000
+++ lib/lp/soyuz/model/publishing.py	2010-02-11 06:25:26 +0000
@@ -656,7 +656,8 @@
 
         build = self.sourcepackagerelease.createBuild(
             distroarchseries=arch, archive=self.archive, pocket=self.pocket)
-        build_queue = build.queueBuild()
+        # Create the builds in suspended mode for disabled archives.
+        build_queue = build.queueBuild(suspended=not self.archive.enabled)
         build_queue.score()
         Store.of(build).flush()
 

=== modified file 'lib/lp/soyuz/scripts/tests/test_populatearchive.py'
--- lib/lp/soyuz/scripts/tests/test_populatearchive.py	2010-01-12 09:01:38 +0000
+++ lib/lp/soyuz/scripts/tests/test_populatearchive.py	2010-02-11 06:25:26 +0000
@@ -3,34 +3,33 @@
 
 __metaclass__ = type
 
+from datetime import datetime
 import os
 import subprocess
 import sys
 import time
 import unittest
 
-from datetime import datetime
-
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
+from canonical.launchpad.scripts import BufferLogger
+from canonical.testing import LaunchpadZopelessLayer
+from canonical.testing.layers import DatabaseLayer
 from lp.registry.interfaces.distribution import IDistributionSet
+from lp.registry.interfaces.person import IPersonSet
+from lp.services.job.interfaces.job import JobStatus
 from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
+from lp.soyuz.interfaces.archivearch import IArchiveArchSet
 from lp.soyuz.interfaces.build import BuildStatus, IBuildSet
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
-from lp.soyuz.interfaces.archivearch import IArchiveArchSet
 from lp.soyuz.interfaces.packagecopyrequest import (
     IPackageCopyRequestSet, PackageCopyStatus)
-from lp.registry.interfaces.person import IPersonSet
-from lp.soyuz.scripts.ftpmaster import (
-    PackageLocationError, SoyuzScriptError)
+from lp.soyuz.scripts.ftpmaster import PackageLocationError, SoyuzScriptError
 from lp.soyuz.scripts.populate_archive import ArchivePopulator
-from canonical.launchpad.scripts import BufferLogger
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import TestCase
-from canonical.testing import LaunchpadZopelessLayer
-from canonical.testing.layers import DatabaseLayer
 
 
 def get_spn(build):
@@ -650,6 +649,39 @@
         rset = getUtility(IArchiveArchSet).getByArchive(copy_archive)
         self.assertEqual(get_family_names(rset), [u'amd64', u'x86'])
 
+    def testBuildsPendingAndSuspended(self):
+        """All builds in the new copy archive are pending and suspended."""
+        def build_in_wrong_state(build):
+            """True if the given build is not (pending and suspended)."""
+            return not (
+                build.buildstate == BuildStatus.NEEDSBUILD and
+                build.buildqueue_record.job.status == JobStatus.SUSPENDED)
+        hoary = getUtility(IDistributionSet)['ubuntu']['hoary']
+
+        # Verify that we have the right source packages in the sample data.
+        self._verifyPackagesInSampleData(hoary)
+
+        extra_args = ['-a', '386']
+        archive = self.runScript(extra_args=extra_args, exists_after=True)
+
+        # Make sure the right source packages were cloned.
+        self._verifyClonedSourcePackages(archive, hoary)
+
+        # Get the binary builds generated for the copy archive at hand.
+        builds = list(getUtility(IBuildSet).getBuildsForArchive(archive))
+        # At least one binary build was generated for the target copy archive.
+        self.assertTrue(len(builds) > 0)
+        # Now check that the binary builds and their associated job records
+        # are in the state expected:
+        #   - binary build: pending
+        #   - job: suspended
+        builds_in_wrong_state = filter(build_in_wrong_state, builds)
+        self.assertEqual (
+            [], builds_in_wrong_state,
+            "The binary builds generated for the target copy archive "
+            "should all be pending and suspended. However, at least one of "
+            "the builds is in the wrong state.")
+
     def testPrivateOriginArchive(self):
         """Try copying from a private archive.