launchpad-reviewers team mailing list archive

[Colin Watson <mp+427772@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-cibuild-deletion into launchpad:master.

Commit message:
Fix CI build deletion when deleting a Git repository

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/427772

We need to delete associated `BuildQueue` and `BuildFarmJob` rows as well, otherwise various things break if there happens to be a running CI build for a repository that's being deleted.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-cibuild-deletion into launchpad:master.

diff --git a/lib/lp/code/model/cibuild.py b/lib/lp/code/model/cibuild.py
index e65493d..7adcc0e 100644
--- a/lib/lp/code/model/cibuild.py
+++ b/lib/lp/code/model/cibuild.py
@@ -29,7 +29,11 @@ from lp.buildmaster.enums import (
 )
 from lp.buildmaster.interfaces.builder import CannotBuild
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource
-from lp.buildmaster.model.buildfarmjob import SpecificBuildFarmJobSourceMixin
+from lp.buildmaster.model.buildfarmjob import (
+    BuildFarmJob,
+    SpecificBuildFarmJobSourceMixin,
+)
+from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.buildmaster.model.packagebuild import PackageBuildMixin
 from lp.code.errors import GitRepositoryBlobNotFound, GitRepositoryScanFault
 from lp.code.interfaces.cibuild import (
@@ -594,11 +598,18 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
         store.flush()
         return cibuild
 
-    def findByGitRepository(self, git_repository, commit_sha1s=None):
-        """See `ICIBuildSet`."""
+    def _findByGitRepositoryClauses(self, git_repository, commit_sha1s=None):
+        """Return a list of Storm clauses to find builds for a repository."""
         clauses = [CIBuild.git_repository == git_repository]
         if commit_sha1s is not None:
             clauses.append(CIBuild.commit_sha1.is_in(commit_sha1s))
+        return clauses
+
+    def findByGitRepository(self, git_repository, commit_sha1s=None):
+        """See `ICIBuildSet`."""
+        clauses = self._findByGitRepositoryClauses(
+            git_repository, commit_sha1s=commit_sha1s
+        )
         return IStore(CIBuild).find(CIBuild, *clauses)
 
     def _isBuildableArchitectureAllowed(self, das):
@@ -759,7 +770,25 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
 
     def deleteByGitRepository(self, git_repository):
         """See `ICIBuildSet`."""
+        # 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.
+        build_clauses = self._findByGitRepositoryClauses(git_repository)
+        store = IStore(CIBuild)
+        buildqueue_records = store.find(
+            BuildQueue,
+            BuildQueue._build_farm_job_id == CIBuild.build_farm_job_id,
+            *build_clauses,
+        )
+        for buildqueue_record in buildqueue_records:
+            buildqueue_record.destroySelf()
+        build_farm_job_ids = list(
+            store.find(CIBuild.build_farm_job_id, *build_clauses)
+        )
         self.findByGitRepository(git_repository).remove()
+        store.find(
+            BuildFarmJob, BuildFarmJob.id.is_in(build_farm_job_ids)
+        ).remove()
 
 
 @implementer(IMacaroonIssuer)
diff --git a/lib/lp/code/model/tests/test_cibuild.py b/lib/lp/code/model/tests/test_cibuild.py
index a72e744..8f555a1 100644
--- a/lib/lp/code/model/tests/test_cibuild.py
+++ b/lib/lp/code/model/tests/test_cibuild.py
@@ -32,6 +32,7 @@ from lp.buildmaster.enums import BuildQueueStatus, BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
 from lp.buildmaster.interfaces.processor import IProcessorSet
+from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.code.errors import GitRepositoryBlobNotFound, GitRepositoryScanFault
 from lp.code.interfaces.cibuild import (
@@ -54,6 +55,7 @@ from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import SourcePackageType
 from lp.services.authserver.xmlrpc import AuthServerAPIView
 from lp.services.config import config
+from lp.services.database.sqlbase import flush_database_caches
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.log.logger import BufferLogger
 from lp.services.macaroons.interfaces import IMacaroonIssuer
@@ -1158,16 +1160,31 @@ class TestCIBuildSet(TestCaseWithFactory):
             builds.extend(
                 [self.factory.makeCIBuild(repository) for _ in range(2)]
             )
+        build_queue = builds[0].queueBuild()
+        other_build = self.factory.makeCIBuild()
+        other_build.queueBuild()
+        store = Store.of(builds[0])
+        store.flush()
+        build_queue_id = build_queue.id
+        build_farm_job_id = removeSecurityProxy(builds[0]).build_farm_job_id
         ci_build_set = getUtility(ICIBuildSet)
 
         ci_build_set.deleteByGitRepository(repositories[0])
 
+        flush_database_caches()
+        # The deleted CI builds are gone.
         self.assertContentEqual(
             [], ci_build_set.findByGitRepository(repositories[0])
         )
         self.assertContentEqual(
             builds[2:], ci_build_set.findByGitRepository(repositories[1])
         )
+        self.assertIsNone(store.get(BuildQueue, build_queue_id))
+        self.assertIsNone(store.get(BuildFarmJob, build_farm_job_id))
+        # Unrelated CI builds are still present.
+        clear_property_cache(other_build)
+        self.assertEqual(other_build, ci_build_set.getByID(other_build.id))
+        self.assertIsNotNone(other_build.buildqueue_record)
 
 
 class TestDetermineDASesToBuild(TestCaseWithFactory):