Merge lp:~al-maisan/launchpad/merge-conflict into lp:launchpad/db-devel

[Muharem Hrnjadovic]

[Muharem Hrnjadovic (al-maisan)]

Fixes the merge conflict between stable and db-devel.

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'cronscripts/code-import-dispatcher.py'
--- cronscripts/code-import-dispatcher.py	2010-02-22 01:36:30 +0000
+++ cronscripts/code-import-dispatcher.py	2010-02-23 16:20:35 +0000
@@ -37,7 +37,7 @@
         globalErrorUtility.configure('codeimportdispatcher')
 
         dispatcher = CodeImportDispatcher(self.logger, self.options.max_jobs)
-        dispatcher.findAndDispatchJob(
+        dispatcher.findAndDispatchJobs(
             ServerProxy(config.codeimportdispatcher.codeimportscheduler_url))
 
 

=== added file 'lib/canonical/launchpad/doc/db-policy.txt'
--- lib/canonical/launchpad/doc/db-policy.txt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/doc/db-policy.txt	2010-02-23 16:20:35 +0000
@@ -0,0 +1,126 @@
+Storm Stores & Database Policies
+================================
+
+Launchpad has multiple master and slave databases. Changes to data are
+made on the master and replicated asynchronously to the slave
+databases. Slave databases will usually lag a few seconds behind their
+master. Under high load they may lag a few minutes behind, during
+maintenance they may lag a few hours behind and if things explode
+while admins are on holiday they may lag days behind.
+
+If know your code needs to change data, or must have the latest posible
+information, you retrieve objects from the master databases that stores
+the data for your database class.
+
+    >>> from canonical.launchpad.interfaces.lpstorm import IMasterStore
+    >>> from lp.registry.model.person import Person
+    >>> import transaction
+
+    >>> writable_janitor = IMasterStore(Person).find(
+    ...     Person, Person.name == 'janitor').one()
+
+    >>> writable_janitor.displayname = 'Jack the Janitor'
+    >>> transaction.commit()
+
+Sometimes though we know we will not make changes and don't care much
+if the information is a little out of date. In these cases you should
+explicitly retrieve objects from a slave.
+
+The more agressively we retrieve objects from slave databases instead
+of the master, the better the overall performance of Launchpad will be.
+We can distribute this load over many slave databases but are limited to
+a single master.
+
+    >>> from canonical.launchpad.interfaces.lpstorm import ISlaveStore
+    >>> ro_janitor = ISlaveStore(Person).find(
+    ...     Person, Person.name == 'janitor').one()
+    >>> ro_janitor is writable_janitor
+    False
+
+    >>> ro_janitor.displayname = 'Janice the Janitor'
+    >>> transaction.commit()
+    Traceback (most recent call last):
+    ...
+    InternalError: transaction is read-only
+
+    >>> transaction.abort()
+
+Much of our code does not know if the objects being retrieved need to be
+updatable to or have to be absolutely up to date. In this case, we
+retrieve objects from the default store. What object being returned
+depends on the currently installed database policy.
+
+    >>> from canonical.launchpad.interfaces.lpstorm import IStore
+    >>> default_janitor = IStore(Person).find(
+    ...     Person, Person.name == 'janitor').one()
+    >>> default_janitor is writable_janitor
+    True
+
+As you can see, the default database policy retrieves objects from
+the master database. This allows our code written before database
+replication was implemented to keep working.
+
+To alter this behavior, you can install a different database policy.
+
+    >>> from canonical.launchpad.webapp.dbpolicy import SlaveDatabasePolicy
+    >>> with SlaveDatabasePolicy():
+    ...     default_janitor = IStore(Person).find(
+    ...         Person, Person.name == 'janitor').one()
+    >>> default_janitor is writable_janitor
+    False
+
+The database policy can also affect what happens when objects are
+explicitly retrieved from a slave or master database. For example,
+if we have code that needs to run during database maintenance or
+code we want to prove only accesses slave database resources, we can
+raise an exception if an attempt is made to access master database
+resources.
+
+    >>> from canonical.launchpad.webapp.dbpolicy import (
+    ...     SlaveOnlyDatabasePolicy)
+    >>> with SlaveOnlyDatabasePolicy():
+    ...     whoops = IMasterStore(Person).find(
+    ...         Person, Person.name == 'janitor').one()
+    Traceback (most recent call last):
+    ...
+    DisallowedStore: master
+
+We can even ensure no database activity occurs at all, for instance
+if we need to guarantee a potentially long running call doesn't access
+the database at all starting a new and potentially long running
+database transaction.
+
+    >>> from canonical.launchpad.webapp.dbpolicy import DatabaseBlockedPolicy
+    >>> with DatabaseBlockedPolicy():
+    ...     whoops = IStore(Person).find(
+    ...         Person, Person.name == 'janitor').one()
+    Traceback (most recent call last):
+    ...
+    DisallowedStore: ('main', 'default')
+
+Database policies can also be installed and uninstalled using the
+IStoreSelector utility for cases where the 'with' syntax cannot
+be used.
+
+    >>> from canonical.launchpad.webapp.interfaces import IStoreSelector
+    >>> getUtility(IStoreSelector).push(SlaveDatabasePolicy())
+    >>> try:
+    ...     default_janitor = IStore(Person).find(
+    ...         Person, Person.name == 'janitor').one()
+    ... finally:
+    ...     db_policy = getUtility(IStoreSelector).pop()
+    >>> default_janitor is ro_janitor
+    True
+
+Casting
+-------
+
+If you need to change an object you have a read only copy of, or are
+unsure if the object is writable or not, you can easily cast it
+to a writable copy. This is a noop if the object is already writable
+so is good defensive programming.
+
+    >>> from canonical.launchpad.interfaces.lpstorm import IMasterObject
+    >>> IMasterObject(ro_janitor) is writable_janitor
+    True
+

=== modified file 'lib/canonical/launchpad/doc/storm.txt'
--- lib/canonical/launchpad/doc/storm.txt	2009-08-21 17:43:28 +0000
+++ lib/canonical/launchpad/doc/storm.txt	2010-02-23 16:20:35 +0000
@@ -1,3 +1,8 @@
+Note: A more readable version of this is in db-policy.txt. Most of this
+doctest will disappear soon when the auth replication set is collapsed
+back into the main replication set as part of login server seperation.
+-- StuartBishop 20100222
+
 In addition to what Storm provides, we also have some Launchpad
 specific Storm tools to cope with our master and slave store arrangement.
 

=== modified file 'lib/canonical/launchpad/ftests/test_system_documentation.py'
--- lib/canonical/launchpad/ftests/test_system_documentation.py	2010-02-10 23:14:56 +0000
+++ lib/canonical/launchpad/ftests/test_system_documentation.py	2010-02-23 16:20:35 +0000
@@ -7,6 +7,8 @@
 """
 # pylint: disable-msg=C0103
 
+from __future__ import with_statement
+
 import logging
 import os
 import unittest
@@ -391,6 +393,8 @@
         one_test = LayeredDocFileSuite(
             path, setUp=setUp, tearDown=tearDown,
             layer=LaunchpadFunctionalLayer,
+            # 'icky way of running doctests with __future__ imports
+            globs={'with_statement': with_statement},
             stdout_logging_level=logging.WARNING
             )
         suite.addTest(one_test)

=== modified file 'lib/canonical/launchpad/webapp/dbpolicy.py'
--- lib/canonical/launchpad/webapp/dbpolicy.py	2010-02-05 12:17:56 +0000
+++ lib/canonical/launchpad/webapp/dbpolicy.py	2010-02-23 16:20:35 +0000
@@ -111,6 +111,17 @@
         """See `IDatabasePolicy`."""
         pass
 
+    def __enter__(self):
+        """See `IDatabasePolicy`."""
+        getUtility(IStoreSelector).push(self)
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        """See `IDatabasePolicy`."""
+        policy = getUtility(IStoreSelector).pop()
+        assert policy is self, (
+            "Unexpected database policy %s returned by store selector"
+            % repr(policy))
+
 
 class DatabaseBlockedPolicy(BaseDatabasePolicy):
     """`IDatabasePolicy` that blocks all access to the database."""

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2010-02-17 11:13:06 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2010-02-23 16:20:35 +0000
@@ -756,6 +756,20 @@
     The publisher adapts the request to `IDatabasePolicy` to
     instantiate the policy for the current request.
     """
+    def __enter__():
+        """Standard Python context manager interface.
+
+        The IDatabasePolicy will install itself using the IStoreSelector
+        utility.
+        """
+
+    def __exit__(exc_type, exc_value, traceback):
+        """Standard Python context manager interface.
+
+        The IDatabasePolicy will uninstall itself using the IStoreSelector
+        utility.
+        """
+
     def getStore(name, flavor):
         """Retrieve a Store.
 

=== modified file 'lib/lp/code/mail/codereviewcomment.py'
--- lib/lp/code/mail/codereviewcomment.py	2009-10-29 23:51:35 +0000
+++ lib/lp/code/mail/codereviewcomment.py	2010-02-23 16:20:35 +0000
@@ -133,7 +133,7 @@
     def _addAttachments(self, ctrl, email):
         """Add the attachments from the original message."""
         # Only reattach the display_aliases.
-        for content, content_type, filename in self.attachments:
+        for content, filename, content_type in self.attachments:
             # Append directly to the controller's list.
             ctrl.addAttachment(
                 content, content_type=content_type, filename=filename)

=== modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
--- lib/lp/code/mail/tests/test_codereviewcomment.py	2009-11-01 23:13:29 +0000
+++ lib/lp/code/mail/tests/test_codereviewcomment.py	2010-02-23 16:20:35 +0000
@@ -215,6 +215,14 @@
         [outgoing_attachment] = mailer.attachments
         self.assertEqual('inc.diff', outgoing_attachment[1])
         self.assertEqual('text/x-diff', outgoing_attachment[2])
+        # The attachments are attached to the outgoing message.
+        person = bmp.target_branch.owner
+        message = mailer.generateEmail(
+            person.preferredemail.email, person).makeMessage()
+        self.assertTrue(message.is_multipart())
+        attachment = message.get_payload()[1]
+        self.assertEqual('inc.diff', attachment.get_filename())
+        self.assertEqual('text/x-diff', attachment['content-type'])
 
     def makeCommentAndParticipants(self):
         """Create a merge proposal and comment.

=== modified file 'lib/lp/codehosting/codeimport/dispatcher.py'
--- lib/lp/codehosting/codeimport/dispatcher.py	2010-02-22 01:36:30 +0000
+++ lib/lp/codehosting/codeimport/dispatcher.py	2010-02-23 16:20:35 +0000
@@ -16,6 +16,7 @@
 import os
 import socket
 import subprocess
+import time
 
 from canonical.config import config
 
@@ -32,13 +33,14 @@
     worker_script = os.path.join(
         config.root, 'scripts', 'code-import-worker-db.py')
 
-    def __init__(self, logger, worker_limit):
+    def __init__(self, logger, worker_limit, _sleep=time.sleep):
         """Initialize an instance.
 
         :param logger: A `Logger` object.
         """
         self.logger = logger
         self.worker_limit = worker_limit
+        self._sleep = _sleep
 
     def getHostname(self):
         """Return the hostname of this machine.
@@ -65,15 +67,38 @@
 
 
     def findAndDispatchJob(self, scheduler_client):
-        """Check for and dispatch a job if necessary."""
+        """Check for and dispatch a job if necessary.
+
+        :return: A boolean, true if a job was found and dispatched.
+        """
 
         job_id = scheduler_client.getJobForMachine(
             self.getHostname(), self.worker_limit)
 
         if job_id == 0:
             self.logger.info("No jobs pending.")
-            return
+            return False
 
         self.logger.info("Dispatching job %d." % job_id)
 
         self.dispatchJob(job_id)
+        return True
+
+    def _getSleepInterval(self):
+        """How long to sleep for until asking for a new job.
+
+        The basic idea is to wait longer if the machine is more heavily
+        loaded, so that less loaded slaves get a chance to grab some jobs.
+
+        We assume worker_limit will be roughly the number of CPUs in the
+        machine, so load/worker_limit is roughly how loaded the machine is.
+        """
+        return 5*os.getloadavg()[0]/self.worker_limit
+
+    def findAndDispatchJobs(self, scheduler_client):
+        """Call findAndDispatchJob until no job is found."""
+        while True:
+            found = self.findAndDispatchJob(scheduler_client)
+            if not found:
+                break
+            self._sleep(self._getSleepInterval())

=== modified file 'lib/lp/codehosting/codeimport/tests/test_dispatcher.py'
--- lib/lp/codehosting/codeimport/tests/test_dispatcher.py	2010-02-22 02:06:57 +0000
+++ lib/lp/codehosting/codeimport/tests/test_dispatcher.py	2010-02-23 16:20:35 +0000
@@ -24,11 +24,11 @@
 class StubSchedulerClient:
     """A scheduler client that returns a pre-arranged answer."""
 
-    def __init__(self, id_to_return):
-        self.id_to_return = id_to_return
+    def __init__(self, ids_to_return):
+        self.ids_to_return = ids_to_return
 
     def getJobForMachine(self, machine, limit):
-        return self.id_to_return
+        return self.ids_to_return.pop(0)
 
 
 class MockSchedulerClient:
@@ -51,9 +51,10 @@
         TestCase.setUp(self)
         self.pushConfig('codeimportdispatcher', forced_hostname='none')
 
-    def makeDispatcher(self, worker_limit=10):
+    def makeDispatcher(self, worker_limit=10, _sleep=lambda delay: None):
         """Make a `CodeImportDispatcher`."""
-        return CodeImportDispatcher(QuietFakeLogger(), worker_limit)
+        return CodeImportDispatcher(
+            QuietFakeLogger(), worker_limit, _sleep=_sleep)
 
     def test_getHostname(self):
         # By default, getHostname return the same as socket.gethostname()
@@ -111,16 +112,16 @@
         calls = []
         dispatcher = self.makeDispatcher()
         dispatcher.dispatchJob = lambda job_id: calls.append(job_id)
-        dispatcher.findAndDispatchJob(StubSchedulerClient(10))
-        self.assertEqual([10], calls)
+        found = dispatcher.findAndDispatchJob(StubSchedulerClient([10]))
+        self.assertEqual(([10], True), (calls, found))
 
     def test_findAndDispatchJob_noJobWaiting(self):
         # If there is no job to dispatch, then we just exit quietly.
         calls = []
         dispatcher = self.makeDispatcher()
         dispatcher.dispatchJob = lambda job_id: calls.append(job_id)
-        dispatcher.findAndDispatchJob(StubSchedulerClient(0))
-        self.assertEqual([], calls)
+        found = dispatcher.findAndDispatchJob(StubSchedulerClient([0]))
+        self.assertEqual(([], False), (calls, found))
 
     def test_findAndDispatchJob_calls_getJobForMachine_with_limit(self):
         # findAndDispatchJob calls getJobForMachine on the scheduler client
@@ -133,5 +134,29 @@
             [(dispatcher.getHostname(), worker_limit)],
             scheduler_client.calls)
 
+    def test_findAndDispatchJobs(self):
+        # findAndDispatchJobs calls getJobForMachine on the scheduler_client,
+        # dispatching jobs, until it indicates that there are no more jobs to
+        # dispatch.
+        calls = []
+        dispatcher = self.makeDispatcher()
+        dispatcher.dispatchJob = lambda job_id: calls.append(job_id)
+        dispatcher.findAndDispatchJobs(StubSchedulerClient([10, 9, 0]))
+        self.assertEqual([10, 9], calls)
+
+    def test_findAndDispatchJobs_sleeps(self):
+        # After finding a job, findAndDispatchJobs sleeps for an interval as
+        # returned by _getSleepInterval.
+        sleep_calls = []
+        interval = self.factory.getUniqueInteger()
+        def _sleep(delay):
+            sleep_calls.append(delay)
+        dispatcher = self.makeDispatcher(_sleep=_sleep)
+        dispatcher.dispatchJob = lambda job_id: None
+        dispatcher._getSleepInterval = lambda : interval
+        dispatcher.findAndDispatchJobs(StubSchedulerClient([10, 0]))
+        self.assertEqual([interval], sleep_calls)
+
+
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/codehosting/scanner/email.py'
--- lib/lp/codehosting/scanner/email.py	2010-01-06 12:15:42 +0000
+++ lib/lp/codehosting/scanner/email.py	2010-02-23 16:20:35 +0000
@@ -38,15 +38,18 @@
     if number_removed == 0:
         return
     if number_removed == 1:
-        contents = '1 revision was removed from the branch.'
+        count = '1 revision'
+        contents = '%s was removed from the branch.' % count
     else:
-        contents = ('%d revisions were removed from the branch.'
-                    % number_removed)
+        count = '%d revisions' % number_removed
+        contents = '%s were removed from the branch.' % count
     # No diff is associated with the removed email.
+    subject = "[Branch %s] %s removed" % (
+        revisions_removed.db_branch.unique_name, count)
     getUtility(IRevisionMailJobSource).create(
         revisions_removed.db_branch, revno='removed',
         from_address=config.canonical.noreply_from_address,
-        body=contents, perform_diff=False, subject=None)
+        body=contents, perform_diff=False, subject=subject)
 
 
 @adapter(events.TipChanged)
@@ -62,9 +65,11 @@
         message = ('First scan of the branch detected %s'
                    ' in the revision history of the branch.' %
                    revisions)
+        subject = "[Branch %s] %s" % (
+            tip_changed.db_branch.unique_name, revisions)
         getUtility(IRevisionMailJobSource).create(
             tip_changed.db_branch, 'initial',
-            config.canonical.noreply_from_address, message, False, None)
+            config.canonical.noreply_from_address, message, False, subject)
     else:
         getUtility(IRevisionsAddedJobSource).create(
             tip_changed.db_branch, tip_changed.db_branch.last_scanned_id,

=== modified file 'lib/lp/codehosting/scanner/tests/test_email.py'
--- lib/lp/codehosting/scanner/tests/test_email.py	2009-07-17 00:26:05 +0000
+++ lib/lp/codehosting/scanner/tests/test_email.py	2010-02-23 16:20:35 +0000
@@ -63,8 +63,12 @@
         self.assertEqual(len(stub.test_emails), 1)
         [initial_email] = stub.test_emails
         expected = 'First scan of the branch detected 0 revisions'
-        email_body = email.message_from_string(initial_email[2]).get_payload()
+        message = email.message_from_string(initial_email[2])
+        email_body = message.get_payload()
         self.assertTextIn(expected, email_body)
+        self.assertEqual(
+            '[Branch %s] 0 revisions' % self.db_branch.unique_name,
+            message['Subject'])
 
     def test_import_revision(self):
         self.commitRevision()
@@ -74,8 +78,12 @@
         [initial_email] = stub.test_emails
         expected = ('First scan of the branch detected 1 revision'
                     ' in the revision history of the=\n branch.')
-        email_body = email.message_from_string(initial_email[2]).get_payload()
+        message = email.message_from_string(initial_email[2])
+        email_body = message.get_payload()
         self.assertTextIn(expected, email_body)
+        self.assertEqual(
+            '[Branch %s] 1 revision' % self.db_branch.unique_name,
+            message['Subject'])
 
     def test_import_uncommit(self):
         self.commitRevision()
@@ -88,9 +96,12 @@
         self.assertEqual(len(stub.test_emails), 1)
         [uncommit_email] = stub.test_emails
         expected = '1 revision was removed from the branch.'
-        email_body = email.message_from_string(
-            uncommit_email[2]).get_payload()
+        message = email.message_from_string(uncommit_email[2])
+        email_body = message.get_payload()
         self.assertTextIn(expected, email_body)
+        self.assertEqual(
+            '[Branch %s] 1 revision removed' % self.db_branch.unique_name,
+            message['Subject'])
 
     def test_import_recommit(self):
         # When scanning the uncommit and new commit there should be an email

=== modified file 'lib/lp/scripts/utilities/importfascist.py'
--- lib/lp/scripts/utilities/importfascist.py	2010-02-04 03:07:25 +0000
+++ lib/lp/scripts/utilities/importfascist.py	2010-02-23 16:20:35 +0000
@@ -173,12 +173,15 @@
                 % self.import_into)
 
 
+# The names of the arguments form part of the interface of __import__(...), and
+# must not be changed, as code may choose to invoke __import__ using keyword
+# arguments - e.g. the encodings module in Python 2.6.
 # pylint: disable-msg=W0102,W0602
-def import_fascist(module_name, globals={}, locals={}, from_list=[], level=-1):
+def import_fascist(name, globals={}, locals={}, fromlist=[], level=-1):
     global naughty_imports
 
     try:
-        module = original_import(module_name, globals, locals, from_list, level)
+        module = original_import(name, globals, locals, fromlist, level)
     except ImportError:
         # XXX sinzui 2008-04-17 bug=277274:
         # import_fascist screws zope configuration module which introspects
@@ -188,18 +191,18 @@
         # time doesn't exist and dies a horrible death because of the import
         # fascist. That's the long explanation for why we special case this
         # module.
-        if module_name.startswith('zope.app.layers.'):
-            module_name = module_name[16:]
-            module = original_import(module_name, globals, locals, from_list, level)
+        if name.startswith('zope.app.layers.'):
+            name = name[16:]
+            module = original_import(name, globals, locals, fromlist, level)
         else:
             raise
     # Python's re module imports some odd stuff every time certain regexes
     # are used.  Let's optimize this.
-    if module_name == 'sre':
+    if name == 'sre':
         return module
 
     # Mailman 2.1 code base is originally circa 1998, so yeah, no __all__'s.
-    if module_name.startswith('Mailman'):
+    if name.startswith('Mailman'):
         return module
 
     # Some uses of __import__ pass None for globals, so handle that.
@@ -215,14 +218,14 @@
 
     # Check the "NotFoundError" policy.
     if (import_into.startswith('canonical.launchpad.database') and
-        module_name == 'zope.exceptions'):
-        if from_list and 'NotFoundError' in from_list:
+        name == 'zope.exceptions'):
+        if fromlist and 'NotFoundError' in fromlist:
             raise NotFoundPolicyViolation(import_into)
 
     # Check the database import policy.
-    if (module_name.startswith(database_root) and
+    if (name.startswith(database_root) and
         not database_import_allowed_into(import_into)):
-        error = DatabaseImportPolicyViolation(import_into, module_name)
+        error = DatabaseImportPolicyViolation(import_into, name)
         naughty_imports.add(error)
         # Raise an error except in the case of browser.traversers.
         # This exception to raising an error is only temporary, until
@@ -231,28 +234,28 @@
             raise error
 
     # Check the import from __all__ policy.
-    if from_list is not None and (
+    if fromlist is not None and (
         import_into.startswith('canonical') or import_into.startswith('lp')):
         # We only want to warn about "from foo import bar" violations in our
         # own code.
-        from_list = list(from_list)
+        fromlist = list(fromlist)
         module_all = getattr(module, '__all__', None)
         if module_all is None:
-            if from_list == ['*']:
+            if fromlist == ['*']:
                 # "from foo import *" is naughty if foo has no __all__
-                error = FromStarPolicyViolation(import_into, module_name)
+                error = FromStarPolicyViolation(import_into, name)
                 naughty_imports.add(error)
                 raise error
         else:
-            if from_list == ['*']:
+            if fromlist == ['*']:
                 # "from foo import *" is allowed if foo has an __all__
                 return module
             if is_test_module(import_into):
                 # We don't bother checking imports into test modules.
                 return module
-            allowed_from_list = valid_imports_not_in_all.get(
-                module_name, set())
-            for attrname in from_list:
+            allowed_fromlist = valid_imports_not_in_all.get(
+                name, set())
+            for attrname in fromlist:
                 # Check that each thing we are importing into the module is
                 # either in __all__, is a module itself, or is a specific
                 # exception.
@@ -264,13 +267,13 @@
                     # You can import modules even when they aren't declared in
                     # __all__.
                     continue
-                if attrname in allowed_from_list:
+                if attrname in allowed_fromlist:
                     # Some things can be imported even if they aren't in
                     # __all__.
                     continue
                 if attrname not in module_all:
                     error = NotInModuleAllPolicyViolation(
-                        import_into, module_name, attrname)
+                        import_into, name, attrname)
                     naughty_imports.add(error)
     return module
 

=== modified file 'lib/lp/soyuz/model/buildqueue.py'
--- lib/lp/soyuz/model/buildqueue.py	2010-02-02 14:01:14 +0000
+++ lib/lp/soyuz/model/buildqueue.py	2010-02-23 16:20:35 +0000
@@ -506,6 +506,9 @@
         result_set = store.find(
             BuildQueue,
             BuildQueue.job == Job.id,
+            # XXX Michael Nelson 2010-02-22 bug=499421
+            # Avoid corrupt build jobs where the builder is None.
+            BuildQueue.builder != None,
             # status is a property. Let's use _status.
             Job._status == JobStatus.RUNNING,
             Job.date_started != None)

=== modified file 'lib/lp/soyuz/tests/test_buildqueue.py'
--- lib/lp/soyuz/tests/test_buildqueue.py	2010-02-22 22:50:46 +0000
+++ lib/lp/soyuz/tests/test_buildqueue.py	2010-02-23 16:20:35 +0000
@@ -177,6 +177,19 @@
         buildqueue = BuildQueue(job=job.id)
         self.assertEquals(buildqueue, self.buildqueueset.getByJob(job))
 
+    def test_getActiveBuildJobs_no_builder_bug499421(self):
+        # An active build queue item that does not have a builder will
+        # not be included in the results and so will not block the
+        # buildd-manager.
+        active_jobs = self.buildqueueset.getActiveBuildJobs()
+        self.assertEqual(1, active_jobs.count())
+        active_job = active_jobs[0]
+        active_job.builder = None
+        self.assertTrue(
+            self.buildqueueset.getActiveBuildJobs().is_empty(),
+            "An active build job must have a builder.")
+
+
 
 class TestBuildQueueBase(TestCaseWithFactory):
     """Setup the test publisher and some builders."""
@@ -248,7 +261,10 @@
 
         # hppa native
         self.builders[(self.hppa_proc.id, False)] = [
-            self.h5, self.h6, self.h7]
+            self.h5,
+            self.h6,
+            self.h7,
+            ]
         # hppa virtual
         self.builders[(self.hppa_proc.id, True)] = [
             self.h1, self.h2, self.h3, self.h4]