Merge lp:~wgrant/launchpad/bug-549340-fix-buildqueue-destruction into lp:launchpad

[William Grant]

Fix destruction of translation template build farm jobs.

This branch fixes BuildQueue.destroySelf to not assume that IBuildFarmJobs are Storm objects. TranslationTemplatesBuildJob isn't -- its DB object is actually a BranchJob.

I've replaced the direct store removal with a call to IBFJ.destroySelf, and implemented it on the two classes that needed it. It actually lives on IBuildFarmBuildJob for now, since TTBJ has security declarations for both IBuildFarmJob and IBranchJob, the latter already defining destroySelf -- this results in a Zope permission definition conflict, even though the permissions are actually the same.

I've also added tests for destruction of TTBJ and SPRBJ BuildQueues. That required the granting of DELETE on SPRBJ to launchpad_main.

[Julian Edwards (julian-edwards)]

I do not like this fix, we should not be adding more SQLObject methods. Instead, there should be a real TranslationTemplatesBuildJob table so that build history is preserved.

I'm a bit flummoxed as to how its DB object is a BranchJob?

review: Disapprove

Unmerged revisions

10606. By William Grant on 2010-03-27

Let IBuildFarmJobs decide their own fate -- don't try to directly remove them from their Store.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-03-15 01:26:43 +0000
+++ database/schema/security.cfg	2010-03-27 11:11:27 +0000
@@ -280,7 +280,7 @@
 public.sourcepackageformatselection     = SELECT
 public.sourcepackagerecipe              = SELECT, INSERT, UPDATE, DELETE
 public.sourcepackagerecipebuild         = SELECT, INSERT, UPDATE, DELETE
-public.sourcepackagerecipebuildjob      = SELECT, INSERT, UPDATE
+public.sourcepackagerecipebuildjob      = SELECT, INSERT, UPDATE, DELETE
 public.sourcepackagerecipedata          = SELECT, INSERT, UPDATE, DELETE
 public.sourcepackagerecipedatainstruction = SELECT, INSERT, UPDATE, DELETE
 public.specificationbranch              = SELECT, INSERT, UPDATE, DELETE

=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py	2010-03-17 05:58:22 +0000
+++ lib/lp/buildmaster/model/buildqueue.py	2010-03-27 11:11:27 +0000
@@ -133,7 +133,7 @@
         job = self.job
         specific_job = self.specific_job
         SQLBase.destroySelf(self)
-        Store.of(specific_job).remove(specific_job)
+        specific_job.destroySelf()
         job.destroySelf()
 
     def manualScore(self, value):

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2010-03-25 15:24:39 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2010-03-27 11:11:27 +0000
@@ -216,3 +216,7 @@
         store = IMasterStore(SourcePackageRecipeBuildJob)
         store.add(specific_job)
         return specific_job
+
+    def destroySelf(self):
+        """See `IBuildFarmJob`."""
+        Store.of(self).remove(self)

=== modified file 'lib/lp/code/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/tests/test_sourcepackagerecipebuild.py	2010-03-25 15:29:31 +0000
+++ lib/lp/code/tests/test_sourcepackagerecipebuild.py	2010-03-27 11:11:27 +0000
@@ -11,6 +11,7 @@
 import transaction
 from zope.component import getUtility
 
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.testing.layers import DatabaseFunctionalLayer
 
 from lp.buildmaster.interfaces.buildbase import IBuildBase
@@ -18,6 +19,8 @@
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuild,
     ISourcePackageRecipeBuildSource)
+from lp.code.model.sourcepackagerecipebuild import (
+    SourcePackageRecipeBuildJob)
 from lp.testing import TestCaseWithFactory
 
 
@@ -92,6 +95,21 @@
         self.assertEqual(
             datetime.timedelta(minutes=2), spb.estimateDuration())
 
+    def test_BuildQueue_destruction(self):
+        # When destroying the related BuildQueue, the SPRBJ is also
+        # destroyed.
+        spb = self.makeSourcePackageRecipeBuild()
+        bq = spb.queueBuild()
+        self.assertEqual(
+            1,
+            IStore(SourcePackageRecipeBuildJob).find(
+                SourcePackageRecipeBuildJob, build=spb).count())
+        bq.destroySelf()
+        self.assertEqual(
+            0,
+            IStore(SourcePackageRecipeBuildJob).find(
+                SourcePackageRecipeBuildJob, build=spb).count())
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/soyuz/interfaces/buildfarmbuildjob.py'
--- lib/lp/soyuz/interfaces/buildfarmbuildjob.py	2010-03-16 05:08:47 +0000
+++ lib/lp/soyuz/interfaces/buildfarmbuildjob.py	2010-03-27 11:11:27 +0000
@@ -19,3 +19,9 @@
     build = Reference(
         IBuild, title=_("Build"), required=True, readonly=True,
         description=_("Build record associated with this job."))
+
+    # XXX wgrant 2010-03-27: This should be on IBuildFarmJob, but
+    # IBranchJob has this as well so TTBJ security dies with a conflict.
+    def destroySelf():
+        """Destroy this job."""
+

=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py	2010-03-25 14:38:25 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py	2010-03-27 11:11:27 +0000
@@ -11,6 +11,7 @@
 import pytz
 
 from storm.locals import Int, Reference, Storm
+from storm.store import Store
 
 from zope.interface import implements
 from zope.component import getUtility
@@ -164,6 +165,10 @@
         """See `IBuildFarmJob`."""
         return self.build.is_virtualized
 
+    def destroySelf(self):
+        """See `IBuildFarmJob`."""
+        Store.of(self).remove(self)
+
     @staticmethod
     def addCandidateSelectionCriteria(processor, virtualized):
         """See `IBuildFarmCandidateJobSelection`."""

=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildjob.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildjob.py	2010-03-06 00:21:57 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildjob.py	2010-03-27 11:11:27 +0000
@@ -11,6 +11,7 @@
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.launchpad.webapp.testing import verifyObject
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
@@ -107,6 +108,22 @@
         # future however the scoring system is to be revisited.
         self.assertEqual(1000, self.specific_job.score())
 
+    def test_BuildQueue_destruction(self):
+        # A TTBJ's BuildQueue can destroy itself, which also destroys
+        # the related BranchJob.
+        self.assertEqual(
+            1,
+            IStore(BranchJob).find(
+                BranchJob, BranchJob.branch == self.branch).count())
+
+        buildqueue = getUtility(IBuildQueueSet).getByJob(
+            self.specific_job.job)
+        buildqueue.destroySelf()
+
+        self.assertEqual(
+            0,
+            IStore(BranchJob).find(BranchJob, branch=self.branch).count())
+
 
 class FakeTranslationTemplatesJobSource(TranslationTemplatesBuildJob):
     """Fake utility class.