launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/discard-builds-bug-684180 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #684180 Deleted sources can leave binaries hanging around forever
  https://bugs.launchpad.net/bugs/684180


As per bug 684180, if someone deletes a source while a build of it is in progress, any binaries produced will cause both the source and the binary to remain in limbo, never getting deleted, until someone forces another deletion.  This makes the publisher much slower as it has to consider the superseded sources each and every time it runs, to see if they are ready to be condemned for deletion.

This is a simple change to make the buildd-manager inspect the state of the source to see if it's still published before accepting the uploaded binaries.  If it's not, we simply discard the files and mark the build as superseded.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/discard-builds-bug-684180/+merge/42923
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/discard-builds-bug-684180 into lp:launchpad.

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2010-11-22 20:53:55 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2010-12-07 11:17:01 +0000
@@ -61,6 +61,7 @@
 from lp.soyuz.adapters.archivedependencies import (
     default_component_dependency_name,
     )
+from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.interfaces.component import IComponentSet
 
 
@@ -288,6 +289,13 @@
         d = method(librarian, slave_status, logger)
         return d
 
+    def _release_builder_and_remove_queue_item(self):
+            # Release the builder for another job.
+            d = self.buildqueue_record.builder.cleanSlave()
+            # Remove BuildQueue record.
+            return d.addCallback(
+                lambda x:self.buildqueue_record.destroySelf())
+
     def _handleStatus_OK(self, librarian, slave_status, logger):
         """Handle a package that built successfully.
 
@@ -300,6 +308,13 @@
         logger.info("Processing successful build %s from builder %s" % (
             self.buildqueue_record.specific_job.build.title,
             self.buildqueue_record.builder.name))
+
+        # Discard this build if its source is no longer published.
+        build = self.buildqueue_record.specific_job.build
+        if not build.current_source_publication:
+            build.status = BuildStatus.SUPERSEDED
+            return self._release_builder_and_remove_queue_item()
+
         # Explode before collect a binary that is denied in this
         # distroseries/pocket
         if not self.archive.allowUpdatesToReleasePocket():
@@ -369,12 +384,7 @@
             # sees half-finished uploads.
             os.rename(grab_dir, os.path.join(target_dir, upload_leaf))
 
-            # Release the builder for another job.
-            d = self.buildqueue_record.builder.cleanSlave()
-
-            # Remove BuildQueue record.
-            return d.addCallback(
-                lambda x:self.buildqueue_record.destroySelf())
+            return self._release_builder_and_remove_queue_item()
 
         d = slave.getFiles(filenames_to_download)
         # Store build information, build record was already updated during

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py'
--- lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py	2010-11-30 11:42:31 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py	2010-12-07 11:17:01 +0000
@@ -15,6 +15,8 @@
     AsynchronousDeferredRunTest,
     )
 
+from storm.store import Store
+
 from twisted.internet import defer
 
 from zope.component import getUtility
@@ -401,6 +403,19 @@
         d = self.builder.updateBuild(self.candidate)
         return d.addCallback(got_update)
 
+    def test_collection_for_deleted_source(self):
+        self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
+        spr = removeSecurityProxy(self.build.source_package_release)
+        pub = self.build.current_source_publication
+        pub.requestDeletion(spr.creator)
+
+        def got_update(ignored):
+            self.assertEqual(
+                BuildStatus.SUPERSEDED, self.build.status)
+
+        d = self.builder.updateBuild(self.candidate)
+        return d.addCallback(got_update)
+
     def test_uploading_collection(self):
         # After a successful build, the status should be UPLOADING.
         self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
@@ -481,13 +496,13 @@
         # the restricted librarian.
         self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
 
-        # Un-publish the source so we don't trip the 'private' field
-        # validator.
-        from storm.store import Store
-        for source in self.build.archive.getPublishedSources():
-            Store.of(source).remove(source)
-        self.build.archive.private = True
-        self.build.archive.buildd_secret = "foo"
+        # Go behind Storm's back since the field validator on
+        # Archive.private prevents us from setting it to True with
+        # existing published sources.
+        Store.of(self.build).execute("""
+            UPDATE archive SET private=True,buildd_secret='foo'
+            WHERE archive.id = %s""" % self.build.archive.id)
+        Store.of(self.build).invalidate()
 
         def got_update(ignored):
             # Librarian needs a commit.  :(