Merge lp:~jelmer/launchpad/681974-building-commit into lp:launchpad

[Jelmer Vernooij]

[r=jtv][ui=none][bug=681974] Commit rather than flush to prevent races between buildd manager and archive uploader.

There is a race condition between the buildd manager and the upload processor, which we seem to've hit a couple of times. The buildd manager in some cases moves the file before it commits the change to the job status, causing the upload processor to blow up.

We did originally consider this but did a database flush rather than a commit. This fixes that.

I haven't added any new tests since I couldn't think of a good way to test this. I'm interested in any ideas on how to test this.

[Jeroen T. Vermeulen (jtv)]

(18:00:31) jtv: good cover letter. I'm surprised it's the buildd manager confusing the upload processor by moving the file before committing, and not another way around.
(18:03:44) jelmer: This is happening with binary builds that the buildd manager has fetched from the builders.
(18:04:25) jelmer: There is another archive uploader instance that processes the source uploads from users, which the buildd manager then sends off to the builders.
(18:08:17) jtv: ah so the data flows in the opposite direction from what I thought.
(18:09:45) jtv: Depending on what the upload processor's transactions look like and even which isolation level it uses, there may _conceivably_ still be race conditions but even then this should narrow it own. And I'm not assuming that you got it wrong anyway. :)
(18:10:06) jtv: I'm also assuming that you're not committing in some untenable state, and therefore r=jtv.

review: Approve (code)

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-11-29 14:51:07 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-12-06 04:48:14 +0000
@@ -19,7 +19,6 @@
 import socket
 import tempfile
 import transaction
-import urllib2
 import xmlrpclib
 
 from sqlobject import (

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2010-11-22 20:53:55 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2010-12-06 04:48:14 +0000
@@ -360,18 +360,18 @@
             if not os.path.exists(target_dir):
                 os.mkdir(target_dir)
 
-            # Flush so there are no race conditions with archiveuploader about
+            # Release the builder for another job.
+            d = self.buildqueue_record.builder.cleanSlave()
+
+            # Commit so there are no race conditions with archiveuploader about
             # self.status.
-            Store.of(self).flush()
+            Store.of(self).commit()
 
             # Move the directory used to grab the binaries into
             # the incoming directory so the upload processor never
             # sees half-finished uploads.
             os.rename(grab_dir, os.path.join(target_dir, upload_leaf))
 
-            # Release the builder for another job.
-            d = self.buildqueue_record.builder.cleanSlave()
-
             # Remove BuildQueue record.
             return d.addCallback(
                 lambda x:self.buildqueue_record.destroySelf())