Merge lp:~al-maisan/launchpad/spec-job-class-503839 into lp:launchpad
- spec-job-class-503839
- Merge into devel
| 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 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Michael Nelson (community) | code | Approve | |
| Björn Tillenius | code | Pending | |
|
Review via email:
|
|||
Commit message
Description of the change
| Muharem Hrnjadovic (al-maisan) wrote : | # |
| Michael Nelson (michael.nelson) wrote : | # |
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.
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(
which would return the specifc build farm job.
I'm wondering if it's really that you need specific_
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_
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_
then it may indeed make sense to *not* use adaption here and re-use the
specific_
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/
> --- lib/lp/
> +++ lib/lp/
> @@ -897,6 +897,16 @@
> <allow
> interface=
> </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=
> + name="PACKAGEBUILD"
> + provides=
>
> <!-- BinaryPackageBu
> <adapter
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -55,6 +55,10 @@
> title=_('The builder behavior required to run this job.'),
> required=False, readonly=True)
>
> + specific_
> + title=_('Job classes that may run on the build farm.'),
> + required=True, readonly=True)
> +
> estimated_duration = Timedelta(
> title=_("Estimated Job Duration"), required=True,
> description=
>
> === modified file 'lib/lp/soyuz/...
| Muharem Hrnjadovic (al-maisan) wrote : | # |
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.
> 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(
>
> which would return the specifc build farm job.
>
> I'm wondering if it's really that you need specific_
> 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_
> 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_
> then it may indeed make sense to *not* use adaption here and re-use the
> specific_
> 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/
>> --- lib/lp/
>> +++ lib/lp/
>> @@ -897,6 +897,16 @@
>> <allow
>> interface=
>> </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=
>> + name="PACKAGEBUILD"
>> + provides=
>>
>> <!-- BinaryPackageBu
>> <adapter
>>
>> === modified file 'lib/lp/
>> --- lib/lp/
>> +++ lib/lp/
>> @@ -55,6 +55,10 @@
>> title=_('The builder behavior required to run this ...
| 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.
> 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(
How would that actually work? IBuildFarmJob is not what we want here.
IBuildQueue.
BuildPackageJob instance.
> which would return the specifc build farm job.
>
> I'm wondering if it's really that you need specific_
> 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_
> 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_
> branch, then it may indeed make sense to *not* use adaption here and
> re-use the specific_
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!
| Muharem Hrnjadovic (al-maisan) 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 :)
> Some comments in line below.
Please see my replies as well as the enclosed incremental diff.
>> === modified file 'lib/lp/
>> --- lib/lp/
>> +++ lib/lp/
>> @@ -897,6 +897,16 @@
>> <allow
>> interface=
>> </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=
>> + name="PACKAGEBUILD"
>> + provides=
>>
>> <!-- BinaryPackageBu
>> <adapter
>>
>> === modified file 'lib/lp/
>> --- lib/lp/
>> +++ lib/lp/
>> @@ -55,6 +55,10 @@
>> title=_('The builder behavior required to run this job.'),
>> required=False, readonly=True)
>>
>> + specific_
>> + title=_('Job classes that may run on the build farm.'),
>> + required=True, readonly=True)
>> +
>> estimated_duration = Timedelta(
>> title=_("Estimated Job Duration"), required=True,
>> description=
>>
>> === modified file 'lib/lp/
>> --- lib/lp/
>> +++ lib/lp/
>> @@ -57,11 +57,31 @@
>> return IBuildFarmJobBe
>>
>> @property
>> + def specific_
>> + """See `IBuildQueue`."""
>> + from zope.component import getSiteManager
>> + from lp.buildmaster.
>
> 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(
>> + # 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(
>
> I was originally going to say that we should be checking first with
> safe_hasat...
| 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/
>>> --- lib/lp/
>>> +++ lib/lp/
>>> @@ -57,11 +57,31 @@
>>> return IBuildFarmJobBe
>>>
>>> @property
>>> + def specific_
>>> + """See `IBuildQueue`."""
>>> + from zope.component import getSiteManager
>>> + from lp.buildmaster.
>>
>> 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/
>>>> --- lib/lp/
>>>> +++ lib/lp/
>>>> @@ -57,11 +57,31 @@
>>>> return IBuildFarmJobBe
>>>>
>>>> @property
>>>> + def specific_
>>>> + """See `IBuildQueue`."""
>>>> + from zope.component import getSiteManager
>>>> + from lp.buildmaster.
>>> 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
| 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) |
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.