1:22 p.m.
There's one bug that got more visible with my patches that
automatically precreate storage on migration. The problem is, if
there's not enough disk space on destination, we successfully finish
the migration instead of failing.
Firstly, we instruct qemu to copy the storage. And as it copies the
data, one write() will return -ENOSPC, eventually, to which qemu
reacts by emitting BLOCK_JOB_ERROR. The event is successfully ignored
by us. Then, since the block job has finished, BLOCK_JOB_COMPLETED
event is emitted too.
Secondly, we are not checking for the block job completion as we
should. Currently, we do the check by issuing 'query-block-jobs' and
then looking in the command's output for the job we are interested in.
If course, we don't fail if the job is not there, in which case the
number of total bytes to be transferred and bytes already transferred,
well they equal both to zero. This is actually the line causing the
bug as it sees both numbers equal to each other and gives green light
to the actual migration.
The fix consist of two parts:
In the first, code handling BLOCK_JOB_ERROR is added. It's a monitor
event and we should have handler for that anyway.
In the second part, the code doing drive mirroring is rewritten so it
waits for the events instead of querying on the monitor repeatedly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 3 ++-
src/conf/domain_conf.h | 1 +
src/qemu/qemu_migration.c | 25 +++++++------------------
src/qemu/qemu_monitor_json.c | 10 ++++++++++
src/qemu/qemu_process.c | 14 ++++++++++++++
5 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d1a483a..751a9b5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -787,7 +787,8 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState,
VIR_DOMAIN_DISK_MIRROR_STATE_LAST,
"none",
"yes",
"abort",
- "pivot")
+ "pivot",
+ "error")
VIR_ENUM_IMPL(virDomainLoader,
VIR_DOMAIN_LOADER_TYPE_LAST,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ac1f4f8..f18fa80 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -654,6 +654,7 @@ typedef enum {
VIR_DOMAIN_DISK_MIRROR_STATE_READY, /* Job in second phase */
VIR_DOMAIN_DISK_MIRROR_STATE_ABORT, /* Job aborted, waiting for event */
VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT, /* Job pivoted, waiting for event */
+ VIR_DOMAIN_DISK_MIRROR_STATE_ERROR, /* Job failed */
VIR_DOMAIN_DISK_MIRROR_STATE_LAST
} virDomainDiskMirrorState;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 77e0b35..e2309ea 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1743,7 +1743,6 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
- virDomainBlockJobInfo info;
/* skip shared, RO and source-less disks */
if (disk->src->shared || disk->src->readonly ||
@@ -1768,6 +1767,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
if (mon_ret < 0)
goto error;
+ disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
lastGood = i;
/* wait for completion */
@@ -1775,38 +1775,27 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
/* Poll every 500ms for progress & to allow cancellation */
struct timespec ts = { .tv_sec = 0, .tv_nsec = 500 * 1000 * 1000ull };
- memset(&info, 0, sizeof(info));
-
- if (qemuDomainObjEnterMonitorAsync(driver, vm,
- QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
- goto error;
if (priv->job.asyncAbort) {
/* explicitly do this *after* we entered the monitor,
* as this is a critical section so we are guaranteed
* priv->job.asyncAbort will not change */
- qemuDomainObjExitMonitor(driver, vm);
priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
_("canceled by client"));
goto error;
}
- mon_ret = qemuMonitorBlockJobInfo(priv->mon, diskAlias, &info,
- NULL);
- qemuDomainObjExitMonitor(driver, vm);
- if (mon_ret < 0)
- goto error;
-
- if (info.cur == info.end) {
+ if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
VIR_DEBUG("Drive mirroring of '%s' completed",
diskAlias);
break;
+ } else if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_ERROR) {
+ virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
+ qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
+ _("canceled by destination"));
+ goto error;
}
- /* XXX Frankly speaking, we should listen to the events,
- * instead of doing this. But this works for now and we
- * are doing something similar in migration itself anyway */
-
virObjectUnlock(vm);
nanosleep(&ts, NULL);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e567aa7..5d6bbca 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -76,6 +76,7 @@ static void qemuMonitorJSONHandlePMWakeup(qemuMonitorPtr mon,
virJSONValuePtr da
static void qemuMonitorJSONHandlePMSuspend(qemuMonitorPtr mon, virJSONValuePtr data);
static void qemuMonitorJSONHandleBlockJobCompleted(qemuMonitorPtr mon, virJSONValuePtr
data);
static void qemuMonitorJSONHandleBlockJobCanceled(qemuMonitorPtr mon, virJSONValuePtr
data);
+static void qemuMonitorJSONHandleBlockJobError(qemuMonitorPtr mon, virJSONValuePtr
data);
static void qemuMonitorJSONHandleBlockJobReady(qemuMonitorPtr mon, virJSONValuePtr
data);
static void qemuMonitorJSONHandleBalloonChange(qemuMonitorPtr mon, virJSONValuePtr
data);
static void qemuMonitorJSONHandlePMSuspendDisk(qemuMonitorPtr mon, virJSONValuePtr
data);
@@ -94,6 +95,7 @@ static qemuEventHandler eventHandlers[] = {
{ "BLOCK_IO_ERROR", qemuMonitorJSONHandleIOError, },
{ "BLOCK_JOB_CANCELLED", qemuMonitorJSONHandleBlockJobCanceled, },
{ "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJobCompleted, },
+ { "BLOCK_JOB_ERROR", qemuMonitorJSONHandleBlockJobError, },
{ "BLOCK_JOB_READY", qemuMonitorJSONHandleBlockJobReady, },
{ "DEVICE_DELETED", qemuMonitorJSONHandleDeviceDeleted, },
{ "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, },
@@ -842,6 +844,14 @@ qemuMonitorJSONHandleBlockJobCanceled(qemuMonitorPtr mon,
}
static void
+qemuMonitorJSONHandleBlockJobError(qemuMonitorPtr mon,
+ virJSONValuePtr data)
+{
+ qemuMonitorJSONHandleBlockJobImpl(mon, data,
+ VIR_DOMAIN_BLOCK_JOB_FAILED);
+}
+
+static void
qemuMonitorJSONHandleBlockJobReady(qemuMonitorPtr mon,
virJSONValuePtr data)
{
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c18204b..b29ecf1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1110,6 +1110,20 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
save = true;
}
+ } else if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
+ switch ((virConnectDomainEventBlockJobStatus) status) {
+ case VIR_DOMAIN_BLOCK_JOB_READY:
+ case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
+ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
+ break;
+ case VIR_DOMAIN_BLOCK_JOB_CANCELED:
+ case VIR_DOMAIN_BLOCK_JOB_FAILED:
+ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ERROR;
+ break;
+ case VIR_DOMAIN_BLOCK_JOB_LAST:
+ VIR_DEBUG("should not get here");
+ break;
+ }
}
}
--
2.0.5