It should be easy to use AMP to perform remote procedure calls

[Free Ekanayaka]

In order to facilitate switching our inter-processes communication
system from DBus to AMP [1], we need an easy way to map AMP commands
issued by remote broker clients (say the monitor) to the services
provided by the broker.

The attached branch adds a MethodCallProtocol that can be used for such
purpose.

[1] http://twistedmatrix.com/documents/9.0.0/api/twisted.protocols.amp.html

 affects landscape-client
 status inprogress
 importance low
 assignee free.ekanayaka
 milestone 1.4.3

[Free Ekanayaka (free.ekanayaka)]

Note that the Twisted amp module is not available on Dapper. However, copying the amp.py file from Hardy in the twisted tree works (all Landscape client AMP tests pass), so I guess we can either package a python-twisted-amp package for dapper sporting the amp.py file, or include the amp.py file in the landscape-client package for Dapper. The latter is probably safer and easier.

[Jamu Kakar (jkakar)]

[1]

+ """A bpickle-compatbile argument."""

s/compatbile/compatible/

[2]

+ method_func = getattr(self.factory.object, method)
+ method_args = args[:]
+ method_kwargs = kwargs.copy()

I don't understand why args and kwargs are copied here. Is it
necessary? Also, I removed the copying behaviour and no tests
failed.

[3]

+ def __init__(self, reactor, socket):

Please call this socket_path to be extra clear about its purpose.

[4]

+ deferred = Deferred()
+ factory = self.factory(self._reactor, deferred.callback)
+ self._reactor.connectUNIX(self._socket, factory)
+ deferred.addCallback(self._connection_made)
+ return deferred

This means we're opening a connection per remote object, right? And
each object will need it's own socket/path, right?

[5]

+ A L{RemoteObject} can send L{MethodCall}s without arguments and withj

s/withj/with/

This appears a few times.

It's looking good. I'm a bit concerned about #4, if I've understood
the code correctly.

[Jamu Kakar (jkakar)]

[6]

Thinking about it more I understand that my previous concerns about
#4 are not an issue. What could be an issue, though, is the 64k
limit in AMP messages. We might have to change the internals of the
protocol to chunk large messages.

+1 on landing this and worrying about #6 in the future.

[Free Ekanayaka (free.ekanayaka)]

Thanks Jamu!

[1], [2], [3], [5]

Fixed. (It isn't indeed necessary to copy args and kwargs)

[4]

As discussed on IRC, I initially went for a single-socket approach, but later realized that the monitor and the manager needs to respond to the watchdog pings and exit calls over AMP, and hence should listen to a socket as well. It looks safer to have the broker communicate with the manager and the monitor over the same socket the watchdog uses, so if case of problems the watchdog can restart the failing service.

[6]

I didn't know about this limitation, we'll have to split large messages in chunks, as you suggest.

[Kapil Thangavelu (hazmat)]

Looks good to me +1. I dig the generic method call api over amp. diffs below are my suggested changes against the branch.

[1] doc string typo.
--- landscape/lib/amp.py 2010-01-18 08:24:51 +0000
+++ landscape/lib/amp.py 2010-01-19 17:57:36 +0000
@@ -82,7 +82,7 @@

         @param method: The name of the remote method to invoke.
         @param args: The positional arguments to pass to the remote method.
- @param args: The keyword arguments to pass to the remote method.
+ @param kwargs: The keyword arguments to pass to the remote method.
         """
         return self.callRemote(MethodCall,
                                method=method, args=args, kwargs=kwargs)

[2] There's another argument copy. I removed it and the tests run fine.

@@ -147,8 +147,8 @@

         def send_method_call(*args, **kwargs):
             result = self._protocol.send_method_call(method=method,
- args=args[:],
- kwargs=kwargs.copy())
+ args=args,
+ kwargs=kwargs)
             return result.addCallback(lambda response: response["result"])

[3] It would be nice but certainly not required to see a unit test with an exception in the remote method implementation, being caught by the client. Mostly as an example. Triggering it by hand i get an UnknownAmpRemoteError, which i guess is to be expected.

[Free Ekanayaka (free.ekanayaka)]

[1], [2]

Fixed.

[3]

It's a very good point. I wrapped the method call in receive_method_call with a try/except block, and now the protocol gracefully fails with a MethodCallError if the remote objects raises an exception. Thanks for bringing this up.

[Jamu Kakar (jkakar)]

This is not easy to test in staging, removing the needs-testing tag.