4:27 p.m.
Patch 5 fixes a long standing problem found by some very slow hosts in
xen's osstest
https://lists.xenproject.org/archives/html/xen-devel/2018-08/msg01945.html
While working on the fix, I discovered other problems in libxl's V3
migration protocol. E.g. a modify job on the migrating VM was not
handled properly across the phases on either src or dst host. Patches
1-4 fix this and other problems found along the way.
Jim Fehlig (5):
libxl: migration: defer removing VM until finish phase
libxl: fix logic in P2P migration
libxl: fix job handling across migration phases on src
libxl: fix job handling across migration phases on dst
libxl: join with thread receiving migration data
src/libxl/libxl_domain.h | 1 +
src/libxl/libxl_driver.c | 7 --
src/libxl/libxl_migration.c | 168 ++++++++++++++++++++++++------------
3 files changed, 114 insertions(+), 62 deletions(-)
--
2.18.0
4:28 p.m.
New subject: [libvirt] [PATCH 1/5] libxl: migration: defer removing VM until finish phase
If for any reason the restore of a VM fails on the destination host
in a migration operation, the VM is removed (if not persistent) from
the virDomainObjList, meaning it is no longer available for additional
cleanup or processing in the finish phase. Defer removing the VM from
the virDomainObjList until the finish phase, which already contains
logic to remove the VM.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_migration.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index b2e5847c58..97f72d0390 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -264,7 +264,6 @@ libxlDoMigrateDstReceive(void *opaque)
libxlDriverPrivatePtr driver = args->conn->privateData;
int recvfd = args->recvfd;
size_t i;
- int ret;
virObjectRef(vm);
virObjectLock(vm);
@@ -274,12 +273,10 @@ libxlDoMigrateDstReceive(void *opaque)
/*
* Always start the domain paused. If needed, unpause in the
* finish phase, after transfer of the domain is complete.
+ * Errors and cleanup are also handled in the finish phase.
*/
- ret = libxlDomainStartRestore(driver, vm, true, recvfd,
- args->migcookie->xenMigStreamVer);
-
- if (ret < 0 && !vm->persistent)
- virDomainObjListRemove(driver->domains, vm);
+ libxlDomainStartRestore(driver, vm, true, recvfd,
+ args->migcookie->xenMigStreamVer);
/* Remove all listen socks from event handler, and close them. */
for (i = 0; i < nsocks; i++) {
--
2.18.0
4:28 p.m.
New subject: [libvirt] [PATCH 2/5] libxl: fix logic in P2P migration
libxlDoMigrateSrcP2P() performs all phases of the migration
protocol for peer-to-peer migration. Unfortunately the logic
was a bit flawed since it is possible to skip the confirm
phase after a successfull begin and prepare phase. Fix the
logic to always call the confirm phase after a successful begin
and perform. Skip the confirm phase if begin or perform fail.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_migration.c | 48 ++++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 19 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 97f72d0390..e4f2895690 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -972,30 +972,35 @@ libxlDoMigrateSrcP2P(libxlDriverPrivatePtr driver,
char *cookieout = NULL;
int cookieoutlen;
bool cancelled = true;
+ bool notify_source = true;
virErrorPtr orig_err = NULL;
int ret = -1;
/* For tunnel migration */
virStreamPtr st = NULL;
struct libxlTunnelControl *tc = NULL;
+ if (dname &&
+ virTypedParamsAddString(¶ms, &nparams, &maxparams,
+ VIR_MIGRATE_PARAM_DEST_NAME, dname) < 0)
+ goto cleanup;
+
+ if (uri &&
+ virTypedParamsAddString(¶ms, &nparams, &maxparams,
+ VIR_MIGRATE_PARAM_URI, uri) < 0)
+ goto cleanup;
+
dom_xml = libxlDomainMigrationSrcBegin(sconn, vm, xmlin,
&cookieout, &cookieoutlen);
+ /*
+ * If dom_xml is non-NULL the begin phase has succeeded, and the
+ * confirm phase must be called to cleanup the migration operation.
+ */
if (!dom_xml)
goto cleanup;
if (virTypedParamsAddString(¶ms, &nparams, &maxparams,
VIR_MIGRATE_PARAM_DEST_XML, dom_xml) < 0)
- goto cleanup;
-
- if (dname &&
- virTypedParamsAddString(¶ms, &nparams, &maxparams,
- VIR_MIGRATE_PARAM_DEST_NAME, dname) < 0)
- goto cleanup;
-
- if (uri &&
- virTypedParamsAddString(¶ms, &nparams, &maxparams,
- VIR_MIGRATE_PARAM_URI, uri) < 0)
- goto cleanup;
+ goto confirm;
/* We don't require the destination to have P2P support
* as it looks to be normal migration from the receiver perpective.
@@ -1006,7 +1011,7 @@ libxlDoMigrateSrcP2P(libxlDriverPrivatePtr driver,
virObjectUnlock(vm);
if (flags & VIR_MIGRATE_TUNNELLED) {
if (!(st = virStreamNew(dconn, 0)))
- goto cleanup;
+ goto confirm;
ret = dconn->driver->domainMigratePrepareTunnel3Params
(dconn, st, params, nparams, cookieout, cookieoutlen, NULL, NULL,
destflags);
} else {
@@ -1016,7 +1021,7 @@ libxlDoMigrateSrcP2P(libxlDriverPrivatePtr driver,
virObjectLock(vm);
if (ret == -1)
- goto cleanup;
+ goto confirm;
if (!(flags & VIR_MIGRATE_TUNNELLED)) {
if (uri_out) {
@@ -1038,8 +1043,10 @@ libxlDoMigrateSrcP2P(libxlDriverPrivatePtr driver,
else
ret = libxlDomainMigrationSrcPerform(driver, vm, NULL, NULL,
uri_out, NULL, flags);
- if (ret < 0)
+ if (ret < 0) {
+ notify_source = false;
orig_err = virSaveLastError();
+ }
cancelled = (ret < 0);
@@ -1067,12 +1074,15 @@ libxlDoMigrateSrcP2P(libxlDriverPrivatePtr driver,
if (!orig_err)
orig_err = virSaveLastError();
- VIR_DEBUG("Confirm3 cancelled=%d vm=%p", cancelled, vm);
- ret = libxlDomainMigrationSrcConfirm(driver, vm, flags, cancelled);
+ confirm:
+ if (notify_source) {
+ VIR_DEBUG("Confirm3 cancelled=%d vm=%p", cancelled, vm);
+ ret = libxlDomainMigrationSrcConfirm(driver, vm, flags, cancelled);
- if (ret < 0)
- VIR_WARN("Guest %s probably left in 'paused' state on source",
- vm->def->name);
+ if (ret < 0)
+ VIR_WARN("Guest %s probably left in 'paused' state on
source",
+ vm->def->name);
+ }
cleanup:
if (flags & VIR_MIGRATE_TUNNELLED) {
--
2.18.0
4:28 p.m.
New subject: [libvirt] [PATCH 3/5] libxl: fix job handling across migration phases on src
The libxlDomainMigrationSrc* functions are a bit flawed in their
handling of modify jobs. A job begins at the start of the begin
phase but ends before the phase completes. No job is running for
the remaining phases of migration on the source host.
Change the logic to keep the job running after a successful begin
phase, and end the job in the confirm phase. The job must also end
in the perform phase in the case of error since confirm phase would
not be executed.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_migration.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index e4f2895690..191973edeb 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -399,6 +399,11 @@ libxlDomainMigrationSrcBegin(virConnectPtr conn,
virDomainDefPtr def;
char *xml = NULL;
+ /*
+ * In the case of successful migration, a job is started here and
+ * terminated in the confirm phase. Errors in the begin or perform
+ * phase will also terminate the job.
+ */
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
goto cleanup;
@@ -428,6 +433,9 @@ libxlDomainMigrationSrcBegin(virConnectPtr conn,
goto endjob;
xml = virDomainDefFormat(def, cfg->caps, VIR_DOMAIN_DEF_FORMAT_SECURE);
+ /* Valid xml means success! EndJob in the confirm phase */
+ if (xml)
+ goto cleanup;
endjob:
libxlDomainObjEndJob(driver, vm);
@@ -1169,6 +1177,14 @@ libxlDomainMigrationSrcPerformP2P(libxlDriverPrivatePtr driver,
ret = libxlDoMigrateSrcP2P(driver, vm, sconn, xmlin, dconn, dconnuri,
dname, uri_str, flags);
+ if (ret < 0) {
+ /*
+ * Confirm phase will not be executed if perform fails. End the
+ * job started in begin phase.
+ */
+ libxlDomainObjEndJob(driver, vm);
+ }
+
cleanup:
orig_err = virSaveLastError();
virObjectUnlock(vm);
@@ -1232,11 +1248,17 @@ libxlDomainMigrationSrcPerform(libxlDriverPrivatePtr driver,
ret = libxlDoMigrateSrcSend(driver, vm, flags, sockfd);
virObjectLock(vm);
- if (ret < 0)
+ if (ret < 0) {
virDomainLockProcessResume(driver->lockManager,
"xen:///system",
vm,
priv->lockState);
+ /*
+ * Confirm phase will not be executed if perform fails. End the
+ * job started in begin phase.
+ */
+ libxlDomainObjEndJob(driver, vm);
+ }
cleanup:
VIR_FORCE_CLOSE(sockfd);
@@ -1386,6 +1408,8 @@ libxlDomainMigrationSrcConfirm(libxlDriverPrivatePtr driver,
ret = 0;
cleanup:
+ /* EndJob for corresponding BeginJob in begin phase */
+ libxlDomainObjEndJob(driver, vm);
virObjectEventStateQueue(driver->domainEventState, event);
virObjectUnref(cfg);
return ret;
--
2.18.0
4:28 p.m.
New subject: [libvirt] [PATCH 4/5] libxl: fix job handling across migration phases on dst
The libxlDomainMigrationDst* functions are a bit flawed in their
handling of modify jobs. A job begins when the destination host
begins receiving the incoming VM and ends after the VM is started.
The finish phase contains another BeginJob/EndJob sequence.
This patch changes the logic to begin a job for the incoming VM
in the prepare phase and end the job in the finish phase.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_driver.c | 7 ----
src/libxl/libxl_migration.c | 65 +++++++++++++++++++++++--------------
2 files changed, 40 insertions(+), 32 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 5a5e792957..73c2ff3546 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -6020,15 +6020,8 @@ libxlDomainMigrateFinish3Params(virConnectPtr dconn,
return NULL;
}
- if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
- virDomainObjEndAPI(&vm);
- return NULL;
- }
-
ret = libxlDomainMigrationDstFinish(dconn, vm, flags, cancelled);
- libxlDomainObjEndJob(driver, vm);
-
virDomainObjEndAPI(&vm);
return ret;
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 191973edeb..54b01a3169 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -266,9 +266,6 @@ libxlDoMigrateDstReceive(void *opaque)
size_t i;
virObjectRef(vm);
- virObjectLock(vm);
- if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
- goto cleanup;
/*
* Always start the domain paused. If needed, unpause in the
@@ -288,10 +285,6 @@ libxlDoMigrateDstReceive(void *opaque)
args->nsocks = 0;
VIR_FORCE_CLOSE(recvfd);
virObjectUnref(args);
-
- libxlDomainObjEndJob(driver, vm);
-
- cleanup:
virDomainObjEndAPI(&vm);
}
@@ -583,6 +576,13 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
goto error;
*def = NULL;
+ /*
+ * Unless an error is encountered in this function, the job will
+ * be terminated in the finish phase.
+ */
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto error;
+
priv = vm->privateData;
if (taint_hook) {
@@ -595,18 +595,18 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
* stream -> pipe -> recvfd of libxlDomainStartRestore
*/
if (pipe(dataFD) < 0)
- goto error;
+ goto endjob;
/* Stream data will be written to pipeIn */
if (virFDStreamOpen(st, dataFD[1]) < 0)
- goto error;
+ goto endjob;
dataFD[1] = -1; /* 'st' owns the FD now & will close it */
if (libxlMigrationDstArgsInitialize() < 0)
- goto error;
+ goto endjob;
if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
- goto error;
+ goto endjob;
args->conn = virObjectRef(dconn);
args->vm = virObjectRef(vm);
@@ -620,12 +620,15 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
if (virThreadCreate(&thread, false, libxlDoMigrateDstReceive, args) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to create thread for receiving migration
data"));
- goto error;
+ goto endjob;
}
ret = 0;
goto done;
+ endjob:
+ libxlDomainObjEndJob(driver, vm);
+
error:
libxlMigrationCookieFree(mig);
VIR_FORCE_CLOSE(dataFD[1]);
@@ -679,6 +682,13 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
goto error;
*def = NULL;
+ /*
+ * Unless an error is encountered in this function, the job will
+ * be terminated in the finish phase.
+ */
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto error;
+
priv = vm->privateData;
if (taint_hook) {
@@ -689,27 +699,27 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
/* Create socket connection to receive migration data */
if (!uri_in) {
if ((hostname = virGetHostname()) == NULL)
- goto error;
+ goto endjob;
if (STRPREFIX(hostname, "localhost")) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("hostname on destination resolved to localhost,"
" but migration requires an FQDN"));
- goto error;
+ goto endjob;
}
if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
- goto error;
+ goto endjob;
priv->migrationPort = port;
if (virAsprintf(uri_out, "tcp://%s:%d", hostname, port) < 0)
- goto error;
+ goto endjob;
} else {
if (!(STRPREFIX(uri_in, "tcp://"))) {
/* not full URI, add prefix tcp:// */
char *tmp;
if (virAsprintf(&tmp, "tcp://%s", uri_in) < 0)
- goto error;
+ goto endjob;
uri = virURIParse(tmp);
VIR_FREE(tmp);
} else {
@@ -720,20 +730,20 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
virReportError(VIR_ERR_INVALID_ARG,
_("unable to parse URI: %s"),
uri_in);
- goto error;
+ goto endjob;
}
if (uri->server == NULL) {
virReportError(VIR_ERR_INVALID_ARG,
_("missing host in migration URI: %s"),
uri_in);
- goto error;
+ goto endjob;
}
hostname = uri->server;
if (uri->port == 0) {
if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
- goto error;
+ goto endjob;
priv->migrationPort = port;
} else {
@@ -741,7 +751,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
}
if (virAsprintf(uri_out, "tcp://%s:%d", hostname, port) < 0)
- goto error;
+ goto endjob;
}
snprintf(portstr, sizeof(portstr), "%d", port);
@@ -751,14 +761,14 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
&socks, &nsocks) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Fail to create socket for incoming migration"));
- goto error;
+ goto endjob;
}
if (libxlMigrationDstArgsInitialize() < 0)
- goto error;
+ goto endjob;
if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
- goto error;
+ goto endjob;
args->conn = virObjectRef(dconn);
args->vm = virObjectRef(vm);
@@ -786,11 +796,14 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
}
if (!nsocks_listen)
- goto error;
+ goto endjob;
ret = 0;
goto done;
+ endjob:
+ libxlDomainObjEndJob(driver, vm);
+
error:
for (i = 0; i < nsocks; i++) {
virNetSocketClose(socks[i]);
@@ -1354,6 +1367,8 @@ libxlDomainMigrationDstFinish(virConnectPtr dconn,
virDomainObjListRemove(driver->domains, vm);
}
+ /* EndJob for corresponding BeginJob in prepare phase */
+ libxlDomainObjEndJob(driver, vm);
virObjectEventStateQueue(driver->domainEventState, event);
virObjectUnref(cfg);
return dom;
--
2.18.0
4:28 p.m.
New subject: [libvirt] [PATCH 5/5] libxl: join with thread receiving migration data
It is possible the incoming VM is not fully started when the finish
phase of migration is executed. In libxlDomainMigrationDstFinish,
wait for the thread receiving the VM to complete before executing
finish phase tasks.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_domain.h | 1 +
src/libxl/libxl_migration.c | 20 ++++++++++++++++----
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index 5d83230cd6..e193881450 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -65,6 +65,7 @@ struct _libxlDomainObjPrivate {
/* console */
virChrdevsPtr devs;
libxl_evgen_domain_death *deathW;
+ virThreadPtr migrationDstReceiveThr;
unsigned short migrationPort;
char *lockState;
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 54b01a3169..fc7ccb53d0 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -297,9 +297,9 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
libxlMigrationDstArgs *args = opaque;
virNetSocketPtr *socks = args->socks;
size_t nsocks = args->nsocks;
+ libxlDomainObjPrivatePtr priv = args->vm->privateData;
virNetSocketPtr client_sock;
int recvfd = -1;
- virThread thread;
size_t i;
/* Accept migration connection */
@@ -318,7 +318,10 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
* the migration data
*/
args->recvfd = recvfd;
- if (virThreadCreate(&thread, false,
+ VIR_FREE(priv->migrationDstReceiveThr);
+ if (VIR_ALLOC(priv->migrationDstReceiveThr) < 0)
+ goto fail;
+ if (virThreadCreate(priv->migrationDstReceiveThr, true,
libxlDoMigrateDstReceive, args) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to create thread for receiving migration
data"));
@@ -557,7 +560,6 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
libxlDriverPrivatePtr driver = dconn->privateData;
virDomainObjPtr vm = NULL;
libxlMigrationDstArgs *args = NULL;
- virThread thread;
bool taint_hook = false;
libxlDomainObjPrivatePtr priv = NULL;
char *xmlout = NULL;
@@ -617,7 +619,10 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
args->nsocks = 0;
mig = NULL;
- if (virThreadCreate(&thread, false, libxlDoMigrateDstReceive, args) < 0) {
+ VIR_FREE(priv->migrationDstReceiveThr);
+ if (VIR_ALLOC(priv->migrationDstReceiveThr) < 0)
+ goto error;
+ if (virThreadCreate(priv->migrationDstReceiveThr, true, libxlDoMigrateDstReceive,
args) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to create thread for receiving migration
data"));
goto endjob;
@@ -1291,6 +1296,13 @@ libxlDomainMigrationDstFinish(virConnectPtr dconn,
virObjectEventPtr event = NULL;
virDomainPtr dom = NULL;
+ if (priv->migrationDstReceiveThr) {
+ virObjectUnlock(vm);
+ virThreadJoin(priv->migrationDstReceiveThr);
+ virObjectLock(vm);
+ VIR_FREE(priv->migrationDstReceiveThr);
+ }
+
virPortAllocatorRelease(priv->migrationPort);
priv->migrationPort = 0;
--
2.18.0
2:29 a.m.
On 09/05/2018 11:27 PM, Jim Fehlig wrote:
Patch 5 fixes a long standing problem found by some very slow hosts
in
xen's osstest
https://lists.xenproject.org/archives/html/xen-devel/2018-08/msg01945.html
While working on the fix, I discovered other problems in libxl's V3
migration protocol. E.g. a modify job on the migrating VM was not
handled properly across the phases on either src or dst host. Patches
1-4 fix this and other problems found along the way.
Jim Fehlig (5):
libxl: migration: defer removing VM until finish phase
libxl: fix logic in P2P migration
libxl: fix job handling across migration phases on src
libxl: fix job handling across migration phases on dst
libxl: join with thread receiving migration data
src/libxl/libxl_domain.h | 1 +
src/libxl/libxl_driver.c | 7 --
src/libxl/libxl_migration.c | 168 ++++++++++++++++++++++++------------
3 files changed, 114 insertions(+), 62 deletions(-)
ACK series.
Michal
2264
days inactive
2270
days old
6 comments
2 participants
participants (2)
-
Jim Fehlig -
Michal Privoznik