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

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