launchpad-reviewers team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/uploadprocessor-invalid-status-test into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #690074 test to ensure archiveuploader ignores unknown build status uploads
  https://bugs.launchpad.net/bugs/690074


We've had some issues with the archive uploader moving builds out of its queue too soon. Those issues should be resolved now. This branch makes the archive uploader a bit more cautious and makes it warn (so an OOPS will be logged) about uploads with an unknown build status rather than simply moving their directory out of the way, from which it is much harder to recover.
-- 
https://code.launchpad.net/~jelmer/launchpad/uploadprocessor-invalid-status-test/+merge/43622
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jelmer/launchpad/uploadprocessor-invalid-status-test into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-11-26 16:27:12 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-12-14 09:30:58 +0000
@@ -2033,6 +2033,44 @@
         self.assertIsNot(None, build.duration)
         self.assertIsNot(None, build.upload_log)
 
+    def testBuildWithInvalidStatus(self):
+        # Builds with an invalid (non-UPLOADING) status should trigger
+        # a warning but be left alone.
+        upload_dir = self.queueUpload("bar_1.0-1")
+        self.processUpload(self.uploadprocessor, upload_dir)
+        source_pub = self.publishPackage('bar', '1.0-1')
+        [build] = source_pub.createMissingBuilds()
+
+        # Move the source from the accepted queue.
+        [queue_item] = self.breezy.getQueueItems(
+            status=PackageUploadStatus.ACCEPTED,
+            version="1.0-1", name="bar")
+        queue_item.setDone()
+
+        build.buildqueue_record.markAsBuilding(self.factory.makeBuilder())
+        build.status = BuildStatus.BUILDING
+
+        shutil.rmtree(upload_dir)
+
+        # Commit so the build cookie has the right ids.
+        self.layer.txn.commit()
+        leaf_name = build.getUploadDirLeaf(build.getBuildCookie())
+        upload_dir = self.queueUpload("bar_1.0-1_binary",
+                queue_entry=leaf_name)
+        self.options.context = 'buildd'
+        self.options.builds = True
+        last_stub_mail_count = len(stub.test_emails)
+        self.uploadprocessor.processBuildUpload(
+            self.incoming_folder, leaf_name)
+        self.layer.txn.commit()
+        # The build status is not changed
+        self.assertTrue(os.path.exists(
+            os.path.join(self.incoming_folder, leaf_name)))
+        self.assertEquals(BuildStatus.BUILDING, build.status)
+        self.assertLogContains(
+            "Expected build status to be 'UPLOADING', was BUILDING. "
+            "Ignoring.")
+
 
 class ParseBuildUploadLeafNameTests(TestCase):
     """Tests for parse_build_upload_leaf_name."""

=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2010-11-24 15:29:49 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2010-12-14 09:30:58 +0000
@@ -224,9 +224,8 @@
         build = buildfarm_job.getSpecificJob()
         if build.status != BuildStatus.UPLOADING:
             self.log.warn(
-                "Expected build status to be 'UPLOADING', was %s. "
-                "Moving to failed.", build.status.name)
-            self.moveProcessedUpload(upload_path, "failed", logger)
+                "Expected build status to be 'UPLOADING', was %s. Ignoring." %
+                build.status.name)
             return
         self.log.debug("Build %s found" % build.id)
         try:

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2010-12-06 04:42:54 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2010-12-14 09:30:58 +0000
@@ -42,7 +42,6 @@
     IStoreSelector,
     MAIN_STORE,
     )
-from canonical.librarian.utils import copy_and_close
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource
 from lp.buildmaster.interfaces.packagebuild import (
@@ -361,6 +360,9 @@
                 os.mkdir(target_dir)
 
             # Release the builder for another job.
+            # This is done before the commit() since the commit will reload
+            # builder.slave from the database, and override any current
+            # slave set manually by the testsuite.
             d = self.buildqueue_record.builder.cleanSlave()
 
             # Commit so there are no race conditions with archiveuploader about