[libvirt] [PATCH v2 0/3] qemu: don't duplicate suspended events and state changes

[Cole Robinson]

On 3/27/19 7:53 AM, Nikolay Shirokovskiy wrote:
> ping
> 
> On 13.02.2019 19:55, John Ferlan wrote:
>>
>>
>> On 2/8/19 2:52 AM, Nikolay Shirokovskiy wrote:
>>> Patches 1 and 2 are already Reviewed-by: John. Patch 3 needs Peter comments.
>>>
>>
>> Right - feel free to add my :
>>
>> Reviewed-by: John Ferlan <jferlan at redhat.com>
>>
>> to the first 2 patches for sure.
>>
>> To help push this along, Peter is again CC'd and of importance is the v1
>> review of patch3:
>>
>> https://www.redhat.com/archives/libvir-list/2018-October/msg00831.html
>>
>> where I noted a specific commit and case to be considered.
>>

To dredge up more context, John is talking about Peter's commit:

commit f569b87f5193a69c50ca714734013cc4f82250f1
Author: Peter Krempa <pkrempa at redhat.com>
Date:   Mon Oct 8 19:38:44 2012 +0200

    snapshot: qemu: Add support for external checkpoints

Which added this chunk:

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9358ad99ed..128b98ee22 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1673,6 +1673,9 @@ static int qemudDomainSuspend(virDomainPtr dom) {
>      if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) {
>          reason = VIR_DOMAIN_PAUSED_MIGRATION;
>          eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED;
> +    } else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) {
> +        reason = VIR_DOMAIN_PAUSED_SNAPSHOT;
> +        eventDetail = -1; /* don't create lifecycle events when doing snapshot */
>      } else {
>          reason = VIR_DOMAIN_PAUSED_USER;
>          eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED;
> @@ -1687,9 +1690,12 @@ static int qemudDomainSuspend(virDomainPtr dom) {
>          if (qemuProcessStopCPUs(driver, vm, reason, QEMU_ASYNC_JOB_NONE) < 0) {
>              goto endjob;
>          }
> -        event = virDomainEventNewFromObj(vm,
> -                                         VIR_DOMAIN_EVENT_SUSPENDED,
> -                                         eventDetail);
> +
> +        if (eventDetail >= 0) {
> +            event = virDomainEventNewFromObj(vm,
> +                                             VIR_DOMAIN_EVENT_SUSPENDED,
> +                                             eventDetail);
> +        }
>      }
>      if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0)
>          goto endjob;

Which disables sending a VIR_DOMAIN_EVENT_SUSPENDED event when we
suspend the VM as an implementation detail of creating an external
snapshot checkpoint. Nikolay's series removes that case, so AFAICT
checkpoints will now send SUSPENDED events.

Current git doesn't perform the same event skippage in the managed
save case (where SUSPENDED event is emitted), or the internal snapshot
case (where SUSPEND+RESUME are emitted when taking a live snapshot).
I can't think of any particular reason why the checkpoint case needs to
be special here. Maybe that's an argument for fixing all of these cases,
though virt-manager which uses internal snapshots has had no problem
with SUSPEND/RESUME events in this area so there's some proof apps
aren't going to choke on a similar case

IMO patches have been reviewed for almost 2 months, no one has objected,
so push em :)

Thanks,
Cole