[libvirt] [PATCH] libxl: add tunnelled migration support
Jim Fehlig
jfehlig at suse.com
Sat Nov 19 00:22:58 UTC 2016
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...
>
> Signed-off-by: Bob Liu <bob.liu at oracle.com>
> ---
> 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?
> + 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().
> +
> + /*
> + * 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.
> + 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.
> + 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. 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.
> +
> + 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?
Regards,
Jim
> + goto cleanup;
> + 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(¶ms, &nparams,
> - VIR_MIGRATE_PARAM_URI, uri_out) < 0) {
> - orig_err = virSaveLastError();
> + if (!(flags & VIR_MIGRATE_TUNNELLED)) {
> + if (uri_out) {
> + if (virTypedParamsReplaceString(¶ms, &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);
> + }
> +
> if (ddomain) {
> virObjectUnref(ddomain);
> ret = 0;
> diff --git a/src/libxl/libxl_migration.h b/src/libxl/libxl_migration.h
> index 8a074a0..fcea558 100644
> --- a/src/libxl/libxl_migration.h
> +++ b/src/libxl/libxl_migration.h
> @@ -29,6 +29,7 @@
> # define LIBXL_MIGRATION_FLAGS \
> (VIR_MIGRATE_LIVE | \
> VIR_MIGRATE_PEER2PEER | \
> + VIR_MIGRATE_TUNNELLED | \
> VIR_MIGRATE_PERSIST_DEST | \
> VIR_MIGRATE_UNDEFINE_SOURCE | \
> VIR_MIGRATE_PAUSED)
> @@ -53,6 +54,14 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver,
> const char *dname);
>
> int
> +libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
> + virStreamPtr st,
> + virDomainDefPtr *def,
> + const char *cookiein,
> + int cookieinlen,
> + unsigned int flags);
> +
> +int
> libxlDomainMigrationPrepare(virConnectPtr dconn,
> virDomainDefPtr *def,
> const char *uri_in,
>
More information about the libvir-list
mailing list