[libvirt] [PATCH 5/5] qemu: Avoid duplicate resume events and state changes

[Jiri Denemark]

On Wed, Sep 19, 2018 at 16:04:13 -0400, John Ferlan wrote:
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 825a9d399b..4771f26938 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> 
> [...]
> 
> >  
> > @@ -5074,13 +5060,8 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver,
> >                  goto endjob;
> >          }
> >  
> > -        if (inPostCopy) {
> > +        if (inPostCopy)
> >              doKill = false;
> > -            event = virDomainEventLifecycleNewFromObj(vm,
> > -                                        VIR_DOMAIN_EVENT_RESUMED,
> > -                                        VIR_DOMAIN_EVENT_RESUMED_POSTCOPY);
> > -            virObjectEventStateQueue(driver->domainEventState, event);
> > -        }
> >      }
> >  
> >      if (mig->jobInfo) {
> > @@ -5111,11 +5092,6 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver,
> >  
> >      dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, vm->def->id);
> >  
> > -    event = virDomainEventLifecycleNewFromObj(vm,
> > -                                              VIR_DOMAIN_EVENT_RESUMED,
> > -                                              VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> > -    virObjectEventStateQueue(driver->domainEventState, event);
> > -
> >      if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
> >          virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER);
> >          event = virDomainEventLifecycleNewFromObj(vm,
> 
> For this path 2 events were generated if @inPostCopy == True - one right
> after the qemuProcessStartCPUs for RESUMED_POSTCOPY and then one for
> SUSPENDED_PAUSED once qemuMigrationDstWaitForCompletion occurred.

I guess you meant s/SUSPENDED_PAUSED/RESUMED_MIGRATED/. Anyway, you made
me think about this and you're right. Or docs even say that two RESUMED
events are sent during post-copy migration. RESUMED_POSTCOPY when the
domain switches from source to the destination host and RESUMED_MIGRATED
once migration is complete... v2 is in progress.

> IIUC, the changes here when @inPostCopy == true would only generate the
> POSTCOPY event, but not the MIGRATED event. Or is there something subtle
> I'm missing. Will the post copy processing generate two resume events?
> If so, we perhaps should document that. If not, then perhaps this second
> event that's being deleted needs to be moved inside that inPostCopy
> condition just above with the note that this one case is "abnormal"

Yes.

> (or you could just do the *StartCPUs again with the RESUMED_MIGRATED
> and close your eyes ;-)).

This wouldn't work, the RESUME handler ignores the event if the domain
is not paused.

> Curious, for the future, would it be useful to change the FakeReboot
> path to generate a different detail reason? It's hard to distinguish
> between someone using virsh resume (or recovery from Save, Dump,
> Snapshot failure) from that fake reboot path - at least from just the
> event. I guess it seems VIR_DOMAIN_EVENT_RESUMED_UNPAUSED is just too
> generic since it's more than just a "normal resume due to admin
> unpause", but that I would think would be extra given the "scope" of
> this particular problem.

Yeah, a new resume reason would be nice in this case, but I don't think
it's a big issue. The guest-agent driven reboot is much better and
hopefully used more often than fake reboot.

> Second curiosity... would it be useful/easy to generate an event on the
> failure scenario for StartCPUs? Knowing from whence we were called, we
> should be able to generate a suspended event rather than having the
> callers decide.

Not really, if StartCPUs fails the vCPUs just remain paused as if
nothing happened. That is, there's no state change to signal using an
event. And I think "cont" can't really fail anyway. Normally it would
just succeed and another STOP event would be sent right away in case the
reason for pausing the CPUs (such as I/O error) is still valid.

Jirka