Launchpad itself

Merge lp:~al-maisan/launchpad/spec-job-class-503839 into lp:launchpad

  1. spec-job-class-503839
  2. Merge into devel
Proposed by Muharem Hrnjadovic on 2010-01-07
Status: Merged
Merged at revision: not available
Proposed branch: lp:~al-maisan/launchpad/spec-job-class-503839
Merge into: lp:launchpad
Diff against target: 182 lines (+114/-3)
4 files modified
lib/lp/soyuz/configure.zcml (+10/-0)
lib/lp/soyuz/interfaces/buildqueue.py (+4/-0)
lib/lp/soyuz/model/buildqueue.py (+22/-3)
lib/lp/soyuz/tests/test_buildqueue.py (+78/-0)
To merge this branch: bzr merge lp:~al-maisan/launchpad/spec-job-class-503839
Reviewer Review Type Date Requested Status
Michael Nelson (community) code 2010-01-07 Approve on 2010-01-07
Björn Tillenius code 2010-01-07 Pending
Review via email: mp+16946@code.launchpad.net
To post a comment you must log in.
Muharem Hrnjadovic (al-maisan) wrote :

Hello there!

This is another step in the renovation of the (Soyuz) build farm. Now that
we're introducing additional build farm job types we need to be able to figure
out what these are without hardcoding that information.

This is mostly required by the general build farm infrastructure i.e. for the
purpose of job dispatch time estimation and candidate job selection.

Pre-implementation discussion (via email) with Gary Poster and others.

Tests to run:

    bin/test -vv -t TestJobClasses

No pertinent "make lint" errors or warnings.

Please see also bug #503839 for more detail.

Michael Nelson (michael.nelson) wrote :
Download full text (9.3 KiB)

Hi Muharem!

It's great seeing the pieces of the generalisation falling into place!

I've got one rather large question about this change: Why can't you just
use a normal adapter? It seems that you've gone the long way around to
implement adapter-like functionality.

That is, you've got a BuildFarmJobType specified on IBuildQueue.job_type,
and you want to find the corresponding class. I don't know whether it is
the best solution, but you *could* in this case, adapt the build_queue
entry itself to the correct IBuildFarmJob implementation?

IBuildFarmJob(specific_build_queue_entry)

which would return the specifc build farm job.

I'm wondering if it's really that you need specific_job_classes for further
work in the build estimations, rather than specifically for this branch. That
is, originally when you were chatting with gary about using the class manager,
I had understood that you needed to *iterate* over the different
classes to build a query... in that case you would certainly need something
like the specific_job_classes that you've provided below. But as it is, you are
not needing to iterate them at all for this branch, and looking at this branch
in isolation would suggest that a normal adapter would be more intuitive.

If it is the case that you will need specific_job_classes for a later branch,
then it may indeed make sense to *not* use adaption here and re-use the
specific_job_classes exactly as you've done, as otherwise new types
would need to define both the utility *and* the adapter. I'd like to get
BjornT's thoughts on that point, hence marking as needs information.

Some comments in line below.

> === modified file 'lib/lp/soyuz/configure.zcml'
> --- lib/lp/soyuz/configure.zcml 2010-01-04 21:15:46 +0000
> +++ lib/lp/soyuz/configure.zcml 2010-01-07 08:23:28 +0000
> @@ -897,6 +897,16 @@
> <allow
> interface="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"/>
> </class>
> + <!--
> + The registration below is used to discover all build farm job classes
> + that implement the `IBuildFarmJob` interface. Please see bug #503839
> + for more detail.
> + The 'name' attribute needs to be set to the appropriate
> + `BuildFarmJobType` enumeration value.
> + -->
> + <utility component="lp.soyuz.model.buildpackagejob.BuildPackageJob"
> + name="PACKAGEBUILD"
> + provides="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/>
>
> <!-- BinaryPackageBuildBehavior -->
> <adapter
>
> === modified file 'lib/lp/soyuz/interfaces/buildqueue.py'
> --- lib/lp/soyuz/interfaces/buildqueue.py 2009-11-27 13:33:55 +0000
> +++ lib/lp/soyuz/interfaces/buildqueue.py 2010-01-07 08:23:28 +0000
> @@ -55,6 +55,10 @@
> title=_('The builder behavior required to run this job.'),
> required=False, readonly=True)
>
> + specific_job_classes = Field(
> + title=_('Job classes that may run on the build farm.'),
> + required=True, readonly=True)
> +
> estimated_duration = Timedelta(
> title=_("Estimated Job Duration"), required=True,
> description=_("Estimated job duration interval."))
>
> === modified file 'lib/lp/soyuz/...

Read more...

review: Needs Information (code)
Muharem Hrnjadovic (al-maisan) wrote :
Download full text (10.1 KiB)

Michael Nelson wrote:
> Review: Needs Information code
> Hi Muharem!
>
> It's great seeing the pieces of the generalisation falling into place!
>
> I've got one rather large question about this change: Why can't you just
> use a normal adapter? It seems that you've gone the long way around to
> implement adapter-like functionality.
>
> That is, you've got a BuildFarmJobType specified on IBuildQueue.job_type,
> and you want to find the corresponding class. I don't know whether it is
> the best solution, but you *could* in this case, adapt the build_queue
> entry itself to the correct IBuildFarmJob implementation?
>
> IBuildFarmJob(specific_build_queue_entry)
>
> which would return the specifc build farm job.
>
> I'm wondering if it's really that you need specific_job_classes for further
> work in the build estimations, rather than specifically for this branch. That
> is, originally when you were chatting with gary about using the class manager,
> I had understood that you needed to *iterate* over the different
> classes to build a query... in that case you would certainly need something
> like the specific_job_classes that you've provided below. But as it is, you are
> not needing to iterate them at all for this branch, and looking at this branch
> in isolation would suggest that a normal adapter would be more intuitive.
>
> If it is the case that you will need specific_job_classes for a later branch,
> then it may indeed make sense to *not* use adaption here and re-use the
> specific_job_classes exactly as you've done, as otherwise new types
> would need to define both the utility *and* the adapter. I'd like to get
> BjornT's thoughts on that point, hence marking as needs information.

Hello Michael, sorry for the terse answer but, yes, I need to iterate
over the farm build job classes.
See e.g. lines 48-54 in the enclosed diff which is an excerpt from
lp:~al-maisan/launchpad/job-delays-504086, the branch I am currently
working on.

> Some comments in line below.
>
>> === modified file 'lib/lp/soyuz/configure.zcml'
>> --- lib/lp/soyuz/configure.zcml 2010-01-04 21:15:46 +0000
>> +++ lib/lp/soyuz/configure.zcml 2010-01-07 08:23:28 +0000
>> @@ -897,6 +897,16 @@
>> <allow
>> interface="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"/>
>> </class>
>> + <!--
>> + The registration below is used to discover all build farm job classes
>> + that implement the `IBuildFarmJob` interface. Please see bug #503839
>> + for more detail.
>> + The 'name' attribute needs to be set to the appropriate
>> + `BuildFarmJobType` enumeration value.
>> + -->
>> + <utility component="lp.soyuz.model.buildpackagejob.BuildPackageJob"
>> + name="PACKAGEBUILD"
>> + provides="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/>
>>
>> <!-- BinaryPackageBuildBehavior -->
>> <adapter
>>
>> === modified file 'lib/lp/soyuz/interfaces/buildqueue.py'
>> --- lib/lp/soyuz/interfaces/buildqueue.py 2009-11-27 13:33:55 +0000
>> +++ lib/lp/soyuz/interfaces/buildqueue.py 2010-01-07 08:23:28 +0000
>> @@ -55,6 +55,10 @@
>> title=_('The builder behavior required to run this ...

noodles.diff
1=== modified file 'lib/lp/soyuz/model/buildqueue.py'
2--- lib/lp/soyuz/model/buildqueue.py 2010-01-07 06:49:04 +0000
3+++ lib/lp/soyuz/model/buildqueue.py 2010-01-07 09:58:59 +0000
4@@ -211,6 +211,9 @@
5 # The job of interest (JOI) is processor independent.
6 builders_for_job = builders_in_total
7
8+ # All builders can take on platform-independent jobs.
9+ builder_stats[(None,None)] = builders_in_total
10+
11 return (builders_in_total, builders_for_job, builder_stats)
12
13 def _freeBuildersCount(self, processor, virtualized):
14@@ -313,6 +316,123 @@
15 else:
16 return int(head_job_delay)
17
18+ def _estimateJobDelay(self, builders_in_total, builder_stats):
19+ """Sum of estimated durations for *pending* jobs ahead in queue.
20+
21+ For the purpose of estimating the dispatch time of the job of
22+ interest (JOI) we need to know the delay caused by all the pending
23+ jobs that are ahead of the JOI in the queue and that compete with it
24+ for builders.
25+
26+ :param builders_in_total: How many active builders are available
27+ altogether?
28+ :param builder_stats: A dictionary with builder counts where the
29+ key is a (processor, virtualized) combination (aka "platform") and
30+ the value is the number of builders that can take on jobs
31+ requiring that combination.
32+ :return: A (sum_of_delays, head_job_platform) tuple where
33+ * 'sum_of_delays' is the estimated delay in seconds and
34+ * 'head_job_platform' is the platform ((processor, virtualized)
35+ combination) required by the job at the head of the queue.
36+ """
37+ # Please note: this method will send only one request to the database.
38+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
39+ my_processor = self.specific_job.processor
40+ my_virtualized = self.specific_job.virtualized
41+ my_platform = (my_processor, my_virtualized)
42+
43+ # Ask all build farm job type classes for a plain SQL SELECT query
44+ # that will yield the pending jobs whose score is equal or better
45+ # than the score of the job of interest (JOI).
46+ # Chain these queries in "SELECT .. UNION SELECT .." fashion and
47+ # use them to obtain the jobs that compete with the JOI for builders.
48+ queries = []
49+ for job_class in self.specific_job_classes.values():
50+ queries.append(
51+ job_class.composePendingJobsQuery(
52+ self.lastscore, my_processor, my_virtualized))
53+ query = '%s ORDER BY lastscore DESC, job' % ' UNION '.join(queries)
54+ job_queue = store.execute(query).get_all()
55+
56+ sum_of_delays = 0
57+
58+ # This will be used to capture per-platform delay totals.
59+ delays = dict()
60+ # This will be used to capture per-platform job counts.
61+ job_counts = dict()
62+
63+ # Which platform is the job at the head of the queue requiring?
64+ head_job_platform = None
65+
66+ # Apply weights to the estimated duration of the jobs as follows:
67+ # - if a job is tied to a processor TP then divide the estimated
68+ # duration of that job by the number of builders that target TP
69+ # since only these can build the job.
70+ # - if the job is processor-independent then divide its estimated
71+ # duration by the total number of builders because any one of
72+ # them may run it.
73+ for job, score, duration, processor, virtualized in job_queue:
74+ if job == self.job.id:
75+ # We have seen all jobs that are ahead of us in the queue
76+ # and can stop now.
77+ # This is guaranteed by the "ORDER BY lastscore DESC.."
78+ # clause above.
79+ break
80+
81+ # For the purpose of estimating the delay for dispatching the job
82+ # at the head of the queue to a builder we need to capture the
83+ # platform it targets.
84+ if head_job_platform is None:
85+ platform = (processor, virtualized)
86+ if my_processor is None:
87+ # The JOI is platform-independent i.e. the highest scored
88+ # job will be the head job.
89+ head_job_platform = platform
90+ else:
91+ # The JOI targets a specific platform. The head job is
92+ # thus the highest scored job that either targets the
93+ # same platform or is platform-independent.
94+ if (my_platform == platform or processor is None):
95+ head_job_platform = platform
96+
97+ if processor is None:
98+ # Processor independent job, can be run on any builder.
99+ builder_count = builders_in_total
100+ else:
101+ # Tied to a particular processor, see how many builders
102+ # can run it.
103+ builder_count = builder_stats.get((processor, virtualized), 0)
104+
105+ if builder_count == 0:
106+ # There is no builder that can run this job, ignore it
107+ # for the purpose of dispatch time estimation.
108+ continue
109+
110+ # Accumulate the delays, and count the number of jobs causing them
111+ # on a (processor, virtualized) basis.
112+ duration = duration.seconds
113+ delays[(processor, virtualized)] = (
114+ delays.setdefault((processor, virtualized), 0) + duration)
115+ job_counts[(processor, virtualized)] = (
116+ job_counts.setdefault((processor, virtualized), 0) + 1)
117+
118+ # Now weight/average the delays based on a jobs/builders comparison.
119+ for platform, duration in delays.iteritems():
120+ jobs = job_counts[platform]
121+ builders = builder_stats[platform]
122+ if jobs < builders:
123+ # There are less jobs than builders that can take them on,
124+ # the delays should be averaged/divided by the number of jobs.
125+ denominator = jobs
126+ else:
127+ denominator = builders
128+ if denominator > 1:
129+ duration = int(duration/float(denominator))
130+
131+ sum_of_delays += duration
132+
133+ return (sum_of_delays, head_job_platform)
134+
135
136 class BuildQueueSet(object):
137 """Utility to deal with BuildQueue content class."""
138
139=== modified file 'lib/lp/soyuz/tests/test_buildqueue.py'
140--- lib/lp/soyuz/tests/test_buildqueue.py 2010-01-07 06:49:04 +0000
141+++ lib/lp/soyuz/tests/test_buildqueue.py 2010-01-07 10:12:09 +0000
142@@ -552,7 +552,7 @@
143 score = 1000
144 duration = 0
145 for build in self.builds:
146- score += 1
147+ score += getattr(self, 'score_increment', 1)
148 duration += 60
149 bq = build.buildqueue_record
150 bq.lastscore = score
151@@ -740,3 +740,96 @@
152 self.assertEqual(
153 bq.specific_job_classes[BuildFarmJobType.BRANCHBUILD],
154 FakeBranchBuild)
155+
156+
157+class TestMultiArchJobDelayEstimation(MultiArchBuildsBase):
158+ """Test estimated job delays with various processors."""
159+ score_increment = 2
160+ def setUp(self):
161+ """Set up a fake 'BRANCHBUILD' build farm job class.
162+
163+ The two platform-independent jobs will have a score of 1017 and 1033
164+ respectively.
165+
166+ gedit, p: hppa, v:False e:0:01:00 *** s: 1002
167+ gedit, p: 386, v:False e:0:02:00 *** s: 1004
168+ firefox, p: hppa, v:False e:0:03:00 *** s: 1006
169+ firefox, p: 386, v:False e:0:04:00 *** s: 1008
170+ apg, p: hppa, v:False e:0:05:00 *** s: 1010
171+ apg, p: 386, v:False e:0:06:00 *** s: 1012
172+ vim, p: hppa, v:False e:0:07:00 *** s: 1014
173+ vim, p: 386, v:False e:0:08:00 *** s: 1016
174+ --> fake1, none none 0:00:22 1017
175+ gcc, p: hppa, v:False e:0:09:00 *** s: 1018
176+ gcc, p: 386, v:False e:0:10:00 *** s: 1020
177+ bison, p: hppa, v:False e:0:11:00 *** s: 1022
178+ bison, p: 386, v:False e:0:12:00 *** s: 1024
179+ flex, p: hppa, v:False e:0:13:00 *** s: 1026
180+ flex, p: 386, v:False e:0:14:00 *** s: 1028
181+ postgres, p: hppa, v:False e:0:15:00 *** s: 1030
182+ postgres, p: 386, v:False e:0:16:00 *** s: 1032
183+ --> fake2, none none 0:03:42 1033
184+
185+ p=processor, v=virtualized, e=estimated_duration, s=score
186+ """
187+ super(TestMultiArchJobDelayEstimation, self).setUp()
188+
189+ from zope import component
190+ from zope.interface import implements
191+ from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
192+
193+ class FakeBranchBuild:
194+ implements(IBuildFarmJob)
195+ def score(self):
196+ return -9999
197+ def getLogFileName(self):
198+ return 'buildlog_fake-branch-build.txt'
199+ def getName(self):
200+ return 'fake-branch-build'
201+ @staticmethod
202+ def composePendingJobsQuery(min_score, processor, virtualized):
203+ return """
204+ (SELECT
205+ 1000001::integer AS job,
206+ 1017::integer AS lastscore,
207+ '22 seconds'::interval AS estimated_duration,
208+ NULL::integer AS processor,
209+ NULL::boolean AS virtualized
210+ UNION
211+ SELECT
212+ 1000002::integer AS job,
213+ 1033::integer AS lastscore,
214+ '222 seconds'::interval AS estimated_duration,
215+ NULL::integer AS processor,
216+ NULL::boolean AS virtualized)
217+ """
218+
219+ # Pretend that our `FakeBranchBuild` class implements the
220+ # `IBuildFarmJob` interface.
221+ component.provideUtility(
222+ FakeBranchBuild, IBuildFarmJob, 'BRANCHBUILD')
223+
224+ def test_job_delay(self):
225+ processor_fam = ProcessorFamilySet().getByName('hppa')
226+ hppa_proc = processor_fam.processors[0]
227+
228+ # One of four builders for the 'apg' build is immediately available.
229+ flex_build, flex_job = find_job(self, 'flex', 'hppa')
230+ check_mintime_to_builder(self, flex_job, hppa_proc, False, 0)
231+
232+ builder_data = flex_job._getBuilderData()
233+ builders_in_total, builders_for_job, builder_stats = builder_data
234+
235+ # The delay is 900 (= 15*60) + 222 seconds, the head job is
236+ # platform-independent.
237+ self.assertEqual(
238+ flex_job._estimateJobDelay(builders_in_total, builder_stats),
239+ (1122, (None, None)))
240+
241+ # Assign the postgres job to a builder.
242+ assign_to_builder(self, 'postgres', 1, 'hppa')
243+ # Now only the 222 seconds (the estimated duration of the
244+ # platform-independent job) should be returned.
245+ self.assertEqual(
246+ flex_job._estimateJobDelay(builders_in_total, builder_stats),
247+ (222, (None, None)))
Muharem Hrnjadovic (al-maisan) wrote :

Michael Nelson wrote:
> Review: Needs Information code
 [..]
> I've got one rather large question about this change: Why can't you
> just use a normal adapter? It seems that you've gone the long way
> around to implement adapter-like functionality.
>
> That is, you've got a BuildFarmJobType specified on
> IBuildQueue.job_type, and you want to find the corresponding class. I
> don't know whether it is the best solution, but you *could* in this
> case, adapt the build_queue entry itself to the correct IBuildFarmJob
> implementation?
>
> IBuildFarmJob(specific_build_queue_entry)
 How would that actually work? IBuildFarmJob is not what we want here.
IBuildQueue.specific_job is supposed to yield something like a
BuildPackageJob instance.

> which would return the specifc build farm job.
>
> I'm wondering if it's really that you need specific_job_classes for
> further work in the build estimations, rather than specifically for
That's the case indeed, sorry for not explaining that better.

> this branch. That is, originally when you were chatting with gary
> about using the class manager, I had understood that you needed to
> *iterate* over the different classes to build a query... in that case
> you would certainly need something like the specific_job_classes that
> you've provided below. But as it is, you are not needing to iterate
> them at all for this branch, and looking at this branch in isolation
> would suggest that a normal adapter would be more intuitive.
Sorry for not explaining in more detail why this is needed please see my
previous email for an example where I do need to iterate over the build
farm job classes.
I also expect that that's something that will be needed when we
generalize the candidate job selection.

> If it is the case that you will need specific_job_classes for a later
> branch, then it may indeed make sense to *not* use adaption here and
> re-use the specific_job_classes exactly as you've done, as otherwise
As stated in the previous reply I do require the iteration over the build
farm job classes for the purpose of job dispatch time estimation.
Being able to register build farm classes in the way suggested makes it
also much easier to test the code.
Please see the end of the diff (lines 193-222) enclosed in my previous
reply for an example.

[..]

Best regards

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

Michael Nelson (michael.nelson) wrote :

Great, thanks for the info Muharem.

So given that you need the class iteration, I'm keen to approve this now (assuming you address the issues I mentioned above inline).

Thanks!

review: Approve (code)
Muharem Hrnjadovic (al-maisan) wrote :
Download full text (8.5 KiB)

Michael Nelson wrote:
[..]

Hello Michael,

thanks again for taking the time to review this branch despite your own
busy schedule. That's very much appreciated :)

> Some comments in line below.
Please see my replies as well as the enclosed incremental diff.

>> === modified file 'lib/lp/soyuz/configure.zcml'
>> --- lib/lp/soyuz/configure.zcml 2010-01-04 21:15:46 +0000
>> +++ lib/lp/soyuz/configure.zcml 2010-01-07 08:23:28 +0000
>> @@ -897,6 +897,16 @@
>> <allow
>> interface="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"/>
>> </class>
>> + <!--
>> + The registration below is used to discover all build farm job classes
>> + that implement the `IBuildFarmJob` interface. Please see bug #503839
>> + for more detail.
>> + The 'name' attribute needs to be set to the appropriate
>> + `BuildFarmJobType` enumeration value.
>> + -->
>> + <utility component="lp.soyuz.model.buildpackagejob.BuildPackageJob"
>> + name="PACKAGEBUILD"
>> + provides="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/>
>>
>> <!-- BinaryPackageBuildBehavior -->
>> <adapter
>>
>> === modified file 'lib/lp/soyuz/interfaces/buildqueue.py'
>> --- lib/lp/soyuz/interfaces/buildqueue.py 2009-11-27 13:33:55 +0000
>> +++ lib/lp/soyuz/interfaces/buildqueue.py 2010-01-07 08:23:28 +0000
>> @@ -55,6 +55,10 @@
>> title=_('The builder behavior required to run this job.'),
>> required=False, readonly=True)
>>
>> + specific_job_classes = Field(
>> + title=_('Job classes that may run on the build farm.'),
>> + required=True, readonly=True)
>> +
>> estimated_duration = Timedelta(
>> title=_("Estimated Job Duration"), required=True,
>> description=_("Estimated job duration interval."))
>>
>> === modified file 'lib/lp/soyuz/model/buildqueue.py'
>> --- lib/lp/soyuz/model/buildqueue.py 2010-01-04 14:30:52 +0000
>> +++ lib/lp/soyuz/model/buildqueue.py 2010-01-07 08:23:28 +0000
>> @@ -57,11 +57,31 @@
>> return IBuildFarmJobBehavior(self.specific_job)
>>
>> @property
>> + def specific_job_classes(self):
>> + """See `IBuildQueue`."""
>> + from zope.component import getSiteManager
>> + from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
>
> Is there a reason for these imports to be inline? If so, please add a
> comment or otherwise move them as per the guidelines.
No other code in the module uses them. I have added a comment to that
effect.

>> +
>> + job_classes = []
>> + components = getSiteManager()
>> + # Get all components that implement the `IBuildFarmJob` interface.
>> + implementations = sorted(components.getUtilitiesFor(IBuildFarmJob))
>> + # The above yields a collection of 2-tuples where the first element
>> + # is the name of the `BuildFarmJobType` enum and the second element
>> + # is the implementing class respectively.
>> + for job_enum_name, job_class in implementations:
>> + job_enum = getattr(BuildFarmJobType, job_enum_name)
>
> I was originally going to say that we should be checking first with
> safe_hasat...

Read more...

spec-job-class-503839-inc.diff
1=== modified file 'lib/lp/soyuz/model/buildqueue.py'
2--- lib/lp/soyuz/model/buildqueue.py 2010-01-07 05:36:53 +0000
3+++ lib/lp/soyuz/model/buildqueue.py 2010-01-07 18:41:24 +0000
4@@ -59,21 +59,23 @@
5 @property
6 def specific_job_classes(self):
7 """See `IBuildQueue`."""
8+ # The reason for keeping the imports below local is that they are not
9+ # of interest to other methods in this module.
10 from zope.component import getSiteManager
11 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
12
13- job_classes = []
14+ job_classes = dict()
15+ # Get all components that implement the `IBuildFarmJob` interface.
16 components = getSiteManager()
17- # Get all components that implement the `IBuildFarmJob` interface.
18 implementations = sorted(components.getUtilitiesFor(IBuildFarmJob))
19 # The above yields a collection of 2-tuples where the first element
20 # is the name of the `BuildFarmJobType` enum and the second element
21 # is the implementing class respectively.
22 for job_enum_name, job_class in implementations:
23 job_enum = getattr(BuildFarmJobType, job_enum_name)
24- job_classes.append((job_enum, job_class))
25+ job_classes[job_enum] = job_class
26
27- return dict(job_classes)
28+ return job_classes
29
30 @property
31 def specific_job(self):
Michael Nelson (michael.nelson) wrote :

On Thu, Jan 7, 2010 at 8:57 PM, Muharem Hrnjadovic
<email address hidden> wrote:
> Michael Nelson wrote:
> [..]
>
> Hello Michael,
>
> thanks again for taking the time to review this branch despite your own
> busy schedule. That's very much appreciated :)

No probs!

>
>> Some comments in line below.
> Please see my replies as well as the enclosed incremental diff.
>
>>> === modified file 'lib/lp/soyuz/model/buildqueue.py'
>>> --- lib/lp/soyuz/model/buildqueue.py 2010-01-04 14:30:52 +0000
>>> +++ lib/lp/soyuz/model/buildqueue.py 2010-01-07 08:23:28 +0000
>>> @@ -57,11 +57,31 @@
>>>          return IBuildFarmJobBehavior(self.specific_job)
>>>
>>>      @property
>>> +    def specific_job_classes(self):
>>> +        """See `IBuildQueue`."""
>>> +        from zope.component import getSiteManager
>>> +        from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
>>
>> Is there a reason for these imports to be inline? If so, please add a
>> comment or otherwise move them as per the guidelines.
> No other code in the module uses them. I have added a comment to that
> effect.

Erm... I didn't realise that was a valid reason? I thought we only did
so if we *had* to (ie. circular imports etc.). Please check our
styleguide... I'm going from memory but I thought the imports should
always be at the top even if no other code in the module uses them.

Cheers, and happy flying!
-Michael

Muharem Hrnjadovic (al-maisan) wrote :

Michael Nelson wrote:
> On Thu, Jan 7, 2010 at 8:57 PM, Muharem Hrnjadovic
> <email address hidden> wrote:
>> Michael Nelson wrote:
>> [..]
>>
>> Hello Michael,
>>
>> thanks again for taking the time to review this branch despite your own
>> busy schedule. That's very much appreciated :)
>
> No probs!
>
>>> Some comments in line below.
>> Please see my replies as well as the enclosed incremental diff.
>>
>>>> === modified file 'lib/lp/soyuz/model/buildqueue.py'
>>>> --- lib/lp/soyuz/model/buildqueue.py 2010-01-04 14:30:52 +0000
>>>> +++ lib/lp/soyuz/model/buildqueue.py 2010-01-07 08:23:28 +0000
>>>> @@ -57,11 +57,31 @@
>>>> return IBuildFarmJobBehavior(self.specific_job)
>>>>
>>>> @property
>>>> + def specific_job_classes(self):
>>>> + """See `IBuildQueue`."""
>>>> + from zope.component import getSiteManager
>>>> + from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
>>> Is there a reason for these imports to be inline? If so, please add a
>>> comment or otherwise move them as per the guidelines.
>> No other code in the module uses them. I have added a comment to that
>> effect.
>
> Erm... I didn't realise that was a valid reason? I thought we only did
> so if we *had* to (ie. circular imports etc.). Please check our
> styleguide... I'm going from memory but I thought the imports should
> always be at the top even if no other code in the module uses them.

Whoops .. sorry .. and thanks for catching this :)

> Cheers, and happy flying!

Thank you! Have a safe journey as well :)

Best regards

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/soyuz/configure.zcml'
2--- lib/lp/soyuz/configure.zcml 2010-01-04 21:15:46 +0000
3+++ lib/lp/soyuz/configure.zcml 2010-01-09 02:49:17 +0000
4@@ -897,6 +897,16 @@
5 <allow
6 interface="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"/>
7 </class>
8+ <!--
9+ The registration below is used to discover all build farm job classes
10+ that implement the `IBuildFarmJob` interface. Please see bug #503839
11+ for more detail.
12+ The 'name' attribute needs to be set to the appropriate
13+ `BuildFarmJobType` enumeration value.
14+ -->
15+ <utility component="lp.soyuz.model.buildpackagejob.BuildPackageJob"
16+ name="PACKAGEBUILD"
17+ provides="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/>
18
19 <!-- BinaryPackageBuildBehavior -->
20 <adapter
21
22=== modified file 'lib/lp/soyuz/interfaces/buildqueue.py'
23--- lib/lp/soyuz/interfaces/buildqueue.py 2009-11-27 13:33:55 +0000
24+++ lib/lp/soyuz/interfaces/buildqueue.py 2010-01-09 02:49:17 +0000
25@@ -55,6 +55,10 @@
26 title=_('The builder behavior required to run this job.'),
27 required=False, readonly=True)
28
29+ specific_job_classes = Field(
30+ title=_('Job classes that may run on the build farm.'),
31+ required=True, readonly=True)
32+
33 estimated_duration = Timedelta(
34 title=_("Estimated Job Duration"), required=True,
35 description=_("Estimated job duration interval."))
36
37=== modified file 'lib/lp/soyuz/model/buildqueue.py'
38--- lib/lp/soyuz/model/buildqueue.py 2010-01-06 15:58:41 +0000
39+++ lib/lp/soyuz/model/buildqueue.py 2010-01-09 02:49:17 +0000
40@@ -12,7 +12,8 @@
41
42 import logging
43
44-from zope.component import getUtility
45+from zope.component import getSiteManager, getUtility
46+
47 from zope.interface import implements
48
49 from sqlobject import (
50@@ -24,7 +25,8 @@
51 from canonical.database.enumcol import EnumCol
52 from canonical.database.sqlbase import SQLBase, sqlvalues
53 from canonical.launchpad.webapp.interfaces import NotFoundError
54-from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
55+from lp.buildmaster.interfaces.buildfarmjob import (
56+ BuildFarmJobType, IBuildFarmJob)
57 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
58 IBuildFarmJobBehavior)
59 from lp.services.job.interfaces.job import JobStatus
60@@ -57,11 +59,28 @@
61 return IBuildFarmJobBehavior(self.specific_job)
62
63 @property
64+ def specific_job_classes(self):
65+ """See `IBuildQueue`."""
66+ job_classes = dict()
67+ # Get all components that implement the `IBuildFarmJob` interface.
68+ components = getSiteManager()
69+ implementations = sorted(components.getUtilitiesFor(IBuildFarmJob))
70+ # The above yields a collection of 2-tuples where the first element
71+ # is the name of the `BuildFarmJobType` enum and the second element
72+ # is the implementing class respectively.
73+ for job_enum_name, job_class in implementations:
74+ job_enum = getattr(BuildFarmJobType, job_enum_name)
75+ job_classes[job_enum] = job_class
76+
77+ return job_classes
78+
79+ @property
80 def specific_job(self):
81 """See `IBuildQueue`."""
82+ specific_class = self.specific_job_classes[self.job_type]
83 store = Store.of(self)
84 result_set = store.find(
85- BuildPackageJob, BuildPackageJob.job == self.job)
86+ specific_class, specific_class.job == self.job)
87 return result_set.one()
88
89 @property
90
91=== modified file 'lib/lp/soyuz/tests/test_buildqueue.py'
92--- lib/lp/soyuz/tests/test_buildqueue.py 2010-01-06 15:58:41 +0000
93+++ lib/lp/soyuz/tests/test_buildqueue.py 2010-01-09 02:49:17 +0000
94@@ -13,6 +13,7 @@
95 IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
96 from canonical.testing import LaunchpadZopelessLayer
97
98+from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
99 from lp.soyuz.interfaces.archive import ArchivePurpose
100 from lp.soyuz.interfaces.build import BuildStatus
101 from lp.soyuz.interfaces.builder import IBuilderSet
102@@ -662,3 +663,80 @@
103
104 # That builder should be available immediately since it's idle.
105 check_mintime_to_builder(self, apg_job, None, None, 0)
106+
107+
108+class TestJobClasses(TestCaseWithFactory):
109+ """Tests covering build farm job type classes."""
110+ layer = LaunchpadZopelessLayer
111+ def setUp(self):
112+ """Set up a native x86 build for the test archive."""
113+ super(TestJobClasses, self).setUp()
114+
115+ self.publisher = SoyuzTestPublisher()
116+ self.publisher.prepareBreezyAutotest()
117+
118+ # First mark all builds in the sample data as already built.
119+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
120+ sample_data = store.find(Build)
121+ for build in sample_data:
122+ build.buildstate = BuildStatus.FULLYBUILT
123+ store.flush()
124+
125+ # We test builds that target a primary archive.
126+ self.non_ppa = self.factory.makeArchive(
127+ name="primary", purpose=ArchivePurpose.PRIMARY)
128+ self.non_ppa.require_virtualized = False
129+
130+ self.builds = []
131+ self.builds.extend(
132+ self.publisher.getPubSource(
133+ sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
134+ archive=self.non_ppa).createMissingBuilds())
135+
136+ def test_BuildPackageJob(self):
137+ """`BuildPackageJob` is one of the job type classes."""
138+ from lp.soyuz.model.buildpackagejob import BuildPackageJob
139+ _build, bq = find_job(self, 'gedit')
140+
141+ # This is a binary package build.
142+ self.assertEqual(
143+ bq.job_type, BuildFarmJobType.PACKAGEBUILD,
144+ "This is a binary package build")
145+
146+ # The class registered for 'PACKAGEBUILD' is `BuildPackageJob`.
147+ self.assertEqual(
148+ bq.specific_job_classes[BuildFarmJobType.PACKAGEBUILD],
149+ BuildPackageJob,
150+ "The class registered for 'PACKAGEBUILD' is `BuildPackageJob`")
151+
152+ # The 'specific_job' object associated with this `BuildQueue`
153+ # instance is of type `BuildPackageJob`.
154+ self.assertTrue(bq.specific_job is not None)
155+ self.assertEqual(
156+ bq.specific_job.__class__, BuildPackageJob,
157+ "The 'specific_job' object associated with this `BuildQueue` "
158+ "instance is of type `BuildPackageJob`")
159+
160+ def test_OtherTypeClasses(self):
161+ """Other job type classes are picked up as well."""
162+ from zope import component
163+ from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
164+ class FakeBranchBuild:
165+ pass
166+
167+ _build, bq = find_job(self, 'gedit')
168+ # First make sure that we don't have a job type class registered for
169+ # 'BRANCHBUILD' yet.
170+ self.assertTrue(
171+ bq.specific_job_classes.get(BuildFarmJobType.BRANCHBUILD) is None)
172+
173+ # Pretend that our `FakeBranchBuild` class implements the
174+ # `IBuildFarmJob` interface.
175+ component.provideUtility(
176+ FakeBranchBuild, IBuildFarmJob, 'BRANCHBUILD')
177+
178+ # Now we should see the `FakeBranchBuild` class "registered" in the
179+ # `specific_job_classes` dictionary under the 'BRANCHBUILD' key.
180+ self.assertEqual(
181+ bq.specific_job_classes[BuildFarmJobType.BRANCHBUILD],
182+ FakeBranchBuild)