Merge lp:~jelmer/launchpad/653720-failedtoupload into lp:launchpad

[Jelmer Vernooij]

Commit after changing build status in the upload processor.

commit() after the build status is being set to FAILEDTOUPLOAD in the upload processor.

At the moment there is no commit so the build status doesn't make it to the database since the uploadprocessor aborts any pending transactions. This means that any failed uploads will be stuck in UPLOADING status even though they have already failed.

[Steve Kowalik (stevenk)]

Thanks for the simple change! My only issue with the branch is the two changes to the result = self.processChangesFile() call, which is longer than 73 characters.

review: Needs Fixing (code*)

[Steve Kowalik (stevenk)]

This looks great.

review: Approve (code*)

[Michael Nelson (michael.nelson)]

Hi Jelmer and Steve,

I can't yet see why we have a txn abort in the finally clause of processUploadQueue? (ie. we abort remaining db-writes even if there were no exceptions (which is why this bug is happening in the first place).

AFAICS, the processChangesFile() method is already aborting the transaction when it finds errors (to ensure the NascentUpload isn't stored in the db?). So it shouldn't be necessary to do the extra abort. If it is necessary (because there is something else we're not seeing being written to the DB), then adding the commit() may be committing more than we think?

Long-term, I think we should sort the above out (ie. not have the extra abort in the finally clause, so that the status would be saved to the db. But it may be that Julian is keen for this fix to go out ASAP, in which case I'll approve it as is.

Julian?

{{{
12:31 < noodles> jelmer: sorry, it's not clear to me why we abort remaining transactions in a finally clause of processUploadQueue?
12:31 < bigjools> jml: when you've got free time *haha* we should do a virtual sprint and decide what needs doing next and possibly do it
12:32 < noodles> jelmer: that is, even if all the uploads are processed fine, we still abort remaining transactions?
12:32 < noodles> s/transactions/db-writes
12:33 < jelmer> noodles: in that particular case there shouldn't be any remaining transactions
12:34 < jelmer> noodles: I'm not entirely sure why aborting the remaining transactions is a good idea, other than being cautious
12:34 < noodles> jelmer: there shouldn't, but isn't this branch trying to fix the fact that there is by committing them earlier before the abort is called?
12:36 < jelmer> noodles: Hmm, that's a good point. Perhaps I could do the abort first then se tthe status / upload the log and commit again?
12:38 < noodles> jelmer: or not do the abort at all? I've only looked briefly, but the only thing I can see that we'd want to abort is the setting of build.date_finished (when the upload fails), but that could be easily avoided by putting it in an else-clause of the following conditional.
12:38 < noodles> But maybe there are other reasons for the abort that I've not seen.
12:39 < jelmer> noodles: I guess to prevent broken uploads from users from filling up the database with garbage on broken uploads
12:40 < jelmer> that's the only possible reason I can come up with
12:44 < noodles> jelmer: where is the db-upload created?
12:44 * noodles scans again
12:44 < noodles> Ah, processChangesFile()...
12:49 < noodles> But the transaction is already aborted within processChangesFile during various failures, so it still doesn't explain why we need the abort in the finally clause of processUploadQueue (which is the cause of this bug).
12:52 < jelmer> hmm, true
}}}

review: Needs Information (code)

[Julian Edwards (julian-edwards)]

The final abort is the belt-and-braces approach. It's always been there as long as I've worked on this stuff and I'd be very nervous about removing it since I don't know what depends on it. Since this will be a CP I'm disinclined to change too much at once, but long-term we should look to remove it.

[Michael Nelson (michael.nelson)]

review: Approve (code)

[Julian Edwards (julian-edwards)]

review: Approve (release-critical)

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-10-04 19:50:45 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-10-07 10:07:17 +0000
@@ -1919,7 +1919,6 @@
         self.uploadprocessor.processBuildUpload(
             self.incoming_folder, leaf_name)
         self.assertEquals(1, len(self.oopses))
-        self.layer.txn.commit()
         self.assertEquals(
             BuildStatus.FAILEDTOUPLOAD, build.status)
         self.assertEquals(builder, build.builder)
 
=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2010-09-16 13:37:25 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2010-10-07 10:07:17 +0000
@@ -258,6 +258,7 @@
             build.status = BuildStatus.FAILEDTOUPLOAD
             build.notify(extra_info="Uploading build %s failed." % upload)
             build.storeUploadLog(logger.buffer.getvalue())
+            self.ztm.commit()
 
     def processUpload(self, fsroot, upload):
         """Process an upload's changes files, and move it to a new directory.