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(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 278b550..26364ec 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -180,6 +180,8 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
> "ide-drive.wwn",
> "scsi-disk.wwn",
> "seccomp-sandbox",
> +
> + "seamless-migration", /* 110 */
> );
>
> struct _qemuCaps {
> @@ -1146,6 +1148,8 @@ qemuCapsComputeCmdFlags(const char *help,
> }
> if (strstr(help, "-spice"))
> qemuCapsSet(caps, QEMU_CAPS_SPICE);
> + if (strstr(help, "seamless-migration="))
> + qemuCapsSet(caps, QEMU_CAPS_SEAMLESS_MIGRATION);
> if (strstr(help, "boot=on"))
> qemuCapsSet(caps, QEMU_CAPS_DRIVE_BOOT);
> if (strstr(help, "serial=s"))
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 4da2a29..2567fd7 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -145,6 +145,7 @@ enum qemuCapsFlags {
> QEMU_CAPS_IDE_DRIVE_WWN = 107, /* Is ide-drive.wwn available? */
> QEMU_CAPS_SCSI_DISK_WWN = 108, /* Is scsi-disk.wwn available? */
> QEMU_CAPS_SECCOMP_SANDBOX = 109, /* -sandbox */
> + QEMU_CAPS_SEAMLESS_MIGRATION = 110, /* seamless-migration for SPICE */
>
> QEMU_CAPS_LAST, /* this must always be the last item */
> };
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index cbf4aee..33f7e55 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6081,6 +6081,14 @@ qemuBuildCommandLine(virConnectPtr conn,
> if (def->graphics[0]->data.spice.copypaste == VIR_DOMAIN_GRAPHICS_SPICE_CLIPBOARD_COPYPASTE_NO)
> virBufferAddLit(&opt, ",disable-copy-paste");
>
> + if (qemuCapsGet(caps, QEMU_CAPS_SEAMLESS_MIGRATION)) {
> + /* If qemu supports seamless migration turn it
> + * unconditionally on. If migration destination
> + * doesn't support it, it fallbacks to previous
> + * migration algorithm silently. */
> + virBufferAddLit(&opt, ",seamless-migration=on");
> + }
> +
> virCommandAddArg(cmd, "-spice");
> virCommandAddArgBuffer(cmd, &opt);
> if (def->graphics[0]->data.spice.keymap)
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 99fc8ce..114d04a 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -891,11 +891,13 @@ static int
> qemuMigrationUpdateJobStatus(struct qemud_driver *driver,
> virDomainObjPtr vm,
> const char *job,
> - enum qemuDomainAsyncJob asyncJob)
> + enum qemuDomainAsyncJob asyncJob,
> + bool wait_for_spice)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> int ret;
> int status;
> + bool spice_migrated = true;
> unsigned long long memProcessed;
> unsigned long long memRemaining;
> unsigned long long memTotal;
> @@ -910,6 +912,13 @@ qemuMigrationUpdateJobStatus(struct qemud_driver *driver,
> &memProcessed,
> &memRemaining,
> &memTotal);
> +
> + /* If qemu says migrated, check spice */
> + if (wait_for_spice && (ret == 0) &&
> + (status == QEMU_MONITOR_MIGRATION_STATUS_COMPLETED))
> + ret = qemuMonitorGetSpiceMigrationStatus(priv->mon,
> + &spice_migrated);
> +
> qemuDomainObjExitMonitorWithDriver(driver, vm);
>
> if (ret < 0 || virTimeMillisNow(&priv->job.info.timeElapsed) < 0) {
> @@ -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.
> ret = 0;
> break;
>
> @@ -967,6 +978,13 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm,
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> const char *job;
> + bool wait_for_spice;
> +
> + /* If guest uses SPICE and supports seamles_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);
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.
> +
> if (virLockManagerPluginUsesState(driver->lockManager) &&
> !cookieout) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1946,7 +1972,8 @@ qemuMigrationRun(struct qemud_driver *driver,
> * connection from qemu which may never be initiated.
> */
> if (qemuMigrationUpdateJobStatus(driver, vm, _("migration job"),
> - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
> + QEMU_ASYNC_JOB_MIGRATION_OUT,
> + wait_for_spice) < 0)
> goto cancel;
>
> while ((fd = accept(spec->dest.unix_socket.sock, NULL, NULL)) < 0) {
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index f36c8a8..7695a81 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1827,6 +1827,30 @@ int qemuMonitorGetMigrationStatus(qemuMonitorPtr mon,
> }
>
>
> +int qemuMonitorGetSpiceMigrationStatus(qemuMonitorPtr mon,
> + bool *spice_migrated)
> +{
> + int ret;
> + VIR_DEBUG("mon=%p", mon);
> +
> + if (!mon) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("monitor must not be NULL"));
> + return -1;
> + }
> +
> + if (mon->json) {
> + ret = qemuMonitorJSONGetSpiceMigrationStatus(mon, spice_migrated);
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("JSON monitor is required"));
This error code (any in fact all other similar uses in that file)
should be VIR_ERR_OPERATION_UNSUPPORTED, since this isn't an
end user configuration issue.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list