launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/build-queue-sizes-bug-609904 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #609904 BuilderSet.getBuildQueueSizes needs to consider virtualized=NULL as false
  https://bugs.launchpad.net/bugs/609904


= 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.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/build-queue-sizes-bug-609904/+merge/31647
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/build-queue-sizes-bug-609904 into lp:launchpad/devel.

=== 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 15:06:11 +0000
@@ -173,12 +173,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 15:06:11 +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)