[libvirt] [PATCH 2/2] check whether the vm is paused before setting it offline

[Daniel P. Berrange]

On Fri, May 20, 2011 at 07:00:54PM +0800, Wen Congyang wrote:
> At 05/20/2011 04:34 PM, Daniel P. Berrange Write:
> > On Fri, May 20, 2011 at 03:09:42PM +0800, Wen Congyang wrote:
> >> If we do not performe a live migration, the vm is always paused when we go to
> >> doPeer2PeerMigrate2() and doPeer2PeerMigrate3(), because we stop the vm in
> >> qemuMigrationPerform(). So we should check it before setting it offline.
> > 
> > The only code which checks for 'if (flags & VIR_MIGRATE_PAUSED)'
> > is the qemuMigrateFinished method. This is called from
> > doPeer2PeerMigrate2 or doPeer2PeerMigrate3.  So putting the
> > change 'flags |= VIR_MIGRATED_PAUSED' does not make any
> > difference to peer2peer migration, and for non-peer2peer
> > migration, this will have already been set in libvirt.c.
> > 
> > So I'm not seeing what bug is being fixed by this change.
> 
> Without this patch, vm is still paused after migration when
> we do p2p migration.
> 
> The vm should be paused only when:
> 1. the vm is paused before migration
> 2. we use the option --suspend.
> 
> We decide whether to restart the vm on the dest host accoring to whether
> VIR_MIGRATED_PAUSED is set.
> 
> If we do not use the option --live and --suspend, and the vm is running
> before migration, the vm should be also running after migration. But
> the vm will be set offline in the function qemuMigrationPerform().
> When we goto to doPeer2PeerMigrate2() or doPeer2PeerMigrate3(),
> the vm is paused and VIR_MIGRATED_PAUSED is set. So the vm is not
> restarted in the function qemuMigrateFinished().

Ahh, you're not using  --live. That will be why I didn't see this
problem.

> The way to solute this bug is that: we should check the origin status of vm
> before setting it offline. This patch does this.

The key fact is that the place which sets VIR_MIGRATE_PAUSED
must come before the call to qemuMigrationSetOffline. Your
patch fixes it by moving the check to VIR_MIGRATE_PAUSED.

I think the better thing is to move the qemuMigrationSetOffline
call down a level - to the same bits of code which call
qemuMigrationSetSpeed. I'll post a patch for this later.

> 
> > The doPeer2PeerMigrate2 and doPeer2PeerMigrate3 methods
> > are intended to be structured (near) identically to the
> > virDomainMigrate2 and virDomainMigrate3 methods in libvirt.c,
> > so I think the code is correct as it already is.
> > 
> >>
> >> ---
> >>  src/qemu/qemu_migration.c |    9 +++------
> >>  1 files changed, 3 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> >> index ab28416..30167f2 100644
> >> --- a/src/qemu/qemu_migration.c
> >> +++ b/src/qemu/qemu_migration.c
> >> @@ -1614,9 +1614,6 @@ static int doPeer2PeerMigrate2(struct qemud_driver *driver,
> >>                                          VIR_DOMAIN_XML_UPDATE_CPU)))
> >>          return -1;
> >>  
> >> -    if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED)
> >> -        flags |= VIR_MIGRATE_PAUSED;
> >> -
> >>      VIR_DEBUG("Prepare2 %p", dconn);
> >>      if (flags & VIR_MIGRATE_TUNNELLED) {
> >>          /*
> >> @@ -1748,9 +1745,6 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver,
> >>      if (!dom_xml)
> >>          goto cleanup;
> >>  
> >> -    if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED)
> >> -        flags |= VIR_MIGRATE_PAUSED;
> >> -
> >>      VIR_DEBUG("Prepare3 %p", dconn);
> >>      cookiein = cookieout;
> >>      cookieinlen = cookieoutlen;
> >> @@ -1985,6 +1979,9 @@ int qemuMigrationPerform(struct qemud_driver *driver,
> >>      memset(&priv->jobInfo, 0, sizeof(priv->jobInfo));
> >>      priv->jobInfo.type = VIR_DOMAIN_JOB_UNBOUNDED;
> >>  
> >> +    if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED)
> >> +        flags |= VIR_MIGRATE_PAUSED;
> >> +
> >>      resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING;
> >>      if (!(flags & VIR_MIGRATE_LIVE) &&
> >>          virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
> > 
> > 

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