Merge lp:~jtv/launchpad/bug-602333 into lp:launchpad

[Jeroen T. Vermeulen]

Delete BuildQueue entries for BranchJobs on any Branch being deleted.

= Bug 602333 =

Deleting branches can fail because of foreign-key constraints in the buildfarm classes. It may be best to read the bug comments before reviewing this.

A branch can have BranchJobs attached. The references look like this:

    Branch <- BranchJob -> Job

Branch deletion breaks the foreign-key constraint by deleting the Job; the schema is set up to let that deletion cascade to BranchJob.

One type of BranchJob is TranslationTemplatesBuildJob. This is a job for the build farm, so it comes with a BuildQueue entry attached (which goes away when the job completes). Looking at the object-oriented side, there is no direct reference between the BuildQueue and the Job; they are linked only in that they both refer to Job.

    Branch <- BranchJob -> Job <- BuildQueue

The reference from BuildQueue to Job does not cascade deletions, so the Job cannot be deleted as long as the BuildQueue exists. Nor would it be reasonable to make it cascade, since BuildQueue in the general case has no connection to any BranchJob that might be deleted in this way. It's a peculiarity of TranslationTemplatesBuildJob.

After discussion with Aaron and Jono, I fixed this in a manner that's consistent with how the tie between Branch and BranchJob is broken: delete any BuildQueue entries straight from the database before deleting the Jobs.

Test:
{{{
./bin/test -vvc -m lp.code.tests.test_branch -t TestBranchDeletion
}}}

You'll also see some drive-by lint cleanups. And I moved two tests that were clearly in the wrong test case.

Jeroen

[Robert Collins (lifeless)]

review: Approve

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-07-09 10:22:32 +0000
+++ lib/lp/code/model/branch.py	2010-07-17 23:18:49 +0000
@@ -46,6 +46,7 @@
     IStoreSelector, MAIN_STORE, SLAVE_FLAVOR)
 
 from lp.app.errors import UserCannotUnsubscribePerson
+from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.code.bzr import (
     BranchFormat, ControlFormat, CURRENT_BRANCH_FORMATS,
     CURRENT_REPOSITORY_FORMATS, RepositoryFormat)
@@ -450,7 +451,7 @@
     @property
     def code_is_browseable(self):
         """See `IBranch`."""
-        return (self.revision_count > 0  or self.last_mirrored != None)
+        return (self.revision_count > 0 or self.last_mirrored != None)
 
     def codebrowse_url(self, *extras):
         """See `IBranch`."""
@@ -890,19 +891,18 @@
             prefix = config.launchpad.bzr_imports_root_url
             return urlappend(prefix, '%08x' % self.id)
         else:
-            raise AssertionError("No pull URL for %r" % (self,))
+            raise AssertionError("No pull URL for %r" % (self, ))
 
     def requestMirror(self):
         """See `IBranch`."""
         if self.branch_type == BranchType.REMOTE:
             raise BranchTypeError(self.unique_name)
-        from canonical.launchpad.interfaces import IStore
-        IStore(self).find(
+        branch = Store.of(self).find(
             Branch,
             Branch.id == self.id,
             Or(Branch.next_mirror_time > UTC_NOW,
-               Branch.next_mirror_time == None)
-            ).set(next_mirror_time=UTC_NOW)
+               Branch.next_mirror_time == None))
+        branch.set(next_mirror_time=UTC_NOW)
         self.next_mirror_time = AutoReload
         return self.next_mirror_time
 
@@ -964,31 +964,45 @@
         """See `IBranch`."""
         return self.destroySelf(break_references=True)
 
+    def _deleteBranchSubscriptions(self):
+        """Delete subscriptions for this branch prior to deleting branch."""
+        subscriptions = Store.of(self).find(
+            BranchSubscription, BranchSubscription.branch == self)
+        subscriptions.remove()
+
+    def _deleteJobs(self):
+        """Delete jobs for this branch prior to deleting branch.
+
+        This deletion includes `BranchJob`s associated with the branch,
+        as well as `BuildQueue` entries for `TranslationTemplateBuildJob`s.
+        """
+        # Avoid circular imports.
+        from lp.code.model.branchjob import BranchJob
+
+        store = Store.of(self)
+        affected_jobs = Select(
+            [BranchJob.jobID],
+            And(BranchJob.job == Job.id, BranchJob.branch == self))
+
+        # Delete BuildQueue entries for affected Jobs.  They would pin
+        # the affected Jobs in the database otherwise.
+        store.find(BuildQueue, BuildQueue.jobID.is_in(affected_jobs)).remove()
+
+        # Delete Jobs.  Their BranchJobs cascade along in the database.
+        store.find(Job, Job.id.is_in(affected_jobs)).remove()
+
     def destroySelf(self, break_references=False):
         """See `IBranch`."""
-        from lp.code.model.branchjob import BranchJob
         from lp.code.interfaces.branchjob import IReclaimBranchSpaceJobSource
         if break_references:
             self._breakReferences()
         if not self.canBeDeleted():
             raise CannotDeleteBranch(
                 "Cannot delete branch: %s" % self.unique_name)
-        # BranchRevisions are taken care of a cascading delete
-        # in the database.
-        store = Store.of(self)
-        # Delete the branch subscriptions.
-        subscriptions = store.find(
-            BranchSubscription, BranchSubscription.branch == self)
-        subscriptions.remove()
-        # Delete any linked jobs.
-        # Using a sub-select here as joins in delete statements is not
-        # valid standard sql.
-        jobs = store.find(
-            Job,
-            Job.id.is_in(Select([BranchJob.jobID],
-                                And(BranchJob.job == Job.id,
-                                    BranchJob.branch == self))))
-        jobs.remove()
+
+        self._deleteBranchSubscriptions()
+        self._deleteJobs()
+
         # Now destroy the branch.
         branch_id = self.id
         SQLBase.destroySelf(self)
@@ -997,8 +1011,10 @@
 
     def commitsForDays(self, since):
         """See `IBranch`."""
+
         class DateTrunc(NamedFunc):
             name = "date_trunc"
+
         results = Store.of(self).find(
             (DateTrunc('day', Revision.revision_date), Count(Revision.id)),
             Revision.id == BranchRevision.revisionID,
@@ -1080,6 +1096,7 @@
     def __init__(self, affected_object, rationale):
         self.affected_object = ProxyFactory(affected_object)
         self.rationale = rationale
+
     def __call__(self):
         """Perform the deletion operation."""
         raise NotImplementedError(DeletionOperation.__call__)
@@ -1161,7 +1178,7 @@
 
     def __init__(self, code_import):
         DeletionOperation.__init__(
-            self, code_import, _( 'This is the import data for this branch.'))
+            self, code_import, _('This is the import data for this branch.'))
 
     def __call__(self):
         from lp.code.model.codeimport import CodeImportSet

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2010-05-27 01:34:14 +0000
+++ lib/lp/code/model/tests/test_branch.py	2010-07-17 23:18:49 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=F0401,E1002
@@ -34,6 +34,7 @@
 from canonical.launchpad.webapp.interfaces import IOpenLaunchBag
 from canonical.testing import DatabaseFunctionalLayer, LaunchpadZopelessLayer
 
+from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.blueprints.interfaces.specification import (
     ISpecificationSet, SpecificationDefinitionStatus)
 from lp.blueprints.model.specificationbranch import (
@@ -78,6 +79,8 @@
     person_logged_in, run_with_login, TestCase, TestCaseWithFactory,
     time_counter)
 from lp.testing.factory import LaunchpadObjectFactory
+from lp.translations.model.translationtemplatesbuildjob import (
+    ITranslationTemplatesBuildJobSource)
 
 
 class TestCodeImport(TestCase):
@@ -1039,6 +1042,39 @@
         # Need to commit the transaction to fire off the constraint checks.
         transaction.commit()
 
+    def test_related_TranslationTemplatesBuildJob_cleaned_out(self):
+        # A TranslationTemplatesBuildJob is a type of BranchJob that
+        # comes with a BuildQueue entry referring to the same Job.
+        # Deleting the branch cleans up the BuildQueue before it can
+        # remove the Job and BranchJob.
+        branch = self.factory.makeAnyBranch()
+        getUtility(ITranslationTemplatesBuildJobSource).create(branch)
+
+        branch.destroySelf(break_references=True)
+
+        Store.of(branch).flush()
+
+    def test_unrelated_TranslationTemplatesBuildJob_intact(self):
+        # No innocent BuildQueue entries are harmed in deleting a
+        # branch.
+        branch = self.factory.makeAnyBranch()
+        other_branch = self.factory.makeAnyBranch()
+        source = getUtility(ITranslationTemplatesBuildJobSource)
+        job = source.create(branch)
+        other_job = source.create(other_branch)
+        store = Store.of(branch)
+
+        branch.destroySelf(break_references=True)
+
+        # The BuildQueue for the job whose branch we deleted is gone.
+        buildqueue = store.find(BuildQueue, BuildQueue.job == job.job)
+        self.assertEqual([], list(buildqueue))
+
+        # The other job's BuildQueue entry is still there.
+        other_buildqueue = store.find(
+            BuildQueue, BuildQueue.job == other_job.job)
+        self.assertNotEqual([], list(other_buildqueue))
+
     def test_createsJobToReclaimSpace(self):
         # When a branch is deleted from the database, a job to remove the
         # branch from disk as well.
@@ -1053,6 +1089,25 @@
             [branch_id],
             [ReclaimBranchSpaceJob(job).branch_id for job in jobs])
 
+    def test_destroySelf_with_SourcePackageRecipe(self):
+        """If branch is a base_branch in a recipe, it is deleted."""
+        recipe = self.factory.makeSourcePackageRecipe()
+        store = Store.of(recipe)
+        recipe.base_branch.destroySelf(break_references=True)
+        # show no DB constraints have been violated
+        store.flush()
+
+    def test_destroySelf_with_SourcePackageRecipe_as_non_base(self):
+        """If branch is referred to by a recipe, it is deleted."""
+        branch1 = self.factory.makeAnyBranch()
+        branch2 = self.factory.makeAnyBranch()
+        recipe = self.factory.makeSourcePackageRecipe(
+            branches=[branch1, branch2])
+        store = Store.of(recipe)
+        branch2.destroySelf(break_references=True)
+        # show no DB constraints have been violated
+        store.flush()
+
 
 class TestBranchDeletionConsequences(TestCase):
     """Test determination and application of branch deletion consequences."""
@@ -1348,25 +1403,6 @@
             {recipe: ('delete', 'This recipe uses this branch.')},
             recipe.base_branch.deletionRequirements())
 
-    def test_destroySelf_with_SourcePackageRecipe(self):
-        """If branch is a base_branch in a recipe, it is deleted."""
-        recipe = self.factory.makeSourcePackageRecipe()
-        store = Store.of(recipe)
-        recipe.base_branch.destroySelf(break_references=True)
-        # show no DB constraints have been violated
-        store.flush()
-
-    def test_destroySelf_with_SourcePackageRecipe_as_non_base(self):
-        """If branch is referred to by a recipe, it is deleted."""
-        branch1 = self.factory.makeAnyBranch()
-        branch2 = self.factory.makeAnyBranch()
-        recipe = self.factory.makeSourcePackageRecipe(
-            branches=[branch1, branch2])
-        store = Store.of(recipe)
-        branch2.destroySelf(break_references=True)
-        # show no DB constraints have been violated
-        store.flush()
-
 
 class StackedBranches(TestCaseWithFactory):
     """Tests for showing branches stacked on another."""

=== modified file 'lib/lp/translations/interfaces/translationtemplatesbuildjob.py'
--- lib/lp/translations/interfaces/translationtemplatesbuildjob.py	2010-02-17 12:58:53 +0000
+++ lib/lp/translations/interfaces/translationtemplatesbuildjob.py	2010-07-17 23:18:49 +0000
@@ -34,3 +34,6 @@
 
     def scheduleTranslationTemplatesBuild(branch):
         """Schedule a translation templates build job, if appropriate."""
+
+    def getByBranch(branch):
+        """Find `TranslationTemplatesBuildJob` for given `Branch`."""

=== modified file 'lib/lp/translations/model/translationtemplatesbuildjob.py'
--- lib/lp/translations/model/translationtemplatesbuildjob.py	2010-05-26 08:53:13 +0000
+++ lib/lp/translations/model/translationtemplatesbuildjob.py	2010-07-17 23:18:49 +0000
@@ -18,8 +18,7 @@
 from canonical.config import config
 
 from canonical.launchpad.interfaces import ILaunchpadCelebrities
-from canonical.launchpad.webapp.interfaces import (
-    DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE, MASTER_FLAVOR)
+from canonical.launchpad.interfaces.lpstorm import IMasterStore, IStore
 
 from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
@@ -118,7 +117,7 @@
     @classmethod
     def create(cls, branch):
         """See `ITranslationTemplatesBuildJobSource`."""
-        store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
+        store = IMasterStore(BranchJob)
 
         # Pass public HTTP URL for the branch.
         metadata = {'branch_url': branch.composePublicURL()}
@@ -128,8 +127,9 @@
         specific_job = TranslationTemplatesBuildJob(branch_job)
         duration_estimate = cls.duration_estimate
 
-        # XXX Danilo Segan: we hard-code processor to the Ubuntu
-        # default processor architecture.  See bug 580429.
+        # XXX Danilo Segan bug=580429: we hard-code processor to the Ubuntu
+        # default processor architecture.  This stops the buildfarm from
+        # accidentally dispatching the jobs to private builders.
         build_queue_entry = BuildQueue(
             estimated_duration=duration_estimate,
             job_type=BuildFarmJobType.TRANSLATIONTEMPLATESBUILD,
@@ -163,9 +163,19 @@
 
         Overridden here to search via a BranchJob, rather than a Job.
         """
-        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+        store = IStore(BranchJob)
         branch_job = store.find(BranchJob, BranchJob.job == job).one()
         if branch_job is None:
             return None
         else:
             return cls(branch_job)
+
+    @classmethod
+    def getByBranch(cls, branch):
+        """See `ITranslationTemplatesBuildJobSource`."""
+        store = IStore(BranchJob)
+        branch_job = store.find(BranchJob, BranchJob.branch == branch).one()
+        if branch_job is None:
+            return None
+        else:
+            return cls(branch_job)