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

[Daniel P. Berrange]

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 :|