Merge lp:~wallyworld/launchpad/recipe-build-removed-recipe into lp:launchpad

[Ian Booth]

[r=leonardr][ui=none][bug=698032] When a recipe build has been removed since a recipe build was started, ensure the archive uploader does not fall over.

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

[Ian Booth (wallyworld)]

To clarify the implementation:

We want to stop the oops when upload processing is attempted for a build with a deleted recipe. We don't want to email any error message but rather we just want to log a warning. We also need to ensure any normal post upload cleanup is done (as per for the standard upload failure case).

So the approach was to check at the start of the process() method to see if the recipe was deleted and flag it, don't attempt the upload processing, and defer the handling of the problem to occur along with other failure mode processing (so that all error handling is done in the one place).

The comments about the transaction boundaries are probably not relevant - it appears the upload processing is wrapped inside a transaction so there's no likelihood of any race condition. But the implementation would handle this case too.

[Leonard Richardson (leonardr)]

This looks fine.

As you say, the code should work even if it's run in multiple transactions and the recipe is deleted during processing. But, maybe you want to test this--add a test that nothing happens when you call notify() on a build whose recipe has been deleted. It shouldn't be difficult.

review: Approve

[Ian Booth (wallyworld)]

I've added a test as requested in
lp.code.model.tests.test_sourcepackagerecipebuild

On 02/02/11 01:51, Leonard Richardson wrote:
> Review: Approve
> This looks fine.
>
> As you say, the code should work even if it's run in multiple transactions and the recipe is deleted during processing. But, maybe you want to test this--add a test that nothing happens when you call notify() on a build whose recipe has been deleted. It shouldn't be difficult.

=== 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-02 05:29:53 +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-02 05:29:53 +0000
@@ -74,6 +74,9 @@
     BuildStatus,
     )
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSet
+from lp.code.interfaces.sourcepackagerecipebuild import (
+    ISourcePackageRecipeBuild,
+    )
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
 from lp.services.log.logger import BufferLogger
@@ -643,11 +646,20 @@
                 "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. We check so that we don't go to the
+            # expense of doing an unnecessary upload. We don't just exit here
+            # because we want the standard cleanup to occur.
+            recipe_deleted = (ISourcePackageRecipeBuild.providedBy(self.build)
+                and self.build.recipe is None)
+            if recipe_deleted:
+                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 +676,16 @@
             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 recipe_deleted:
+                # For a deleted recipe, no need to notify that uploading has
+                # failed - we just log a warning.
+                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-28 05:56:59 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2011-02-02 05:29:53 +0000
@@ -292,6 +292,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()
 
@@ -334,6 +337,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:
 
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2011-01-31 22:52:20 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2011-02-02 05:29:53 +0000
@@ -457,6 +457,22 @@
         self.assertEqual(
             expected.body, message.get_payload(decode=True))
 
+    def test_notify_when_recipe_deleted(self):
+        """Notify does nothing if recipe has been deleted."""
+        person = self.factory.makePerson(name='person')
+        cake = self.factory.makeSourcePackageRecipe(
+            name=u'recipe', owner=person)
+        pantry = self.factory.makeArchive(name='ppa')
+        secret = self.factory.makeDistroSeries(name=u'distroseries')
+        build = self.factory.makeSourcePackageRecipeBuild(
+            recipe=cake, distroseries=secret, archive=pantry)
+        removeSecurityProxy(build).status = BuildStatus.FULLYBUILT
+        cake.destroySelf()
+        IStore(build).flush()
+        build.notify()
+        notifications = pop_notifications()
+        self.assertEquals(0, len(notifications))
+
     def test_handleStatusNotifies(self):
         """"handleStatus causes notification, even if OK."""