[Libvir] Unified Xen patch (incomplete, for discussion)
I've spent the last day or so producing a unified Xen patch. In such a model a connection contains only a single underlying driver, and it is the responsibility of the (unified) Xen driver to try all the different methods it knows. Attached is an incomplete patch for this, for discussion, plus the two extra files of the unified driver itself (for some reason CVS won't give me a diff including these added files). Some points to note: The new structure of the drivers is: <pre> libvirt.c | +------- xen_unified.c | | | +--- xen_internal.c (hypervisor) | | | +--- proxy_internal.c * (proxy) | | | +--- xend_internal.c * (XenD) | | | +--- xs_internal.c (XenStore) | | | +--- xm_internal.c * (inactive domains) | +-------- qemu_internal.c * | +-------- test.c * = not updated yet, so these don't compile </pre> I haven't renamed the Xen sub-drivers. That's really to make the patch easier to read. There is definitely a case for renaming the drivers more logically to reflect the structure above. All Xen-specific hacks in libvirt.c have been moved to xen_unified.c Error handling in the case where a driver doesn't support a libvirt function is now considerably better. Each driver keeps its private data private. At the moment xen_unified pretty much does the "try the drivers in a loop until one succeeds" strategy which used to be in libvirt.c. There is a case for making it do direct calls to the "right" driver, but for simplicity I haven't gone that far. Again for simplicity the Xen sub-drivers still use struct virDriver. They should use their own custom 'struct virXenDriver' or whatever. The effects of this are slight but noticable - some parameters are now no longer used in the sub-drivers, so marked ATTRIBUTE_UNUSED. There is also a single networkDriver pointer in the connect struct. We should modify the functions to handle the case where this is NULL because we couldn't bring up the network functions (all network functions in this case would just return an error). libvirt in CVS fails to run if libvirtd isn't up. I won't be able to get back to this before tomorrow afternoon, so plenty of time for discussion! Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in UK and Wales under Company Registration No. 3798903 Directors: Michael Cunningham (USA), Charlie Peters (USA) and David Owens (Ireland)
This is the second version of the unified Xen patch, and the first completed version. It fixes a lot of broken things from the earlier version. However the patch needs a lot more testing before it could be considered for inclusion. Indeed, suggestions for how to test this would be welcome. I'm following up places where 'make check' is failing with this patch: hopefully none of them will be serious. But I wonder how much coverage 'make check' gives me. Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 Directors: Michael Cunningham (USA), Charlie Peters (USA) and David Owens (Ireland)
On Fri, Mar 30, 2007 at 01:48:14PM +0100, Richard W.M. Jones wrote:
This is the second version of the unified Xen patch, and the first completed version. It fixes a lot of broken things from the earlier version. However the patch needs a lot more testing before it could be considered for inclusion.
Indeed, suggestions for how to test this would be welcome. I'm following up places where 'make check' is failing with this patch: hopefully none of them will be serious. But I wonder how much coverage 'make check' gives me.
Not really very much of the driver API itself yet. At most we're testing operation of the 'test' driver for 15 or so virsh commands. We don't have coverage of the Xen drivers really because that's more of a functional or integration test than unit test & thus impossible to add to regular make check in any way that would be reliable for all developers. The core part of the test suite is coveraging the XML <-> SEXPR conversions and the XML <-> /etc/xen config files, and the capabilities stuff. We could probably do with adding a separate 'make xencheck' which does a formal integration test against a running Xen HV instance from a known starting point. Regards. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Ignore the just posted "first complete version". It had some stupid stuff going on with connection opens. This version fixes all that, and passes 'make check'. Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 Directors: Michael Cunningham (USA), Charlie Peters (USA) and David Owens (Ireland)
On Fri, Mar 30, 2007 at 02:27:46PM +0100, Richard W.M. Jones wrote:
Ignore the just posted "first complete version". It had some stupid stuff going on with connection opens. This version fixes all that, and passes 'make check'.
/* These dispatch functions follow the model used historically * by libvirt.c -- trying each low-level Xen driver in turn * until one succeeds. However since we know what low-level * drivers can perform which functions, it is probably better * in future to optimise these dispatch functions to just call * the single function (or small number of appropriate functions) * in the low level drivers directly. */
For now, keeping the same internal driver model will help debugging any issues in this conversion. I agree that we should later re-factor this further to explicitly call the appropriate methods directly because I don't think merely iterating over driver is particularly useful. For any given libvirt method is is easy to define exactly how we should fetch the info. The current situation is a little odd - if we doing 'pause' then we should always just go straight to the hypervisor. If that fails there is no point trying any others because both XenD / XenStored ultimately call into the hypervisor too. I don't see the point if the XenStoreD driver at all. It is lower priority than the XenD driver is, and since XenD driver has 100% coverage of all APIs there is no circumstance in which the XenStoreD driver will ever be called. There's a couple of helper methods in the xenstored.c file which are used directly be xend.c / xml.c, but aside from that the rest is 100% untested by any of us.
static int xenUnifiedDomainSuspend (virDomainPtr dom) { int i;
/* Try non-hypervisor methods first, then hypervisor direct method * as a last resort. */ for (i = 0; i < nb_drivers; ++i) if (i != hypervisor_offset && drivers[i]->domainSuspend && drivers[i]->domainSuspend (dom) == 0) return 0;
if (drivers[hypervisor_offset]->domainSuspend && drivers[hypervisor_offset]->domainSuspend (dom) == 0) return 0;
return -1; }
Why do we try the non-hypervisor method first ? What does XenD give us that we don't get just by going to the HV immediately, aside from higher overheads. Likewise for Resume/Destroy/SetVCPUs. If there is a compelling reason why we must call XenD, then we shouldn't try to fallback to the HV at all. If there isn't a compelling reason, then we should try HV first. Was this how the existing code already worked, if so i guess we should leave as is until we can cleanup like I described earlier ? If any knows why, we should at least comment this voodoo...
+static int +qemuNetworkClose (virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* NB: This will fail to close the connection properly + * qemuRegister is changed so that it only registers the + * network (not the connection driver). + */ + return 0; +}
I'm not sure what you mean by this comment ? I'm fairly sure we need to have code in qemuNetworkClose() though to close the QEMU socket when the XenD driver is used QEMU driver for the networking APIs. Apart from the 2 questions about suspend/resume/destroy APIs and the QEMU networking code, this patch looks fine for inclusion to me. Although it is a big patch, it is basically a straight forward re-factoring with no major operational changes aside from in the open/close routines. I'm going to actually try it out for real shortly.... Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel P. Berrange wrote:
static int xenUnifiedDomainSuspend (virDomainPtr dom) { int i;
/* Try non-hypervisor methods first, then hypervisor direct method * as a last resort. */ for (i = 0; i < nb_drivers; ++i) if (i != hypervisor_offset && drivers[i]->domainSuspend && drivers[i]->domainSuspend (dom) == 0) return 0;
if (drivers[hypervisor_offset]->domainSuspend && drivers[hypervisor_offset]->domainSuspend (dom) == 0) return 0;
return -1; }
Why do we try the non-hypervisor method first ? What does XenD give us that we don't get just by going to the HV immediately, aside from higher overheads.
Ah, so don't confuse my code/comment up there with the application of rational thought :-) I've just copied what is in libvirt.c right now. To whit: int virDomainSuspend(virDomainPtr domain) { //.... /* * Go though the driver registered entry points but use the * XEN_HYPERVISOR directly only as a last mechanism */ for (i = 0;i < conn->nb_drivers;i++) { if ((conn->drivers[i] != NULL) && (conn->drivers[i]->no != VIR_DRV_XEN_HYPERVISOR) && (conn->drivers[i]->domainSuspend != NULL)) { if (conn->drivers[i]->domainSuspend(domain) == 0) return (0); } } for (i = 0;i < conn->nb_drivers;i++) { if ((conn->drivers[i] != NULL) && (conn->drivers[i]->no == VIR_DRV_XEN_HYPERVISOR) && (conn->drivers[i]->domainSuspend != NULL)) { if (conn->drivers[i]->domainSuspend(domain) == 0) return (0); } } //.... } The new code in xen_unified just duplicates that functionality.
Was this how the existing code already worked, if so i guess we should leave as is until we can cleanup like I described earlier ? If any knows why, we should at least comment this voodoo...
:-)
+static int +qemuNetworkClose (virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* NB: This will fail to close the connection properly + * qemuRegister is changed so that it only registers the + * network (not the connection driver). + */ + return 0; +}
I'm not sure what you mean by this comment ? I'm fairly sure we need to have code in qemuNetworkClose() though to close the QEMU socket when the XenD driver is used QEMU driver for the networking APIs.
So the current code is complicated and somewhat devious. In the current code, the qemu network close just reuses qemuClose. In the unified case this fails because it tries to double-free a structure. My first time around this I got around the double-free by keeping track with a usage counter. However that had its own problems, so seeing that currently we always register the qemu connection and qemu network drivers together, I just created a separate qemuNetworkClose function which does nothing. If on the other hand in future we will use qemu network without qemu connections, then we'll need to change this (hence added comments). Note that this applies to registration (ie. vir*Register), not whether or not we manage to connect to libvirtd.
Apart from the 2 questions about suspend/resume/destroy APIs and the QEMU networking code, this patch looks fine for inclusion to me. Although it is a big patch, it is basically a straight forward re-factoring with no major operational changes aside from in the open/close routines.
I'm going to actually try it out for real shortly....
Yeah, I'm also testing it now. Rich.
On Mon, Apr 02, 2007 at 03:15:37PM +0100, Richard W.M. Jones wrote:
+static int +qemuNetworkClose (virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* NB: This will fail to close the connection properly + * qemuRegister is changed so that it only registers the + * network (not the connection driver). + */ + return 0; +}
I'm not sure what you mean by this comment ? I'm fairly sure we need to have code in qemuNetworkClose() though to close the QEMU socket when the XenD driver is used QEMU driver for the networking APIs.
So the current code is complicated and somewhat devious. In the current code, the qemu network close just reuses qemuClose. In the unified case this fails because it tries to double-free a structure.
My first time around this I got around the double-free by keeping track with a usage counter. However that had its own problems, so seeing that currently we always register the qemu connection and qemu network drivers together, I just created a separate qemuNetworkClose function which does nothing. If on the other hand in future we will use qemu network without qemu connections, then we'll need to change this (hence added comments). Note that this applies to registration (ie. vir*Register), not whether or not we manage to connect to libvirtd.
The Xen driver *already* activates the QEMU networking driver so we have to deal with that situation now. virsh net-list for example fails with this patch applied now. The problem is that QEMU network driver impl with this patch is just casting the privateData straight to qemudPrivatePtr regardless of whether the QEMU driver is actually active. I'm attaching a patch which applies on-top of yours to change the qemudNetworkOpen method to allocate a block of private data specific for the neworking functions. If the conn->driver->name == QEMU, then we re-use the existing QEMUD socket, otherwise we open a new one.
Apart from the 2 questions about suspend/resume/destroy APIs and the QEMU networking code, this patch looks fine for inclusion to me. Although it is a big patch, it is basically a straight forward re-factoring with no major operational changes aside from in the open/close routines.
I'm going to actually try it out for real shortly....
Yeah, I'm also testing it now.
I discovered the aforemetioned issue with Xen driver not being able to use any of the network driver methods. There was another interaction with the proxy code - the proxy driver should not be activated if running as root. In addition if running as non-root, any Xen driver is allowed to fail, except for the proxy driver. The attached patch tries to alter the xen_unified.c do_open logic to deal with that. The proxy itself was SEGV'ing because it didn't allocate the private data blob, so the patch fixes that too. I can now open QEMU & XEn as root, and use networking functions, as well as opening both as non-root. Finally, the libvirt.c was not checking if conn->networkDriver was NULL, so if no network driver was activated, the code would SEGV on any network functions instead of returning operation not supported error. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel P. Berrange wrote:
Finally, the libvirt.c was not checking if conn->networkDriver was NULL, so if no network driver was activated, the code would SEGV on any network functions instead of returning operation not supported error.
Right, as expected! This last one was actually on my to-do list to fix, and it looks like you got there first - thanks :-) Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 Directors: Michael Cunningham (USA), Charlie Peters (USA) and David Owens (Ireland)
On Mon, Apr 02, 2007 at 03:05:19PM +0100, Daniel P. Berrange wrote:
On Fri, Mar 30, 2007 at 02:27:46PM +0100, Richard W.M. Jones wrote:
Ignore the just posted "first complete version". It had some stupid stuff going on with connection opens. This version fixes all that, and passes 'make check'.
/* These dispatch functions follow the model used historically * by libvirt.c -- trying each low-level Xen driver in turn * until one succeeds. However since we know what low-level * drivers can perform which functions, it is probably better * in future to optimise these dispatch functions to just call * the single function (or small number of appropriate functions) * in the low level drivers directly. */
For now, keeping the same internal driver model will help debugging any issues in this conversion. I agree that we should later re-factor this further to explicitly call the appropriate methods directly because I don't think merely iterating over driver is particularly useful. For any given libvirt method is is easy to define exactly how we should fetch the info.
The current situation is a little odd - if we doing 'pause' then we should always just go straight to the hypervisor. If that fails there is no point trying any others because both XenD / XenStored ultimately call into the hypervisor too.
The difference in the xend case is that then Xend becomes aware of the status change.
I don't see the point if the XenStoreD driver at all. It is lower priority than the XenD driver is, and since XenD driver has 100% coverage of all APIs there is no circumstance in which the XenStoreD driver will ever be called. There's a couple of helper methods in the xenstored.c file which are used directly be xend.c / xml.c, but aside from that the rest is 100% untested by any of us.
In general agreed, except we don't know about weird cases, for example going though xend to shutdown a domain in emergency may not work, while the xenstored mechanism may still work because it's direct.
static int xenUnifiedDomainSuspend (virDomainPtr dom) { int i;
/* Try non-hypervisor methods first, then hypervisor direct method * as a last resort. */ for (i = 0; i < nb_drivers; ++i) if (i != hypervisor_offset && drivers[i]->domainSuspend && drivers[i]->domainSuspend (dom) == 0) return 0;
if (drivers[hypervisor_offset]->domainSuspend && drivers[hypervisor_offset]->domainSuspend (dom) == 0) return 0;
return -1; }
Why do we try the non-hypervisor method first ? What does XenD give us that we don't get just by going to the HV immediately, aside from higher overheads.
Xend may become confused because the state changed without it being aware of that .
Likewise for Resume/Destroy/SetVCPUs. If there is a compelling reason why we must call XenD, then we shouldn't try to fallback to the HV at all. If there isn't a compelling reason, then we should try HV first.
fallback is useful because Xend may start failing say under load or memory pressure in domain 0, in general the current approach is IMHO more reliable.
Was this how the existing code already worked, if so i guess we should leave as is until we can cleanup like I described earlier ? If any knows why, we should at least comment this voodoo...
/* * Xen is woodoo and xend is not the most reliable piece of code * a bit of redundancy may help. */ Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones