Re: [RFC] Xend XML-RPC Refactoring

[Anthony Liguori <aliguori@us.ibm.com>]

Ewan Mellor wrote:
> Anthony, I've reviewed your XML-RPC patches.  It all looks good, so I'm in
> favour of putting them in straight away.
Thanks!
> I'd like to get the second patch in
> straight away as well as the first, that is (for those watching at home) to
> change xm to using XML-RPC, because I would like to deprecate the old
> protocol sooner, rather than later.  Making the XML-RPC interface the primary
> control path for Xen 3.0.2 will encourage third-parties to code to that rather
> than to the s-expression protocol, and that's a good thing.
>   
Okay.  So I'm going to take this to mean that we would deprecate 
S-Expression/HTTP for 3.0.2 and remove it in 3.0.3?  That would be 
really nice.
>>  xroot = XendRoot.instance()
>> @@ -113,4 +114,5 @@
>>          path = xroot.get_xend_unix_path()
>>          log.info('unix path=' + path)
>>          servers.add(UnixHttpServer(path=path, root=root))
>> +    servers.add(XMLRPCServer())
>>      return servers
>>     
>
> It would be prudent to make it possible to turn the XMLRPCServer off, just as we
> can turn the other servers off.
>   
Yeah, this is certainly sane.
>> +class ServerProxy(xmlrpclib.ServerProxy):
>> +    def __init__(self, uri, transport=None, encoding=None, verbose=0,
>> +                 allow_none=1):
>> +        if transport == None:
>> +            protocol = uri.split(':')[0]
>> +            if protocol == 'httpu':
>> +                uri = 'http:' + ':'.join(uri.split(':')[1:])
>> +                transport = UnixTransport()
>>     
>
> How about
>
> (protocol, rest) = uri.split(':', 1)
> if protocol == 'httpu':
>     uri = 'http:' + rest
>     transport = UnixTransport()
>   
Haven't seen that before.  Quite useful :-)
>> +from xen.xend import (XendDomain, XendDomainInfo, XendNode,
>> +                      XendLogging, XendDmesg)
>>     
>
> This syntax is only in Python 2.4+, so we have to use
>
> from xen.xend import XendDomain, XendDomainInfo, XendNode, \
>                      XendLogging, XendDmesg
>   
Yeah, thanks for the catch.
>> [Snip]
>>
>> +def get_log():
>> +    f = open(XendLogging.getLogFilename(), 'r')
>> +    ret = f.read()
>> +    f.close()
>> +    return ret
>>     
>
> This will leak a file descriptor if f.read() throws an exception.  We need
>
> f = open(XendLogging.getLogFIlename(), 'r')
> try:
>     return f.read()
> finally:
>     f.close()
>   
Yeah, again, thanks for the catch :-)
>> +    def run(self):
>> +        self.server = UnixXMLRPCServer("/var/run/xend-xmlrpc.sock")
>>     
>
> This filename constant needs to go somewhere else; XendClient is probably the
> best place for it.  (I've never liked the way that files in xen/xm have to go
> poking around in xen/xend for things that they need, but I don't have the
> stomach for a major file rearrangement at the moment, so XendClient will have
> to do.)
>   
Okay.  I'm fine with that.
>> +        # Functions in XendNode and XendDmesg
>> +        for type, lst, n in [(XendNode, ['info',
>> 'cpu_bvt_slice_set'], 'node'),
>> +                             (XendDmesg, ['info', 'clear'],
>> 'node.dmesg')]:
>> +            inst = type.instance()
>> +            for name in lst:
>> +                self.server.register_function(getattr(inst, name),
>> +                                              "xend.%s.%s" % (n,
>> name))
>> +
>>     
>
> This is all a bit skanky, and could be easily cleaned up by introducing a
> naming convention for XendDomain, XendNode, etc.
Yes, skanky is very much an understatement.  I found XendDomain and 
XendDomainInfo particularly nasty to work with considering that they use 
different naming conventions too.
>   How about if we prefixed
> every function that we wish to expose to the messaging layer with
> "public_"?  So for example XendDomainInfo.send_sysrq would be named
> public_send_sysrq instead.  Then, we could use that to guide the
> function registration, rather than having exclude lists and inline lists
> of callable methods.
>   
Isn't using an underscore a convention for making methods private in 
Python?  I think, at least, pydoc ignores functions that start with an 
underscore.
> This would make your patch a bit more invasive, in that the renaming
> would cause it to touch more files, but I don't have a problem with that
> -- I think the change is justified.
>   
Yes, that would make things much nicer so I'm happy to do it.  I clearly 
wanted to avoid making more changes than necessary at first :-)
>> [Snip the rest of this patch]
>>
>> # HG changeset patch
>> # User anthony@rhesis.austin.ibm.com
>> # Node ID 951f0d589164e0cffc785cf23d82118b3e0b0495
>> # Parent  095ac0d95d9cc154ec8fc3dba1a67f02f79771ac
>> This changeset is a major refactoring of XendClient to use the XML-RPC
>> transport for Xend and also to make a few changes to xm so that it knows about
>> the new Exception types.
>>
>> xm-test has been passing with this changeset for the past couple of weeks.
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>> diff -r 095ac0d95d9c -r 951f0d589164 tools/python/xen/xend/XendClient.py
>> --- a/tools/python/xen/xend/XendClient.py	Tue Feb 28 22:08:47 2006
>> +++ b/tools/python/xen/xend/XendClient.py	Tue Feb 28 22:10:14 2006
>>
>> [Snip lots]
>>  
>>      def xend_node_get_dmesg(self):
>> -            return self.xendGet(self.nodeurl('dmesg'))
>> +        return self.srv.xend.node.dmesg.info()
>>     
>
> What you've done here is slipped the new XML-RPC layer under the existing
> XendClient API.  I don't think that we need to support that API at all.  All
> the functions here just turn into stubs onto ServerProxy, and I don't think
> that those stubs buy us anything.
>   
Nope.  Just compatibility.  I know there are people using the XendClient 
API.  I also know we've never declared that to be a fixed API.  I didn't 
want to be the one to break it though :-)
> What I'd do here is just this:
>
> XendClient.py:
>
> XEND_XMLRPC_UNIX_SOCKET = '/var/run/xend-xmlrpc.sock'
>
> server = ServerProxy('httpu://' + XEND_XMLRPC_UNIX_SOCKET) 
>
> and then push _all_ the other code into the client (main.py, create.py, etc).
> So these would change from
>
>     from xen.xend.XendClient import server
>     print server.xend_node_get_dmesg()
>
> to
>
>     from xen.xend.XendClient import server
>     print server.xend.node.dmesg.info()
>
> XendClient.py just dissolves into nothingness, and the clients are no more
> complicated.
>   
Yes, this would be great.  If we don't mind breaking XendClient, I'm 
happy to do it.
>>      def xend_domain_sysrq(self, id, key):
>> -        return self.xendPost(self.domainurl(id),
>> -                             {'op'      : 'sysrq',
>> -                              'key'     : key})
>> +        return self.srv.xend.domain.send_sysrq(self.lookup(id), key)
>>     
>
> For calls like this, where we currently look up the domain ID before making the
> real call, I would push the lookup to the server; there's no need for two
> roundtrips for this kind of call.  I think that if you remove the lookup call,
> it will just work now in any case, because XMLRPCServer.lookup uses
> XendDomain.domain_lookup_by_name_or_id.
>   
Yeah, lookup() is a bit of a hack.  I can go through and audit the 
functions to make sure that they all accept either names or IDs.
>> +     # FIXME
>> +     def xend_domain_restore(self, filename):
>> +        return self.srv.xend.domain.restore(filename)
>>     
>
> What does this FIXME mean?
>   
Sorry, that shouldn't have been there.  I already fixed that one :-)
>> diff -r 095ac0d95d9c -r 951f0d589164 tools/python/xen/xend/XendDomain.py
>> --- a/tools/python/xen/xend/XendDomain.py     Tue Feb 28 22:08:47 2006
>> +++ b/tools/python/xen/xend/XendDomain.py     Tue Feb 28 22:10:14 2006
>> @@ -385,7 +385,7 @@
>>              val = dominfo.destroy()
>>          else:
>>              try:
>> -                val = xc.domain_destroy(domid)
>> +                val = xc.domain_destroy(int(domid))
>>              except Exception, ex:
>>                  raise XendError(str(ex))
>>          return val
>>     
>
> I can see why you've done this, but it just led me to wonder why
> XendDomain.domain_destroy exists at all.  All it's doing is looking up a
> domain ID, checking that it's not the privileged domain (in the wrong
> order!) calling XendDomainInfo.destroy, and calling xc.domain_destroy in
> the case of an exception.  We should move the check and the exception
> handling into XendDomainInfo.destroy, and then we can dispatch to that
> method straight away, without needing XendDomain.domain_destroy at all.
> The messaging layer already has the capability to lookup domain IDs --
> we should use it.
>   
Ok, sounds sane to me.
> The reason I noticed is that I have been trying to make it so that
> XendDomain and XendDomainInfo only receive arguments of the correct type,
> with the casts to integers etc handled by the messaging layer.  This
> change you've made highlights the fact that there is now nowhere for
> that type conversion to take place, because the messaging layer is more
> generic, and doesn't understand the calls that it is dispatching.
> That's OK -- it's a consequence of the removal of all that code in
> SrvDomain -- but it's something to be aware of.  If we adopt the naming
> convention that I suggest above, then all methods accept only arguments
> of the correct type _except_ for those named "public_xyz" -- those
> methods must validate and convert their arguments appropriately.
>   
Why?  If a public_ function expects a domid and gets passed a string, it 
ought to throw an exception and the client should get it as an error.

This particular hack was to account for a place in xm where it was 
passing a string version of the domain ID.  The real problem here was 
the xm code.  I wanted to avoid modifying that code though to 
demonstrate that the XML-RPC stuff could be a drop-in-replacement.  Of 
course, now that that exercise is complete, there's no reason not to go 
in and fix the original source of the problem.
>> @@ -416,6 +414,8 @@
>>  def err(msg):
>>      """Print an error to stderr and exit.
>>      """
>> +    import traceback
>> +    traceback.print_exc()
>>      print >>sys.stderr, "Error:", msg
>>      sys.exit(1)
>>     
>
> This looks like debug code -- I'm not even sure how this traceback helps
> you very much.  It should be disabled by default, or removed altogether.
>   
Yeah, sorry, I missed that.
>>  from xen.xend import PrettyPrint
>> @@ -1036,23 +1036,13 @@
>>              else:
>>                  err("Error connecting to xend: %s." % ex[1])
>>              sys.exit(1)
>> -        except xen.xend.XendError.XendError, ex:
>> -            if len(args) > 0:
>> -                handle_xend_error(argv[1], args, ex)
>> -            else:
>> -                print "Unexpected error:", sys.exc_info()[0]
>> -                print
>> -                print "Please report to xen-devel@lists.xensource.com"
>> -                raise
>> -        except xen.xend.XendProtocol.XendError, ex:
>> -            if len(args) > 0:
>> -                handle_xend_error(argv[1], args, ex)
>> -            else:
>> -                print "Unexpected error:", sys.exc_info()[0]
>> -                print
>> -                print "Please report to xen-devel@lists.xensource.com"
>> -                raise
>>          except SystemExit:
>> +            sys.exit(1)
>> +        except xmlrpclib.Fault, ex:
>> +#            print "Xend generated an internal fault:"
>> +#            sys.stderr.write(ex.faultString)
>> +#            sys.exit(1)
>> +            print "Error: Internal Xend error"
>>     
>
> I expect we can do better than just printing "Internal Xend Error".  How
> much structure and useful information is there in an xmlrpclib.Fault at
> the moment?
>   
We can pass whatever we want.   xmlrpclib.Fault contain an integer code 
and a string message.  For now, I'm just always passing a 1 for the 
faultCode and sending the Xend traceback as the faultMessage.

I'm not sure how fancy we want to get for the first drop of this code.  
Any suggestions?

BTW, it's printing 'Internal Xend Error' because xm-test has a test case 
that checks for that specific result :-)
>   
>>              sys.exit(1)
>>          except:
>>              print "Unexpected error:", sys.exc_info()[0]
>>     
>
> Looks like that's it!  Thanks for all your hard work, Anthony, this is
> going to make a big difference to Xend's usefulness and maintainability,
> and you've done a good job of it.  Let's get it into 3.0.2.
>   
Thanks for looking at the code!  I don't think there's that much change 
necessary.  I should be able to turn around another patch in a couple days.

Regards,

Anthony Liguori
> Cheers,
>
> Ewan.
>