launchpad-reviewers team mailing list archive

[Colin Watson <mp+417644@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:ci-build-fix-dispatch into launchpad:master.

Commit message:
Fix dispatch of CI build jobs

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

`CIBuild.getConfiguration` calls out to an external service, and so can't be called from buildd-manager's main thread because it would block the Twisted reactor.  A better approach would be to store the relevant bits of job configuration in the database, but a quick fix for now is to defer this request to a thread, similar to the approach taken in `lp.soyuz.adapters.archivedependencies._get_sources_list_for_dependencies` for retrieving GPG keys from the keyserver.

To test this, I extended `GitHostingFixture` to detect the case where the default timeout hasn't been set, which would cause `GitHostingClient` to fail in a real environment.  I'd like to make the fixture do this by default, but attempting that caused a number of other (probably artificial) test failures which would need to be fixed first.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:ci-build-fix-dispatch into launchpad:master.

diff --git a/lib/lp/code/model/cibuildbehaviour.py b/lib/lp/code/model/cibuildbehaviour.py
index 77f04af..7cd13a0 100644
--- a/lib/lp/code/model/cibuildbehaviour.py
+++ b/lib/lp/code/model/cibuildbehaviour.py
@@ -11,6 +11,7 @@ import json
 import os
 
 from twisted.internet import defer
+from twisted.internet.threads import deferToThread
 from zope.component import adapter
 from zope.interface import implementer
 
@@ -24,6 +25,11 @@ from lp.buildmaster.model.buildfarmjobbehaviour import (
     BuildFarmJobBehaviourBase,
     )
 from lp.code.interfaces.cibuild import ICIBuild
+from lp.services.timeout import default_timeout
+from lp.services.webapp.interaction import (
+    ANONYMOUS,
+    setupInteraction,
+    )
 from lp.soyuz.adapters.archivedependencies import (
     get_sources_list_for_building,
     )
@@ -65,10 +71,26 @@ class CIBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
     def extraBuildArgs(self, logger=None):
         """Return extra builder arguments for this build."""
         build = self.build
-        try:
-            configuration = self.build.getConfiguration(logger=logger)
-        except Exception as e:
-            raise CannotBuild(str(e))
+        # Preload the build's repository so that it can be accessed from
+        # another thread.
+        build.git_repository.id
+
+        # XXX cjwatson 2022-03-24: Work around a design error.  We ought to
+        # have arranged to store the relevant bits of the configuration
+        # (i.e. `stages` below) in the database so that we don't need to
+        # fetch it again here.  It isn't safe to run blocking network
+        # requests in buildd-manager's main thread, since that would block
+        # the Twisted reactor; defer the request to a thread for now, but
+        # we'll need to work out a better fix once we have time.
+        def get_configuration():
+            setupInteraction(ANONYMOUS)
+            with default_timeout(15.0):
+                try:
+                    return build.getConfiguration(logger=logger)
+                except Exception as e:
+                    raise CannotBuild(str(e))
+
+        configuration = yield deferToThread(get_configuration)
         stages = []
         if not configuration.pipeline:
             raise CannotBuild(
diff --git a/lib/lp/code/model/tests/test_cibuildbehaviour.py b/lib/lp/code/model/tests/test_cibuildbehaviour.py
index 808144a..9e3b904 100644
--- a/lib/lp/code/model/tests/test_cibuildbehaviour.py
+++ b/lib/lp/code/model/tests/test_cibuildbehaviour.py
@@ -69,10 +69,6 @@ from lp.services.log.logger import (
     DevNullLogger,
     )
 from lp.services.statsd.tests import StatsMixin
-from lp.services.timeout import (
-    get_default_timeout_function,
-    set_default_timeout_function,
-    )
 from lp.services.webapp import canonical_url
 from lp.soyuz.adapters.archivedependencies import (
     get_sources_list_for_building,
@@ -182,9 +178,6 @@ class TestAsyncCIBuildBehaviour(StatsMixin, TestCIBuildBehaviourBase):
         self.useFixture(MockPatch("time.time", return_value=self.now))
         self.addCleanup(shut_down_default_process_pool)
         self.setUpStats()
-        self.addCleanup(
-            set_default_timeout_function, get_default_timeout_function())
-        set_default_timeout_function(lambda: None)
 
     def makeJob(self, configuration=_unset, **kwargs):
         # We need a builder in these tests, in order that requesting a proxy
@@ -208,7 +201,7 @@ class TestAsyncCIBuildBehaviour(StatsMixin, TestCIBuildBehaviourBase):
                     job.build.distro_arch_series.distroseries.name,
                     job.build.distro_arch_series.architecturetag)).encode()
         hosting_fixture = self.useFixture(
-            GitHostingFixture(blob=configuration))
+            GitHostingFixture(blob=configuration, enforce_timeout=True))
         if configuration is None:
             hosting_fixture.getBlob.failure = GitRepositoryBlobNotFound(
                 job.build.git_repository.getInternalPath(), ".launchpad.yaml",
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index f566b60..d368213 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -45,6 +45,7 @@ from lp.registry.interfaces.series import SeriesStatus
 from lp.services.database.sqlbase import cursor
 from lp.services.memcache.testing import MemcacheFixture
 from lp.services.propertycache import get_property_cache
+from lp.services.timeout import get_default_timeout_function
 from lp.testing import (
     run_with_login,
     time_counter,
@@ -303,30 +304,48 @@ class BranchHostingFixture(fixtures.Fixture):
             self.memcache_fixture = self.useFixture(MemcacheFixture())
 
 
+class FakeMethodEnforceTimeout(FakeMethod):
+    """A variant of `FakeMethod` that requires a default timeout.
+
+    Since `GitHostingClient` requires that `set_default_timeout_function` has
+    been called, `GitHostingFixture` requires that as well.
+    """
+
+    def __call__(self, *args, **kwargs):
+        if get_default_timeout_function() is None:
+            raise AssertionError("No default timeout function was set.")
+        return super().__call__(*args, **kwargs)
+
+
 class GitHostingFixture(fixtures.Fixture):
     """A fixture that temporarily registers a fake Git hosting client."""
 
     def __init__(self, default_branch="refs/heads/master",
                  refs=None, commits=None, log=None, diff=None, merge_diff=None,
-                 merges=None, blob=None, disable_memcache=True):
-        self.create = FakeMethod()
-        self.getProperties = FakeMethod(
+                 merges=None, blob=None, disable_memcache=True,
+                 enforce_timeout=False):
+        fake_method_factory = (
+            FakeMethodEnforceTimeout if enforce_timeout else FakeMethod)
+        self.create = fake_method_factory()
+        self.getProperties = fake_method_factory(
             result={"default_branch": default_branch, "is_available": True})
-        self.setProperties = FakeMethod()
-        self.getRefs = FakeMethod(result=({} if refs is None else refs))
-        self.getCommits = FakeMethod(
+        self.setProperties = fake_method_factory()
+        self.getRefs = fake_method_factory(
+            result=({} if refs is None else refs))
+        self.getCommits = fake_method_factory(
             result=([] if commits is None else commits))
-        self.getLog = FakeMethod(result=([] if log is None else log))
-        self.getDiff = FakeMethod(result=({} if diff is None else diff))
-        self.getMergeDiff = FakeMethod(
+        self.getLog = fake_method_factory(result=([] if log is None else log))
+        self.getDiff = fake_method_factory(
+            result=({} if diff is None else diff))
+        self.getMergeDiff = fake_method_factory(
             result={} if merge_diff is None else merge_diff)
-        self.detectMerges = FakeMethod(
+        self.detectMerges = fake_method_factory(
             result=({} if merges is None else merges))
-        self.getBlob = FakeMethod(result=blob)
-        self.delete = FakeMethod()
+        self.getBlob = fake_method_factory(result=blob)
+        self.delete = fake_method_factory()
         self.disable_memcache = disable_memcache
-        self.repackRepository = FakeMethod()
-        self.collectGarbage = FakeMethod()
+        self.repackRepository = fake_method_factory()
+        self.collectGarbage = fake_method_factory()
 
     def _setUp(self):
         self.useFixture(ZopeUtilityFixture(self, IGitHostingClient))