launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:buildmaster-fetch-in-process-default into launchpad:master.

Commit message:
Default buildmaster.download_in_subprocess to on

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

This involves a few test fixes here and there.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:buildmaster-fetch-in-process-default into launchpad:master.

diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index d036aad..c19e617 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -22,6 +22,7 @@ from twisted.internet import (
     defer,
     reactor as default_reactor,
     )
+from twisted.internet.interfaces import IReactorCore
 from twisted.internet.protocol import Protocol
 from twisted.web import xmlrpc
 from twisted.web.client import (
@@ -185,9 +186,10 @@ def default_process_pool(reactor=None):
     if _default_process_pool is None:
         _default_process_pool = make_download_process_pool()
         _default_process_pool.start()
-        shutdown_id = reactor.addSystemEventTrigger(
-            "during", "shutdown", _default_process_pool.stop)
-        _default_process_pool_shutdown = (reactor, shutdown_id)
+        if IReactorCore.providedBy(reactor):
+            shutdown_id = reactor.addSystemEventTrigger(
+                "during", "shutdown", _default_process_pool.stop)
+            _default_process_pool_shutdown = (reactor, shutdown_id)
     return _default_process_pool
 
 
@@ -233,8 +235,11 @@ class BuilderSlave(object):
         if reactor is None:
             reactor = default_reactor
         self.reactor = reactor
-        self._download_in_subprocess = bool(
-            getFeatureFlag('buildmaster.download_in_subprocess'))
+        download_in_subprocess_flag = getFeatureFlag(
+            'buildmaster.download_in_subprocess')
+        self._download_in_subprocess = (
+            bool(download_in_subprocess_flag)
+            if download_in_subprocess_flag is not None else True)
         if pool is None:
             pool = default_pool(reactor=reactor)
         self.pool = pool
diff --git a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
index f837bb0..b1db827 100644
--- a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
@@ -342,9 +342,9 @@ class TestHandleStatusMixin(WithScenarios):
 
     def setUp(self):
         super(TestHandleStatusMixin, self).setUp()
-        if self.download_in_subprocess:
+        if not self.download_in_subprocess:
             self.useFixture(FeatureFixture(
-                {'buildmaster.download_in_subprocess': 'on'}))
+                {'buildmaster.download_in_subprocess': ''}))
         self.factory = LaunchpadObjectFactory()
         self.build = self.makeBuild()
         # For the moment, we require a builder for the build so that
diff --git a/lib/lp/buildmaster/tests/test_interactor.py b/lib/lp/buildmaster/tests/test_interactor.py
index 0c4cf14..c314f85 100644
--- a/lib/lp/buildmaster/tests/test_interactor.py
+++ b/lib/lp/buildmaster/tests/test_interactor.py
@@ -80,7 +80,6 @@ from lp.soyuz.model.binarypackagebuildbehaviour import (
     BinaryPackageBuildBehaviour,
     )
 from lp.testing import (
-    clean_up_reactor,
     TestCase,
     TestCaseWithFactory,
     )
@@ -682,7 +681,8 @@ class TestSlaveTimeouts(TestCase):
     # Testing that the methods that call callRemote() all time out
     # as required.
 
-    run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted
+    run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
+        timeout=10)
 
     def setUp(self):
         super(TestSlaveTimeouts, self).setUp()
@@ -734,11 +734,6 @@ class TestSlaveConnectionTimeouts(TestCase):
         self.clock = Clock()
         self.addCleanup(shut_down_default_process_pool)
 
-    def tearDown(self):
-        # We need to remove any DelayedCalls that didn't actually get called.
-        clean_up_reactor()
-        super(TestSlaveConnectionTimeouts, self).tearDown()
-
     def test_connection_timeout(self):
         # The default timeout of 30 seconds should not cause a timeout,
         # only the config value should.
@@ -772,9 +767,9 @@ class TestSlaveWithLibrarian(WithScenarios, TestCaseWithFactory):
 
     def setUp(self):
         super(TestSlaveWithLibrarian, self).setUp()
-        if self.download_in_subprocess:
+        if not self.download_in_subprocess:
             self.useFixture(FeatureFixture(
-                {'buildmaster.download_in_subprocess': 'on'}))
+                {'buildmaster.download_in_subprocess': ''}))
         self.slave_helper = self.useFixture(SlaveTestHelpers())
         if self.download_in_subprocess:
             self.addCleanup(shut_down_default_process_pool)
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index 2d98baf..46b421c 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -175,6 +175,7 @@ class TestAsyncOCIRecipeBuildBehaviour(MakeOCIBuildMixin, TestCaseWithFactory):
         self.useFixture(fixtures.MockPatch(
             "time.time", return_value=self.now))
         self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
+        self.addCleanup(shut_down_default_process_pool)
 
     @defer.inlineCallbacks
     def test_composeBuildRequest(self):
@@ -429,8 +430,8 @@ class TestHandleStatusForOCIRecipeBuild(WithScenarios,
         super(TestHandleStatusForOCIRecipeBuild, self).setUp()
         self.useFixture(fixtures.FakeLogger())
         features = {OCI_RECIPE_ALLOW_CREATE: 'on'}
-        if self.download_in_subprocess:
-            features['buildmaster.download_in_subprocess'] = 'on'
+        if not self.download_in_subprocess:
+            features['buildmaster.download_in_subprocess'] = ''
         self.useFixture(FeatureFixture(features))
         self.build = self.makeBuild()
         # For the moment, we require a builder for the build so that
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index 883ad47..9cf570f 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -57,6 +57,7 @@ from lp.buildmaster.enums import (
     BuildBaseImageType,
     BuildStatus,
     )
+from lp.buildmaster.interactor import shut_down_default_process_pool
 from lp.buildmaster.interfaces.builder import CannotBuild
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
     IBuildFarmJobBehaviour,
@@ -305,6 +306,7 @@ class TestAsyncSnapBuildBehaviour(TestSnapBuildBehaviourBase):
         self.now = time.time()
         self.useFixture(fixtures.MockPatch(
             "time.time", return_value=self.now))
+        self.addCleanup(shut_down_default_process_pool)
 
     def makeJob(self, **kwargs):
         # We need a builder slave in these tests, in order that requesting a