launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25078
[Merge] ~cjwatson/launchpad:buildmaster-remove-in-process-fetch into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:buildmaster-remove-in-process-fetch into launchpad:master.
Commit message:
Remove buildd-manager's in-process fetch mode
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/387936
The subprocess mode is now the default, and seems to be working well.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:buildmaster-remove-in-process-fetch into launchpad:master.
diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index c19e617..9a8c4f2 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -12,7 +12,6 @@ from collections import namedtuple
import logging
import os.path
import sys
-import tempfile
import traceback
from ampoule.pool import ProcessPool
@@ -23,13 +22,8 @@ from twisted.internet import (
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 (
- Agent,
- HTTPConnectionPool,
- ResponseDone,
- )
+from twisted.web.client import HTTPConnectionPool
from zope.security.proxy import (
isinstance as zope_isinstance,
removeSecurityProxy,
@@ -53,7 +47,6 @@ from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
IBuildFarmJobBehaviour,
)
from lp.services.config import config
-from lp.services.features import getFeatureFlag
from lp.services.job.runner import (
QuietAMPConnector,
VirtualEnvProcessStarter,
@@ -68,46 +61,6 @@ class QuietQueryFactory(xmlrpc._QueryFactory):
noisy = False
-class FileWritingProtocol(Protocol):
- """A protocol that saves data to a file."""
-
- def __init__(self, finished, file_to_write):
- self.finished = finished
- if isinstance(file_to_write, (bytes, unicode)):
- self.filename = file_to_write
- self.file = tempfile.NamedTemporaryFile(
- mode="wb", prefix=os.path.basename(self.filename) + "_",
- dir=os.path.dirname(self.filename), delete=False)
- else:
- self.filename = None
- self.file = file_to_write
-
- def dataReceived(self, data):
- try:
- self.file.write(data)
- except IOError:
- try:
- self.file.close()
- except IOError:
- pass
- self.file = None
- self.finished.errback()
-
- def connectionLost(self, reason):
- try:
- if self.file is not None:
- self.file.close()
- if self.filename is not None and reason.check(ResponseDone):
- os.rename(self.file.name, self.filename)
- except IOError:
- self.finished.errback()
- else:
- if reason.check(ResponseDone):
- self.finished.callback(None)
- else:
- self.finished.errback(reason)
-
-
class LimitedHTTPConnectionPool(HTTPConnectionPool):
"""A connection pool with an upper limit on open connections."""
@@ -148,15 +101,7 @@ def default_pool(reactor=None):
if reactor is None:
reactor = default_reactor
if _default_pool is None:
- # Circular import.
- from lp.buildmaster.manager import SlaveScanner
- # Short cached connection timeout to avoid potential weirdness with
- # virtual builders that reboot frequently.
- _default_pool = LimitedHTTPConnectionPool(
- reactor, config.builddmaster.download_connections)
- _default_pool.maxPersistentPerHost = (
- config.builddmaster.idle_download_connections_per_builder)
- _default_pool.cachedConnectionTimeout = SlaveScanner.SCAN_INTERVAL
+ _default_pool = HTTPConnectionPool(reactor)
return _default_pool
@@ -235,20 +180,12 @@ class BuilderSlave(object):
if reactor is None:
reactor = default_reactor
self.reactor = reactor
- 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
- if self._download_in_subprocess:
- if process_pool is None:
- process_pool = default_process_pool(reactor=reactor)
- self.process_pool = process_pool
- else:
- self.process_pool = None
+ if process_pool is None:
+ process_pool = default_process_pool(reactor=reactor)
+ self.process_pool = process_pool
@classmethod
def makeBuilderSlave(cls, builder_url, vm_host, timeout, reactor=None,
@@ -325,29 +262,16 @@ class BuilderSlave(object):
"""
file_url = self.getURL(sha_sum)
try:
- # Select download behaviour according to the
- # buildmaster.download_in_subprocess feature rule: if enabled,
- # defer the download to a subprocess; if disabled, download the
- # file asynchronously in Twisted. We've found that in practice
- # the asynchronous approach only works well up to a bit over a
- # hundred builders, and beyond that it struggles to keep up with
- # incoming packets in time to avoid TCP timeouts (perhaps
- # because of too much synchronous work being done on the reactor
- # thread). The exact reason for this is as yet unproven, so we
- # use a feature rule to allow us to try out different
- # approaches.
- if self._download_in_subprocess:
- yield self.process_pool.doWork(
- DownloadCommand,
- file_url=file_url, path_to_write=path_to_write,
- timeout=self.timeout)
- else:
- response = yield Agent(self.reactor, pool=self.pool).request(
- "GET", file_url)
- finished = defer.Deferred()
- response.deliverBody(
- FileWritingProtocol(finished, path_to_write))
- yield finished
+ # Download the file in a subprocess. We used to download it
+ # asynchronously in Twisted, but in practice this only worked well
+ # up to a bit over a hundred builders; beyond that it struggled to
+ # keep up with incoming packets in time to avoid TCP timeouts
+ # (perhaps because of too much synchronous work being done on the
+ # reactor thread).
+ yield self.process_pool.doWork(
+ DownloadCommand,
+ file_url=file_url, path_to_write=path_to_write,
+ timeout=self.timeout)
if logger is not None:
logger.info("Grabbed %s" % file_url)
except Exception as e:
diff --git a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
index b1db827..8cbda44 100644
--- a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
@@ -13,7 +13,6 @@ import os
import shutil
import tempfile
-from testscenarios import WithScenarios
from testtools import ExpectedException
from testtools.twistedsupport import AsynchronousDeferredRunTest
from twisted.internet import defer
@@ -44,7 +43,6 @@ from lp.buildmaster.tests.mock_slaves import (
)
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.services.config import config
-from lp.services.features.testing import FeatureFixture
from lp.services.log.logger import BufferLogger
from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
from lp.testing import (
@@ -321,17 +319,8 @@ class TestVerifySuccessfulBuildMixin:
self.assertRaises(AssertionError, behaviour.verifySuccessfulBuild)
-class TestHandleStatusMixin(WithScenarios):
- """Tests for `IPackageBuild`s handleStatus method.
-
- This should be run in a test file with
- `load_tests = load_tests_apply_scenarios`.
- """
-
- scenarios = [
- ('download_in_twisted', {'download_in_subprocess': False}),
- ('download_in_subprocess', {'download_in_subprocess': True}),
- ]
+class TestHandleStatusMixin:
+ """Tests for `IPackageBuild`s handleStatus method."""
layer = LaunchpadZopelessLayer
run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=30)
@@ -342,9 +331,6 @@ class TestHandleStatusMixin(WithScenarios):
def setUp(self):
super(TestHandleStatusMixin, self).setUp()
- if not self.download_in_subprocess:
- self.useFixture(FeatureFixture(
- {'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 db4495a..ee3ed01 100644
--- a/lib/lp/buildmaster/tests/test_interactor.py
+++ b/lib/lp/buildmaster/tests/test_interactor.py
@@ -16,17 +16,8 @@ import signal
import tempfile
from lpbuildd.slave import BuilderStatus
-from lpbuildd.tests.harness import BuilddSlaveTestSetup
from six.moves import xmlrpc_client
-from testscenarios import (
- load_tests_apply_scenarios,
- WithScenarios,
- )
-from testtools.matchers import (
- ContainsAll,
- HasLength,
- MatchesDict,
- )
+from testtools.matchers import ContainsAll
from testtools.testcase import ExpectedException
from testtools.twistedsupport import (
assert_fails_with,
@@ -34,10 +25,7 @@ from testtools.twistedsupport import (
AsynchronousDeferredRunTestForBrokenTwisted,
)
import treq
-from twisted.internet import (
- defer,
- reactor as default_reactor,
- )
+from twisted.internet import defer
from twisted.internet.task import Clock
from twisted.python.failure import Failure
@@ -51,7 +39,6 @@ from lp.buildmaster.interactor import (
BuilderInteractor,
BuilderSlave,
extract_vitals_from_db,
- LimitedHTTPConnectionPool,
make_download_process_pool,
shut_down_default_process_pool,
)
@@ -72,7 +59,6 @@ from lp.buildmaster.tests.mock_slaves import (
WaitingSlave,
)
from lp.services.config import config
-from lp.services.features.testing import FeatureFixture
from lp.services.twistedsupport.testing import TReqFixture
from lp.services.twistedsupport.treq import check_status
from lp.soyuz.model.binarypackagebuildbehaviour import (
@@ -755,26 +741,17 @@ class TestSlaveConnectionTimeouts(TestCase):
return assert_fails_with(d, defer.CancelledError)
-class TestSlaveWithLibrarian(WithScenarios, TestCaseWithFactory):
+class TestSlaveWithLibrarian(TestCaseWithFactory):
"""Tests that need more of Launchpad to run."""
- scenarios = [
- ('download_in_twisted', {'download_in_subprocess': False}),
- ('download_in_subprocess', {'download_in_subprocess': True}),
- ]
-
layer = LaunchpadZopelessLayer
run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
timeout=20)
def setUp(self):
super(TestSlaveWithLibrarian, self).setUp()
- if not self.download_in_subprocess:
- self.useFixture(FeatureFixture(
- {'buildmaster.download_in_subprocess': ''}))
self.slave_helper = self.useFixture(SlaveTestHelpers())
- if self.download_in_subprocess:
- self.addCleanup(shut_down_default_process_pool)
+ self.addCleanup(shut_down_default_process_pool)
def test_ensurepresent_librarian(self):
# ensurepresent, when given an http URL for a file will download the
@@ -828,7 +805,6 @@ class TestSlaveWithLibrarian(WithScenarios, TestCaseWithFactory):
for sha1, local_file in files:
with open(local_file) as f:
self.assertEqual(content_map[sha1], f.read())
- return slave.pool.closeCachedConnections()
def finished_uploading(ignored):
d = slave.getFiles(files)
@@ -858,15 +834,10 @@ class TestSlaveWithLibrarian(WithScenarios, TestCaseWithFactory):
# connections.
contents = [self.factory.getUniqueString() for _ in range(10)]
self.slave_helper.getServerSlave()
- pool = LimitedHTTPConnectionPool(default_reactor, 2)
- if self.download_in_subprocess:
- process_pool = make_download_process_pool(min=1, max=2)
- process_pool.start()
- self.addCleanup(process_pool.stop)
- else:
- process_pool = None
- slave = self.slave_helper.getClientSlave(
- pool=pool, process_pool=process_pool)
+ process_pool = make_download_process_pool(min=1, max=2)
+ process_pool.start()
+ self.addCleanup(process_pool.stop)
+ slave = self.slave_helper.getClientSlave(process_pool=process_pool)
files = []
content_map = {}
@@ -876,16 +847,8 @@ class TestSlaveWithLibrarian(WithScenarios, TestCaseWithFactory):
for sha1, local_file in files:
with open(local_file) as f:
self.assertEqual(content_map[sha1], f.read())
- port = BuilddSlaveTestSetup().daemon_port
- if self.download_in_subprocess:
- # Only two workers were used.
- self.assertEqual(2, len(process_pool.processes))
- else:
- # Only two connections were used.
- self.assertThat(
- slave.pool._connections,
- MatchesDict({("http", "localhost", port): HasLength(2)}))
- return slave.pool.closeCachedConnections()
+ # Only two workers were used.
+ self.assertEqual(2, len(process_pool.processes))
def finished_uploading(ignored):
d = slave.getFiles(files)
@@ -918,7 +881,3 @@ class TestSlaveWithLibrarian(WithScenarios, TestCaseWithFactory):
yield slave.getFiles([(empty_sha1, temp_name)])
with open(temp_name) as f:
self.assertEqual(b'', f.read())
- yield slave.pool.closeCachedConnections()
-
-
-load_tests = load_tests_apply_scenarios
diff --git a/lib/lp/code/model/tests/test_recipebuilder.py b/lib/lp/code/model/tests/test_recipebuilder.py
index 1ef74fe..8acc5b8 100644
--- a/lib/lp/code/model/tests/test_recipebuilder.py
+++ b/lib/lp/code/model/tests/test_recipebuilder.py
@@ -11,7 +11,6 @@ import os.path
import shutil
import tempfile
-from testscenarios import load_tests_apply_scenarios
from testtools.matchers import MatchesListwise
from testtools.twistedsupport import AsynchronousDeferredRunTest
import transaction
@@ -458,6 +457,3 @@ class TestVerifySuccessfulBuildForSPRBuild(
class TestHandleStatusForSPRBuild(
MakeSPRecipeBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
"""IPackageBuild.handleStatus works with SPRecipe builds."""
-
-
-load_tests = load_tests_apply_scenarios
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index d30254e..e6b6208 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -20,10 +20,6 @@ import fixtures
from fixtures import MockPatch
import pytz
from six.moves.urllib_parse import urlsplit
-from testscenarios import (
- load_tests_apply_scenarios,
- WithScenarios,
- )
from testtools import ExpectedException
from testtools.matchers import (
AfterPreprocessing,
@@ -176,6 +172,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):
@@ -402,8 +399,7 @@ class TestAsyncOCIRecipeBuildBehaviour(MakeOCIBuildMixin, TestCaseWithFactory):
self.assertEqual(distroseries.name, slave.call_log[1][5]['series'])
-class TestHandleStatusForOCIRecipeBuild(WithScenarios,
- MakeOCIBuildMixin,
+class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
TestCaseWithFactory):
# This is mostly copied from TestHandleStatusMixin, however
# we can't use all of those tests, due to the way OCIRecipeBuildBehaviour
@@ -411,11 +407,6 @@ class TestHandleStatusForOCIRecipeBuild(WithScenarios,
# available. There's also some differences in the filemap handling, as
# we need a much more complex filemap here.
- scenarios = [
- ('download_in_twisted', {'download_in_subprocess': False}),
- ('download_in_subprocess', {'download_in_subprocess': True}),
- ]
-
layer = LaunchpadZopelessLayer
run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
timeout=20)
@@ -429,10 +420,7 @@ class TestHandleStatusForOCIRecipeBuild(WithScenarios,
def setUp(self):
super(TestHandleStatusForOCIRecipeBuild, self).setUp()
self.useFixture(fixtures.FakeLogger())
- features = {OCI_RECIPE_ALLOW_CREATE: 'on'}
- if not self.download_in_subprocess:
- features['buildmaster.download_in_subprocess'] = ''
- self.useFixture(FeatureFixture(features))
+ self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
self.build = self.makeBuild()
# For the moment, we require a builder for the build so that
# handleStatus_OK can get a reference to the slave.
@@ -706,6 +694,3 @@ class TestGetUploadMethodsForOCIRecipeBuild(
def setUp(self):
self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
super(TestGetUploadMethodsForOCIRecipeBuild, self).setUp()
-
-
-load_tests = load_tests_apply_scenarios
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index 24118e8..6c1ddb0 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -67,10 +67,6 @@ socket_timeout: 40
# datatype: integer
virtualized_socket_timeout: 30
-# The maximum number of idle file download connections per builder that
-# may be kept open.
-idle_download_connections_per_builder: 10
-
# The maximum number of file download connections that may be open
# across all builders.
download_connections: 2048
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index 9cf570f..6fe3015 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -19,7 +19,6 @@ import fixtures
from pymacaroons import Macaroon
import pytz
from six.moves.urllib_parse import urlsplit
-from testscenarios import load_tests_apply_scenarios
from testtools import ExpectedException
from testtools.matchers import (
AfterPreprocessing,
@@ -847,6 +846,3 @@ class TestVerifySuccessfulBuildForSnapBuild(
class TestHandleStatusForSnapBuild(
MakeSnapBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
"""IPackageBuild.handleStatus works with Snap builds."""
-
-
-load_tests = load_tests_apply_scenarios
diff --git a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
index d445091..aa79d81 100644
--- a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
@@ -13,7 +13,6 @@ import shutil
import tempfile
from storm.store import Store
-from testscenarios import load_tests_apply_scenarios
from testtools.matchers import MatchesListwise
from testtools.twistedsupport import AsynchronousDeferredRunTest
import transaction
@@ -636,6 +635,3 @@ class TestVerifySuccessfulBuildForBinaryPackageBuild(
class TestHandleStatusForBinaryPackageBuild(
MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
"""IPackageBuild.handleStatus works with binary builds."""
-
-
-load_tests = load_tests_apply_scenarios
diff --git a/lib/lp/soyuz/tests/test_livefsbuildbehaviour.py b/lib/lp/soyuz/tests/test_livefsbuildbehaviour.py
index 3c3ee29..4820132 100644
--- a/lib/lp/soyuz/tests/test_livefsbuildbehaviour.py
+++ b/lib/lp/soyuz/tests/test_livefsbuildbehaviour.py
@@ -11,7 +11,6 @@ from datetime import datetime
import os.path
import pytz
-from testscenarios import load_tests_apply_scenarios
from testtools.matchers import MatchesListwise
from testtools.twistedsupport import AsynchronousDeferredRunTest
import transaction
@@ -343,6 +342,3 @@ class TestVerifySuccessfulBuildForLiveFSBuild(
class TestHandleStatusForLiveFSBuild(
MakeLiveFSBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
"""IPackageBuild.handleStatus works with LiveFS builds."""
-
-
-load_tests = load_tests_apply_scenarios
