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

[Daniel P. Berrange]

On Wed, Sep 26, 2012 at 08:57:28AM +0200, Michal Privoznik wrote:
> 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?

Ok, that's a good enough reason for me. I didn't see that this was called
every 50ms from the code diff.

I would request that you rearrange the code, since I find that pattern
somewhat unreadable. eg instead do

  bool wait_for_spice = false;

  if ((vm->def->ngraphics == 1) &&
      (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) &&
      qemuCapsGet(priv->caps, QEMU_CAPS_SEAMLESS_MIGRATION)
    wait_for_spice = true;

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