launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01172
[Merge] lp:~julian-edwards/launchpad/builderslave-resume into lp:launchpad/devel
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/builderslave-resume into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Make BuilderSlave.resume() return a Deferred (copying the code from RecordingSlave) and move the RecordingSlave tests from test_manager into test_builder. The older resume() was not being used as it's synchronous - this new one is not being used yet either.
--
https://code.launchpad.net/~julian-edwards/launchpad/builderslave-resume/+merge/36351
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/builderslave-resume into lp:launchpad/devel.
=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py 2010-09-21 09:05:34 +0000
+++ lib/lp/buildmaster/model/builder.py 2010-09-22 17:28:52 +0000
@@ -17,7 +17,6 @@
import logging
import os
import socket
-import subprocess
import tempfile
import urllib2
import xmlrpclib
@@ -34,6 +33,7 @@
Count,
Sum,
)
+from twisted.internet import defer
from zope.component import getUtility
from zope.interface import implements
@@ -80,6 +80,7 @@
from lp.services.job.model.job import Job
from lp.services.osutils import until_no_eintr
from lp.services.propertycache import cachedproperty
+from lp.services.twistedsupport.processmonitor import ProcessWithTimeout
# XXX Michael Nelson 2010-01-13 bug=491330
# These dependencies on soyuz will be removed when getBuildRecords()
# is moved.
@@ -170,26 +171,27 @@
fileurl = urlappend(self.urlbase, filelocation)
return urllib2.urlopen(fileurl)
- def resume(self):
- """Resume a virtual builder.
-
- It uses the configuration command-line (replacing 'vm_host') and
- return its output.
-
- :return: a (stdout, stderr, subprocess exitcode) triple
+ def resume(self, clock=None):
+ """Resume the builder in an asynchronous fashion.
+
+ We use the builddmaster configuration 'socket_timeout' as
+ the process timeout.
+
+ :param clock: An optional twisted.internet.task.Clock to override
+ the default clock. For use in tests.
+
+ :return: a Deferred
"""
- # XXX: This executes the vm_resume_command
- # synchronously. RecordingSlave does so asynchronously. Since we
- # always want to do this asynchronously, there's no need for the
- # duplication.
resume_command = config.builddmaster.vm_resume_command % {
'vm_host': self.vm_host}
- resume_argv = resume_command.split()
- resume_process = subprocess.Popen(
- resume_argv, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
- stdout, stderr = resume_process.communicate()
+ # Twisted API requires string but the configuration provides unicode.
+ resume_argv = [str(term) for term in resume_command.split()]
- return (stdout, stderr, resume_process.returncode)
+ d = defer.Deferred()
+ p = ProcessWithTimeout(
+ d, config.builddmaster.socket_timeout, clock=clock)
+ p.spawnProcess(resume_argv[0], tuple(resume_argv))
+ return d
def cacheFile(self, logger, libraryfilealias):
"""Make sure that the file at 'libraryfilealias' is on the slave.
=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py 2010-09-21 16:23:35 +0000
+++ lib/lp/buildmaster/tests/test_builder.py 2010-09-22 17:28:52 +0000
@@ -5,17 +5,25 @@
import errno
import os
+import signal
import socket
import xmlrpclib
+import fixtures
+
from testtools.content import Content
from testtools.content_type import UTF8_TEXT
+from twisted.internet.task import Clock
+from twisted.python.failure import Failure
+from twisted.trial.unittest import TestCase as TrialTestCase
+
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
from canonical.buildd.slave import BuilderStatus
from canonical.buildd.tests.harness import BuilddSlaveTestSetup
+from canonical.config import config
from canonical.database.sqlbase import flush_database_updates
from canonical.launchpad.webapp.interfaces import (
DEFAULT_FLAVOR,
@@ -24,7 +32,8 @@
)
from canonical.testing.layers import (
DatabaseFunctionalLayer,
- LaunchpadZopelessLayer
+ LaunchpadZopelessLayer,
+ TwistedLayer,
)
from lp.buildmaster.enums import BuildStatus
from lp.buildmaster.interfaces.builder import IBuilder, IBuilderSet
@@ -49,7 +58,6 @@
)
from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
from lp.testing import (
- TestCase,
TestCaseWithFactory,
)
from lp.testing.fakemethod import FakeMethod
@@ -467,19 +475,11 @@
self.builder.current_build_behavior, BinaryPackageBuildBehavior)
-class TestSlave(TestCase):
- """
- Integration tests for BuilderSlave that verify how it works against a
- real slave server.
- """
-
- # XXX: JonathanLange 2010-09-20 bug=643521: There are also tests for
- # BuilderSlave in buildd-slave.txt and in other places. The tests here
- # ought to become the canonical tests for BuilderSlave vs running buildd
- # XML-RPC server interaction.
+class SlaveTestHelpers(fixtures.Fixture):
# The URL for the XML-RPC service set up by `BuilddSlaveTestSetup`.
- TEST_URL = 'http://localhost:8221/rpc/'
+ BASE_URL = 'http://localhost:8221'
+ TEST_URL = '%s/rpc/' % (BASE_URL,)
def getServerSlave(self):
"""Set up a test build slave server.
@@ -488,11 +488,14 @@
"""
tachandler = BuilddSlaveTestSetup()
tachandler.setUp()
- def addLogFile(exc_info):
- self.addDetail(
- 'xmlrpc-log-file',
- Content(UTF8_TEXT, lambda: open(tachandler.logfile, 'r').read()))
- self.addOnException(addLogFile)
+ # Basically impossible to do this w/ TrialTestCase. But it would be
+ # really nice to keep it.
+ #
+ # def addLogFile(exc_info):
+ # self.addDetail(
+ # 'xmlrpc-log-file',
+ # Content(UTF8_TEXT, lambda: open(tachandler.logfile, 'r').read())
+ # self.addOnException(addLogFile)
self.addCleanup(tachandler.tearDown)
return tachandler
@@ -527,7 +530,7 @@
:return: The build id returned by the slave.
"""
if build_id is None:
- build_id = self.getUniqueString()
+ build_id = 'random-build-id'
tachandler = self.getServerSlave()
chroot_file = 'fake-chroot'
dsc_file = 'thing'
@@ -537,10 +540,33 @@
build_id, 'debian', chroot_file, {'.dsc': dsc_file},
{'ogrecomponent': 'main'})
+
+class TestSlave(TrialTestCase):
+ """
+ Integration tests for BuilderSlave that verify how it works against a
+ real slave server.
+ """
+
+ layer = TwistedLayer
+
+ def setUp(self):
+ super(TestSlave, self).setUp()
+ self.slave_helper = SlaveTestHelpers()
+ self.slave_helper.setUp()
+ self.addCleanup(self.slave_helper.cleanUp)
+
+ # XXX: JonathanLange 2010-09-20 bug=643521: There are also tests for
+ # BuilderSlave in buildd-slave.txt and in other places. The tests here
+ # ought to become the canonical tests for BuilderSlave vs running buildd
+ # XML-RPC server interaction.
+
+ # The URL for the XML-RPC service set up by `BuilddSlaveTestSetup`.
+ TEST_URL = 'http://localhost:8221/rpc/'
+
def test_abort(self):
- slave = self.getClientSlave()
+ slave = self.slave_helper.getClientSlave()
# We need to be in a BUILDING state before we can abort.
- self.triggerGoodBuild(slave)
+ self.slave_helper.triggerGoodBuild(slave)
result = slave.abort()
self.assertEqual(result, BuilderStatus.ABORTING)
@@ -549,12 +575,12 @@
# valid chroot & filemaps works and returns a BuilderStatus of
# BUILDING.
build_id = 'some-id'
- slave = self.getClientSlave()
- result = self.triggerGoodBuild(slave, build_id)
+ slave = self.slave_helper.getClientSlave()
+ result = self.slave_helper.triggerGoodBuild(slave, build_id)
self.assertEqual([BuilderStatus.BUILDING, build_id], result)
def test_clean(self):
- slave = self.getClientSlave()
+ slave = self.slave_helper.getClientSlave()
# XXX: JonathanLange 2010-09-21: Calling clean() on the slave requires
# it to be in either the WAITING or ABORTED states, and both of these
# states are very difficult to achieve in a test environment. For the
@@ -564,15 +590,15 @@
def test_echo(self):
# Calling 'echo' contacts the server which returns the arguments we
# gave it.
- self.getServerSlave()
- slave = self.getClientSlave()
+ self.slave_helper.getServerSlave()
+ slave = self.slave_helper.getClientSlave()
result = slave.echo('foo', 'bar', 42)
self.assertEqual(['foo', 'bar', 42], result)
def test_info(self):
# Calling 'info' gets some information about the slave.
- self.getServerSlave()
- slave = self.getClientSlave()
+ self.slave_helper.getServerSlave()
+ slave = self.slave_helper.getClientSlave()
result = slave.info()
# We're testing the hard-coded values, since the version is hard-coded
# into the remote slave, the supported build managers are hard-coded
@@ -588,17 +614,17 @@
def test_initial_status(self):
# Calling 'status' returns the current status of the slave. The
# initial status is IDLE.
- self.getServerSlave()
- slave = self.getClientSlave()
+ self.slave_helper.getServerSlave()
+ slave = self.slave_helper.getClientSlave()
status = slave.status()
self.assertEqual([BuilderStatus.IDLE, ''], status)
def test_status_after_build(self):
# Calling 'status' returns the current status of the slave. After a
# build has been triggered, the status is BUILDING.
- slave = self.getClientSlave()
+ slave = self.slave_helper.getClientSlave()
build_id = 'status-build-id'
- self.triggerGoodBuild(slave, build_id)
+ self.slave_helper.triggerGoodBuild(slave, build_id)
status = slave.status()
self.assertEqual([BuilderStatus.BUILDING, build_id], status[:2])
[log_file] = status[2:]
@@ -606,15 +632,89 @@
def test_ensurepresent_not_there(self):
# ensurepresent checks to see if a file is there.
- self.getServerSlave()
- slave = self.getClientSlave()
+ self.slave_helper.getServerSlave()
+ slave = self.slave_helper.getClientSlave()
result = slave.ensurepresent('blahblah', None, None, None)
self.assertEqual([False, 'No URL'], result)
def test_ensurepresent_actually_there(self):
# ensurepresent checks to see if a file is there.
- tachandler = self.getServerSlave()
- slave = self.getClientSlave()
- self.makeCacheFile(tachandler, 'blahblah')
+
+ tachandler = self.slave_helper.getServerSlave()
+ slave = self.slave_helper.getClientSlave()
+ self.slave_helper.makeCacheFile(tachandler, 'blahblah')
result = slave.ensurepresent('blahblah', None, None, None)
self.assertEqual([True, 'No URL'], result)
+
+ def test_resumeHost_success(self):
+ # On a successful resume resume() fires the returned deferred
+ # callback with 'None'.
+ self.slave_helper.getServerSlave()
+ slave = self.slave_helper.getClientSlave()
+
+ # The configuration testing command-line.
+ self.assertEqual(
+ 'echo %(vm_host)s', config.builddmaster.vm_resume_command)
+
+ # On success the response is None.
+ def check_resume_success(response):
+ out, err, code = response
+ self.assertEqual(os.EX_OK, code)
+ self.assertEqual("%s\n" % slave.vm_host, out)
+ d = slave.resume()
+ d.addBoth(check_resume_success)
+ return d
+
+ def test_resumeHost_failure(self):
+ # On a failed resume, 'resumeHost' fires the returned deferred
+ # errorback with the `ProcessTerminated` failure.
+ self.slave_helper.getServerSlave()
+ slave = self.slave_helper.getClientSlave()
+
+ # Override the configuration command-line with one that will fail.
+ failed_config = """
+ [builddmaster]
+ vm_resume_command: test "%(vm_host)s = 'no-sir'"
+ """
+ config.push('failed_resume_command', failed_config)
+ self.addCleanup(config.pop, 'failed_resume_command')
+
+ # On failures, the response is a twisted `Failure` object containing
+ # a tuple.
+ def check_resume_failure(failure):
+ out, err, code = failure.value
+ # The process will exit with a return code of "1".
+ self.assertEqual(code, 1)
+ d = slave.resume()
+ d.addBoth(check_resume_failure)
+ return d
+
+ def test_resumeHost_timeout(self):
+ # On a resume timeouts, 'resumeHost' fires the returned deferred
+ # errorback with the `TimeoutError` failure.
+ self.slave_helper.getServerSlave()
+ slave = self.slave_helper.getClientSlave()
+
+ # Override the configuration command-line with one that will timeout.
+ timeout_config = """
+ [builddmaster]
+ vm_resume_command: sleep 5
+ socket_timeout: 1
+ """
+ config.push('timeout_resume_command', timeout_config)
+ self.addCleanup(config.pop, 'timeout_resume_command')
+
+ # On timeouts, the response is a twisted `Failure` object containing
+ # a `TimeoutError` error.
+ def check_resume_timeout(failure):
+ self.assertIsInstance(failure, Failure)
+ out, err, code = failure.value
+ self.assertEqual(code, signal.SIGKILL)
+ clock = Clock()
+ d = slave.resume(clock=clock)
+ # Move the clock beyond the socket_timeout but earlier than the
+ # sleep 5. This stops the test having to wait for the timeout.
+ # Fast tests FTW!
+ clock.advance(2)
+ d.addBoth(check_resume_timeout)
+ return d
=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py 2010-09-20 10:21:40 +0000
+++ lib/lp/buildmaster/tests/test_manager.py 2010-09-22 17:28:52 +0000
@@ -124,73 +124,6 @@
self.assertEqual(['', '', os.EX_OK], self.slave.resume())
self.assertTrue(self.slave.resume_requested)
- def test_resumeHost_success(self):
- # On a successful resume resumeHost() fires the returned deferred
- # callback with 'None'.
-
- # The configuration testing command-line.
- self.assertEqual(
- 'echo %(vm_host)s', config.builddmaster.vm_resume_command)
-
- # On success the response is None.
- def check_resume_success(response):
- out, err, code = response
- self.assertEqual(os.EX_OK, code)
- self.assertEqual("%s\n" % self.slave.vm_host, out)
- d = self.slave.resumeSlave()
- d.addBoth(check_resume_success)
- return d
-
- def test_resumeHost_failure(self):
- # On a failed resume, 'resumeHost' fires the returned deferred
- # errorback with the `ProcessTerminated` failure.
-
- # Override the configuration command-line with one that will fail.
- failed_config = """
- [builddmaster]
- vm_resume_command: test "%(vm_host)s = 'no-sir'"
- """
- config.push('failed_resume_command', failed_config)
- self.addCleanup(config.pop, 'failed_resume_command')
-
- # On failures, the response is a twisted `Failure` object containing
- # a tuple.
- def check_resume_failure(failure):
- out, err, code = failure.value
- # The process will exit with a return code of "1".
- self.assertEqual(code, 1)
- d = self.slave.resumeSlave()
- d.addBoth(check_resume_failure)
- return d
-
- def test_resumeHost_timeout(self):
- # On a resume timeouts, 'resumeHost' fires the returned deferred
- # errorback with the `TimeoutError` failure.
-
- # Override the configuration command-line with one that will timeout.
- timeout_config = """
- [builddmaster]
- vm_resume_command: sleep 5
- socket_timeout: 1
- """
- config.push('timeout_resume_command', timeout_config)
- self.addCleanup(config.pop, 'timeout_resume_command')
-
- # On timeouts, the response is a twisted `Failure` object containing
- # a `TimeoutError` error.
- def check_resume_timeout(failure):
- self.assertIsInstance(failure, Failure)
- out, err, code = failure.value
- self.assertEqual(code, signal.SIGKILL)
- clock = Clock()
- d = self.slave.resumeSlave(clock=clock)
- # Move the clock beyond the socket_timeout but earlier than the
- # sleep 5. This stops the test having to wait for the timeout.
- # Fast tests FTW!
- clock.advance(2)
- d.addBoth(check_resume_timeout)
- return d
-
class TestingXMLRPCProxy:
"""This class mimics a twisted XMLRPC Proxy class."""
Follow ups
