Merge lp:~jelmer/launchpad/later-upload-move into lp:launchpad

[Jelmer Vernooij]

[r=jelmer][ui=none][bug=681508] Commit build status changes to database before moving the upload directory.

We've had two bugs with builds that failed to upload. They would be moved out of the queue but before their build status was updated (to FULLYBUILT or FAILEDTOUPLOAD).

This changes the move to be right after the database commit. This means the window for inconsistency is very small.

[Jelmer Vernooij (jelmer)]

review: Approve

[Jelmer Vernooij (jelmer)]

(I had a pre-implementation discussion about this with Julian, but he hasn't done an actual code review).

[Julian Edwards (julian-edwards)]

Post-landing review:

I think this needs a better explanation (via a comment) of what happens if process-upload fails between committing and moving the upload directory away.

From the pre-imp you said that it would notice the next time that it ran that the build was already processed and deal with it appropriately. Is that action tested?

Also, I don't think you need to assert that there's no debug output in the test.

review: Needs Information

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-10-21 04:19:36 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-11-26 16:30:45 +0000
@@ -1924,7 +1924,7 @@
         log_contents = build.upload_log.read()
         self.assertTrue('ERROR: Exception while processing upload '
             in log_contents)
-        self.assertTrue('DEBUG: Moving upload directory '
+        self.assertFalse('DEBUG: Moving upload directory '
             in log_contents)
 
     def testBinaryPackageBuilds(self):
 
=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2010-11-22 16:35:22 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2010-11-26 16:30:45 +0000
@@ -250,7 +250,6 @@
             UploadStatusEnum.FAILED: "failed",
             UploadStatusEnum.REJECTED: "rejected",
             UploadStatusEnum.ACCEPTED: "accepted"}[result]
-        self.moveProcessedUpload(upload_path, destination, logger)
         build.date_finished = datetime.datetime.now(pytz.UTC)
         if not (result == UploadStatusEnum.ACCEPTED and
                 build.verifySuccessfulUpload() and
@@ -258,7 +257,8 @@
             build.status = BuildStatus.FAILEDTOUPLOAD
             build.notify(extra_info="Uploading build %s failed." % upload)
             build.storeUploadLog(logger.buffer.getvalue())
-            self.ztm.commit()
+        self.ztm.commit()
+        self.moveProcessedUpload(upload_path, destination, logger)
 
     def processUpload(self, fsroot, upload):
         """Process an upload's changes files, and move it to a new directory.