launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/fix-buildmaster-twisted-agent into lp:launchpad.

Commit message:
Fix BuilderSlave.getFiles to accept file-like objects again.  Transferring build logs from slaves requires this.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1584744 in Launchpad itself: "buildd-manager dispatches all downloads at once and can hit EMFILE"
  https://bugs.launchpad.net/launchpad/+bug/1584744

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/fix-buildmaster-twisted-agent/+merge/295833

Fix BuilderSlave.getFiles to accept file-like objects again.  Transferring build logs from slaves requires this.

I also improved the logging a bit, so that we can see builder-side URLs again.  The new downloader makes this a bit harder, since a factory is no longer associated with a single URL, but the results of this are reasonably tolerable on dogfood.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/fix-buildmaster-twisted-agent into lp:launchpad.

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2016-05-25 15:31:48 +0000
+++ lib/lp/buildmaster/interactor.py	2016-05-26 14:04:28 +0000
@@ -57,10 +57,14 @@
 class FileWritingProtocol(Protocol):
     """A protocol that saves data to a file."""
 
-    def __init__(self, finished, filename):
+    def __init__(self, finished, file_to_write):
         self.finished = finished
-        self.filename = filename
-        self.file = None
+        if isinstance(file_to_write, (bytes, unicode)):
+            self.filename = file_to_write
+            self.file = None
+        else:
+            self.filename = None
+            self.file = file_to_write
 
     def dataReceived(self, data):
         if self.file is None:
@@ -77,7 +81,8 @@
 
     def connectionLost(self, reason):
         try:
-            self.file.close()
+            if self.file is not None:
+                self.file.close()
         except IOError:
             self.finished.errback()
         else:
@@ -224,6 +229,10 @@
                 'ensurepresent', sha1sum, url, username, password),
             self.timeout * 5)
 
+    def getURL(self, sha1):
+        """Get the URL for a file on the builder with a given SHA-1."""
+        return urlappend(self._file_cache_url, sha1).encode('utf8')
+
     def getFile(self, sha_sum, file_to_write):
         """Fetch a file from the builder.
 
@@ -234,7 +243,7 @@
         :return: A Deferred that calls back when the download is done, or
             errback with the error string.
         """
-        file_url = urlappend(self._file_cache_url, sha_sum).encode('utf8')
+        file_url = self.getURL(sha_sum)
         d = Agent(self.reactor, pool=self.pool).request("GET", file_url)
 
         def got_response(response):

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2015-02-17 07:57:31 +0000
+++ lib/lp/buildmaster/manager.py	2016-05-26 14:04:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Soyuz buildd slave manager logic."""
@@ -8,6 +8,7 @@
 __all__ = [
     'BuilddManager',
     'BUILDD_MANAGER_LOG_NAME',
+    'SlaveScanner',
     ]
 
 import datetime

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehaviour.py'
--- lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2015-09-23 21:44:36 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2016-05-26 14:04:28 +0000
@@ -291,8 +291,9 @@
         os.makedirs(upload_path)
 
         filenames_to_download = []
-        for filename in filemap:
-            logger.info("Grabbing file: %s" % filename)
+        for filename, sha1 in filemap.items():
+            logger.info("Grabbing file: %s (%s)" % (
+                filename, self._slave.getURL(sha1)))
             out_file_name = os.path.join(upload_path, filename)
             # If the evaluated output file name is not within our
             # upload path, then we don't try to copy this or any
@@ -300,7 +301,7 @@
             if not os.path.realpath(out_file_name).startswith(upload_path):
                 raise BuildDaemonError(
                     "Build returned a file named %r." % filename)
-            filenames_to_download.append((filemap[filename], out_file_name))
+            filenames_to_download.append((sha1, out_file_name))
         yield self._slave.getFiles(filenames_to_download)
 
         transaction.commit()

=== modified file 'lib/lp/buildmaster/tests/mock_slaves.py'
--- lib/lp/buildmaster/tests/mock_slaves.py	2016-05-25 15:31:48 +0000
+++ lib/lp/buildmaster/tests/mock_slaves.py	2016-05-26 14:04:28 +0000
@@ -37,6 +37,7 @@
 from lp.buildmaster.interactor import BuilderSlave
 from lp.buildmaster.interfaces.builder import CannotFetchFile
 from lp.services.config import config
+from lp.services.webapp import urlappend
 from lp.testing.sampledata import I386_ARCHITECTURE_NAME
 
 
@@ -136,6 +137,10 @@
 
         return d.addCallback(check_present)
 
+    def getURL(self, sha1):
+        return urlappend(
+            'http://localhost:8221/filecache/', sha1).encode('utf8')
+
     def getFiles(self, files):
         dl = defer.gatherResults([
             self.getFile(builder_file, local_file)

=== modified file 'lib/lp/buildmaster/tests/test_interactor.py'
--- lib/lp/buildmaster/tests/test_interactor.py	2016-05-25 15:31:48 +0000
+++ lib/lp/buildmaster/tests/test_interactor.py	2016-05-26 14:04:28 +0000
@@ -862,3 +862,19 @@
             dl.append(d)
 
         return defer.DeferredList(dl).addCallback(finished_uploading)
+
+    @defer.inlineCallbacks
+    def test_getFiles_with_file_objects(self):
+        # getFiles works with file-like objects as well as file names.
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        temp_fd, temp_name = tempfile.mkstemp()
+        self.addCleanup(os.remove, temp_name)
+        lf = self.factory.makeLibraryFileAlias(
+            'content.txt', content='content')
+        self.layer.txn.commit()
+        yield slave.ensurepresent(lf.content.sha1, lf.http_url, "", "")
+        yield slave.getFiles([(lf.content.sha1, os.fdopen(temp_fd, "w"))])
+        with open(temp_name) as f:
+            self.assertEqual('content', f.read())
+        yield slave.pool.closeCachedConnections()