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

[Daniel Veillard]

On Sun, Oct 18, 2009 at 02:33:26AM +0200, Maximilian Wilhelm wrote:
> Anno domini 2009 Chris Lalancette scripsit:
> 
> > > 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;
> > > +        }
> 
> When thinking about the handling of these error cases, I think that
> they all are equal from a users/management tools point of view:
>   The migration worked, but something in the process of making the new
>   VM persistant failed. So if we raise an error here, we IMO should do
>   in in all three cases or do it nowhere.
> 
>   What would be the additional win of telling the user, we failed to
>   lookup the VM on the destination host (for whatever reason) or
>   failed to dump the XML in contrast to not telling him the define
>   went wrong?
> 
> So the prefect solution would IMO be to raise some kind of
> FAILED_TO_MAKE_VM_PERSIST error which would indicate the situation
> clearly to the application above.
> 
> Comments?

  Yes adding a different error with a clear semantic so that the
management code can understand the current situation and not assume the
domain in still running on the source node sounds the right approach to
me. We can't make this a completely atomic operation, so we need to deal
with this !

  Patch welcome :-) Thanks !

Any chance you could provide an updated patch for the Xen side
independantly, or did I missed it ?

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/