[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3] qemu: wait for SPICE to migrate



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]