Re: [libvirt] [PATCH] Introduce yet another migration version in API.

On 11/02/10 - 01:08:53PM, Daniel P. Berrange wrote: > This patch attempts to introduce a version 3 that uses the > improved 5 step sequence > > * Src: Begin > - Generate XML to pass to dst > - Generate optional cookie to pass to dst > > * Dst: Prepare > - Get ready to accept incoming VM > - Generate optional cookie to pass to src > > * Src: Perform > - Start migration and wait for send completion > - Generate optional cookie to pass to dst > > * Dst: Finish > - Wait for recv completion and check status > - Kill off VM if failed, resume if success > - Generate optional cookie to pass to src > > * Src: Confirm > - Kill off VM if success, resume if failed > > The API is designed to allow both input and output cookies > in all methods where applicable. This lets us pass around > arbitrary extra driver specific data between src & dst during > migration. Combined with the extra 'Begin' method this lets > us pass lease information from source to dst at the start of > migration > > Moving the killing of the source VM out of Perform and > into Confirm, means we can now recover if the dst host > can't successfully Finish receiving migration data. Yeah, this seems like a pretty good sequence. Like mentioned below, if the last step fails, there isn't a whole lot we can do, except make sure to report it and let management kill it off. One comment inline... <snip> > diff --git a/src/libvirt.c b/src/libvirt.c > index e24fb9e..04a7cd0 100644 > --- a/src/libvirt.c > +++ b/src/libvirt.c ... > +/* > + * Sequence v3: > + * > + * Src: Begin > + * - Generate XML to pass to dst > + * - Generate optional cookie to pass to dst > + * > + * Dst: Prepare > + * - Get ready to accept incoming VM > + * - Generate optional cookie to pass to src > + * > + * Src: Perform > + * - Start migration and wait for send completion > + * - Generate optional cookie to pass to dst > + * > + * Dst: Finish > + * - Wait for recv completion and check status > + * - Kill off VM if failed, resume if success > + * - Generate optional cookie to pass to src > + * > + * Src: Confirm > + * - Kill off VM if success, resume if failed > + * > + */ > +static virDomainPtr > +virDomainMigrateVersion3 (virDomainPtr domain, > + virConnectPtr dconn, > + unsigned long flags, > + const char *dname, > + const char *uri, > + unsigned long bandwidth) > +{ > + virDomainPtr ddomain = NULL; > + char *uri_out = NULL; > + char *cookiesrc = NULL; > + char *cookiedst = NULL; > + char *dom_xml = NULL; > + int cookiesrclen = 0; > + int cookiedstlen = 0; > + int ret; > + virDomainInfo info; > + virErrorPtr orig_err = NULL; > + > + if (!domain->conn->driver->domainMigrateBegin3) { > + virLibConnError (domain->conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__); > + virDispatchError(domain->conn); > + return NULL; > + } > + dom_xml = domain->conn->driver->domainMigrateBegin3 > + (domain->conn, &cookiesrc, &cookiesrclen, uri, flags, dname, > + bandwidth); > + if (!dom_xml) > + goto done; > + > + ret = virDomainGetInfo (domain, &info); > + if (ret == 0 && info.state == VIR_DOMAIN_PAUSED) { > + flags |= VIR_MIGRATE_PAUSED; > + } > + > + ret = dconn->driver->domainMigratePrepare3 > + (dconn, cookiesrc, cookiesrclen, &cookiedst, &cookiedstlen, > + uri, &uri_out, flags, dname, bandwidth, dom_xml); > + VIR_FREE (dom_xml); > + if (ret == -1) > + goto done; > + > + if (uri == NULL && uri_out == NULL) { > + virLibConnError (domain->conn, VIR_ERR_INTERNAL_ERROR, > + _("domainMigratePrepare2 did not set uri")); > + virDispatchError(domain->conn); > + goto done; > + } > + if (uri_out) > + uri = uri_out; /* Did domainMigratePrepare2 change URI? */ > + assert (uri != NULL); I think we should get rid of this whole "uri_out" concept, while we are adding a new protocol. Basically, it allows the destination to "tell" the source where to migrate to, but there is no way to really discover that. What you really need are: 1) The ability to let the user specify which network should be used for migration. 2) The ability to determine the "default" network, in the absence of 1). We already get 1) from the passed in uri, if it is specified. We can easily figure out 2) from the source, since we know we can reach the destination via dconn. So if nothing is specified for 1), we can just default to the same network that the libvirt transport is using. This will eliminate a lot of the headaches we have with migration protocol version 2 with trying to determine the hostname on the destination side.