Launchpad itself

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

  1. disabled-copyarch-519665
  2. Merge into devel
Proposed by Muharem Hrnjadovic
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~al-maisan/launchpad/disabled-copyarch-519665
Merge into: lp:launchpad
Diff against target: 138 lines (+55/-13)
4 files modified
lib/lp/buildmaster/interfaces/buildbase.py (+6/-2)
lib/lp/buildmaster/model/buildbase.py (+6/-1)
lib/lp/soyuz/model/publishing.py (+2/-1)
lib/lp/soyuz/scripts/tests/test_populatearchive.py (+41/-9)
To merge this branch: bzr merge lp:~al-maisan/launchpad/disabled-copyarch-519665
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) code Approve
Gavin Panella (community) Approve
Review via email: mp+19019@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Muharem Hrnjadovic (al-maisan) wrote :

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.

Revision history for this message
Gavin Panella (allenap) wrote :

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
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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)
Revision history for this message
Muharem Hrnjadovic (al-maisan) wrote :

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

disabled-copyarch-519665-inc.diff
1=== modified file 'lib/lp/soyuz/scripts/tests/test_populatearchive.py'
2--- lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-02-10 15:54:40 +0000
3+++ lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-02-11 06:22:10 +0000
4@@ -651,11 +651,11 @@
5
6 def testBuildsPendingAndSuspended(self):
7 """All builds in the new copy archive are pending and suspended."""
8- def build_not_pending_and_suspended(build):
9- """True if the given build is not pending and suspended."""
10- return (
11- build.buildstate != BuildStatus.NEEDSBUILD or
12- build.buildqueue_record.job.status != JobStatus.SUSPENDED)
13+ def build_in_wrong_state(build):
14+ """True if the given build is not (pending and suspended)."""
15+ return not (
16+ build.buildstate == BuildStatus.NEEDSBUILD and
17+ build.buildqueue_record.job.status == JobStatus.SUSPENDED)
18 hoary = getUtility(IDistributionSet)['ubuntu']['hoary']
19
20 # Verify that we have the right source packages in the sample data.
21@@ -667,11 +667,20 @@
22 # Make sure the right source packages were cloned.
23 self._verifyClonedSourcePackages(archive, hoary)
24
25- # Now check that we have the build records expected.
26+ # Get the binary builds generated for the copy archive at hand.
27 builds = list(getUtility(IBuildSet).getBuildsForArchive(archive))
28+ # At least one binary build was generated for the target copy archive.
29 self.assertTrue(len(builds) > 0)
30- wrong_builds = filter(build_not_pending_and_suspended, builds)
31- self.assertEqual(0, len(wrong_builds))
32+ # Now check that the binary builds and their associated job records
33+ # are in the state expected:
34+ # - binary build: pending
35+ # - job: suspended
36+ builds_in_wrong_state = filter(build_in_wrong_state, builds)
37+ self.assertEqual (
38+ [], builds_in_wrong_state,
39+ "The binary builds generated for the target copy archive "
40+ "should all be pending and suspended. However, at least one of "
41+ "the builds is in the wrong state.")
42
43 def testPrivateOriginArchive(self):
44 """Try copying from a private archive.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

All good. Thanks for adding the final polish!

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/buildmaster/interfaces/buildbase.py'
2--- lib/lp/buildmaster/interfaces/buildbase.py 2010-02-02 16:44:53 +0000
3+++ lib/lp/buildmaster/interfaces/buildbase.py 2010-02-11 06:25:26 +0000
4@@ -129,8 +129,12 @@
5 Invoke getFileFromSlave method with 'buildlog' identifier.
6 """
7
8- def queueBuild():
9- """Create a BuildQueue entry for this build."""
10+ def queueBuild(suspended=False):
11+ """Create a BuildQueue entry for this build.
12+
13+ :param suspended: Whether the associated `Job` instance should be
14+ created in a suspended state.
15+ """
16
17 def estimateDuration():
18 """Estimate the build duration."""
19
20=== modified file 'lib/lp/buildmaster/model/buildbase.py'
21--- lib/lp/buildmaster/model/buildbase.py 2010-02-03 16:21:57 +0000
22+++ lib/lp/buildmaster/model/buildbase.py 2010-02-11 06:25:26 +0000
23@@ -356,9 +356,14 @@
24 self.buildduration = RIGHT_NOW - self.buildqueue_record.date_started
25 self.dependencies = slave_status.get('dependencies')
26
27- def queueBuild(self):
28+ def queueBuild(self, suspended=False):
29 """See `IBuildBase`"""
30 specific_job = self.makeJob()
31+
32+ # This build queue job is to be created in a suspended state.
33+ if suspended:
34+ specific_job.job.suspend()
35+
36 duration_estimate = self.estimateDuration()
37 queue_entry = BuildQueue(
38 estimated_duration=duration_estimate,
39
40=== modified file 'lib/lp/soyuz/model/publishing.py'
41--- lib/lp/soyuz/model/publishing.py 2010-01-31 19:26:34 +0000
42+++ lib/lp/soyuz/model/publishing.py 2010-02-11 06:25:26 +0000
43@@ -656,7 +656,8 @@
44
45 build = self.sourcepackagerelease.createBuild(
46 distroarchseries=arch, archive=self.archive, pocket=self.pocket)
47- build_queue = build.queueBuild()
48+ # Create the builds in suspended mode for disabled archives.
49+ build_queue = build.queueBuild(suspended=not self.archive.enabled)
50 build_queue.score()
51 Store.of(build).flush()
52
53
54=== modified file 'lib/lp/soyuz/scripts/tests/test_populatearchive.py'
55--- lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-01-12 09:01:38 +0000
56+++ lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-02-11 06:25:26 +0000
57@@ -3,34 +3,33 @@
58
59 __metaclass__ = type
60
61+from datetime import datetime
62 import os
63 import subprocess
64 import sys
65 import time
66 import unittest
67
68-from datetime import datetime
69-
70 from zope.component import getUtility
71 from zope.security.proxy import removeSecurityProxy
72
73 from canonical.config import config
74+from canonical.launchpad.scripts import BufferLogger
75+from canonical.testing import LaunchpadZopelessLayer
76+from canonical.testing.layers import DatabaseLayer
77 from lp.registry.interfaces.distribution import IDistributionSet
78+from lp.registry.interfaces.person import IPersonSet
79+from lp.services.job.interfaces.job import JobStatus
80 from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
81+from lp.soyuz.interfaces.archivearch import IArchiveArchSet
82 from lp.soyuz.interfaces.build import BuildStatus, IBuildSet
83 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
84-from lp.soyuz.interfaces.archivearch import IArchiveArchSet
85 from lp.soyuz.interfaces.packagecopyrequest import (
86 IPackageCopyRequestSet, PackageCopyStatus)
87-from lp.registry.interfaces.person import IPersonSet
88-from lp.soyuz.scripts.ftpmaster import (
89- PackageLocationError, SoyuzScriptError)
90+from lp.soyuz.scripts.ftpmaster import PackageLocationError, SoyuzScriptError
91 from lp.soyuz.scripts.populate_archive import ArchivePopulator
92-from canonical.launchpad.scripts import BufferLogger
93 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
94 from lp.testing import TestCase
95-from canonical.testing import LaunchpadZopelessLayer
96-from canonical.testing.layers import DatabaseLayer
97
98
99 def get_spn(build):
100@@ -650,6 +649,39 @@
101 rset = getUtility(IArchiveArchSet).getByArchive(copy_archive)
102 self.assertEqual(get_family_names(rset), [u'amd64', u'x86'])
103
104+ def testBuildsPendingAndSuspended(self):
105+ """All builds in the new copy archive are pending and suspended."""
106+ def build_in_wrong_state(build):
107+ """True if the given build is not (pending and suspended)."""
108+ return not (
109+ build.buildstate == BuildStatus.NEEDSBUILD and
110+ build.buildqueue_record.job.status == JobStatus.SUSPENDED)
111+ hoary = getUtility(IDistributionSet)['ubuntu']['hoary']
112+
113+ # Verify that we have the right source packages in the sample data.
114+ self._verifyPackagesInSampleData(hoary)
115+
116+ extra_args = ['-a', '386']
117+ archive = self.runScript(extra_args=extra_args, exists_after=True)
118+
119+ # Make sure the right source packages were cloned.
120+ self._verifyClonedSourcePackages(archive, hoary)
121+
122+ # Get the binary builds generated for the copy archive at hand.
123+ builds = list(getUtility(IBuildSet).getBuildsForArchive(archive))
124+ # At least one binary build was generated for the target copy archive.
125+ self.assertTrue(len(builds) > 0)
126+ # Now check that the binary builds and their associated job records
127+ # are in the state expected:
128+ # - binary build: pending
129+ # - job: suspended
130+ builds_in_wrong_state = filter(build_in_wrong_state, builds)
131+ self.assertEqual (
132+ [], builds_in_wrong_state,
133+ "The binary builds generated for the target copy archive "
134+ "should all be pending and suspended. However, at least one of "
135+ "the builds is in the wrong state.")
136+
137 def testPrivateOriginArchive(self):
138 """Try copying from a private archive.
139