Re: [PATCH] libxl: add tunnelled migration support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/19/2016 08:22 AM, Jim Fehlig wrote:
> On 11/10/2016 09:14 PM, Bob Liu wrote:
>> Tunnelled migration doesn't require any extra network connections beside the
>> libvirt daemon.
>> It's capable of strong encryption and the default option of openstack-nova.
>>
>> This patch adds the tunnelled migration(Tunnel3params) support to libxl.
>> On the src side, the data flow is:
>>  * libxlDoMigrateSend() -> pipe
>>  * libxlTunnel3MigrationFunc() polls pipe out and then write to dest stream.
>>
>> While on the dest side:
>> Stream -> pipe -> 'recvfd of libxlDomainStartRestore'
>>
>> The usage is the same as p2p migration, execpt adding one extra '--tunnelled' to
>> the libvirt p2p migration command.
> 
> Hi Bob,
> 
> Thanks for the patch! A few comments below...
> 

Thank you for your review.

>>
>> Signed-off-by: Bob Liu <bob.liu@xxxxxxxxxx>
>> ---
>>  src/libxl/libxl_driver.c    |  58 ++++++++-
>>  src/libxl/libxl_migration.c | 281 +++++++++++++++++++++++++++++++++++++++++---
>>  src/libxl/libxl_migration.h |   9 ++
>>  3 files changed, 331 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index b66cb1f..bc2633b 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -5918,6 +5918,61 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>  }
>>
>>  static int
>> +libxlDomainMigratePrepareTunnel3Params(virConnectPtr dconn,
>> +                                       virStreamPtr st,
>> +                                       virTypedParameterPtr params,
>> +                                       int nparams,
>> +                                       const char *cookiein,
>> +                                       int cookieinlen,
>> +                                       char **cookieout ATTRIBUTE_UNUSED,
>> +                                       int *cookieoutlen ATTRIBUTE_UNUSED,
>> +                                       unsigned int flags)
>> +{
>> +    libxlDriverPrivatePtr driver = dconn->privateData;
>> +    virDomainDefPtr def = NULL;
>> +    const char *dom_xml = NULL;
>> +    const char *dname = NULL;
>> +    const char *uri_in = NULL;
>> +
>> +#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>> +    virReportUnsupportedError();
>> +    return -1;
>> +#endif
>> +
>> +    virCheckFlags(LIBXL_MIGRATION_FLAGS, -1);
>> +    if (virTypedParamsValidate(params, nparams, LIBXL_MIGRATION_PARAMETERS) < 0)
>> +        goto error;
>> +
>> +    if (virTypedParamsGetString(params, nparams,
>> +                                VIR_MIGRATE_PARAM_DEST_XML,
>> +                                &dom_xml) < 0 ||
>> +        virTypedParamsGetString(params, nparams,
>> +                                VIR_MIGRATE_PARAM_DEST_NAME,
>> +                                &dname) < 0 ||
>> +        virTypedParamsGetString(params, nparams,
>> +                                VIR_MIGRATE_PARAM_URI,
>> +                                &uri_in) < 0)
>> +
>> +        goto error;
>> +
>> +    if (!(def = libxlDomainMigrationPrepareDef(driver, dom_xml, dname)))
>> +        goto error;
>> +
>> +    if (virDomainMigratePrepareTunnel3ParamsEnsureACL(dconn, def) < 0)
>> +        goto error;
>> +
>> +    if (libxlDomainMigrationPrepareTunnel3(dconn, st, &def, cookiein,
>> +                                           cookieinlen, flags) < 0)
> 
> With the exception of the above two calls, this function is identical to libxlDomainMigratePrepare3Params. 
> Perhaps you can use some macro trickery or a common function to reduce the duplicate code?
> 

How about adding a 'is_tunnel' parameter to libxlDomainMigratePrepare3Params()?

>> +        goto error;
>> +
>> +    return 0;
>> +
>> + error:
>> +    virDomainDefFree(def);
>> +    return -1;
>> +}
>> +
>> +static int
>>  libxlDomainMigratePrepare3Params(virConnectPtr dconn,
>>                                   virTypedParameterPtr params,
>>                                   int nparams,
>> @@ -6017,7 +6072,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
>>      if (virDomainMigratePerform3ParamsEnsureACL(dom->conn, vm->def) < 0)
>>          goto cleanup;
>>
>> -    if (flags & VIR_MIGRATE_PEER2PEER) {
>> +    if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) {
>>          if (libxlDomainMigrationPerformP2P(driver, vm, dom->conn, dom_xml,
>>                                             dconnuri, uri, dname, flags) < 0)
>>              goto cleanup;
>> @@ -6501,6 +6556,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>      .nodeDeviceReset = libxlNodeDeviceReset, /* 1.2.3 */
>>      .domainMigrateBegin3Params = libxlDomainMigrateBegin3Params, /* 1.2.6 */
>>      .domainMigratePrepare3Params = libxlDomainMigratePrepare3Params, /* 1.2.6 */
>> +    .domainMigratePrepareTunnel3Params = libxlDomainMigratePrepareTunnel3Params, /* 2.5.0 */
>>      .domainMigratePerform3Params = libxlDomainMigratePerform3Params, /* 1.2.6 */
>>      .domainMigrateFinish3Params = libxlDomainMigrateFinish3Params, /* 1.2.6 */
>>      .domainMigrateConfirm3Params = libxlDomainMigrateConfirm3Params, /* 1.2.6 */
>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>> index 534abb8..f3152dc 100644
>> --- a/src/libxl/libxl_migration.c
>> +++ b/src/libxl/libxl_migration.c
>> @@ -44,6 +44,7 @@
>>  #include "libxl_migration.h"
>>  #include "locking/domain_lock.h"
>>  #include "virtypedparam.h"
>> +#include "fdstream.h"
>>
>>  #define VIR_FROM_THIS VIR_FROM_LIBXL
>>
>> @@ -484,6 +485,90 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver,
>>  }
>>
>>  int
>> +libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
>> +                                   virStreamPtr st,
>> +                                   virDomainDefPtr *def,
>> +                                   const char *cookiein,
>> +                                   int cookieinlen,
>> +                                   unsigned int flags)
>> +{
>> +    libxlMigrationCookiePtr mig = NULL;
>> +    libxlDriverPrivatePtr driver = dconn->privateData;
>> +    virDomainObjPtr vm = NULL;
>> +    libxlMigrationDstArgs *args = NULL;
>> +    virThread thread;
>> +    int dataFD[2] = { -1, -1 };
>> +    int ret = -1;
>> +
>> +    if (libxlMigrationEatCookie(cookiein, cookieinlen, &mig) < 0)
>> +        goto error;
>> +
>> +    if (mig->xenMigStreamVer > LIBXL_SAVE_VERSION) {
>> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>> +                       _("Xen migration stream version '%d' is not supported on this host"),
>> +                       mig->xenMigStreamVer);
>> +        goto error;
>> +    }
>> +
>> +    if (!(vm = virDomainObjListAdd(driver->domains, *def,
>> +                                   driver->xmlopt,
>> +                                   VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
>> +                                   VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
>> +                                   NULL)))
>> +        goto error;
> 
> I think having a separate function here is fine since there's quite a bit of difference, but you'll need to update it to account for hook support added by Cedric. 
> E.g. see the hook code in libxlDomainMigrationPrepare().
> 

Okay, will be updated.

>> +
>> +    /*
>> +     * The data flow of tunnel3 migration in the dest side:
>> +     * stream -> pipe -> recvfd of libxlDomainStartRestore
>> +     */
>> +    if (pipe(dataFD) < 0)
>> +        goto error;
>> +
>> +    /* Stream data will be written to pipeIn */
>> +    if (virFDStreamOpen(st, dataFD[1]) < 0)
>> +        goto error;
>> +    dataFD[1] = -1; /* 'st' owns the FD now & will close it */
>> +
>> +    if (libxlMigrationDstArgsInitialize() < 0)
>> +        goto error;
>> +
>> +    if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>> +        goto error;
>> +
>> +    args->conn = dconn;
>> +    args->vm = vm;
>> +    args->flags = flags;
>> +    args->migcookie = mig;
>> +    /* Receive from pipeOut */
>> +    args->recvfd = dataFD[0];
>> +    args->nsocks = 0;
>> +    if (virThreadCreate(&thread, false, libxlDoMigrateReceive, args) < 0) {
>> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> +                       _("Failed to create thread for receiving migration data"));
>> +        goto error;
>> +    }
>> +
>> +    ret = 0;
>> +    goto done;
>> +
>> + error:
>> +    VIR_FORCE_CLOSE(dataFD[1]);
>> +    VIR_FORCE_CLOSE(dataFD[0]);
>> +    virObjectUnref(args);
>> +    /* Remove virDomainObj from domain list */
>> +    if (vm) {
>> +        virDomainObjListRemove(driver->domains, vm);
>> +        vm = NULL;
>> +    }
>> +
>> + done:
> 
> libxlMigrationCookieFree(mig)? Hmm, maybe it would be worth thinking about ways to extract some of the commonalities between this function and libxlDomainMigratePrepare().
> It would help avoid such bugs in the future.
> 

I'm a bit confused about libxlMigrationCookieFree() even in libxlDomainMigratePrepare().

The msg is set to NULL before call libxlMigrationCookieFree(), so I think the free is meaningless.

libxlDomainMigrationPrepare():
....
 687     args->migcookie = mig;
 688     mig = NULL;
...
 727  done:
 728     libxlMigrationCookieFree(mig);


And we can't free the migcookie, because the libxlMigrateReceive > libxlDoMigrateReceive thread will have to use it.
The virObjectUnref(args) call will free the actuall migcookie if I understand right.


>> +    if (vm)
>> +        virObjectUnlock(vm);
>> +
>> +    return ret;
>> +}
>> +
>> +int
>>  libxlDomainMigrationPrepare(virConnectPtr dconn,
>>                              virDomainDefPtr *def,
>>                              const char *uri_in,
>> @@ -710,9 +795,154 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>>      return ret;
>>  }
>>
>> -/* This function is a simplification of virDomainMigrateVersion3Full
>> - * excluding tunnel support and restricting it to migration v3
>> - * with params since it was the first to be introduced in libxl.
>> +typedef struct _libxlTunnelMigrationThread libxlTunnelMigrationThread;
>> +struct _libxlTunnelMigrationThread {
>> +    virThread thread;
> 
> I think the 'thread' field would be better placed in the libxlTunnelControl struct. It looks to only be used in {Start,Stop}Tunnel.
> 

Will update.

>> +    virStreamPtr st;
>> +    int srcFD;
>> +};
>> +#define TUNNEL_SEND_BUF_SIZE 65536
>> +
>> +/*
>> + * The data flow of tunnel3 migration in the src side:
>> + * libxlDoMigrateSend() -> pipe
>> + * libxlTunnel3MigrationFunc() polls pipe out and then write to dest stream.
>> + */
>> +static void libxlTunnel3MigrationFunc(void *arg)
>> +{
>> +    libxlTunnelMigrationThread *data = (libxlTunnelMigrationThread *)arg;
>> +    char *buffer = NULL;
>> +    struct pollfd fds[1];
>> +    int timeout = -1;
>> +
>> +    if (VIR_ALLOC_N(buffer, TUNNEL_SEND_BUF_SIZE) < 0) {
>> +        virReportError(errno, "%s", _("poll failed in migration tunnel"));
>> +        return;
>> +    }
>> +
>> +    fds[0].fd = data->srcFD;
>> +    for (;;) {
>> +        int ret;
>> +
>> +        fds[0].events = POLLIN;
>> +        fds[0].revents = 0;
>> +        ret = poll(fds, ARRAY_CARDINALITY(fds), timeout);
>> +        if (ret < 0) {
>> +            if (errno == EAGAIN || errno == EINTR)
>> +                continue;
>> +            virReportError(errno, "%s",
>> +                           _("poll failed in libxlTunnel3MigrationFunc"));
>> +            goto cleanup;
>> +        }
>> +
>> +        if (ret == 0) {
>> +            VIR_DEBUG("poll got timeout");
> 
> Nit: Is that debug message true with a negative timeout? 
> The man page isn't really clear, only saying that a return value "of 0 indicates that the call timed out and no file descriptors were ready".
> 
>> +            break;
>> +        }
>> +
>> +        if (fds[0].revents & (POLLIN | POLLERR | POLLHUP)) {
>> +            int nbytes;
>> +
>> +            nbytes = read(data->srcFD, buffer, TUNNEL_SEND_BUF_SIZE);
>> +            if (nbytes > 0) {
>> +                /* Write to dest stream */
>> +                if (virStreamSend(data->st, buffer, nbytes) < 0) {
>> +                    virReportError(errno, "%s",
>> +                                   _("tunnelled migration failed to send to virStream"));
>> +                    virStreamAbort(data->st);
>> +                    goto cleanup;
>> +                }
>> +            } else if (nbytes < 0) {
>> +                virReportError(errno, "%s",
>> +                               _("tunnelled migration failed to read from xen side"));
>> +                    virStreamAbort(data->st);
>> +                    goto cleanup;
>> +            } else {
>> +                /* EOF; transferred all data */
>> +                break;
>> +            }
>> +        }
>> +    }
>> +
>> +    if (virStreamFinish(data->st) < 0)
>> +        virReportError(errno, "%s",
>> +                       _("tunnelled migration failed to finish stream"));
>> + cleanup:
>> +    VIR_FREE(buffer);
>> +
>> +    return;
>> +}
>> +
>> +struct libxlTunnelControl {
>> +    libxlTunnelMigrationThread tmThread;
> 
> Can this be replaced with the virThread field in libxlTunnelMigrationThread struct? 
> I don't see why the libxlTunnelMigrationThread struct needs to be embedded here. 

It's used to save the threadPtr, so that libxlMigrationStopTunnel() can cancel the thread.

> Also, we should use consistent style when declaring these structures.
> 
>> +    int dataFD[2];
>> +};
>> +
>> +static int
>> +libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
>> +                          virDomainObjPtr vm,
>> +                          unsigned long flags,
>> +                          virStreamPtr st,
>> +                          struct libxlTunnelControl *tc)
>> +{
>> +    libxlTunnelMigrationThread *tmThreadPtr = NULL;
>> +    int ret = -1;
>> +
>> +    if (VIR_ALLOC(tc) < 0)
>> +        goto out;
> 
> It seems better to have the calling function, which owns the libxlTunnelControl struct, to allocate it. 
> In this function we should allocate and fill the libxlTunnelMigrationThread struct, for use by the thread running libxlTunnel3MigrationFunc.
> 


Actually in the previous version, I put all this stuff after the below if directly.
 969     if (flags & VIR_MIGRATE_TUNNELLED)
 970         ret = libxlMigrationStartTunnel(driver, vm, flags, st, tc);
 971     else

No structure 'libxlTunnelMigrationThread' and libxlMigrationStartTunnel/StopTunnel at all.
E.g.

>      VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out));
> -    ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
> -                                      uri_out, NULL, flags);
> +    if (flags & VIR_MIGRATE_TUNNELLED) {
> +        if (VIR_ALLOC(libxlTunnelMigationThreadPtr) < 0)
> +            goto cleanup;
> +        if (pipe(dataFD) < 0) {
> +            virReportError(errno, "%s", _("Unable to make pipes"));
> +            goto cleanup;
> +        }
> +        /* Read from pipe */
> +        libxlTunnelMigationThreadPtr->srcFD = dataFD[0];
> +        /* Write to dest stream */
> +        libxlTunnelMigationThreadPtr->st = st;
> +        if (virThreadCreate(&libxlTunnelMigationThreadPtr->thread, true,
> +                            libxlTunnel3MigrationFunc,
> +                            libxlTunnelMigationThreadPtr) < 0) {
> +            virReportError(errno, "%s",
> +                           _("Unable to create tunnel migration thread"));
> +            goto cleanup;
> +        }
>  
> +        virObjectUnlock(vm);
> +        /* Send data to pipe */
> +        ret = libxlDoMigrateSend(driver, vm, flags, dataFD[1]);
> +        virObjectLock(vm);
> +    } else {
> +        ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
> +                                          uri_out, NULL, flags);
> +    }


But Joao thought this may not easy to be read.

Which way do you prefer to accept?

>> +
>> +    tc->dataFD[0] = -1;
>> +    tc->dataFD[1] = -1;
>> +    if (pipe(tc->dataFD) < 0) {
>> +        virReportError(errno, "%s", _("Unable to make pipes"));
>> +        goto out;
>> +    }
>> +
>> +    tmThreadPtr = &tc->tmThread;
>> +    /* Read from pipe */
>> +    tmThreadPtr->srcFD = tc->dataFD[0];
>> +    /* Write to dest stream */
>> +    tmThreadPtr->st = st;
>> +    if (virThreadCreate(&tmThreadPtr->thread, true,
>> +                        libxlTunnel3MigrationFunc,
>> +                        tmThreadPtr) < 0) {
>> +        virReportError(errno, "%s",
>> +                       _("Unable to create tunnel migration thread"));
>> +        goto out;
>> +    }
>> +
>> +    virObjectUnlock(vm);
>> +    /* Send data to pipe */
>> +    ret = libxlDoMigrateSend(driver, vm, flags, tc->dataFD[1]);
>> +    virObjectLock(vm);
>> +
>> + out:
>> +    /* libxlMigrationStopTunnel will be called in libxlDoMigrateP2P to free
>> +     * all resources for us. */
>> +    return ret;
>> +}
>> +
>> +static void libxlMigrationStopTunnel(struct libxlTunnelControl *tc)
>> +{
>> +    libxlTunnelMigrationThread *tmThreadPtr = NULL;
>> +
>> +    if (!tc)
>> +        return;
>> +
>> +    tmThreadPtr = &tc->tmThread;
>> +    if (tmThreadPtr) {
>> +        virThreadCancel(&tmThreadPtr->thread);
>> +        virThreadJoin(&tmThreadPtr->thread);
>> +    }
>> +
>> +    VIR_FORCE_CLOSE(tc->dataFD[0]);
>> +    VIR_FORCE_CLOSE(tc->dataFD[1]);
>> +    VIR_FREE(tc);
>> +}
>> +
>> +/* This function is a simplification of virDomainMigrateVersion3Full and
>> + * restricting it to migration v3 with params since it was the first to be
>> + * introduced in libxl.
>>   */
>>  static int
>>  libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
>> @@ -737,6 +967,9 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
>>      bool cancelled = true;
>>      virErrorPtr orig_err = NULL;
>>      int ret = -1;
>> +    /* For tunnel migration */
>> +    virStreamPtr st = NULL;
>> +    struct libxlTunnelControl *tc = NULL;
>>
>>      dom_xml = libxlDomainMigrationBegin(sconn, vm, xmlin,
>>                                          &cookieout, &cookieoutlen);
>> @@ -764,29 +997,40 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
>>
>>      VIR_DEBUG("Prepare3");
>>      virObjectUnlock(vm);
>> -    ret = dconn->driver->domainMigratePrepare3Params
>> -        (dconn, params, nparams, cookieout, cookieoutlen, NULL, NULL, &uri_out, destflags);
>> +    if (flags & VIR_MIGRATE_TUNNELLED) {
>> +        if (!(st = virStreamNew(dconn, 0)))
> 
> Shouldn't there be a corresponding virStreamFree() somewhere? How is the virStream object disposed?
> 
> 
>> +            goto cleanup;

virStream is disposed in the cleanup, please see below.

>> +        ret = dconn->driver->domainMigratePrepareTunnel3Params
>> +            (dconn, st, params, nparams, cookieout, cookieoutlen, NULL, NULL, destflags);
>> +    } else {
>> +        ret = dconn->driver->domainMigratePrepare3Params
>> +            (dconn, params, nparams, cookieout, cookieoutlen, NULL, NULL, &uri_out, destflags);
>> +    }
>>      virObjectLock(vm);
>>
>>      if (ret == -1)
>>          goto cleanup;
>>
>> -    if (uri_out) {
>> -        if (virTypedParamsReplaceString(&params, &nparams,
>> -                                        VIR_MIGRATE_PARAM_URI, uri_out) < 0) {
>> -            orig_err = virSaveLastError();
>> +    if (!(flags & VIR_MIGRATE_TUNNELLED)) {
>> +        if (uri_out) {
>> +            if (virTypedParamsReplaceString(&params, &nparams,
>> +                                            VIR_MIGRATE_PARAM_URI, uri_out) < 0) {
>> +                orig_err = virSaveLastError();
>> +                goto finish;
>> +            }
>> +        } else {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                           _("domainMigratePrepare3 did not set uri"));
>>              goto finish;
>>          }
>> -    } else {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                       _("domainMigratePrepare3 did not set uri"));
>> -        goto finish;
>>      }
>>
>>      VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out));
>> -    ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
>> -                                      uri_out, NULL, flags);
>> -
>> +    if (flags & VIR_MIGRATE_TUNNELLED)
>> +        ret = libxlMigrationStartTunnel(driver, vm, flags, st, tc);
>> +    else
>> +        ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
>> +                                          uri_out, NULL, flags);
>>      if (ret < 0)
>>          orig_err = virSaveLastError();
>>
>> @@ -824,6 +1068,11 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
>>                   vm->def->name);
>>
>>   cleanup:
>> +    if (flags & VIR_MIGRATE_TUNNELLED) {
>> +        libxlMigrationStopTunnel(tc);
>> +        virObjectUnref(st);

Here.

>> +    }
>> +

Thanks,
Bob

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux