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

[Jim Fehlig <jfehlig@xxxxxxxx>]

Daniel P. Berrange wrote:
> 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
>   

Nod.  I have an old, dusty patchset that integrates virlockd in the
libxl driver.  The series includes introducing a conf file for the libxl
driver.  I'll get back to working on that before long.

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

Ok, done in V8.

>
>   
>> +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.
>   

V8 includes a simple libxlDomainDefCheckABIStability() function

https://www.redhat.com/archives/libvir-list/2014-June/msg00231.html

Thanks for the comments!

Regards,
Jim



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