launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/couple-of-oopses into lp:launchpad.

Commit message:
Don't double-upload build results when the log download fails, don't squash IntegrityErrors in LibraryFileAliasSet.create, and verify that a Swift connection is what it says it is before returning it to the librarian ConnectionPool.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1422199 in Launchpad itself: "buildd-manager can repeatedly upload builds if handleStatus fails in the wrong place"
  https://bugs.launchpad.net/launchpad/+bug/1422199
  Bug #1422207 in Launchpad itself: "LibraryFileAliasSet.create thinks all IntegrityErrors mean that the filename contains slashes"
  https://bugs.launchpad.net/launchpad/+bug/1422207

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/couple-of-oopses/+merge/249788

An OOPS fix, an OOPS change and an added OOPS:

 - Bug #1422199: Minimise the risk of duplicate buildd-manager uploads
   by moving the non-transactional filesystem operations as close to
   the commit as possible. Previously the log download could time out
   and abort the transaction after the upload had already been moved
   into incoming/.

 - Bug #1422207: Stop squashing IntegrityErrors in
   LibraryFileAliasSet.create. Pre-check the filename rather than
   waiting for the database to reject it and hoping that the error
   we're catching is the right one. This will let us diagnose some odd
   process-upload failures.

 - Bug #1420046: Librarian's Swift ConnectionPool now verifies that
   anything it's given is actually a swiftclient.Connection. Hopefully
   this will give us useful tracebacks at the source of the None
   connections.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/couple-of-oopses into lp:launchpad.

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehaviour.py'
--- lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2014-06-26 12:33:51 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2015-02-16 01:16:18 +0000
@@ -221,19 +221,24 @@
             % (self.build.build_cookie, self.build.title,
                self.build.buildqueue_record.builder.name, status))
         if status == 'OK':
+            yield self.storeLogFromSlave()
+            # handleSuccess will sometimes perform write operations
+            # outside the database transaction, so a failure between
+            # here and the commit can cause duplicated results. For
+            # example, a BinaryPackageBuild will end up in the upload
+            # queue twice if notify() crashes.
             yield self.handleSuccess(slave_status, logger)
         elif status in fail_status_map:
             # XXX wgrant: The builder should be set long before here, but
             # currently isn't.
+            yield self.storeLogFromSlave()
             self.build.updateStatus(
                 fail_status_map[status],
                 builder=self.build.buildqueue_record.builder,
                 slave_status=slave_status)
-            transaction.commit()
         else:
             raise BuildDaemonError(
                 "Build returned unexpected status: %r" % status)
-        yield self.storeLogFromSlave()
         if notify:
             self.build.notify()
         self.build.buildqueue_record.destroySelf()

=== modified file 'lib/lp/services/librarian/model.py'
--- lib/lp/services/librarian/model.py	2014-09-01 11:44:56 +0000
+++ lib/lp/services/librarian/model.py	2015-02-16 01:16:18 +0000
@@ -255,11 +255,9 @@
             client = getUtility(IRestrictedLibrarianClient)
         else:
             client = getUtility(ILibrarianClient)
-        try:
-            fid = client.addFile(
-                name, size, file, contentType, expires, debugID)
-        except IntegrityError:
+        if '/' in name:
             raise InvalidFilename("Filename cannot contain slashes.")
+        fid = client.addFile(name, size, file, contentType, expires, debugID)
         lfa = IMasterStore(LibraryFileAlias).find(
             LibraryFileAlias, LibraryFileAlias.id == fid).one()
         assert lfa is not None, "client.addFile didn't!"

=== modified file 'lib/lp/services/librarianserver/swift.py'
--- lib/lp/services/librarianserver/swift.py	2014-12-16 09:20:15 +0000
+++ lib/lp/services/librarianserver/swift.py	2015-02-16 01:16:18 +0000
@@ -346,6 +346,9 @@
         exception has been raised (apart from a 404), don't trust the
         swift_connection and throw it away.
         '''
+        if not isinstance(swift_connection, swiftclient.Connection):
+            raise AssertionError(
+                "%r is not a swiftclient Connection." % swift_connection)
         if swift_connection not in self._pool:
             self._pool.append(swift_connection)
             while len(self._pool) > self.MAX_POOL_SIZE: