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(a)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?
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(¶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);
Here.
> + }
> +
Thanks,
Bob