4:26 a.m.
When QEMU exits on destination during migration, the source reports
either success (if the failure happened at the very end) or unhelpful
"unexpectedly failed" error message, only the destination host knows
more about the error, but usually there's no way to get the error back
to the source because the domain is already gone at the time
virDomainMigrateFinish* is called.
This series changes libvirt to remember incoming migration errors so
that they can be sent back to a source host.
https://bugzilla.redhat.com/show_bug.cgi?id=1090093
Jiri Denemark (5):
Introduce virHashAtomic
Introduce virErrorCopyNew
qemu: Remember incoming migration errors
qemu: Don't report false error from MigrateFinish
qemu: Use error from Finish instead of "unexpectedly failed"
docs/apibuild.py | 1 +
include/libvirt/virterror.h | 1 +
src/libvirt-domain.c | 30 ++++++++++++-
src/libvirt_private.syms | 4 ++
src/qemu/qemu_conf.h | 3 ++
src/qemu/qemu_driver.c | 31 +++++++++----
src/qemu/qemu_migration.c | 104 ++++++++++++++++++++++++++++++++++++++++++--
src/qemu/qemu_migration.h | 7 +++
src/qemu/qemu_monitor.c | 14 ++++++
src/qemu/qemu_monitor.h | 2 +
src/qemu/qemu_process.c | 4 ++
src/util/virerror.c | 22 ++++++++++
src/util/virerror.h | 1 +
src/util/virhash.c | 81 ++++++++++++++++++++++++++++++++++
src/util/virhash.h | 10 +++++
15 files changed, 302 insertions(+), 13 deletions(-)
--
2.4.5
4:26 a.m.
New subject: [libvirt] [PATCH v2 1/5] Introduce virHashAtomic
This is a self-locking wrapper around virHashTable. Only a limited set
of APIs are implemented now (the ones which are used in the following
patch) as more can be added on demand.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- s/virHashLockable/virHashAtomic/
src/libvirt_private.syms | 3 ++
src/util/virhash.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virhash.h | 10 ++++++
3 files changed, 94 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 720afdf..65168b1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1476,6 +1476,9 @@ virFirewallStartTransaction;
# util/virhash.h
virHashAddEntry;
+virHashAtomicNew;
+virHashAtomicSteal;
+virHashAtomicUpdate;
virHashCreate;
virHashEqual;
virHashForEach;
diff --git a/src/util/virhash.c b/src/util/virhash.c
index e3c1880..3cfcc69 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -31,6 +31,7 @@
#include "virhashcode.h"
#include "virrandom.h"
#include "virstring.h"
+#include "virobject.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -76,6 +77,28 @@ struct _virHashTable {
virHashKeyFree keyFree;
};
+struct _virHashAtomic {
+ virObjectLockable parent;
+ virHashTablePtr hash;
+};
+
+static virClassPtr virHashAtomicClass;
+static void virHashAtomicDispose(void *obj);
+
+static int virHashAtomicOnceInit(void)
+{
+ virHashAtomicClass = virClassNew(virClassForObjectLockable(),
+ "virHashAtomic",
+ sizeof(virHashAtomic),
+ virHashAtomicDispose);
+ if (!virHashAtomicClass)
+ return -1;
+ else
+ return 0;
+}
+VIR_ONCE_GLOBAL_INIT(virHashAtomic)
+
+
static uint32_t virHashStrCode(const void *name, uint32_t seed)
{
return virHashCodeGen(name, strlen(name), seed);
@@ -178,6 +201,36 @@ virHashTablePtr virHashCreate(ssize_t size, virHashDataFree
dataFree)
virHashStrFree);
}
+
+virHashAtomicPtr
+virHashAtomicNew(ssize_t size,
+ virHashDataFree dataFree)
+{
+ virHashAtomicPtr hash;
+
+ if (virHashAtomicInitialize() < 0)
+ return NULL;
+
+ if (!(hash = virObjectLockableNew(virHashAtomicClass)))
+ return NULL;
+
+ if (!(hash->hash = virHashCreate(size, dataFree))) {
+ virObjectUnref(hash);
+ return NULL;
+ }
+ return hash;
+}
+
+
+static void
+virHashAtomicDispose(void *obj)
+{
+ virHashAtomicPtr hash = obj;
+
+ virHashFree(hash->hash);
+}
+
+
/**
* virHashGrow:
* @table: the hash table
@@ -360,6 +413,21 @@ virHashUpdateEntry(virHashTablePtr table, const void *name,
return virHashAddOrUpdateEntry(table, name, userdata, true);
}
+int
+virHashAtomicUpdate(virHashAtomicPtr table,
+ const void *name,
+ void *userdata)
+{
+ int ret;
+
+ virObjectLock(table);
+ ret = virHashAddOrUpdateEntry(table->hash, name, userdata, true);
+ virObjectUnlock(table);
+
+ return ret;
+}
+
+
/**
* virHashLookup:
* @table: the hash table
@@ -409,6 +477,19 @@ void *virHashSteal(virHashTablePtr table, const void *name)
return data;
}
+void *
+virHashAtomicSteal(virHashAtomicPtr table,
+ const void *name)
+{
+ void *data;
+
+ virObjectLock(table);
+ data = virHashSteal(table->hash, name);
+ virObjectUnlock(table);
+
+ return data;
+}
+
/**
* virHashSize:
diff --git a/src/util/virhash.h b/src/util/virhash.h
index a137137..50b9a04 100644
--- a/src/util/virhash.h
+++ b/src/util/virhash.h
@@ -21,6 +21,9 @@
typedef struct _virHashTable virHashTable;
typedef virHashTable *virHashTablePtr;
+typedef struct _virHashAtomic virHashAtomic;
+typedef virHashAtomic *virHashAtomicPtr;
+
/*
* function types:
*/
@@ -101,6 +104,8 @@ typedef void (*virHashKeyFree)(void *name);
*/
virHashTablePtr virHashCreate(ssize_t size,
virHashDataFree dataFree);
+virHashAtomicPtr virHashAtomicNew(ssize_t size,
+ virHashDataFree dataFree);
virHashTablePtr virHashCreateFull(ssize_t size,
virHashDataFree dataFree,
virHashKeyCode keyCode,
@@ -119,6 +124,9 @@ int virHashAddEntry(virHashTablePtr table,
int virHashUpdateEntry(virHashTablePtr table,
const void *name,
void *userdata);
+int virHashAtomicUpdate(virHashAtomicPtr table,
+ const void *name,
+ void *userdata);
/*
* Remove an entry from the hash table.
@@ -140,6 +148,8 @@ void *virHashLookup(const virHashTable *table, const void *name);
* Retrieve & remove the userdata.
*/
void *virHashSteal(virHashTablePtr table, const void *name);
+void *virHashAtomicSteal(virHashAtomicPtr table,
+ const void *name);
/*
* Get the hash table's key/value pairs and have them optionally sorted.
--
2.4.5
4:35 a.m.
New subject: [libvirt] [PATCH v2 1/5] Introduce virHashAtomic
On Fri, Jul 10, 2015 at 11:26:14 +0200, Jiri Denemark wrote:
This is a self-locking wrapper around virHashTable. Only a limited
set
of APIs are implemented now (the ones which are used in the following
patch) as more can be added on demand.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- s/virHashLockable/virHashAtomic/
src/libvirt_private.syms | 3 ++
src/util/virhash.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virhash.h | 10 ++++++
3 files changed, 94 insertions(+)
ACK
4:26 a.m.
New subject: [libvirt] [PATCH v2 2/5] Introduce virErrorCopyNew
A helper function for copying error objects.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- new patch
docs/apibuild.py | 1 +
src/libvirt_private.syms | 1 +
src/util/virerror.c | 19 +++++++++++++++++++
src/util/virerror.h | 1 +
4 files changed, 22 insertions(+)
diff --git a/docs/apibuild.py b/docs/apibuild.py
index 69f991d..f934fb2 100755
--- a/docs/apibuild.py
+++ b/docs/apibuild.py
@@ -102,6 +102,7 @@ ignored_functions = {
"virDomainMigratePrepare3Params": "private function for
migration",
"virDomainMigrateConfirm3Params": "private function for
migration",
"virDomainMigratePrepareTunnel3Params": "private function for tunnelled
migration",
+ "virErrorCopyNew": "private",
}
ignored_macros = {
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 65168b1..1a03268 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1365,6 +1365,7 @@ ebtablesRemoveForwardAllowIn;
# util/virerror.h
virDispatchError;
+virErrorCopyNew;
virErrorInitialize;
virErrorSetErrnoFromLastError;
virLastErrorIsSystemErrno;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index fae627b..3ec12a3 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -214,6 +214,25 @@ virCopyError(virErrorPtr from,
return ret;
}
+
+virErrorPtr
+virErrorCopyNew(virErrorPtr err)
+{
+ virErrorPtr ret;
+
+ if (!err)
+ return NULL;
+
+ if (VIR_ALLOC_QUIET(ret) < 0)
+ return NULL;
+
+ if (virCopyError(err, ret) < 0)
+ VIR_FREE(ret);
+
+ return ret;
+}
+
+
static virErrorPtr
virLastErrorObject(void)
{
diff --git a/src/util/virerror.h b/src/util/virerror.h
index baa2d08..2348648 100644
--- a/src/util/virerror.h
+++ b/src/util/virerror.h
@@ -185,6 +185,7 @@ void virReportOOMErrorFull(int domcode,
virRaiseErrorObject(__FILE__, __FUNCTION__, __LINE__, obj)
int virSetError(virErrorPtr newerr);
+virErrorPtr virErrorCopyNew(virErrorPtr err);
void virDispatchError(virConnectPtr conn);
const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);
--
2.4.5
4:36 a.m.
New subject: [libvirt] [PATCH v2 2/5] Introduce virErrorCopyNew
On Fri, Jul 10, 2015 at 11:26:15 +0200, Jiri Denemark wrote:
A helper function for copying error objects.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- new patch
docs/apibuild.py | 1 +
src/libvirt_private.syms | 1 +
src/util/virerror.c | 19 +++++++++++++++++++
src/util/virerror.h | 1 +
4 files changed, 22 insertions(+)
ACK
4:38 a.m.
New subject: [libvirt] [PATCH v2 2/5] Introduce virErrorCopyNew
On Fri, Jul 10, 2015 at 11:26:15 +0200, Jiri Denemark wrote:
A helper function for copying error objects.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- new patch
docs/apibuild.py | 1 +
src/libvirt_private.syms | 1 +
src/util/virerror.c | 19 +++++++++++++++++++
src/util/virerror.h | 1 +
4 files changed, 22 insertions(+)
...
diff --git a/src/util/virerror.c b/src/util/virerror.c
index fae627b..3ec12a3 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -214,6 +214,25 @@ virCopyError(virErrorPtr from,
return ret;
}
+
+virErrorPtr
+virErrorCopyNew(virErrorPtr err)
+{
+ virErrorPtr ret;
+
+ if (!err)
+ return NULL;
Perhaps drop the above hunk so that it's enforced that this func returns
NULL only on failure. ACK stads if you choose to do so.
+
+ if (VIR_ALLOC_QUIET(ret) < 0)
+ return NULL;
+
+ if (virCopyError(err, ret) < 0)
+ VIR_FREE(ret);
+
+ return ret;
+}
4:26 a.m.
New subject: [libvirt] [PATCH v2 3/5] qemu: Remember incoming migration errors
If QEMU fails during incoming migration, the domain disappears including
a possibly useful error message read from QEMU log file. Let's remember
the error in virQEMUDriver so that Finish can report more than just "no
such domain".
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- s/virHashLockable/virHashAtomic/
- use virErrorCopyNew
- more comments
src/qemu/qemu_conf.h | 3 +++
src/qemu/qemu_driver.c | 31 ++++++++++++++++++-------
src/qemu/qemu_migration.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++-
src/qemu/qemu_migration.h | 7 ++++++
src/qemu/qemu_monitor.c | 14 +++++++++++
src/qemu/qemu_monitor.h | 2 ++
src/qemu/qemu_process.c | 4 ++++
7 files changed, 111 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index b74c283..3f73929 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -252,6 +252,9 @@ struct _virQEMUDriver {
/* Immutable pointer, self-clocking APIs */
virCloseCallbacksPtr closeCallbacks;
+
+ /* Immutable pointer, self-locking APIs */
+ virHashAtomicPtr migrationErrors;
};
typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c4b3979..c8cbd57 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -775,6 +775,9 @@ qemuStateInitialize(bool privileged,
if (!(qemu_driver->sharedDevices = virHashCreate(30, qemuSharedDeviceEntryFree)))
goto error;
+ if (qemuMigrationErrorInit(qemu_driver) < 0)
+ goto error;
+
if (privileged) {
char *channeldir;
@@ -1091,6 +1094,7 @@ qemuStateCleanup(void)
virObjectUnref(qemu_driver->remotePorts);
virObjectUnref(qemu_driver->webSocketPorts);
virObjectUnref(qemu_driver->migrationPorts);
+ virObjectUnref(qemu_driver->migrationErrors);
virObjectUnref(qemu_driver->xmlopt);
@@ -12117,6 +12121,7 @@ qemuDomainMigrateFinish2(virConnectPtr dconn,
if (!vm) {
virReportError(VIR_ERR_NO_DOMAIN,
_("no domain with matching name '%s'"), dname);
+ qemuMigrationErrorReport(driver, dname);
goto cleanup;
}
@@ -12566,11 +12571,16 @@ qemuDomainMigrateFinish3(virConnectPtr dconn,
virCheckFlags(QEMU_MIGRATION_FLAGS, NULL);
- if (!dname ||
- !(vm = virDomainObjListFindByName(driver->domains, dname))) {
+ if (!dname) {
+ virReportError(VIR_ERR_NO_DOMAIN, "%s", _("missing domain
name"));
+ return NULL;
+ }
+
+ vm = virDomainObjListFindByName(driver->domains, dname);
+ if (!vm) {
virReportError(VIR_ERR_NO_DOMAIN,
- _("no domain with matching name '%s'"),
- NULLSTR(dname));
+ _("no domain with matching name '%s'"), dname);
+ qemuMigrationErrorReport(driver, dname);
return NULL;
}
@@ -12609,11 +12619,16 @@ qemuDomainMigrateFinish3Params(virConnectPtr dconn,
&dname) < 0)
return NULL;
- if (!dname ||
- !(vm = virDomainObjListFindByName(driver->domains, dname))) {
+ if (!dname) {
+ virReportError(VIR_ERR_NO_DOMAIN, "%s", _("missing domain
name"));
+ return NULL;
+ }
+
+ vm = virDomainObjListFindByName(driver->domains, dname);
+ if (!vm) {
virReportError(VIR_ERR_NO_DOMAIN,
- _("no domain with matching name '%s'"),
- NULLSTR(dname));
+ _("no domain with matching name '%s'"), dname);
+ qemuMigrationErrorReport(driver, dname);
return NULL;
}
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 7257182..58874ee 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5543,8 +5543,10 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
- if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_IN))
+ if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_IN)) {
+ qemuMigrationErrorReport(driver, vm->def->name);
goto cleanup;
+ }
qemuMigrationJobStartPhase(driver, vm,
v3proto ? QEMU_MIGRATION_PHASE_FINISH3
@@ -5570,6 +5572,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
if (!virDomainObjIsActive(vm) && !(flags & VIR_MIGRATE_OFFLINE)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest unexpectedly quit"));
+ qemuMigrationErrorReport(driver, vm->def->name);
goto endjob;
}
@@ -6094,3 +6097,57 @@ qemuMigrationJobFinish(virQEMUDriverPtr driver, virDomainObjPtr
vm)
{
qemuDomainObjEndAsyncJob(driver, vm);
}
+
+
+static void
+qemuMigrationErrorFree(void *data,
+ const void *name ATTRIBUTE_UNUSED)
+{
+ virErrorPtr err = data;
+ virFreeError(err);
+}
+
+int
+qemuMigrationErrorInit(virQEMUDriverPtr driver)
+{
+ driver->migrationErrors = virHashAtomicNew(64, qemuMigrationErrorFree);
+ if (driver->migrationErrors)
+ return 0;
+ else
+ return -1;
+}
+
+/**
+ * This function consumes @err; the caller should consider the @err pointer
+ * invalid after calling this function.
+ */
+void
+qemuMigrationErrorSave(virQEMUDriverPtr driver,
+ const char *name,
+ virErrorPtr err)
+{
+ if (!err)
+ return;
+
+ VIR_DEBUG("Saving incoming migration error for domain %s: %s",
+ name, err->message);
+ if (virHashAtomicUpdate(driver->migrationErrors, name, err) < 0) {
+ VIR_WARN("Failed to save migration error for domain '%s'",
name);
+ virFreeError(err);
+ }
+}
+
+void
+qemuMigrationErrorReport(virQEMUDriverPtr driver,
+ const char *name)
+{
+ virErrorPtr err;
+
+ if (!(err = virHashAtomicSteal(driver->migrationErrors, name)))
+ return;
+
+ VIR_DEBUG("Restoring saved incoming migration error for domain %s: %s",
+ name, err->message);
+ virSetError(err);
+ virFreeError(err);
+}
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index 48c2e8c..fa14274 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -193,4 +193,11 @@ int qemuMigrationFetchJobStatus(virQEMUDriverPtr driver,
qemuDomainAsyncJob asyncJob,
qemuDomainJobInfoPtr jobInfo);
+int qemuMigrationErrorInit(virQEMUDriverPtr driver);
+void qemuMigrationErrorSave(virQEMUDriverPtr driver,
+ const char *name,
+ virErrorPtr err);
+void qemuMigrationErrorReport(virQEMUDriverPtr driver,
+ const char *name);
+
#endif /* __QEMU_MIGRATION_H__ */
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 6b54f71..4f30b15 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1057,6 +1057,20 @@ qemuMonitorSend(qemuMonitorPtr mon,
}
+/**
+ * This function returns a new virError object; the caller is responsible
+ * for freeing it.
+ */
+virErrorPtr
+qemuMonitorLastError(qemuMonitorPtr mon)
+{
+ if (mon->lastError.code == VIR_ERR_OK)
+ return NULL;
+
+ return virErrorCopyNew(&mon->lastError);
+}
+
+
virJSONValuePtr
qemuMonitorGetOptions(qemuMonitorPtr mon)
{
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index ab7d5a7..1e7b6bb 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -240,6 +240,8 @@ qemuMonitorPtr qemuMonitorOpenFD(virDomainObjPtr vm,
void qemuMonitorClose(qemuMonitorPtr mon);
+virErrorPtr qemuMonitorLastError(qemuMonitorPtr mon);
+
int qemuMonitorSetCapabilities(qemuMonitorPtr mon);
int qemuMonitorSetLink(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 129cd69..2a529f7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -310,6 +310,10 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
auditReason = "failed";
}
+ if (qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_IN))
+ qemuMigrationErrorSave(driver, vm->def->name,
+ qemuMonitorLastError(priv->mon));
+
event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
eventReason);
--
2.4.5
4:41 a.m.
New subject: [libvirt] [PATCH v2 3/5] qemu: Remember incoming migration errors
On Fri, Jul 10, 2015 at 11:26:16 +0200, Jiri Denemark wrote:
If QEMU fails during incoming migration, the domain disappears
including
a possibly useful error message read from QEMU log file. Let's remember
the error in virQEMUDriver so that Finish can report more than just "no
such domain".
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- s/virHashLockable/virHashAtomic/
- use virErrorCopyNew
- more comments
src/qemu/qemu_conf.h | 3 +++
src/qemu/qemu_driver.c | 31 ++++++++++++++++++-------
src/qemu/qemu_migration.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++-
src/qemu/qemu_migration.h | 7 ++++++
src/qemu/qemu_monitor.c | 14 +++++++++++
src/qemu/qemu_monitor.h | 2 ++
src/qemu/qemu_process.c | 4 ++++
7 files changed, 111 insertions(+), 9 deletions(-)
ACK
4:26 a.m.
New subject: [libvirt] [PATCH v2 4/5] qemu: Don't report false error from MigrateFinish
virDomainMigrateFinish* APIs were unfortunately designed to return the
pointer to the domain on destination and NULL on error. This looks OK in
normal cases but the same API is also called when we know migration
failed and thus we expect Finish to return NULL even if it actually did
all it was supposed to do without any error. The call is defined to
return nonnull domain pointer over RPC, which means returning NULL will
always result in an error being send. If this was not in fact an error,
the API itself wouldn't set anything to the thread local virError, which
makes the RPC layer come up with it's own "Library function returned
error but did not set virError" error.
This is quite confusing and also hard to detect by the caller. This
patch adds a special error code which can be used to check that Finish
successfully aborted migration.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- no change
include/libvirt/virterror.h | 1 +
src/qemu/qemu_migration.c | 6 ++++++
src/util/virerror.c | 3 +++
3 files changed, 10 insertions(+)
diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 6325030..f716cb9 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -307,6 +307,7 @@ typedef enum {
VIR_ERR_CPU_INCOMPATIBLE = 91, /* given CPU is incompatible with host
CPU*/
VIR_ERR_XML_INVALID_SCHEMA = 92, /* XML document doesn't validate against
schema */
+ VIR_ERR_MIGRATE_FINISH_OK = 93, /* Finish API succeeded but it is expected to
return NULL */
} virErrorNumber;
/**
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 58874ee..a9cbada 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5751,6 +5751,12 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
}
virObjectUnref(caps);
virObjectUnref(cfg);
+
+ /* Set a special error if Finish is expected to return NULL as a result of
+ * successful call with retcode != 0
+ */
+ if (retcode != 0 && !dom && !virGetLastError())
+ virReportError(VIR_ERR_MIGRATE_FINISH_OK, NULL);
return dom;
}
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 3ec12a3..26ac691 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -1372,6 +1372,9 @@ virErrorMsg(virErrorNumber error, const char *info)
else
errmsg = _("XML document failed to validate against schema:
%s");
break;
+ case VIR_ERR_MIGRATE_FINISH_OK:
+ errmsg = _("migration successfully aborted");
+ break;
}
return errmsg;
}
--
2.4.5
8:51 a.m.
New subject: [libvirt] [PATCH v2 4/5] qemu: Don't report false error from MigrateFinish
On Fri, Jul 10, 2015 at 11:26:17AM +0200, Jiri Denemark wrote:
virDomainMigrateFinish* APIs were unfortunately designed to return
the
pointer to the domain on destination and NULL on error. This looks OK in
normal cases but the same API is also called when we know migration
failed and thus we expect Finish to return NULL even if it actually did
all it was supposed to do without any error. The call is defined to
return nonnull domain pointer over RPC, which means returning NULL will
always result in an error being send. If this was not in fact an error,
the API itself wouldn't set anything to the thread local virError, which
makes the RPC layer come up with it's own "Library function returned
error but did not set virError" error.
This is quite confusing and also hard to detect by the caller. This
patch adds a special error code which can be used to check that Finish
successfully aborted migration.
FWIW, I have come to the conclusion that it is impossible to 100%
reliably detect successful from the virDomainMigrate* APIs, because
it is possible that the libvirt connection to the dest host may
fail, while the domain is in fact successfully running on the target.
So there is always the possbility to get a error from the RPC layer
despite everything at QEMU level being 100% successful.
So in openstack we now ignore the return code for virDomainMigrate
and instead try to detect succcess from the domain job info which
will show that the domain on the source has completed, even if we
loose the dest host RPC connection.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4:26 a.m.
New subject: [libvirt] [PATCH v2 5/5] qemu: Use error from Finish instead of "unexpectedly failed"
When QEMU exits on destination during migration, the source reports
either success (if the failure happened at the very end) or unhelpful
"unexpectedly failed" error message. However, the Finish API called on
the destination may report a real error so let's use it instead of the
generic one.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- cleaner control flow
- more comments
src/libvirt-domain.c | 30 ++++++++++++++++++++++++++++--
src/qemu/qemu_migration.c | 39 +++++++++++++++++++++++++++++++++++++--
2 files changed, 65 insertions(+), 4 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 909c264..837933f 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3175,8 +3175,34 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
(dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen,
NULL, uri, destflags, cancelled);
}
- if (cancelled && ddomain)
- VIR_ERROR(_("finish step ignored that migration was cancelled"));
+
+ if (cancelled) {
+ if (ddomain) {
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
+ } else {
+ /* If Finish reported a useful error, use it instead of the
+ * original "migration unexpectedly failed" error.
+ *
+ * This is ugly but we can't do better with the APIs we have. We
+ * only replace the error if Finish was called with cancelled == 1
+ * and reported a real error (old libvirt would report an error
+ * from RPC instead of MIGRATE_FINISH_OK), which only happens when
+ * the domain died on destination. To further reduce a possibility
+ * of false positives we also check that Perform returned
+ * VIR_ERR_OPERATION_FAILED.
+ */
+ if (orig_err &&
+ orig_err->domain == VIR_FROM_QEMU &&
+ orig_err->code == VIR_ERR_OPERATION_FAILED) {
+ virErrorPtr err = virGetLastError();
+ if (err->domain == VIR_FROM_QEMU &&
+ err->code != VIR_ERR_MIGRATE_FINISH_OK) {
+ virFreeError(orig_err);
+ orig_err = NULL;
+ }
+ }
+ }
+ }
/* If ddomain is NULL, then we were unable to start
* the guest on the target, and must restart on the
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a9cbada..d789110 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -4982,8 +4982,34 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver,
dconnuri, uri, destflags, cancelled);
qemuDomainObjExitRemote(vm);
}
- if (cancelled && ddomain)
- VIR_ERROR(_("finish step ignored that migration was cancelled"));
+
+ if (cancelled) {
+ if (ddomain) {
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
+ } else {
+ /* If Finish reported a useful error, use it instead of the
+ * original "migration unexpectedly failed" error.
+ *
+ * This is ugly but we can't do better with the APIs we have. We
+ * only replace the error if Finish was called with cancelled == 1
+ * and reported a real error (old libvirt would report an error
+ * from RPC instead of MIGRATE_FINISH_OK), which only happens when
+ * the domain died on destination. To further reduce a possibility
+ * of false positives we also check that Perform returned
+ * VIR_ERR_OPERATION_FAILED.
+ */
+ if (orig_err &&
+ orig_err->domain == VIR_FROM_QEMU &&
+ orig_err->code == VIR_ERR_OPERATION_FAILED) {
+ virErrorPtr err = virGetLastError();
+ if (err->domain == VIR_FROM_QEMU &&
+ err->code != VIR_ERR_MIGRATE_FINISH_OK) {
+ virFreeError(orig_err);
+ orig_err = NULL;
+ }
+ }
+ }
+ }
/* If ddomain is NULL, then we were unable to start
* the guest on the target, and must restart on the
@@ -5719,6 +5745,15 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
/* Guest is successfully running, so cancel previous auto destroy */
qemuProcessAutoDestroyRemove(driver, vm);
} else if (!(flags & VIR_MIGRATE_OFFLINE)) {
+ qemuDomainJobInfo info;
+
+ /* Check for a possible error on the monitor in case Finish was called
+ * earlier than monitor EOF handler got a chance to process the error
+ */
+ qemuMigrationFetchJobStatus(driver, vm,
+ QEMU_ASYNC_JOB_MIGRATION_IN,
+ &info);
+
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
VIR_QEMU_PROCESS_STOP_MIGRATED);
virDomainAuditStop(vm, "failed");
--
2.4.5
4:45 a.m.
New subject: [libvirt] [PATCH v2 5/5] qemu: Use error from Finish instead of "unexpectedly failed"
On Fri, Jul 10, 2015 at 11:26:18 +0200, Jiri Denemark wrote:
When QEMU exits on destination during migration, the source reports
either success (if the failure happened at the very end) or unhelpful
"unexpectedly failed" error message. However, the Finish API called on
the destination may report a real error so let's use it instead of the
generic one.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
Notes:
Version 2:
- cleaner control flow
- more comments
src/libvirt-domain.c | 30 ++++++++++++++++++++++++++++--
src/qemu/qemu_migration.c | 39 +++++++++++++++++++++++++++++++++++++--
2 files changed, 65 insertions(+), 4 deletions(-)
ACK
3447
days inactive
3454
days old
11 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Peter Krempa