launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-properly-delete-builds into lp:launchpad.

Commit message:
Delete BuildQueue and BuildFarmJob rows when deleting SnapBuilds.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-properly-delete-builds/+merge/294897

Delete BuildQueue and BuildFarmJob rows when deleting SnapBuilds.

This supersedes https://code.launchpad.net/~cjwatson/launchpad/snap-no-delete-with-pending-builds/+merge/294712.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-properly-delete-builds into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2016-05-16 14:33:34 +0000
+++ lib/lp/snappy/model/snap.py	2016-05-17 11:00:11 +0000
@@ -33,6 +33,8 @@
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.buildmaster.interfaces.processor import IProcessorSet
+from lp.buildmaster.model.buildfarmjob import BuildFarmJob
+from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.buildmaster.model.processor import Processor
 from lp.code.interfaces.branch import IBranch
 from lp.code.interfaces.branchcollection import (
@@ -424,6 +426,19 @@
         """See `ISnap`."""
         store = IStore(Snap)
         store.find(SnapArch, SnapArch.snap == self).remove()
+        # Remove build jobs.  There won't be many queued builds, so we can
+        # afford to do this the safe but slow way via BuildQueue.destroySelf
+        # rather than in bulk.
+        buildqueue_records = store.find(
+            BuildQueue,
+            BuildQueue._build_farm_job_id == SnapBuild.build_farm_job_id,
+            SnapBuild.snap == self)
+        for buildqueue_record in buildqueue_records:
+            buildqueue_record.destroySelf()
+        build_farm_job_ids = list(store.find(
+            BuildFarmJob.id,
+            BuildFarmJob.id == SnapBuild.build_farm_job_id,
+            SnapBuild.snap == self))
         # XXX cjwatson 2016-02-27 bug=322972: Requires manual SQL due to
         # lack of support for DELETE FROM ... USING ... in Storm.
         store.execute("""
@@ -436,6 +451,8 @@
         store.find(SnapBuild, SnapBuild.snap == self).remove()
         getUtility(IWebhookSet).delete(self.webhooks)
         store.remove(self)
+        store.find(
+            BuildFarmJob, BuildFarmJob.id.is_in(build_farm_job_ids)).remove()
 
 
 class SnapArch(Storm):

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2016-05-16 15:08:30 +0000
+++ lib/lp/snappy/tests/test_snap.py	2016-05-17 11:00:11 +0000
@@ -24,6 +24,7 @@
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.interfaces.processor import IProcessorSet
+from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.distribution import IDistributionSet
@@ -32,9 +33,9 @@
     ONE_DAY_AGO,
     UTC_NOW,
     )
-from lp.services.database.interfaces import IMasterStore
 from lp.services.database.sqlbase import flush_database_caches
 from lp.services.features.testing import FeatureFixture
+from lp.services.propertycache import clear_property_cache
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.snappy.interfaces.snap import (
     BadSnapSearchContext,
@@ -453,16 +454,31 @@
             registrant=owner, owner=owner, distroseries=distroseries,
             name=u"condemned")
         build = self.factory.makeSnapBuild(snap=snap)
+        build_queue = build.queueBuild()
         snapfile = self.factory.makeSnapFile(snapbuild=build)
         self.assertTrue(getUtility(ISnapSet).exists(owner, u"condemned"))
+        other_build = self.factory.makeSnapBuild()
+        other_build.queueBuild()
+        store = Store.of(build)
+        store.flush()
         build_id = build.id
+        build_queue_id = build_queue.id
+        build_farm_job_id = removeSecurityProxy(build).build_farm_job_id
         snapfile_id = removeSecurityProxy(snapfile).id
         with person_logged_in(snap.owner):
             snap.destroySelf()
         flush_database_caches()
+        # The deleted snap and its builds are gone.
         self.assertFalse(getUtility(ISnapSet).exists(owner, u"condemned"))
         self.assertIsNone(getUtility(ISnapBuildSet).getByID(build_id))
-        self.assertIsNone(IMasterStore(SnapFile).get(SnapFile, snapfile_id))
+        self.assertIsNone(store.get(BuildQueue, build_queue_id))
+        self.assertIsNone(store.get(BuildFarmJob, build_farm_job_id))
+        self.assertIsNone(store.get(SnapFile, snapfile_id))
+        # Unrelated builds are still present.
+        clear_property_cache(other_build)
+        self.assertEqual(
+            other_build, getUtility(ISnapBuildSet).getByID(other_build.id))
+        self.assertIsNotNone(other_build.buildqueue_record)
 
     def test_related_webhooks_deleted(self):
         owner = self.factory.makePerson()