launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:request-proxy-token-subprocess into launchpad:master.

Commit message:
Download snap proxy tokens in a subprocess

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/389175

We still see mysterious treq timeouts when requesting proxy tokens.  By contrast, the process-pool-based approach that we use for fetching files seems much more robust, so let's just use it here too.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:request-proxy-token-subprocess into launchpad:master.

diff --git a/lib/lp/buildmaster/downloader.py b/lib/lp/buildmaster/downloader.py
index 9da2b89..0e1f28f 100644
--- a/lib/lp/buildmaster/downloader.py
+++ b/lib/lp/buildmaster/downloader.py
@@ -12,7 +12,8 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 __all__ = [
     'DownloadCommand',
-    'DownloadProcess',
+    'RequestProcess',
+    'RequestProxyTokenCommand',
     ]
 
 import os.path
@@ -42,8 +43,25 @@ class DownloadCommand(amp.Command):
         }
 
 
-class DownloadProcess(AMPChild):
-    """A subprocess that downloads a file to disk."""
+class RequestProxyTokenCommand(amp.Command):
+
+    arguments = [
+        (b"url", amp.Unicode()),
+        (b"auth_header", amp.String()),
+        (b"proxy_username", amp.Unicode()),
+        ]
+    response = [
+        (b"username", amp.Unicode()),
+        (b"secret", amp.Unicode()),
+        (b"timestamp", amp.Unicode()),
+        ]
+    errors = {
+        RequestException: b"REQUEST_ERROR",
+        }
+
+
+class RequestProcess(AMPChild):
+    """A subprocess that performs requests for buildd-manager."""
 
     @DownloadCommand.responder
     def downloadCommand(self, file_url, path_to_write, timeout):
@@ -64,3 +82,13 @@ class DownloadProcess(AMPChild):
             f.close()
             os.rename(f.name, path_to_write)
         return {}
+
+    @RequestProxyTokenCommand.responder
+    def requestProxyTokenCommand(self, url, auth_header, proxy_username):
+        session = Session()
+        session.trust_env = False
+        response = session.post(
+            url, headers={"Authorization": auth_header},
+            json={"username": proxy_username})
+        response.raise_for_status()
+        return response.json()
diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index 8b05dea..7450a0b 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -31,7 +31,7 @@ from zope.security.proxy import (
 
 from lp.buildmaster.downloader import (
     DownloadCommand,
-    DownloadProcess,
+    RequestProcess,
     )
 from lp.buildmaster.enums import (
     BuilderCleanStatus,
@@ -91,7 +91,7 @@ def make_download_process_pool(**kwargs):
     # currently configurable, but if we find we need to tweak it further
     # then we should add a configuration setting for it.
     kwargs.setdefault("maxIdle", 300)
-    return ProcessPool(DownloadProcess, starter=starter, **kwargs)
+    return ProcessPool(RequestProcess, starter=starter, **kwargs)
 
 
 def default_process_pool(reactor=None):
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index 0fb6c1d..3fdf8b4 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -76,7 +76,6 @@ from lp.services.features.testing import FeatureFixture
 from lp.services.log.logger import DevNullLogger
 from lp.services.propertycache import get_property_cache
 from lp.services.webapp import canonical_url
-from lp.snappy.model.snapbuildbehaviour import proxy_pool
 from lp.soyuz.adapters.archivedependencies import (
     get_sources_list_for_building,
     )
@@ -119,7 +118,6 @@ class MakeOCIBuildMixin:
         slave = self.useFixture(SlaveTestHelpers()).getClientSlave()
         job.setBuilder(builder, slave)
         self.addCleanup(slave.pool.closeCachedConnections)
-        self.addCleanup(proxy_pool(slave.reactor).closeCachedConnections)
         self.addCleanup(shut_down_default_process_pool)
 
         # Taken from test_archivedependencies.py
@@ -217,7 +215,7 @@ class TestAsyncOCIRecipeBuildBehaviour(MakeOCIBuildMixin, TestCaseWithFactory):
                         Equals(b"Basic " + base64.b64encode(
                             b"admin-launchpad.test:admin-secret"))]),
                     b"Content-Type": MatchesListwise([
-                        Equals(b"application/json; charset=UTF-8"),
+                        Equals(b"application/json"),
                         ]),
                     }),
                 "content": AfterPreprocessing(json.loads, MatchesDict({
diff --git a/lib/lp/snappy/model/snapbuildbehaviour.py b/lib/lp/snappy/model/snapbuildbehaviour.py
index 370f3f3..f9bd56c 100644
--- a/lib/lp/snappy/model/snapbuildbehaviour.py
+++ b/lib/lp/snappy/model/snapbuildbehaviour.py
@@ -21,16 +21,12 @@ from six.moves.urllib.parse import (
     urlsplit,
     urlunsplit,
     )
-import treq
-from twisted.internet import (
-    defer,
-    reactor as default_reactor,
-    )
-from twisted.web.client import HTTPConnectionPool
+from twisted.internet import defer
 from zope.component import adapter
 from zope.interface import implementer
 from zope.security.proxy import removeSecurityProxy
 
+from lp.buildmaster.downloader import RequestProxyTokenCommand
 from lp.buildmaster.enums import BuildBaseImageType
 from lp.buildmaster.interfaces.builder import CannotBuild
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
@@ -43,7 +39,6 @@ from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
 from lp.services.features import getFeatureFlag
 from lp.services.twistedsupport import cancel_on_timeout
-from lp.services.twistedsupport.treq import check_status
 from lp.snappy.interfaces.snap import (
     SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG,
     SnapBuildArchiveOwnerMismatch,
@@ -55,18 +50,6 @@ from lp.soyuz.adapters.archivedependencies import (
 from lp.soyuz.interfaces.archive import ArchiveDisabled
 
 
-_proxy_pool = None
-
-
-def proxy_pool(reactor=None):
-    global _proxy_pool
-    if reactor is None:
-        reactor = default_reactor
-    if _proxy_pool is None:
-        _proxy_pool = HTTPConnectionPool(reactor)
-    return _proxy_pool
-
-
 def format_as_rfc3339(timestamp):
     """Return a RFC3339 representation of the given timestamp.
 
@@ -117,14 +100,10 @@ class SnapProxyMixin:
         auth_string = '{}:{}'.format(admin_username, secret).strip()
         auth_header = b'Basic ' + base64.b64encode(auth_string)
 
-        response = yield treq.post(
-            url, headers={'Authorization': auth_header},
-            json={'username': proxy_username},
-            pool=proxy_pool(self._slave.reactor),
-            timeout=config.builddmaster.authentication_timeout,
-            reactor=self._slave.reactor)
-        response = yield check_status(response)
-        token = yield treq.json_content(response)
+        token = yield self._slave.process_pool.doWork(
+            RequestProxyTokenCommand,
+            url=url, auth_header=auth_header,
+            proxy_username=proxy_username)
         defer.returnValue(token)
 
 
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index 4d3e282..37bc7d2 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -94,7 +94,6 @@ from lp.snappy.interfaces.snap import (
     )
 from lp.snappy.model.snapbuildbehaviour import (
     format_as_rfc3339,
-    proxy_pool,
     SnapBuildBehaviour,
     )
 from lp.soyuz.adapters.archivedependencies import (
@@ -317,7 +316,6 @@ class TestAsyncSnapBuildBehaviour(TestSnapBuildBehaviourBase):
         slave = self.useFixture(SlaveTestHelpers()).getClientSlave()
         job.setBuilder(builder, slave)
         self.addCleanup(slave.pool.closeCachedConnections)
-        self.addCleanup(proxy_pool(slave.reactor).closeCachedConnections)
         return job
 
     @defer.inlineCallbacks
@@ -359,7 +357,7 @@ class TestAsyncSnapBuildBehaviour(TestSnapBuildBehaviourBase):
                         Equals(b"Basic " + base64.b64encode(
                             b"admin-launchpad.test:admin-secret"))]),
                     b"Content-Type": MatchesListwise([
-                        Equals(b"application/json; charset=UTF-8"),
+                        Equals(b"application/json"),
                         ]),
                     }),
                 "content": AfterPreprocessing(json.loads, MatchesDict({