Merge lp:~julian-edwards/launchpad/timeouts-feature-bug-681426 into lp:launchpad

[Julian Edwards]

Work around a bug in builder Xen guests where it sometimes appears to drop the first network packet. We send a fake ping message before building.

See the bug for gory details.

I've also removed some unnecessary code that was checking for failures that could never happen at that part of the code. The failures are all caught in the same place in the manager code now.

bin/test -cvv test_builder test_virtual_job_dispatch_pings_before_building

[Jonathan Lange (jml)]

Good to land. Thanks for the comment. Oh, you should probably link to the bug report in the comment.

review: Approve

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-11-18 17:54:06 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-11-25 17:40:47 +0000
@@ -512,39 +512,27 @@
         else:
             d = defer.succeed(None)
 
-        def resume_done(ignored):
+        def ping_done(ignored):
             return self.current_build_behavior.dispatchBuildToSlave(
                 build_queue_item.id, logger)
 
-        def eb_slave_failure(failure):
-            failure.trap(BuildSlaveFailure)
-            e = failure.value
-            self.failBuilder(
-                "Exception (%s) when setting up to new job" % (e,))
-
-        def eb_cannot_fetch_file(failure):
-            failure.trap(CannotFetchFile)
-            e = failure.value
-            message = """Slave '%s' (%s) was unable to fetch file.
-            ****** URL ********
-            %s
-            ****** INFO *******
-            %s
-            *******************
-            """ % (self.name, self.url, e.file_url, e.error_information)
-            raise BuildDaemonError(message)
-
-        def eb_socket_error(failure):
-            failure.trap(socket.error)
-            e = failure.value
-            error_message = "Exception (%s) when setting up new job" % (e,)
-            d = self.handleTimeout(logger, error_message)
-            return d.addBoth(lambda ignored: failure)
+        def resume_done(ignored):
+            # Before we try and contact the resumed slave, we're going
+            # to send it a message.  This is to ensure it's accepting
+            # packets from the outside world, because testing has shown
+            # that the first packet will randomly fail for no apparent
+            # reason.  This could be a quirk of the Xen guest, we're not
+            # sure.  We also don't care about the result from this message,
+            # just that it's sent, hence the "addBoth".
+            # See bug 586359.
+            if self.virtualized:
+                d = self.slave.echo("ping")
+            else:
+                d = defer.succeed(None)
+            d.addBoth(ping_done)
+            return d
 
         d.addCallback(resume_done)
-        d.addErrback(eb_slave_failure)
-        d.addErrback(eb_cannot_fetch_file)
-        d.addErrback(eb_socket_error)
         return d
 
     def failBuilder(self, reason):

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2010-11-22 10:11:44 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2010-11-25 17:40:47 +0000
@@ -247,6 +247,19 @@
             self.assertEqual(BuildStatus.BUILDING, build.status)
         return d.addCallback(check_build_started)
 
+    def test_virtual_job_dispatch_pings_before_building(self):
+        # We need to send a ping to the builder to work around a bug
+        # where sometimes the first network packet sent is dropped.
+        builder, build = self._setupBinaryBuildAndBuilder()
+        candidate = build.queueBuild()
+        removeSecurityProxy(builder)._findBuildCandidate = FakeMethod(
+            result=candidate)
+        d = builder.findAndStartJob()
+        def check_build_started(candidate):
+            self.assertIn(
+                ('echo', 'ping'), removeSecurityProxy(builder.slave).call_log)
+        return d.addCallback(check_build_started)
+
     def test_slave(self):
         # Builder.slave is a BuilderSlave that points at the actual Builder.
         # The Builder is only ever used in scripts that run outside of the

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py'
--- lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py	2010-10-31 00:05:10 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py	2010-11-25 17:40:47 +0000
@@ -124,7 +124,11 @@
         build_log = [
             ('build', build_id, 'binarypackage', chroot.content.sha1,
              filemap_names, extra_args)]
-        return upload_logs + build_log
+        if builder.virtualized:
+            result = [('echo', 'ping')] + upload_logs + build_log
+        else:
+            result = upload_logs + build_log
+        return result
 
     def startBuild(self, builder, candidate):
         builder = removeSecurityProxy(builder)