[libvirt] Re: [PATCH] Implement finer grained migration control for Xen

[Chris Lalancette]

Maximilian Wilhelm wrote:
> Anno domini 2009 Daniel Veillard scripsit:
> 
>> On Mon, Oct 12, 2009 at 01:32:26PM +0200, Chris Lalancette wrote:
>>> Normally, when you migrate a domain from host A to host B,
>>> the domain on host A remains defined but shutoff and the domain
>>> on host B remains running but is a "transient".  Add a new
>>> flag to virDomainMigrate() to allow the original domain to be
>>> undefined on source host A, and a new flag to virDomainMigrate() to
>>> allow the new domain to be persisted on the destination host B.
> 
>>> Signed-off-by: Chris Lalancette <clalance at redhat.com>
> 
> Thanks for the ground work!
> 
> Attached you can find a patch implementing the same flags for Xen:
> 
>  * src/xen/xen_driver.c: Add support for VIR_MIGRATE_PERSIST_DEST flag
>  * src/xen/xend_internal.c: Add support for VIR_MIGRATE_UNDEFINE_SOURCE flag
> 
> I'm not totaly sure if there are better ways to handle all the error
> cases. The current solution seemed to be a same one for me.

Well, I do think that we need to return a proper error in all cases except for
the one with the long comment.  I've pointed out where I think we need to add
error messages below.  Otherwise, I think it looks good.

> 
> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
> index 5273a11..18882c0 100644
> --- a/src/xen/xen_driver.c
> +++ b/src/xen/xen_driver.c
> @@ -1204,9 +1204,53 @@ xenUnifiedDomainMigrateFinish (virConnectPtr dconn,
>                                 const char *cookie ATTRIBUTE_UNUSED,
>                                 int cookielen ATTRIBUTE_UNUSED,
>                                 const char *uri ATTRIBUTE_UNUSED,
> -                               unsigned long flags ATTRIBUTE_UNUSED)
> +                               unsigned long flags)
>  {
> -    return xenUnifiedDomainLookupByName (dconn, dname);
> +    virDomainPtr dom = NULL;
> +    char *domain_xml = NULL;
> +    virDomainPtr dom_new = NULL;
> +
> +    dom = xenUnifiedDomainLookupByName (dconn, dname);
> +    if (! dom) {

You are probably going to want to raise some error here, otherwise the user will
get back "unknown error", which isn't very helpful.

> +        return NULL;
> +    }
> +
> +    if (flags & VIR_MIGRATE_PERSIST_DEST) {
> +        domain_xml = xenDaemonDomainDumpXML (dom, 0, NULL);
> +	if (! domain_xml) {
> +            goto failure;
> +        }

This seems whitespace damaged (using tabs instead of spaces), and also needs to
raise a proper error.

> +
> +        dom_new = xenDaemonDomainDefineXML (dconn, domain_xml);
> +        if (! dom_new) {
> +            /* Hmpf.  Migration was successful, but making it persistent
> +             * was not.  If we report successful, then when this domain
> +             * shuts down, management tools are in for a surprise.  On the
> +             * other hand, if we report failure, then the management tools
> +             * might try to restart the domain on the source side, even
> +             * though the domain is actually running on the destination.
> +             * Return a NULL dom pointer, and hope that this is a rare
> +             * situation and management tools are smart.
> +             */
> +            goto failure;
> +        }
> +
> +        /* Free additional reference added by Define */
> +        virDomainFree (dom_new);
> +    }
> +
> +    VIR_FREE (domain_xml);
> +
> +    return dom;
> +
> +
> +failure:
> +    virDomainFree (dom);
> +
> +    VIR_FREE (domain_xml);
> +
> +    return NULL;
> +    
>  }
>  
>  static int
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index 27d215e..9fbb616 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -4386,6 +4386,8 @@ xenDaemonDomainMigratePerform (virDomainPtr domain,
>      int ret;
>      char *p, *hostname = NULL;
>  
> +    int undefined_source = 0;
> +
>      /* Xen doesn't support renaming domains during migration. */
>      if (dname) {
>          virXendError (conn, VIR_ERR_NO_SUPPORT,
> @@ -4404,11 +4406,25 @@ xenDaemonDomainMigratePerform (virDomainPtr domain,
>          return -1;
>      }
>  
> -    /* Check the flags. */
> +    /*
> +     * Check the flags.
> +     */
>      if ((flags & VIR_MIGRATE_LIVE)) {
>          strcpy (live, "1");
>          flags &= ~VIR_MIGRATE_LIVE;
>      }
> +
> +    /* Undefine the VM on the source host after migration ? */
> +    if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) {
> +       undefined_source = 1;
> +       flags &= ~VIR_MIGRATE_UNDEFINE_SOURCE;
> +    }
> +
> +    /* Ignore the persist_dest flag here */
> +    if (flags & VIR_MIGRATE_PERSIST_DEST) {
> +        flags &= ~VIR_MIGRATE_PERSIST_DEST;
> +    }

Again a nit, but I think the libvirt style currently is to leave braces off for
single line statements.  That is, this should be:

if (flags & VIR_MIGRATE_PERSIST_DEST)
    flags &= ~VIR_MIGRATE_PERSIST_DEST;

> +
>      /* XXX we could easily do tunnelled & peer2peer migration too
>         if we want to. support these... */
>      if (flags != 0) {
> @@ -4494,6 +4510,10 @@ xenDaemonDomainMigratePerform (virDomainPtr domain,
>                     NULL);
>      VIR_FREE (hostname);
>  
> +    if (ret == 0 && undefined_source) {
> +        xenDaemonDomainUndefine (domain);
> +    }
> +

Same here.

>      DEBUG0("migration done");
>  
>      return ret;
> 
> 


-- 
Chris Lalancette