launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/extend-ensurepresent-timeout into lp:launchpad.

Commit message:
Give BuilderInteractor.ensurepresent a longer timeout, as it downloads large files from the librarian synchronously.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/extend-ensurepresent-timeout/+merge/200943

We're now seeing the occasional very large orig.tar.gz, even up to a gigabyte or four. These can take a while to download from the librarian. This branch extends the timeout on BuilderInteractor.ensurepresent, to hopefully let those large downloads complete even in unfavourable network conditions.

I also fixed a test to cope with the new lp-buildd in ppa:launchpad, which has a new "livefs" build manager that Launchpad doesn't yet know about.
-- 
https://code.launchpad.net/~wgrant/launchpad/extend-ensurepresent-timeout/+merge/200943
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/extend-ensurepresent-timeout into lp:launchpad.

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2013-12-04 07:28:52 +0000
+++ lib/lp/buildmaster/interactor.py	2014-01-09 01:19:48 +0000
@@ -21,7 +21,6 @@
     removeSecurityProxy,
     )
 
-from lp.buildmaster.enums import BuildQueueStatus
 from lp.buildmaster.interfaces.builder import (
     BuildDaemonError,
     CannotFetchFile,
@@ -90,8 +89,8 @@
             server_proxy = proxy
         return cls(server_proxy, builder_url, vm_host, timeout, reactor)
 
-    def _with_timeout(self, d):
-        return cancel_on_timeout(d, self.timeout, self.reactor)
+    def _with_timeout(self, d, timeout=None):
+        return cancel_on_timeout(d, timeout or self.timeout, self.reactor)
 
     def abort(self):
         """Abort the current build."""
@@ -114,10 +113,14 @@
         return self._with_timeout(self._server.callRemote('status_dict'))
 
     def ensurepresent(self, sha1sum, url, username, password):
+        """Attempt to ensure the given file is present."""
         # XXX: Nothing external calls this. Make it private.
-        """Attempt to ensure the given file is present."""
-        return self._with_timeout(self._server.callRemote(
-            'ensurepresent', sha1sum, url, username, password))
+        # Use a larger timeout than other calls, as this synchronously
+        # downloads large files.
+        return self._with_timeout(
+            self._server.callRemote(
+                'ensurepresent', sha1sum, url, username, password),
+            self.timeout * 5)
 
     def getFile(self, sha_sum, file_to_write):
         """Fetch a file from the builder.

=== modified file 'lib/lp/buildmaster/tests/test_interactor.py'
--- lib/lp/buildmaster/tests/test_interactor.py	2013-11-28 08:51:32 +0000
+++ lib/lp/buildmaster/tests/test_interactor.py	2014-01-09 01:19:48 +0000
@@ -15,6 +15,7 @@
     AsynchronousDeferredRunTestForBrokenTwisted,
     SynchronousDeferredRunTest,
     )
+from testtools.matchers import ContainsAll
 from twisted.internet import defer
 from twisted.internet.task import Clock
 from twisted.python.failure import Failure
@@ -439,21 +440,23 @@
         d = slave.echo('foo', 'bar', 42)
         return d.addCallback(self.assertEqual, ['foo', 'bar', 42])
 
+    @defer.inlineCallbacks
     def test_info(self):
         # Calling 'info' gets some information about the slave.
         self.slave_helper.getServerSlave()
         slave = self.slave_helper.getClientSlave()
-        d = slave.info()
+        info = yield slave.info()
         # We're testing the hard-coded values, since the version is hard-coded
         # into the remote slave, the supported build managers are hard-coded
         # into the tac file for the remote slave and config is returned from
         # the configuration file.
-        return d.addCallback(
-            self.assertEqual,
-            ['1.0',
-             'i386',
-             ['sourcepackagerecipe',
-              'translation-templates', 'binarypackage', 'debian']])
+        self.assertEqual(3, len(info))
+        self.assertEqual(['1.0', 'i386'], info[:2])
+        self.assertThat(
+            info[2],
+            ContainsAll(
+                ('sourcepackagerecipe', 'translation-templates',
+                 'binarypackage', 'debian')))
 
     @defer.inlineCallbacks
     def test_initial_status(self):
@@ -604,8 +607,8 @@
         self.slave = self.slave_helper.getClientSlave(
             reactor=self.clock, proxy=self.proxy)
 
-    def assertCancelled(self, d):
-        self.clock.advance(config.builddmaster.socket_timeout + 1)
+    def assertCancelled(self, d, timeout=None):
+        self.clock.advance((timeout or config.builddmaster.socket_timeout) + 1)
         return assert_fails_with(d, defer.CancelledError)
 
     def test_timeout_abort(self):
@@ -625,7 +628,8 @@
 
     def test_timeout_ensurepresent(self):
         return self.assertCancelled(
-            self.slave.ensurepresent(None, None, None, None))
+            self.slave.ensurepresent(None, None, None, None),
+            config.builddmaster.socket_timeout * 5)
 
     def test_timeout_build(self):
         return self.assertCancelled(