launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/recipe-build-removed-recipe into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #698032 recipe build for removed recipe triggers uploader exception
  https://bugs.launchpad.net/bugs/698032

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/recipe-build-removed-recipe/+merge/48103

See bug 698032. When a recipe build has been removed since a recipe build was started the archive uploader will fall over.

== Implementation ==

Add a check to the upload processor to see if a recipe has been deleted. Not quite as simple as that, since there's a potential race condition whereby any check placed at the beginning of the process could pass and the recipe could then be deleted during the processing (the effects will depend on the transaction boundaries etc). So, the implementation is to check at the beginning and again at the end, deferring any error processing till the last check. The error processing is to log a warning. Any post build clean up is done as per a build failure to ensure any files are not left lying around.

A check was also added to the mail notification method of SourcePackageRecipeBuild to stop any recipe deletions causing that part to error.

== Tests ==

Addded a new test to lp.archiveloader.tests.test_uploadprocessor:
- testSourcePackageRecipeBuild_deleted_recipe()

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archiveuploader/uploadprocessor.py
  lib/lp/archiveuploader/tests/test_uploadprocessor.py
  lib/lp/code/model/sourcepackagerecipebuild.py

./lib/lp/archiveuploader/tests/test_uploadprocessor.py
    1263: E301 expected 1 blank line, found 2
-- 
https://code.launchpad.net/~wallyworld/launchpad/recipe-build-removed-recipe/+merge/48103
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/recipe-build-removed-recipe into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-01-27 17:09:28 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-02-01 03:17:58 +0000
@@ -2040,6 +2040,40 @@
         body = mail.get_payload(decode=True)
         self.assertIn('Upload Log: http', body)
 
+    def doDeletedRecipeBuild(self):
+        # A source package recipe build will fail if the recipe is deleted.
+
+        # Upload a source package
+        archive = self.factory.makeArchive()
+        archive.require_virtualized = False
+        build = self.factory.makeSourcePackageRecipeBuild(sourcename=u"bar",
+            distroseries=self.breezy, archive=archive)
+        bq = self.factory.makeSourcePackageRecipeBuildJob(recipe_build=build)
+        # Commit so the build cookie has the right ids.
+        Store.of(build).flush()
+        leaf_name = build.getUploadDirLeaf(build.getBuildCookie())
+        os.mkdir(os.path.join(self.incoming_folder, leaf_name))
+        self.options.context = 'buildd'
+        self.options.builds = True
+        build.jobStarted()
+        build.recipe.destroySelf()
+        # Commit so date_started is recorded and doesn't cause constraint
+        # violations later.
+        Store.of(build).flush()
+        build.status = BuildStatus.UPLOADING
+        build.date_finished = UTC_NOW
+        BuildUploadHandler(self.uploadprocessor, self.incoming_folder,
+            leaf_name).process()
+        self.layer.txn.commit()
+        return build
+
+    def testSourcePackageRecipeBuild_deleted_recipe(self):
+        build = self.doDeletedRecipeBuild()
+        self.assertEquals(BuildStatus.FAILEDTOUPLOAD, build.status)
+        self.assertEquals(None, build.builder)
+        self.assertIsNot(None, build.duration)
+        self.assertIs(None, build.upload_log)
+
     def testBuildWithInvalidStatus(self):
         # Builds with an invalid (non-UPLOADING) status should trigger
         # a warning but be left alone.

=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2011-01-26 21:49:54 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2011-02-01 03:17:58 +0000
@@ -643,11 +643,16 @@
                 "Expected build status to be 'UPLOADING', was %s. Ignoring." %
                 self.build.status.name)
             return
-        self.processor.log.debug("Build %s found" % self.build.id)
         try:
-            [changes_file] = self.locateChangesFiles()
-            logger.debug("Considering changefile %s" % changes_file)
-            result = self.processChangesFile(changes_file, logger)
+            # The recipe may have been deleted so we need to flag that here
+            # and will handle below.
+            if self.build.recipe is None:
+                result = UploadStatusEnum.FAILED
+            else:
+                self.processor.log.debug("Build %s found" % self.build.id)
+                [changes_file] = self.locateChangesFiles()
+                logger.debug("Considering changefile %s" % changes_file)
+                result = self.processChangesFile(changes_file, logger)
         except (KeyboardInterrupt, SystemExit):
             raise
         except:
@@ -664,9 +669,14 @@
             not self.build.verifySuccessfulUpload()):
             self.build.status = BuildStatus.FAILEDTOUPLOAD
         if self.build.status != BuildStatus.FULLYBUILT:
-            self.build.storeUploadLog(logger.getLogBuffer())
-            self.build.notify(extra_info="Uploading build %s failed." %
-                              self.upload)
+            if self.build.recipe is None:
+                self.processor.log.warn(
+                    "Recipe for build %s was deleted. Ignoring." %
+                    self.upload)
+            else:
+                self.build.storeUploadLog(logger.getLogBuffer())
+                self.build.notify(extra_info="Uploading build %s failed." %
+                                  self.upload)
         else:
             self.build.notify()
         self.processor.ztm.commit()

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2011-01-20 22:01:29 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2011-02-01 03:17:58 +0000
@@ -289,6 +289,9 @@
 
     def notify(self, extra_info=None):
         """See `IPackageBuild`."""
+        # If our recipe has been deleted, any notification will fail.
+        if self.recipe is None:
+            return
         mailer = SourcePackageRecipeBuildMailer.forStatus(self)
         mailer.sendAll()
 
@@ -331,6 +334,7 @@
         """See `IPackageBuild`."""
         d = super(SourcePackageRecipeBuild, self)._handleStatus_OK(
             librarian, slave_status, logger)
+
         def uploaded_build(ignored):
             # Base implementation doesn't notify on success.
             if self.status == BuildStatus.FULLYBUILT: