Re: [libvirt] [PATCH] qemu: Propagate storage errors on migration

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&gt; --- 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;

+ } } }