Re: [Xen-devel] [libvirt] [PATCH V7] libxl: add migration support

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

On Thu, May 08, 2014 at 03:56:51PM -0600, Jim Fehlig wrote:
> This patch adds initial migration support to the libxl driver,
> using the VIR_DRV_FEATURE_MIGRATION_PARAMS family of migration
> functions.
> 
> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
> 
> V6 here
>   https://www.redhat.com/archives/libvir-list/2014-May/msg00017.html
> 
> In V7:
> There were no comments on V6, but during testing I noticed that
> 'virsh migrate --suspend ...' was not being honored.  Fixed that
> in this version so that the domain is paused on the destination
> once migration completes.
> 
>  po/POTFILES.in              |   1 +
>  src/Makefile.am             |   3 +-
>  src/libxl/libxl_conf.h      |   6 +
>  src/libxl/libxl_domain.h    |   1 +
>  src/libxl/libxl_driver.c    | 235 ++++++++++++++++++
>  src/libxl/libxl_migration.c | 581 
> ++++++++++++++++++++++++++++++++++++++++++++
>  src/libxl/libxl_migration.h |  79 ++++++
>  7 files changed, 905 insertions(+), 1 deletion(-)


> @@ -301,6 +303,13 @@ libxlStateInitialize(bool privileged,
>                                LIBXL_VNC_PORT_MAX)))
>          goto error;
>  
> +    /* Allocate bitmap for migration port reservation */
> +    if (!(libxl_driver->migrationPorts =
> +          virPortAllocatorNew(_("migration"),
> +                              LIBXL_MIGRATION_PORT_MIN,
> +                              LIBXL_MIGRATION_PORT_MAX)))
> +        goto error;
> +

No need todo it in this patch, but experiance with QEMU is that people will
want this range to be configurable in /etc/libvirt/libxl.conf. So I'd
suggest adding this later

> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> new file mode 100644
> index 0000000..e3699c5
> --- /dev/null
> +++ b/src/libxl/libxl_migration.c
> @@ -0,0 +1,581 @@
> +/*
> + * libxl_migration.c: methods for handling migration with libxenlight
> + *
> + * Copyright (C) 2014 SUSE LINUX Products GmbH, Nuernberg, Germany.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Authors:
> + *     Jim Fehlig <jfehlig@xxxxxxxx>
> + *     Chunyan Liu <cyliu@xxxxxxxx>
> + */
> +
> +#include <config.h>
> +
> +#include "internal.h"
> +#include "virlog.h"
> +#include "virerror.h"
> +#include "virconf.h"
> +#include "datatypes.h"
> +#include "virfile.h"
> +#include "viralloc.h"
> +#include "viruuid.h"
> +#include "vircommand.h"
> +#include "virstring.h"
> +#include "virobject.h"
> +#include "rpc/virnetsocket.h"
> +#include "libxl_domain.h"
> +#include "libxl_driver.h"
> +#include "libxl_conf.h"
> +#include "libxl_migration.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_LIBXL
> +
> +VIR_LOG_INIT("libxl.libxl_migration");
> +
> +typedef struct _libxlMigrationDstArgs {
> +    virObject parent;
> +
> +    virConnectPtr conn;
> +    virDomainObjPtr vm;
> +    unsigned int flags;
> +
> +    /* for freeing listen sockets */
> +    virNetSocketPtr *socks;
> +    size_t nsocks;
> +} libxlMigrationDstArgs;
> +
> +static virClassPtr libxlMigrationDstArgsClass;
> +
> +static void
> +libxlMigrationDstArgsDispose(void *obj)
> +{
> +    libxlMigrationDstArgs *args = obj;
> +
> +    VIR_FREE(args->socks);
> +}
> +
> +static int
> +libxlMigrationDstArgsOnceInit(void)
> +{
> +    if (!(libxlMigrationDstArgsClass = virClassNew(virClassForObject(),
> +                                                   "libxlMigrationDstArgs",
> +                                                   
> sizeof(libxlMigrationDstArgs),
> +                                                   
> libxlMigrationDstArgsDispose)))
> +        return -1;
> +
> +    return 0;
> +}
> +
> +VIR_ONCE_GLOBAL_INIT(libxlMigrationDstArgs)
> +
> +static void
> +libxlDoMigrateReceive(virNetSocketPtr sock,
> +                      int events ATTRIBUTE_UNUSED,
> +                      void *opaque)
> +{
> +    libxlMigrationDstArgs *args = opaque;
> +    virConnectPtr conn = args->conn;
> +    virDomainObjPtr vm = args->vm;
> +    virNetSocketPtr *socks = args->socks;
> +    size_t nsocks = args->nsocks;
> +    bool paused = args->flags & VIR_MIGRATE_PAUSED;
> +    libxlDriverPrivatePtr driver = conn->privateData;
> +    virNetSocketPtr client_sock;
> +    int recvfd = -1;
> +    size_t i;
> +    int ret;
> +
> +    virNetSocketAccept(sock, &client_sock);
> +    if (client_sock == NULL) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("Fail to accept migration connection"));
> +        goto cleanup;
> +    }
> +    VIR_DEBUG("Accepted migration connection\n");
> +    recvfd = virNetSocketDupFD(client_sock, true);
> +    virObjectUnref(client_sock);
> +
> +    virObjectLock(vm);
> +    ret = libxlDomainStart(driver, vm, paused, recvfd);
> +    virObjectUnlock(vm);
> +
> +    if (ret < 0 && !vm->persistent)
> +        virDomainObjListRemove(driver->domains, vm);
> +
> + cleanup:
> +    /* Remove all listen socks from event handler, and close them. */
> +    for (i = 0; i < nsocks; i++) {
> +        virNetSocketUpdateIOCallback(socks[i], 0);
> +        virNetSocketRemoveIOCallback(socks[i]);
> +        virNetSocketClose(socks[i]);
> +        virObjectUnref(socks[i]);

I think you should set  socks[i] = NULL and after the loop also set 
args->nsocks = 0, just to ensure nothing else can access these now
free'd socks.


> +char *
> +libxlDomainMigrationBegin(virConnectPtr conn,
> +                          virDomainObjPtr vm,
> +                          const char *xmlin)
> +{
> +    libxlDriverPrivatePtr driver = conn->privateData;
> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> +    virDomainDefPtr tmpdef = NULL;
> +    virDomainDefPtr def;
> +    char *xml = NULL;
> +
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> +        goto cleanup;
> +
> +    if (xmlin) {
> +        if (!(tmpdef = virDomainDefParseString(xmlin, cfg->caps,
> +                                               driver->xmlopt,
> +                                               1 << VIR_DOMAIN_VIRT_XEN,
> +                                               VIR_DOMAIN_XML_INACTIVE)))
> +            goto endjob;

I think you want to call an equivalent of qemuDomainDefCheckABIStability
here. THis ensures the user supplied XML only contains changes that are
not going to impact guest machine ABI.

> +
> +        def = tmpdef;
> +    } else {
> +        def = vm->def;
> +    }
> +
> +    if (!libxlDomainMigrationIsAllowed(def))
> +        goto endjob;
> +
> +    xml = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE);
> +
> + cleanup:
> +    if (vm)
> +        virObjectUnlock(vm);
> +
> +    virDomainDefFree(tmpdef);
> +    virObjectUnref(cfg);
> +    return xml;
> +
> + endjob:
> +    if (!libxlDomainObjEndJob(driver, vm))
> +        vm = NULL;
> +    goto cleanup;
> +}


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel