launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-903688 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #903688 in Launchpad itself: "Build farm jobs failing: read-only transaction."
  https://bugs.launchpad.net/launchpad/+bug/903688

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-903688/+merge/85485

= Summary =

Fallout from my making the build manager default to read-only database transactions.  We're getting broken builders.  The current-build status they show is "read-only transaction."  William points out that one of the implementations of postprocessCandidate may try to delete a redundant job; it needs to be wrapped in a read-write database transaction so that it can.


== Proposed fix ==

Wrap the postprocessCandidate loop in a read-write transaction policy.


== Pre-implementation notes ==

Discussed with wgrant.  There were two options: wrap the object deletion inside that one postprocessCandidate implementation in a read-write policy, hidden from the management code that normally takes care of such things; or wrap the relevant bit of management code itself in one, and risk nasty Twisted stuff happening further down the call chain.

No Deferreds are returned here, so we chose to take our chances with the latter.


== Implementation details ==

There was also the choice of whether to make each postprocessCandidate its own transaction or to wrap the entire loop.  In practice I don't think it matters much, since only one postprocessCandidate currently changes job state.


== Tests ==

I'm running all Soyuz & buildmaster tests, but here's one I added specifically for this:
{{{
./bin/test -vvc lp.buildmaster.tests.test_builder -t postprocesses
}}}

== Demo and Q/A ==

The build farm should work!


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/buildmaster/model/builder.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-903688/+merge/85485
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-903688 into lp:launchpad.

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2011-11-24 07:27:35 +0000
+++ lib/lp/buildmaster/model/builder.py	2011-12-13 13:36:24 +0000
@@ -801,13 +801,17 @@
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
         candidate_jobs = store.execute(query).get_all()
 
-        for (candidate_id,) in candidate_jobs:
-            candidate = getUtility(IBuildQueueSet).get(candidate_id)
-            job_class = job_classes[candidate.job_type]
-            candidate_approved = job_class.postprocessCandidate(
-                candidate, logger)
-            if candidate_approved:
-                return candidate
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=False):
+            for (candidate_id,) in candidate_jobs:
+                candidate = getUtility(IBuildQueueSet).get(candidate_id)
+                job_class = job_classes[candidate.job_type]
+                candidate_approved = job_class.postprocessCandidate(
+                    candidate, logger)
+                if candidate_approved:
+                    transaction.commit()
+                    return candidate
+            transaction.commit()
 
         return None
 

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2011-11-22 06:59:01 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2011-12-13 13:36:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test Builder features."""
@@ -8,13 +8,14 @@
 import tempfile
 import xmlrpclib
 
+from lpbuildd.slave import BuilderStatus
 from testtools.deferredruntest import (
     assert_fails_with,
     AsynchronousDeferredRunTest,
     AsynchronousDeferredRunTestForBrokenTwisted,
     SynchronousDeferredRunTest,
     )
-
+import transaction
 from twisted.internet.defer import (
     CancelledError,
     DeferredList,
@@ -22,15 +23,12 @@
 from twisted.internet.task import Clock
 from twisted.python.failure import Failure
 from twisted.web.client import getPage
-
 from zope.component import getUtility
 from zope.security.proxy import (
     isinstance as zope_isinstance,
     removeSecurityProxy,
     )
 
-from lpbuildd.slave import BuilderStatus
-
 from canonical.config import config
 from canonical.database.sqlbase import flush_database_updates
 from canonical.launchpad.webapp.interfaces import (
@@ -45,6 +43,7 @@
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.builder import (
     CannotFetchFile,
+    CannotResumeHost,
     IBuilder,
     IBuilderSet,
     )
@@ -52,7 +51,6 @@
     IBuildFarmJobBehavior,
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
-from lp.buildmaster.interfaces.builder import CannotResumeHost
 from lp.buildmaster.model.builder import (
     BuilderSlave,
     ProxyWithConnectionTimeout,
@@ -74,6 +72,8 @@
     TrivialBehavior,
     WaitingSlave,
     )
+from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.log.logger import BufferLogger
 from lp.soyuz.enums import (
@@ -534,6 +534,26 @@
         # And the old_candidate is superseded:
         self.assertEqual(BuildStatus.SUPERSEDED, build.status)
 
+    def test_findBuildCandidate_postprocesses_in_read_write_policy(self):
+        # _findBuildCandidate invokes BuildFarmJob.postprocessCandidate,
+        # which may modify the database.  This happens in a read-write
+        # transaction even if _findBuildCandidate itself runs in a
+        # read-only transaction policy.
+
+        # PackageBuildJob.postprocessCandidate will attempt to delete
+        # security builds.
+        pub = self.publisher.getPubSource(
+            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
+            archive=self.factory.makeArchive(),
+            pocket=PackagePublishingPocket.SECURITY)
+        pub.createMissingBuilds()
+        transaction.commit()
+        with DatabaseTransactionPolicy(read_only=True):
+            removeSecurityProxy(self.frog_builder)._findBuildCandidate()
+            # The test is that this passes without a "transaction is
+            # read-only" error.
+            transaction.commit()
+
     def test_acquireBuildCandidate_marks_building(self):
         # acquireBuildCandidate() should call _findBuildCandidate and
         # mark the build as building.