On Tue, Nov 09, 2010 at 01:33:50PM -0500, Chris Lalancette wrote:
> 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.
While I understand your desire to change this, I think it would be a very
bad idea todo so. These semantics aren't expressed in the API at all,
and neither the app or the user knows whether libvirt is about to use
version 1, 2 or 3 of the migration protocol. Thus having the semantics
of the URI change based on version 1, 2 or 3 would be very surprising,
and make it impossible to predict the semantics when invoking the API.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list