On 05/04/2012 04:40 PM, Jiri Denemark wrote:
> When we added the default USB controller into domain XML, we efficiently
> broke migration to older versions of libvirt that didn't support USB
> controllers at all (0.9.4 and earlier) even for domains that doesn't use
s/doesn't/don't/
> anything that the older libvirt can't provide. We still want to present
> the default USB controller in any XML seen by a user/app but we can
> safely remove it from the domain XML used during migration. If we are
> migrating to a new enough libvirt, it will add the controller XML back,
> while older libvirt won't be confused with it although it will still
> tell qemu to create the controller.
>
> Similar approach can be used in the future whenever we find out we
> always enabled some kind of device without properly advertising it in
> domain XML.
Memballoon would be such a device, if we care about migration to even
older libvirt.
> ---
> src/qemu/qemu_domain.c | 60 ++++++++++++++++++++++++++++++++++++++++----
> src/qemu/qemu_domain.h | 10 +++++--
> src/qemu/qemu_driver.c | 21 +++++++++------
> src/qemu/qemu_migration.c | 10 ++++---
> src/qemu/qemu_process.c | 8 +++---
> 5 files changed, 83 insertions(+), 26 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b105644..3752ddf 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1227,11 +1227,14 @@ int
> qemuDomainDefFormatBuf(struct qemud_driver *driver,
> virDomainDefPtr def,
> unsigned int flags,
> + bool compatible,
> virBuffer *buf)
Rather than adding a new 'compatible' flag and having to adjust _every_
caller, could we instead add a new VIR_DOMAIN_XML_INTERNAL flag to be
or'd in to flags only for the callers that care? On the other hand,
we'd have to filter that flag back out before calling into domain_conf,
so I guess this approach is as good as any.
> {
> int ret = -1;
> virCPUDefPtr cpu = NULL;
> virCPUDefPtr def_cpu = def->cpu;
> + virDomainControllerDefPtr *controllers = NULL;
> + int ncontrollers = 0;
>
> /* Update guest CPU requirements according to host CPU */
> if ((flags & VIR_DOMAIN_XML_UPDATE_CPU) &&
> @@ -1249,21 +1252,64 @@ qemuDomainDefFormatBuf(struct qemud_driver *driver,
> def->cpu = cpu;
> }
>
> + if (compatible) {
> + int i;
> + virDomainControllerDefPtr usb = NULL;
> +
> + /* If only the default USB controller is present, we can remove it
> + * and make the XML compatible with older versions of libvirt which
> + * didn't support USB controllers in the XML but always added the
> + * default one to qemu anyway.
> + */
> + for (i = 0; i < def->ncontrollers; i++) {
> + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) {
> + if (usb) {
> + usb = NULL;
> + break;
> + }
> + usb = def->controllers[i];
> + }
> + }
> + if (usb && usb->idx == 0 && usb->model == -1) {
Just like memballoon, if we ever want to explicitly represent an XML
with no usb controller, we will have to provide <controller type='usb'
mode='none'>; but there, the model would not be -1, so this would
properly be migrated.
> + VIR_DEBUG("Removing default USB controller from domain '%s'"
> + " for migration compatibility", def->name);
> + controllers = def->controllers;
> + ncontrollers = def->ncontrollers;
> + if (VIR_ALLOC_N(def->controllers, ncontrollers - 1) < 0) {
> + controllers = NULL;
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + def->ncontrollers = 0;
> + for (i = 0; i < ncontrollers; i++) {
> + if (controllers[i] != usb)
> + def->controllers[def->ncontrollers++] = controllers[i];
> + }
> + }
> + }
> +
> ret = virDomainDefFormatInternal(def, flags, buf);
>
> cleanup:
> def->cpu = def_cpu;
> virCPUDefFree(cpu);
> + if (controllers) {
> + VIR_FREE(def->controllers);
> + def->controllers = controllers;
> + def->ncontrollers = ncontrollers;
> + }
This looks like it will correctly trim _only_ the default usb controller
from modern xml, and restore it after the migration has been prepared.
On the receiving side, modern libvirt will know to reconstruct the
default usb controller.
> +++ b/src/qemu/qemu_driver.c
> @@ -2630,9 +2630,9 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom,
> virDomainDefFree(def);
> goto endjob;
> }
> - xml = qemuDomainDefFormatLive(driver, def, true);
> + xml = qemuDomainDefFormatLive(driver, def, true, true);
> } else {
> - xml = qemuDomainDefFormatLive(driver, vm->def, true);
> + xml = qemuDomainDefFormatLive(driver, vm->def, true, true);
Good - save is indeed a migration case, since you can save on one
machine and load on another.
> @@ -10406,7 +10408,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> } else {
> /* Easiest way to clone inactive portion of vm->def is via
> * conversion in and back out of xml. */
> - if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true)) ||
> + if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, false)) ||
I also agree with this one - although reverting to a snapshot is like
loading a saved image, we don't currently migrate snapshots so they are
only useful within the same host, so the revert should never be by an
older libvirt that didn't recognize the usb controller.
> @@ -433,6 +433,7 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver,
> mig->persistent,
> VIR_DOMAIN_XML_INACTIVE |
> VIR_DOMAIN_XML_SECURE,
> + true,
> buf) < 0)
And of course, the main driver for this patch is migration.
It looks to me like you passed the correct 'compatible' flag in all the
callers, and that this does allow for cross-migration from newer to
older xml.
ACK.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list