Launchpad itself

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

  1. later-upload-move
  2. Merge into devel
Proposed by Jelmer Vernooij
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: no longer in the source branch.
Merged at revision: 11991
Proposed branch: lp:~jelmer/launchpad/later-upload-move
Merge into: lp:launchpad
Diff against target: 34 lines (+3/-3)
2 files modified
lib/lp/archiveuploader/tests/test_uploadprocessor.py (+1/-1)
lib/lp/archiveuploader/uploadprocessor.py (+2/-2)
To merge this branch: bzr merge lp:~jelmer/launchpad/later-upload-move
Reviewer Review Type Date Requested Status
Julian Edwards (community) Needs Information
Jelmer Vernooij (community) Approve
Review via email: mp+41749@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) :
review: Approve
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
2--- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2010-10-21 04:19:36 +0000
3+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2010-11-26 16:30:45 +0000
4@@ -1924,7 +1924,7 @@
5 log_contents = build.upload_log.read()
6 self.assertTrue('ERROR: Exception while processing upload '
7 in log_contents)
8- self.assertTrue('DEBUG: Moving upload directory '
9+ self.assertFalse('DEBUG: Moving upload directory '
10 in log_contents)
11
12 def testBinaryPackageBuilds(self):
13
14=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
15--- lib/lp/archiveuploader/uploadprocessor.py 2010-11-22 16:35:22 +0000
16+++ lib/lp/archiveuploader/uploadprocessor.py 2010-11-26 16:30:45 +0000
17@@ -250,7 +250,6 @@
18 UploadStatusEnum.FAILED: "failed",
19 UploadStatusEnum.REJECTED: "rejected",
20 UploadStatusEnum.ACCEPTED: "accepted"}[result]
21- self.moveProcessedUpload(upload_path, destination, logger)
22 build.date_finished = datetime.datetime.now(pytz.UTC)
23 if not (result == UploadStatusEnum.ACCEPTED and
24 build.verifySuccessfulUpload() and
25@@ -258,7 +257,8 @@
26 build.status = BuildStatus.FAILEDTOUPLOAD
27 build.notify(extra_info="Uploading build %s failed." % upload)
28 build.storeUploadLog(logger.buffer.getvalue())
29- self.ztm.commit()
30+ self.ztm.commit()
31+ self.moveProcessedUpload(upload_path, destination, logger)
32
33 def processUpload(self, fsroot, upload):
34 """Process an upload's changes files, and move it to a new directory.