On 25.09.2012 19:57, Daniel P. Berrange wrote:
On Thu, Sep 20, 2012 at 01:29:21PM +0200, Michal Privoznik wrote:
Recently, there have been some improvements made to qemu so it supports seamless migration or something very close to it. However, it requires libvirt interaction. Once qemu is migrated, the SPICE server needs to send its internal state to the destination. Once it's done, it fires SPICE_MIGRATE_COMPLETED event and this fact is advertised in 'query-spice' output as well. We must not kill qemu until SPICE server finishes the transfer. --- src/qemu/qemu_capabilities.c | 4 +++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 8 ++++++ src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++++-- src/qemu/qemu_monitor.c | 24 +++++++++++++++++++ src/qemu/qemu_monitor.h | 2 + src/qemu/qemu_monitor_json.c | 52 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 ++ 8 files changed, 124 insertions(+), 3 deletions(-)
@@ -940,6 +949,8 @@ qemuMigrationUpdateJobStatus(struct qemud_driver *driver,
case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED: priv->job.info.type = VIR_DOMAIN_JOB_COMPLETED; + if (spice_migrated) + priv->job.info.type = VIR_DOMAIN_JOB_COMPLETED;
This addition doesn't do anything, since the previous line has already set the same value.
Yeah, there should be '-' sign on the previous line...
I don't find this very readable - it is better to do it as a regular if() IMHO. eg
bool wait_for_spice = false;
if (...) wait_for_spice = true;
switch (priv->job.asyncJob) { case QEMU_ASYNC_JOB_MIGRATION_OUT: @@ -988,7 +1006,8 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm, /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
- if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) < 0) + if (qemuMigrationUpdateJobStatus(driver, vm, job, + asyncJob, wait_for_spice) < 0) goto cleanup;
if (dconn && virConnectIsAlive(dconn) <= 0) { @@ -1840,6 +1859,7 @@ qemuMigrationRun(struct qemud_driver *driver, int fd = -1; unsigned long migrate_speed = resource ? resource : priv->migMaxBandwidth; virErrorPtr orig_err = NULL; + bool wait_for_spice;
VIR_DEBUG("driver=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu, " @@ -1848,6 +1868,12 @@ qemuMigrationRun(struct qemud_driver *driver, cookieout, cookieoutlen, flags, resource, spec, spec->destType, spec->fwdType);
+ /* If guest uses SPICE and supports seamless migration we have to hold up + * migration finish until SPICE server transfers its data */ + wait_for_spice = (vm->def->ngraphics == 1) && + (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) && + qemuCapsGet(priv->caps, QEMU_CAPS_SEAMLESS_MIGRATION);
Same comment as before.
Since both callers of qemuMigrateUpdateJobStatus() have to repeat this logic, why not push this down into that method itself and make 'wait_for_spice' be a local variable there instead of a param.
Well, I wanted to optimize here since qemuMigrateUpdateJobStatus is called every 50 ms so I think accessing memory on each run would hurt the performance (and this really do a lot of mem traffic since it access vm->def->[n]graphics; qemuCaps). Or is this a premature optimization which should be left to compiler and it's caching capabilities? Michal