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

Nikolay Shirokovskiy posted 3 patches 28 weeks ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1549612380-898556-1-git-send-email-nshirokovskiy@virtuozzo.com
src/qemu/qemu_domain.c    | 34 ++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h    |  7 +++++++
src/qemu/qemu_driver.c    | 26 +++-----------------------
src/qemu/qemu_migration.c | 42 ++++++------------------------------------
src/qemu/qemu_migration.h |  4 ----
src/qemu/qemu_process.c   | 38 +++++++++++++++++++++++++-------------
6 files changed, 75 insertions(+), 76 deletions(-)

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

Posted by Nikolay Shirokovskiy 28 weeks ago
Patches 1 and 2 are already Reviewed-by: John. Patch 3 needs Peter comments.

Diff from v1:
============
- minor rebase changes
- minor changes according to review

[1] PATCH v1 : https://www.redhat.com/archives/libvir-list/2018-October/msg00591.html

Nikolay Shirokovskiy (3):
  qemu: Pass stop reason from qemuProcessStopCPUs to stop handler
  qemu: Map suspended state reason to suspended event detail
  qemu: Don't duplicate suspend events and state changes

 src/qemu/qemu_domain.c    | 34 ++++++++++++++++++++++++++++++++++
 src/qemu/qemu_domain.h    |  7 +++++++
 src/qemu/qemu_driver.c    | 26 +++-----------------------
 src/qemu/qemu_migration.c | 42 ++++++------------------------------------
 src/qemu/qemu_migration.h |  4 ----
 src/qemu/qemu_process.c   | 38 +++++++++++++++++++++++++-------------
 6 files changed, 75 insertions(+), 76 deletions(-)

-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 27 weeks ago

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@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.


John

> Diff from v1:
> ============
> - minor rebase changes
> - minor changes according to review
> 
> [1] PATCH v1 : https://www.redhat.com/archives/libvir-list/2018-October/msg00591.html
> 
> Nikolay Shirokovskiy (3):
>   qemu: Pass stop reason from qemuProcessStopCPUs to stop handler
>   qemu: Map suspended state reason to suspended event detail
>   qemu: Don't duplicate suspend events and state changes
> 
>  src/qemu/qemu_domain.c    | 34 ++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h    |  7 +++++++
>  src/qemu/qemu_driver.c    | 26 +++-----------------------
>  src/qemu/qemu_migration.c | 42 ++++++------------------------------------
>  src/qemu/qemu_migration.h |  4 ----
>  src/qemu/qemu_process.c   | 38 +++++++++++++++++++++++++-------------
>  6 files changed, 75 insertions(+), 76 deletions(-)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Nikolay Shirokovskiy 21 weeks ago
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@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.
> 
> 
> John
> 
>> Diff from v1:
>> ============
>> - minor rebase changes
>> - minor changes according to review
>>
>> [1] PATCH v1 : https://www.redhat.com/archives/libvir-list/2018-October/msg00591.html
>>
>> Nikolay Shirokovskiy (3):
>>   qemu: Pass stop reason from qemuProcessStopCPUs to stop handler
>>   qemu: Map suspended state reason to suspended event detail
>>   qemu: Don't duplicate suspend events and state changes
>>
>>  src/qemu/qemu_domain.c    | 34 ++++++++++++++++++++++++++++++++++
>>  src/qemu/qemu_domain.h    |  7 +++++++
>>  src/qemu/qemu_driver.c    | 26 +++-----------------------
>>  src/qemu/qemu_migration.c | 42 ++++++------------------------------------
>>  src/qemu/qemu_migration.h |  4 ----
>>  src/qemu/qemu_process.c   | 38 +++++++++++++++++++++++++-------------
>>  6 files changed, 75 insertions(+), 76 deletions(-)
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Cole Robinson 20 weeks ago
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@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list