Merge lp:~julian-edwards/launchpad/build-queue-sizes-bug-609904 into lp:launchpad

[Julian Edwards]

= Summary =
Put pending builds that have virtualized=False in the virtual build queues on
/builders

== Proposed fix ==
IBuilderSet.getBuildQueueSizes() is horribly broken because
BuildQueue.virtualized can be True, False or None. If it's None then the
query will potentially return three groups instead of the expected two, and
because of the bad code in the following "for" loop it will overwrite the
dictionary entry for "nonvirt" depending on which group it sees first in the
results iteration.

== Implementation details ==
Simple fix in the query to coalesce the result so that if it's NULL we turn it
into a True. Also made the inline "if" more explicit.

== Tests ==
bin/test -cvvt builder.txt

== Demo and Q/A ==
Easy to check on edge, there's about 160 pending translations builds
erroneously showing up in the distro builders queue right now!

= Launchpad lint =

Checking for conflicts and issues in changed files.

[Māris Fogels (mars)]

Hi Julian,

I have a bit of difficulty reviewing this without domain-specific knowledge, so I'll trust you that the code is ok. But I did have a question about the tests.

You see, I'm a bit confused by the doc/ directory and the files within. When I am reading a documentation file I don't care what the 'virtualized' flag does, I just care that it exists. I also don't want to read the gritty details of how it behaves correctly when a certain bug is present (which is what the new doctest code is for). The implementation details and bugfix test should be off in a unit test somewhere.

So, is the doctest the right place to exercise the bugfix? What is the purpose of doc/builder.txt?

I'm marking this as 'Needs Information' for now.

Maris

review: Needs Information (code)

[Julian Edwards (julian-edwards)]

Thanks for the review Maris.

<bigjools> mars: thank you
<bigjools> mars: so, no, the doctest is absolutely not the right place for
that test but I don't have the time to convert that all to unit tests and I am
loathe to write a new unit test and not include the existing ones.

I'm happy to file a cleanup bug about it.

[Māris Fogels (mars)]

As discussed on IRC: I'm marking this as able to land now, but on the condition that someone be scheduled to rewrite this doctest section into a buildmaster unit test. I am not willing to let the doctests change in this way.

(We probably want an XXX line for bookkeeping and kanban)

Maris

review: Approve (code)

=== modified file 'lib/lp/buildmaster/doc/builder.txt'
--- lib/lp/buildmaster/doc/builder.txt	2010-06-25 08:45:00 +0000
+++ lib/lp/buildmaster/doc/builder.txt	2010-08-03 17:51:03 +0000
@@ -1,3 +1,8 @@
+XXX Julian 2010-08-03 bug=613096
+Most of this doctest is really a unit test in disguise.  It should get
+re-written and unit tests moved to buildmaster/tests/test_builder.py
+
+
 =============
 Builder Class
 =============
@@ -173,12 +178,27 @@
     >>> from lp.soyuz.interfaces.processor import IProcessorFamilySet
     >>> i386_family = getUtility(IProcessorFamilySet).getByName('x86')
     >>> recipe_bq.processor = i386_family.processors[0]
+    >>> recipe_bq.virtualized = True
+    >>> transaction.commit()
     
-    >>> transaction.commit()
     >>> queue_sizes = builderset.getBuildQueueSizes()
     >>> print queue_sizes['virt']['386']
     (1L, datetime.timedelta(0, 64))
 
+Any BuildQueues with a null `virtualized` property are considered virtual
+by default.
+
+    >>> recipe_bq = factory.makeSourcePackageRecipeBuildJob()
+    >>> recipe_bq.virtualized = None
+    >>> recipe_bq.processor = i386_family.processors[0]
+    >>> transaction.commit()
+    >>> queue_sizes = builderset.getBuildQueueSizes()
+
+The virtual queue size has increased accordingly:
+
+    >>> print queue_sizes['virt']['386']
+    (2L, datetime.timedelta(0, 128))
+
 
 Resuming buildd slaves
 ======================

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-08-02 02:13:52 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-08-03 17:51:03 +0000
@@ -24,7 +24,7 @@
 
 from sqlobject import (
     BoolCol, ForeignKey, IntCol, SQLObjectNotFound, StringCol)
-from storm.expr import Count, Sum
+from storm.expr import Coalesce, Count, Sum
 from storm.store import Store
 from zope.component import getUtility
 from zope.interface import implements
@@ -704,15 +704,18 @@
             Count(),
             Sum(BuildQueue.estimated_duration),
             Processor,
-            BuildQueue.virtualized),
+            Coalesce(BuildQueue.virtualized, True)),
             Processor.id == BuildQueue.processorID,
             Job.id == BuildQueue.jobID,
             Job._status == JobStatus.WAITING).group_by(
-                Processor, BuildQueue.virtualized)
+                Processor, Coalesce(BuildQueue.virtualized, True))
 
         result_dict = {'virt': {}, 'nonvirt': {}}
         for size, duration, processor, virtualized in results:
-            virt_str = 'virt' if virtualized else 'nonvirt'
+            if virtualized is False:
+                virt_str = 'nonvirt'
+            else:
+                virt_str = 'virt'
             result_dict[virt_str][processor.name] = (
                 size, duration)