[libvirt] [PATCH 03/16] Introduce migration cookies to QEMU driver

[Daniel P. Berrange]

On Mon, May 16, 2011 at 02:05:13PM +0100, Daniel P. Berrange wrote:
> On Thu, May 12, 2011 at 01:17:21PM -0600, Eric Blake wrote:
> > On 05/12/2011 10:04 AM, Daniel P. Berrange wrote:
> > > The migration protocol has support for a 'cookie' parameter which
> > > is an opaque array of bytes as far as libvirt is concerned. Drivers
> > > may use this for passing around arbitrary extra data they might
> > > need during migration. The QEMU driver needs to do a few things:
> > > 
> > > +static int
> > > +qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
> > > +                            xmlXPathContextPtr ctxt,
> > > +                            int flags ATTRIBUTE_UNUSED)
> > > +{
> > > +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> > > +    char *tmp;
> > > +
> > > +    /* We don't store the uuid, name, hostname, or hostuuid
> > > +     * values. We just compare them to local data to do some
> > > +     * sanity checking on migration operation
> > > +     */
> > 
> > Should we do sanity checking that no unknown XML elements appear in the
> > cookie?  And/or validate that flags contains no unexpected bits?  That
> > is, should you insert virCheckFlags(0, -1) here, or in
> > qemuMigrationEatCookie?  And rather than just using virXPathString to
> > probe whether a particular XML element is present, shouldn't you instead
> > do an iteration over all XML elements in order to detect unrecognized
> > elements?
> > 
> > Otherwise, what I'm afraid of is that the cookie eater (whether the
> > destination eating the cookie from Begin, or the source eating the
> > cookie from Prepare) will be running an earlier libvirt version than the
> > baker; if the baker added a mandatory flag to the cookie, but the eater
> > is unaware to look for that element and silently ignores it, then we
> > risk silent botching of migration.
> > 
> > Do we have enough infrastructure in place for source and destination to
> > agree on what features are mandatory vs. optional in the cookie, to
> > allow for omission of optional elements that would make migration nicer
> > but aren't fatal if left out?  That is, a baker can always try a flag,
> > then retry without the flag, but retries can get expensive if there were
> > an alternative to first do a capability query to determine the common
> > subset of flags to use in the first place.
> 
> Well, the migration cookies were intended to be optional data,
> which if omitted, don't result in bad stuff.
> 
> eg, if the graphics relocation data doesn't exist, the end user
> will simply need to manually reconnect.
> 
> I think we do need some stricter checking on the lock state
> data though, to validate that the same lock manager is
> present on both sides.
> 
> > > @@ -342,6 +612,15 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver,
> > >      event = virDomainEventNewFromObj(vm,
> > >                                       VIR_DOMAIN_EVENT_STARTED,
> > >                                       VIR_DOMAIN_EVENT_STARTED_MIGRATED);
> > > +
> > > +    if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, 0) < 0) {
> > > +        /* We could tear down the whole guest here, but
> > > +         * cookie data is (so far) non-critical, so that
> > > +         * seems a little harsh. We'll just warn for now.
> > > +         */
> > > +        VIR_WARN("Unable to encode migration cookie");
> > > +    }
> > > +
> > >      ret = 0;
> > >  
> > >  endjob:
> > > @@ -369,7 +648,7 @@ cleanup:
> > >          virDomainObjUnlock(vm);
> > >      if (event)
> > >          qemuDomainEventQueue(driver, event);
> > > -    qemuDriverUnlock(driver);
> > > +    qemuMigrationCookieFree(mig);
> > 
> > Umm, did you really intend to drop the qemuDriverUnlock line?
> 
> No, that's a merge rebase error

Actually no it wasn't. This is a bug that's being fixed. The caller of
this method, already unlocks the driver on completion, so we had a
double unlock.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|