launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01187
[Merge] lp:~jml/launchpad/better-slave-mock into lp:launchpad/devel
Jonathan Lange has proposed merging lp:~jml/launchpad/better-slave-mock into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
This branch moves BlockingProxy from lp.codehosting.vfs and into lp.services.twistedsupport.xmlrpc. It then rejiggers the BuilderSlave class to use a callRemote style proxy rather than a getattr style one.
The branch also adds DeferredBlockingProxy, which is much like BlockingProxy, except its guaranteed to return Deferreds. It then changes codehosting to use this DeferredBlockingProxy. The net effect is to move generic code deeper down the stack, and make the codehosting vfs more focused on being a codehosting vfs.
I was going to actually improve the mocks, but I figured that would be easier to do when driven by actual tests.
--
https://code.launchpad.net/~jml/launchpad/better-slave-mock/+merge/36417
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/better-slave-mock into lp:launchpad/devel.
=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py 2010-09-21 18:43:27 +0000
+++ lib/lp/buildmaster/model/builder.py 2010-09-23 08:08:57 +0000
@@ -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.xmlrpc import BlockingProxy
# XXX Michael Nelson 2010-01-13 bug=491330
# These dependencies on soyuz will be removed when getBuildRecords()
# is moved.
@@ -137,43 +138,55 @@
# should make a test double that doesn't do any XML-RPC and can be used to
# make testing easier & tests faster.
- def __init__(self, urlbase, vm_host):
- """Initialise a Server with specific parameter to our buildfarm."""
- self.vm_host = vm_host
- self.urlbase = urlbase
- rpc_url = urlappend(urlbase, "rpc")
- self._server = xmlrpclib.Server(
+ def __init__(self, proxy, file_cache_url, vm_host):
+ """Initialise a Server with specific parameter to our buildfarm.
+
+ :param proxy: An XML-RPC proxy, implementing 'callRemote'. It must
+ support passing and returning None objects.
+ :param file_cache_url: The URL of the file cache.
+ :param vm_host: The VM host to use when resuming.
+ """
+ self._vm_host = vm_host
+ self._file_cache_url = file_cache_url
+ self._server = proxy
+
+ @classmethod
+ def makeBlockingSlave(cls, url_base, vm_host):
+ rpc_url = urlappend(url_base, 'rpc')
+ server_proxy = xmlrpclib.ServerProxy(
rpc_url, transport=TimeoutTransport(), allow_none=True)
+ file_cache_url = urlappend(url_base, 'filecache')
+ return cls(BlockingProxy(server_proxy), file_cache_url, vm_host)
def abort(self):
"""Abort the current build."""
- return self._server.abort()
+ return self._server.callRemote('abort')
def clean(self):
"""Clean up the waiting files and reset the slave's internal state."""
- return self._server.clean()
+ return self._server.callRemote('clean')
def echo(self, *args):
"""Echo the arguments back."""
- return self._server.echo(*args)
+ return self._server.callRemote('echo', *args)
def info(self):
"""Return the protocol version and the builder methods supported."""
- return self._server.info()
+ return self._server.callRemote('info')
def status(self):
"""Return the status of the build daemon."""
- return self._server.status()
+ return self._server.callRemote('status')
def ensurepresent(self, sha1sum, url, username, password):
"""Attempt to ensure the given file is present."""
- return self._server.ensurepresent(sha1sum, url, username, password)
+ return self._server.callRemote(
+ 'ensurepresent', sha1sum, url, username, password)
def getFile(self, sha_sum):
"""Construct a file-like object to return the named file."""
- filelocation = "filecache/%s" % sha_sum
- fileurl = urlappend(self.urlbase, filelocation)
- return urllib2.urlopen(fileurl)
+ file_url = urlappend(self._file_cache_url, sha_sum)
+ return urllib2.urlopen(file_url)
def resume(self):
"""Resume a virtual builder.
@@ -188,7 +201,7 @@
# 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}
+ 'vm_host': self._vm_host}
resume_argv = resume_command.split()
resume_process = subprocess.Popen(
resume_argv, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
@@ -203,9 +216,10 @@
:param libraryfilealias: An `ILibraryFileAlias`.
"""
url = libraryfilealias.http_url
- logger.debug("Asking builder on %s to ensure it has file %s "
- "(%s, %s)" % (self.urlbase, libraryfilealias.filename,
- url, libraryfilealias.content.sha1))
+ logger.debug(
+ "Asking builder on %s to ensure it has file %s (%s, %s)" % (
+ self._file_cache_url, libraryfilealias.filename, url,
+ libraryfilealias.content.sha1))
self.sendFileToSlave(libraryfilealias.content.sha1, url)
def sendFileToSlave(self, sha1, url, username="", password=""):
@@ -226,8 +240,8 @@
the build job type.
"""
try:
- return self._server.build(
- buildid, builder_type, chroot_sha1, filemap, args)
+ return self._server.callRemote(
+ 'build', buildid, builder_type, chroot_sha1, filemap, args)
except xmlrpclib.Fault, info:
raise BuildSlaveFailure(info)
@@ -444,7 +458,7 @@
# the slave object, which is usually an XMLRPC client, with a
# stub object that removes the need to actually create a buildd
# slave in various states - which can be hard to create.
- return BuilderSlave(self.url, self.vm_host)
+ return BuilderSlave.makeBlockingSlave(self.url, self.vm_host)
def setSlaveForTesting(self, proxy):
"""See IBuilder."""
=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py 2010-09-22 11:26:19 +0000
+++ lib/lp/buildmaster/tests/test_builder.py 2010-09-23 08:08:57 +0000
@@ -501,7 +501,7 @@
Points to a fixed URL that is also used by `BuilddSlaveTestSetup`.
"""
- return BuilderSlave(self.TEST_URL, 'vmhost')
+ return BuilderSlave.makeBlockingSlave(self.TEST_URL, 'vmhost')
def makeCacheFile(self, tachandler, filename):
"""Make a cache file available on the remote slave.
=== modified file 'lib/lp/codehosting/inmemory.py'
--- lib/lp/codehosting/inmemory.py 2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/inmemory.py 2010-09-23 08:08:57 +0000
@@ -16,6 +16,7 @@
escape,
unescape,
)
+from twisted.internet import defer
from zope.component import (
adapter,
getSiteManager,
@@ -913,8 +914,11 @@
def __init__(self, endpoint):
self.endpoint = endpoint
- def callRemote(self, method_name, *args):
+ def _callRemote(self, method_name, *args):
result = getattr(self.endpoint, method_name)(*args)
if isinstance(result, Fault):
raise result
return result
+
+ def callRemote(self, method_name, *args):
+ return defer.maybeDeferred(self._callRemote, method_name, *args)
=== modified file 'lib/lp/codehosting/vfs/__init__.py'
--- lib/lp/codehosting/vfs/__init__.py 2010-08-21 07:49:13 +0000
+++ lib/lp/codehosting/vfs/__init__.py 2010-09-23 08:08:57 +0000
@@ -5,7 +5,6 @@
__all__ = [
'AsyncLaunchpadTransport',
- 'BlockingProxy',
'branch_id_to_path',
'BranchFileSystemClient',
'get_lp_server',
@@ -25,7 +24,6 @@
make_branch_mirrorer,
)
from lp.codehosting.vfs.branchfsclient import (
- BlockingProxy,
BranchFileSystemClient,
)
=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py 2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/vfs/branchfs.py 2010-09-23 08:08:57 +0000
@@ -102,7 +102,6 @@
)
from lp.codehosting.bzrutils import get_stacked_on_url
from lp.codehosting.vfs.branchfsclient import (
- BlockingProxy,
BranchFileSystemClient,
)
from lp.codehosting.vfs.transport import (
@@ -112,7 +111,10 @@
get_readonly_transport,
TranslationError,
)
-from lp.services.twistedsupport.xmlrpc import trap_fault
+from lp.services.twistedsupport.xmlrpc import (
+ DeferredBlockingProxy,
+ trap_fault,
+ )
class BadUrl(Exception):
@@ -189,7 +191,7 @@
def get_ro_server():
"""Get a Launchpad internal server for scanning branches."""
proxy = xmlrpclib.ServerProxy(config.codehosting.codehosting_endpoint)
- codehosting_endpoint = BlockingProxy(proxy)
+ codehosting_endpoint = DeferredBlockingProxy(proxy)
branch_transport = get_readonly_transport(
get_transport(config.codehosting.internal_branch_by_id_root))
return LaunchpadInternalServer(
@@ -212,7 +214,7 @@
return DirectDatabaseLaunchpadServer('lp-internal:///', transport)
else:
proxy = xmlrpclib.ServerProxy(config.codehosting.codehosting_endpoint)
- codehosting_endpoint = BlockingProxy(proxy)
+ codehosting_endpoint = DeferredBlockingProxy(proxy)
return LaunchpadInternalServer(
'lp-internal:///', codehosting_endpoint, transport)
@@ -719,7 +721,7 @@
codehosting_client = xmlrpclib.ServerProxy(codehosting_endpoint_url)
lp_server = LaunchpadServer(
- BlockingProxy(codehosting_client), user_id, branch_transport,
+ DeferredBlockingProxy(codehosting_client), user_id, branch_transport,
seen_new_branch_hook)
return lp_server
=== modified file 'lib/lp/codehosting/vfs/branchfsclient.py'
--- lib/lp/codehosting/vfs/branchfsclient.py 2010-08-10 03:24:22 +0000
+++ lib/lp/codehosting/vfs/branchfsclient.py 2010-09-23 08:08:57 +0000
@@ -8,7 +8,6 @@
__metaclass__ = type
__all__ = [
- 'BlockingProxy',
'BranchFileSystemClient',
'NotInCache',
]
@@ -20,15 +19,6 @@
from lp.code.interfaces.codehosting import BRANCH_TRANSPORT
-class BlockingProxy:
-
- def __init__(self, proxy):
- self._proxy = proxy
-
- def callRemote(self, method_name, *args):
- return getattr(self._proxy, method_name)(*args)
-
-
class NotInCache(Exception):
"""Raised when we try to get a path from the cache that's not present."""
@@ -50,7 +40,8 @@
seen_new_branch_hook=None, _now=time.time):
"""Construct a caching codehosting_endpoint.
- :param codehosting_endpoint: An XML-RPC proxy that implements callRemote.
+ :param codehosting_endpoint: An XML-RPC proxy that implements
+ callRemote and returns Deferreds.
:param user_id: The database ID of the user who will be making these
requests. An integer.
:param expiry_time: If supplied, only cache the results of
@@ -116,9 +107,8 @@
:param branch_path: The path to the branch to create.
:return: A `Deferred` that fires the ID of the created branch.
"""
- return defer.maybeDeferred(
- self._codehosting_endpoint.callRemote, 'createBranch',
- self._user_id, branch_path)
+ return self._codehosting_endpoint.callRemote(
+ 'createBranch', self._user_id, branch_path)
def branchChanged(self, branch_id, stacked_on_url, last_revision_id,
control_string, branch_string, repository_string):
@@ -126,8 +116,7 @@
:param branch_id: The database ID of the branch.
"""
- return defer.maybeDeferred(
- self._codehosting_endpoint.callRemote,
+ return self._codehosting_endpoint.callRemote(
'branchChanged', self._user_id, branch_id, stacked_on_url,
last_revision_id, control_string, branch_string,
repository_string)
@@ -137,8 +126,7 @@
try:
return defer.succeed(self._getFromCache(path))
except NotInCache:
- deferred = defer.maybeDeferred(
- self._codehosting_endpoint.callRemote,
+ deferred = self._codehosting_endpoint.callRemote(
'translatePath', self._user_id, path)
deferred.addCallback(self._addToCache, path)
return deferred
=== modified file 'lib/lp/codehosting/vfs/tests/test_transport.py'
--- lib/lp/codehosting/vfs/tests/test_transport.py 2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/vfs/tests/test_transport.py 2010-09-23 08:08:57 +0000
@@ -19,7 +19,7 @@
from lp.codehosting.inmemory import InMemoryFrontend
from lp.codehosting.tests.helpers import TestResultWrapper
from lp.codehosting.vfs.branchfs import LaunchpadInternalServer
-from lp.codehosting.vfs.branchfsclient import BlockingProxy
+from lp.services.twistedsupport.xmlrpc import DeferredBlockingProxy
class TestingServer(LaunchpadInternalServer):
@@ -44,7 +44,8 @@
# unreliable for tests that involve particular errors.
LaunchpadInternalServer.__init__(
self, 'lp-testing-%s:///' % id(self),
- BlockingProxy(branchfs), LocalTransport(local_path_to_url('.')))
+ DeferredBlockingProxy(branchfs),
+ LocalTransport(local_path_to_url('.')))
self._chroot_servers = []
def get_bogus_url(self):
=== modified file 'lib/lp/services/twistedsupport/tests/test_xmlrpc.py'
--- lib/lp/services/twistedsupport/tests/test_xmlrpc.py 2010-08-20 20:31:18 +0000
+++ lib/lp/services/twistedsupport/tests/test_xmlrpc.py 2010-09-23 08:08:57 +0000
@@ -10,7 +10,12 @@
from twisted.python.failure import Failure
from twisted.trial.unittest import TestCase
-from lp.services.twistedsupport.xmlrpc import trap_fault
+from lp.services.twistedsupport import extract_result
+from lp.services.twistedsupport.xmlrpc import (
+ BlockingProxy,
+ DeferredBlockingProxy,
+ trap_fault,
+ )
from lp.services.xmlrpc import LaunchpadFault
@@ -86,5 +91,47 @@
self.assertEqual(TestFaultOne.msg_template, fault.faultString)
+class TestBlockingProxy(TestCase):
+
+ def test_callRemote_calls_attribute(self):
+ class ExampleProxy:
+ def ok(self, a, b, c=None):
+ return (c, b, a)
+ proxy = BlockingProxy(ExampleProxy())
+ result = proxy.callRemote('ok', 2, 3, c=8)
+ self.assertEqual((8, 3, 2), result)
+
+ def test_callRemote_reraises_attribute(self):
+ class ExampleProxy:
+ def not_ok(self, a, b, c=None):
+ raise RuntimeError((c, b, a))
+ proxy = BlockingProxy(ExampleProxy())
+ error = self.assertRaises(
+ RuntimeError, proxy.callRemote, 'not_ok', 2, 3, c=8)
+ self.assertEqual(str(error), str((8, 3, 2)))
+
+
+class TestDeferredBlockingProxy(TestCase):
+
+ def test_callRemote_calls_attribute(self):
+ class ExampleProxy:
+ def ok(self, a, b, c=None):
+ return (c, b, a)
+ proxy = DeferredBlockingProxy(ExampleProxy())
+ d = proxy.callRemote('ok', 2, 3, c=8)
+ result = extract_result(d)
+ self.assertEqual((8, 3, 2), result)
+
+ def test_callRemote_traps_failure(self):
+ class ExampleProxy:
+ def not_ok(self, a, b, c=None):
+ raise RuntimeError((c, b, a))
+ proxy = DeferredBlockingProxy(ExampleProxy())
+ d = proxy.callRemote('not_ok', 2, 3, c=8)
+ error = self.assertRaises(RuntimeError, extract_result, d)
+ self.assertEqual(str(error), str((8, 3, 2)))
+
+
+
def test_suite():
return unittest.TestLoader().loadTestsFromName(__name__)
=== modified file 'lib/lp/services/twistedsupport/xmlrpc.py'
--- lib/lp/services/twistedsupport/xmlrpc.py 2010-04-09 12:39:23 +0000
+++ lib/lp/services/twistedsupport/xmlrpc.py 2010-09-23 08:08:57 +0000
@@ -5,12 +5,52 @@
__metaclass__ = type
__all__ = [
+ 'BlockingProxy',
+ 'DeferredBlockingProxy',
'trap_fault',
]
+from twisted.internet import defer
from twisted.web.xmlrpc import Fault
+class BlockingProxy:
+ """Make an xmlrpclib.ServerProxy behave like a Twisted XML-RPC proxy.
+
+ This is useful for writing code that needs to work in both a synchronous
+ and asynchronous fashion.
+
+ Also, some people prefer the callRemote style of invocation, which is more
+ explicit.
+ """
+
+ def __init__(self, proxy):
+ """Construct a `BlockingProxy`.
+
+ :param proxy: An xmlrpclib.ServerProxy.
+ """
+ self._proxy = proxy
+
+ def callRemote(self, method_name, *args, **kwargs):
+ return getattr(self._proxy, method_name)(*args, **kwargs)
+
+
+class DeferredBlockingProxy(BlockingProxy):
+ """Make an xmlrpclib.ServerProxy behave more like a Twisted XML-RPC proxy.
+
+ This is almost exactly like 'BlockingProxy', except that this returns
+ Deferreds. It is guaranteed to be exactly as synchronous as the passed-in
+ proxy. That means if you pass in a normal xmlrpclib proxy you ought to be
+ able to use `lp.services.twistedsupport.extract_result` to get the result.
+ """
+
+ def callRemote(self, method_name, *args, **kwargs):
+ return defer.maybeDeferred(
+ super(DeferredBlockingProxy, self).callRemote,
+ method_name, *args, **kwargs)
+
+
+
def trap_fault(failure, *fault_classes):
"""Trap a fault, based on fault code.
