[libvirt] [PATCH] qemu: Check QEMU error on failed migration

When migration fails, QEMU may provide a description of the error in the reply to query-migrate QMP command. We can fetch this error and use it instead of the generic "unexpectedly failed" message. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 36 +++++++++++++++++++++++------------- src/qemu/qemu_migration.h | 3 ++- src/qemu/qemu_monitor.c | 8 ++++++-- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 18 ++++++++++++++---- src/qemu/qemu_monitor_json.h | 3 ++- tests/qemumonitorjsontest.c | 29 ++++++++++++++++++++++++++--- 8 files changed, 77 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7c6f1674a9..cc79d7d4e9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13130,7 +13130,8 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver, jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) { if (events && jobInfo->status != QEMU_DOMAIN_JOB_STATUS_ACTIVE && - qemuMigrationFetchStats(driver, vm, QEMU_ASYNC_JOB_NONE, jobInfo) < 0) + qemuMigrationFetchStats(driver, vm, QEMU_ASYNC_JOB_NONE, + jobInfo, NULL) < 0) goto cleanup; if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE && diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index dd60071bfd..b286d68061 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1382,7 +1382,8 @@ int qemuMigrationFetchStats(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, - qemuDomainJobInfoPtr jobInfo) + qemuDomainJobInfoPtr jobInfo, + char **error) { qemuDomainObjPrivatePtr priv = vm->privateData; qemuMonitorMigrationStats stats; @@ -1391,7 +1392,7 @@ qemuMigrationFetchStats(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; - rv = qemuMonitorGetMigrationStats(priv->mon, &stats); + rv = qemuMonitorGetMigrationStats(priv->mon, &stats, error); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0) return -1; @@ -1427,12 +1428,15 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainJobInfoPtr jobInfo = priv->job.current; - + char *error = NULL; bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT); + int ret = -1; - if (!events && - qemuMigrationFetchStats(driver, vm, asyncJob, jobInfo) < 0) - return -1; + if (!events || + jobInfo->stats.status == QEMU_MONITOR_MIGRATION_STATUS_ERROR) { + if (qemuMigrationFetchStats(driver, vm, asyncJob, jobInfo, &error) < 0) + return -1; + } qemuMigrationUpdateJobType(jobInfo); @@ -1440,17 +1444,18 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver, case QEMU_DOMAIN_JOB_STATUS_NONE: virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), qemuMigrationJobName(vm), _("is not active")); - return -1; + goto cleanup; case QEMU_DOMAIN_JOB_STATUS_FAILED: virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), - qemuMigrationJobName(vm), _("unexpectedly failed")); - return -1; + qemuMigrationJobName(vm), + error ? error : _("unexpectedly failed")); + goto cleanup; case QEMU_DOMAIN_JOB_STATUS_CANCELED: virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"), qemuMigrationJobName(vm), _("canceled by client")); - return -1; + goto cleanup; case QEMU_DOMAIN_JOB_STATUS_COMPLETED: case QEMU_DOMAIN_JOB_STATUS_ACTIVE: @@ -1459,7 +1464,12 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver, case QEMU_DOMAIN_JOB_STATUS_POSTCOPY: break; } - return 0; + + ret = 0; + + cleanup: + VIR_FREE(error); + return ret; } @@ -1577,7 +1587,7 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, } if (events) - ignore_value(qemuMigrationFetchStats(driver, vm, asyncJob, jobInfo)); + ignore_value(qemuMigrationFetchStats(driver, vm, asyncJob, jobInfo, NULL)); qemuDomainJobInfoUpdateTime(jobInfo); qemuDomainJobInfoUpdateDowntime(jobInfo); @@ -3177,7 +3187,7 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver, if (virDomainObjGetState(vm, &reason) == VIR_DOMAIN_PAUSED && reason == VIR_DOMAIN_PAUSED_POSTCOPY && qemuMigrationFetchStats(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT, - jobInfo) < 0) + jobInfo, NULL) < 0) VIR_WARN("Could not refresh migration statistics"); qemuDomainJobInfoUpdateTime(jobInfo); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 57c747934d..63a4325624 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -282,7 +282,8 @@ int qemuMigrationFetchStats(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, - qemuDomainJobInfoPtr jobInfo); + qemuDomainJobInfoPtr jobInfo, + char **error); int qemuMigrationErrorInit(virQEMUDriverPtr driver); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7a26785878..551cbb77c7 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2632,12 +2632,16 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon, int qemuMonitorGetMigrationStats(qemuMonitorPtr mon, - qemuMonitorMigrationStatsPtr stats) + qemuMonitorMigrationStatsPtr stats, + char **error) { QEMU_CHECK_MONITOR(mon); + if (error) + *error = NULL; + if (mon->json) - return qemuMonitorJSONGetMigrationStats(mon, stats); + return qemuMonitorJSONGetMigrationStats(mon, stats, error); else return qemuMonitorTextGetMigrationStats(mon, stats); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index d9c27acaef..ed57589db1 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -695,7 +695,8 @@ struct _qemuMonitorMigrationStats { }; int qemuMonitorGetMigrationStats(qemuMonitorPtr mon, - qemuMonitorMigrationStatsPtr stats); + qemuMonitorMigrationStatsPtr stats, + char **error); typedef enum { QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a9070fe636..a4b7708b99 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2792,7 +2792,8 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon, static int qemuMonitorJSONGetMigrationStatsReply(virJSONValuePtr reply, - qemuMonitorMigrationStatsPtr stats) + qemuMonitorMigrationStatsPtr stats, + char **error) { virJSONValuePtr ret; virJSONValuePtr ram; @@ -2801,6 +2802,7 @@ qemuMonitorJSONGetMigrationStatsReply(virJSONValuePtr reply, const char *statusstr; int rc; double mbps; + const char *tmp; ret = virJSONValueObjectGetObject(reply, "return"); @@ -2839,11 +2841,18 @@ qemuMonitorJSONGetMigrationStatsReply(virJSONValuePtr reply, switch ((qemuMonitorMigrationStatus) stats->status) { case QEMU_MONITOR_MIGRATION_STATUS_INACTIVE: case QEMU_MONITOR_MIGRATION_STATUS_SETUP: - case QEMU_MONITOR_MIGRATION_STATUS_ERROR: case QEMU_MONITOR_MIGRATION_STATUS_CANCELLED: case QEMU_MONITOR_MIGRATION_STATUS_LAST: break; + case QEMU_MONITOR_MIGRATION_STATUS_ERROR: + if (error) { + tmp = virJSONValueObjectGetString(ret, "error-desc"); + if (tmp && VIR_STRDUP(*error, tmp) < 0) + return -1; + } + break; + case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE: case QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY: case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED: @@ -2987,7 +2996,8 @@ qemuMonitorJSONGetMigrationStatsReply(virJSONValuePtr reply, int qemuMonitorJSONGetMigrationStats(qemuMonitorPtr mon, - qemuMonitorMigrationStatsPtr stats) + qemuMonitorMigrationStatsPtr stats, + char **error) { int ret = -1; virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-migrate", @@ -3005,7 +3015,7 @@ int qemuMonitorJSONGetMigrationStats(qemuMonitorPtr mon, if (qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup; - if (qemuMonitorJSONGetMigrationStatsReply(reply, stats) < 0) + if (qemuMonitorJSONGetMigrationStatsReply(reply, stats, error) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index f418c74264..7c45be6725 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -141,7 +141,8 @@ int qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon, qemuMonitorMigrationParamsPtr params); int qemuMonitorJSONGetMigrationStats(qemuMonitorPtr mon, - qemuMonitorMigrationStatsPtr stats); + qemuMonitorMigrationStatsPtr stats, + char **error); int qemuMonitorJSONGetMigrationCapabilities(qemuMonitorPtr mon, char ***capabilities); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index df3ef0a932..475fd270e1 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1907,6 +1907,7 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationStats(const void *data) qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); int ret = -1; qemuMonitorMigrationStats stats, expectedStats; + char *error = NULL; if (!test) return -1; @@ -1931,21 +1932,43 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationStats(const void *data) " }" " }," " \"id\": \"libvirt-13\"" + "}") < 0 || + qemuMonitorTestAddItem(test, "query-migrate", + "{" + " \"return\": {" + " \"status\": \"failed\"," + " \"error-desc\": \"It's broken\"" + " }," + " \"id\": \"libvirt-14\"" "}") < 0) goto cleanup; - if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), &stats) < 0) + if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), + &stats, &error) < 0) goto cleanup; - if (memcmp(&stats, &expectedStats, sizeof(stats)) != 0) { + if (memcmp(&stats, &expectedStats, sizeof(stats)) != 0 || error) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Invalid migration status"); + "Invalid migration statistics"); + goto cleanup; + } + + memset(&stats, 0, sizeof(stats)); + if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), + &stats, &error) < 0) + goto cleanup; + + if (stats.status != QEMU_MONITOR_MIGRATION_STATUS_ERROR || + STRNEQ_NULLABLE(error, "It's broken")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Invalid failed migration status"); goto cleanup; } ret = 0; cleanup: qemuMonitorTestFree(test); + VIR_FREE(error); return ret; } -- 2.14.2

On Thu, Oct 12, 2017 at 03:48:29PM +0200, Jiri Denemark wrote:
When migration fails, QEMU may provide a description of the error in the reply to query-migrate QMP command. We can fetch this error and use it instead of the generic "unexpectedly failed" message.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 36 +++++++++++++++++++++++------------- src/qemu/qemu_migration.h | 3 ++- src/qemu/qemu_monitor.c | 8 ++++++-- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 18 ++++++++++++++---- src/qemu/qemu_monitor_json.h | 3 ++- tests/qemumonitorjsontest.c | 29 ++++++++++++++++++++++++++--- 8 files changed, 77 insertions(+), 26 deletions(-)
[...]
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index df3ef0a932..475fd270e1 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1907,6 +1907,7 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationStats(const void *data) qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); int ret = -1; qemuMonitorMigrationStats stats, expectedStats; + char *error = NULL;
if (!test) return -1; @@ -1931,21 +1932,43 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationStats(const void *data) " }" " }," " \"id\": \"libvirt-13\"" + "}") < 0 || + qemuMonitorTestAddItem(test, "query-migrate", + "{" + " \"return\": {" + " \"status\": \"failed\"," + " \"error-desc\": \"It's broken\"" + " }," + " \"id\": \"libvirt-14\"" "}") < 0) goto cleanup;
- if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), &stats) < 0) + if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), + &stats, &error) < 0) goto cleanup;
- if (memcmp(&stats, &expectedStats, sizeof(stats)) != 0) { + if (memcmp(&stats, &expectedStats, sizeof(stats)) != 0 || error) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Invalid migration status"); + "Invalid migration statistics"); + goto cleanup; + }
Do we need to pass the "&error" for the first call of qemuMonitorJSONGetMigrationStats() since we know the answer?
+ + memset(&stats, 0, sizeof(stats)); + if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), + &stats, &error) < 0) + goto cleanup; + + if (stats.status != QEMU_MONITOR_MIGRATION_STATUS_ERROR || + STRNEQ_NULLABLE(error, "It's broken")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Invalid failed migration status"); goto cleanup; }
ret = 0; cleanup: qemuMonitorTestFree(test); + VIR_FREE(error); return ret; }
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

On Mon, Oct 16, 2017 at 17:18:58 +0200, Pavel Hrdina wrote:
On Thu, Oct 12, 2017 at 03:48:29PM +0200, Jiri Denemark wrote:
When migration fails, QEMU may provide a description of the error in the reply to query-migrate QMP command. We can fetch this error and use it instead of the generic "unexpectedly failed" message.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ... - if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), &stats) < 0) + if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), + &stats, &error) < 0) goto cleanup;
- if (memcmp(&stats, &expectedStats, sizeof(stats)) != 0) { + if (memcmp(&stats, &expectedStats, sizeof(stats)) != 0 || error) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Invalid migration status"); + "Invalid migration statistics"); + goto cleanup; + }
Do we need to pass the "&error" for the first call of qemuMonitorJSONGetMigrationStats() since we know the answer?
Well, this is true for all tests. This is just testing that error stays unset if there's no error reported by QEMU.
+ + memset(&stats, 0, sizeof(stats)); + if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), + &stats, &error) < 0) + goto cleanup; + + if (stats.status != QEMU_MONITOR_MIGRATION_STATUS_ERROR || + STRNEQ_NULLABLE(error, "It's broken")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "Invalid failed migration status"); goto cleanup; }
ret = 0; cleanup: qemuMonitorTestFree(test); + VIR_FREE(error); return ret; }
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Thanks, pushed. Jirka
participants (2)
-
Jiri Denemark
-
Pavel Hrdina