[Qemu-devel] [PATCH v4 0/3] Migration Events

[Qemu-devel] [PATCH v4 0/3] Migration Events

[Juan Quintela]

Hi

By popular demand (i.e. libvirt) we have another round of Migration events patches.

Notice that althought this is v4, code has changed enough since previous submission that was done from scratch.
(Pervious submisinon was from Wed, 5 May 2010).

In this series:
- We remove extra assignment of MIGRATION_STATUS_SETUP
- Migration events for source (we recycle the state from query-migrate)
- Migration events for target (we recycle previous ones)

As you can see, we win the greener price of the year.

This patches are on top of my previous series for optional sections.  You can get the whole bunch at:


https://github.com/juanquintela/qemu/commits/migration-events

Please review, Juan



Juan Quintela (3):
  migration: Remove duplicated assignement of SETUP status
  migration: create migration event
  migration: Add migration events on target side

 docs/qmp/qmp-events.txt | 16 ++++++++++++++++
 migration/migration.c   | 19 ++++++++++++++++---
 qapi/event.json         | 14 ++++++++++++++
 3 files changed, 46 insertions(+), 3 deletions(-)

-- 
2.4.1

[Qemu-devel] [PATCH 1/3] migration: Remove duplicated assignement of SETUP status

[Juan Quintela]

We assign the MIGRATION_STATUS_SETUP status in two places.  Just in
sucession.  Just remove the second one.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 1035689..2925587 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -849,9 +849,6 @@ static void *migration_thread(void *opaque)

 void migrate_fd_connect(MigrationState *s)
 {
-    s->state = MIGRATION_STATUS_SETUP;
-    trace_migrate_set_state(MIGRATION_STATUS_SETUP);
-
     /* This is a best 1st approximation. ns to ms */
     s->expected_downtime = max_downtime/1000000;
     s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
-- 
2.4.1

[Qemu-devel] [PATCH 2/3] migration: create migration event

[Juan Quintela]

We have one argument that tells us what event has happened.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 docs/qmp/qmp-events.txt | 16 ++++++++++++++++
 migration/migration.c   | 12 ++++++++++++
 qapi/event.json         | 14 ++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index 4c13d48..3797709 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -473,6 +473,22 @@ Example:
 { "timestamp": {"seconds": 1290688046, "microseconds": 417172},
   "event": "SPICE_MIGRATE_COMPLETED" }

+MIGRATION
+---------
+
+Emitted when a migration event happens
+
+Data: None.
+
+ - "status": migration status
+     "": error has been ignored
+     "report": error has been reported to the device
+     "stop": the VM is going to stop because of the error
+
+Example:
+
+{"timestamp": {"seconds": 1432121972, "microseconds": 744001},
+ "event": "MIGRATION", "data": {"status": "completed"}}

 STOP
 ----
diff --git a/migration/migration.c b/migration/migration.c
index 2925587..6b1cead 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -25,6 +25,7 @@
 #include "qemu/thread.h"
 #include "qmp-commands.h"
 #include "trace.h"
+#include "qapi-event.h"

 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */

@@ -401,6 +402,7 @@ static void migrate_fd_cleanup(void *opaque)
         if (s->state == MIGRATION_STATUS_CANCELLING) {
             migrate_set_state(s, MIGRATION_STATUS_CANCELLING,
                               MIGRATION_STATUS_CANCELLED);
+            qapi_event_send_migration(MIGRATION_STATUS_CANCELLED, &error_abort);
         }
     }

@@ -412,6 +414,7 @@ void migrate_fd_error(MigrationState *s)
     trace_migrate_fd_error();
     assert(s->file == NULL);
     s->state = MIGRATION_STATUS_FAILED;
+    qapi_event_send_migration(MIGRATION_STATUS_FAILED, &error_abort);
     trace_migrate_set_state(MIGRATION_STATUS_FAILED);
     notifier_list_notify(&migration_state_notifiers, s);
 }
@@ -430,6 +433,7 @@ static void migrate_fd_cancel(MigrationState *s)
         }
         migrate_set_state(s, old_state, MIGRATION_STATUS_CANCELLING);
     } while (s->state != MIGRATION_STATUS_CANCELLING);
+    qapi_event_send_migration(MIGRATION_STATUS_CANCELLING, &error_abort);

     /*
      * If we're unlucky the migration code might be stuck somewhere in a
@@ -497,6 +501,7 @@ static MigrationState *migrate_init(const MigrationParams *params)
                decompress_thread_count;
     s->bandwidth_limit = bandwidth_limit;
     s->state = MIGRATION_STATUS_SETUP;
+    qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
     trace_migrate_set_state(MIGRATION_STATUS_SETUP);

     s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
@@ -590,6 +595,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
         s->state = MIGRATION_STATUS_FAILED;
+        qapi_event_send_migration(MIGRATION_STATUS_FAILED, &error_abort);
         return;
     }

@@ -751,6 +757,7 @@ static void *migration_thread(void *opaque)
     qemu_savevm_state_begin(s->file, &s->params);

     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
+    qapi_event_send_migration(MIGRATION_STATUS_ACTIVE, &error_abort);
     migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_ACTIVE);

     while (s->state == MIGRATION_STATUS_ACTIVE) {
@@ -781,12 +788,16 @@ static void *migration_thread(void *opaque)
                 if (ret < 0) {
                     migrate_set_state(s, MIGRATION_STATUS_ACTIVE,
                                       MIGRATION_STATUS_FAILED);
+                    qapi_event_send_migration(MIGRATION_STATUS_FAILED,
+                                              &error_abort);
                     break;
                 }

                 if (!qemu_file_get_error(s->file)) {
                     migrate_set_state(s, MIGRATION_STATUS_ACTIVE,
                                       MIGRATION_STATUS_COMPLETED);
+                    qapi_event_send_migration(MIGRATION_STATUS_COMPLETED,
+                                              &error_abort);
                     break;
                 }
             }
@@ -795,6 +806,7 @@ static void *migration_thread(void *opaque)
         if (qemu_file_get_error(s->file)) {
             migrate_set_state(s, MIGRATION_STATUS_ACTIVE,
                               MIGRATION_STATUS_FAILED);
+            qapi_event_send_migration(MIGRATION_STATUS_FAILED, &error_abort);
             break;
         }
         current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
diff --git a/qapi/event.json b/qapi/event.json
index 378dda5..fe5e182 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -243,6 +243,20 @@
 { 'event': 'SPICE_MIGRATE_COMPLETED' }

 ##
+# @MIGRATION
+#
+# Emitted when a migration event happens
+#
+# @status: @MigrationStatus describing the current migration status.
+#          If this field is not returned, no migration process
+#          has been initiated
+#
+# Since: 2.4
+##
+{ 'event': 'MIGRATION',
+  'data': {'status': 'MigrationStatus'}}
+
+##
 # @ACPI_DEVICE_OST
 #
 # Emitted when guest executes ACPI _OST method.
-- 
2.4.1

[Qemu-devel] [PATCH 3/3] migration: Add migration events on target side

[Juan Quintela]

We reuse the migration events from the source side, sending them on the
appropiate place.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 6b1cead..2bf587b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -89,6 +89,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;

+    qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
     if (!strcmp(uri, "defer")) {
         deferred_incoming_migration(errp);
     } else if (strstart(uri, "tcp:", &p)) {
@@ -116,14 +117,17 @@ static void process_incoming_migration_co(void *opaque)
     Error *local_err = NULL;
     int ret;

+    qapi_event_send_migration(MIGRATION_STATUS_ACTIVE, &error_abort);
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
     free_xbzrle_decoded_buf();
     if (ret < 0) {
+        qapi_event_send_migration(MIGRATION_STATUS_FAILED, &error_abort);
         error_report("load of migration failed: %s", strerror(-ret));
         migrate_decompress_threads_join();
         exit(EXIT_FAILURE);
     }
+    qapi_event_send_migration(MIGRATION_STATUS_COMPLETED, &error_abort);
     qemu_announce_self();

     /* Make sure all file formats flush their mutable metadata */
-- 
2.4.1

Re: [Qemu-devel] [PATCH 1/3] migration: Remove duplicated assignement of SETUP status

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On 05/20/2015 09:35 AM, Juan Quintela wrote:
> We assign the MIGRATION_STATUS_SETUP status in two places.  Just in
> sucession.  Just remove the second one.

s/sucession/succession/

Where's the second place? Not in migrate_fd_connect...

okay, migrate_init() does the same thing, and I guess you're arguing
that it was always called either just before or just after
migrate_fd_connect can be called. But mentioning it in the commit
message would help.

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 3 ---
>  1 file changed, 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] migration: create migration event

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]

On 05/20/2015 09:35 AM, Juan Quintela wrote:
> We have one argument that tells us what event has happened.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  docs/qmp/qmp-events.txt | 16 ++++++++++++++++
>  migration/migration.c   | 12 ++++++++++++
>  qapi/event.json         | 14 ++++++++++++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index 4c13d48..3797709 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -473,6 +473,22 @@ Example:
>  { "timestamp": {"seconds": 1290688046, "microseconds": 417172},
>    "event": "SPICE_MIGRATE_COMPLETED" }
> 
> +MIGRATION
> +---------
> +
> +Emitted when a migration event happens
> +
> +Data: None.
> +
> + - "status": migration status
> +     "": error has been ignored

Uggh. Looking for an empty string is awkward.

> +     "report": error has been reported to the device
> +     "stop": the VM is going to stop because of the error
> +
> +Example:
> +
> +{"timestamp": {"seconds": 1432121972, "microseconds": 744001},
> + "event": "MIGRATION", "data": {"status": "completed"}}

The example lists "completed", but the documentation does not mention
it. Might be good to expand the docs to mention all states, and/or point
to the enum definition.


> +++ b/qapi/event.json
> @@ -243,6 +243,20 @@
>  { 'event': 'SPICE_MIGRATE_COMPLETED' }
> 
>  ##
> +# @MIGRATION
> +#
> +# Emitted when a migration event happens
> +#
> +# @status: @MigrationStatus describing the current migration status.
> +#          If this field is not returned, no migration process
> +#          has been initiated

Rather than returning an empty string,...

> +#
> +# Since: 2.4
> +##
> +{ 'event': 'MIGRATION',
> +  'data': {'status': 'MigrationStatus'}}

...this field should be marked optional, as in '*status'.  Then in your
callers, you'll have to pass true or false for has_status, so that you
can omit status when there is none.  But really, when will this event
ever be omitted if migration has not been initiated?  Maybe it is just
bogus documentation that you can return an empty string, as I didn't see
any addition of a call to qapi_event_send_migration() that would pass an
empty string on the wire.  So it sounds to me like the interface is
okay, but the documentation is wrong.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] migration: Remove duplicated assignement of SETUP status

[John Snow]

On 05/20/2015 11:58 AM, Eric Blake wrote:
> On 05/20/2015 09:35 AM, Juan Quintela wrote:
>> We assign the MIGRATION_STATUS_SETUP status in two places.  Just
>> in sucession.  Just remove the second one.
> 
> s/sucession/succession/
> 

Also s/assignement/assignment/ in the subject.

> Where's the second place? Not in migrate_fd_connect...
> 
> okay, migrate_init() does the same thing, and I guess you're
> arguing that it was always called either just before or just after 
> migrate_fd_connect can be called. But mentioning it in the commit 
> message would help.
> 
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com> --- 
>> migration/migration.c | 3 --- 1 file changed, 3 deletions(-)
>> 
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Re: [Qemu-devel] [PATCH 3/3] migration: Add migration events on target side

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On 05/20/2015 09:35 AM, Juan Quintela wrote:
> We reuse the migration events from the source side, sending them on the
> appropiate place.

s/appropiate/appropriate/

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

> @@ -116,14 +117,17 @@ static void process_incoming_migration_co(void *opaque)
>      Error *local_err = NULL;
>      int ret;
> 
> +    qapi_event_send_migration(MIGRATION_STATUS_ACTIVE, &error_abort);
>      ret = qemu_loadvm_state(f);
>      qemu_fclose(f);
>      free_xbzrle_decoded_buf();
>      if (ret < 0) {
> +        qapi_event_send_migration(MIGRATION_STATUS_FAILED, &error_abort);
>          error_report("load of migration failed: %s", strerror(-ret));
>          migrate_decompress_threads_join();
>          exit(EXIT_FAILURE);

Does qapi_event_send_* send an event right away, or merely just queue
things up to be sent at a safe point later in the overall event loop?
Or put another way, could the early exit() here prevent the actual error
from being delivered, because we don't return control to the event loop
to actually flush the pending event queue?

But whether or not we have a design issue on the failure path, at least
something got printed via error_report, and the real benefit of this
patch is the event on success; so I'm fine with giving this patch:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] migration: create migration event

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/20/2015 09:35 AM, Juan Quintela wrote:
>> We have one argument that tells us what event has happened.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  docs/qmp/qmp-events.txt | 16 ++++++++++++++++
>>  migration/migration.c   | 12 ++++++++++++
>>  qapi/event.json         | 14 ++++++++++++++
>>  3 files changed, 42 insertions(+)
>> 
>> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
>> index 4c13d48..3797709 100644
>> --- a/docs/qmp/qmp-events.txt
>> +++ b/docs/qmp/qmp-events.txt
>> @@ -473,6 +473,22 @@ Example:
>>  { "timestamp": {"seconds": 1290688046, "microseconds": 417172},
>>    "event": "SPICE_MIGRATE_COMPLETED" }
>> 
>> +MIGRATION
>> +---------
>> +
>> +Emitted when a migration event happens
>> +
>> +Data: None.
>> +
>> + - "status": migration status
>> +     "": error has been ignored
>
> Uggh. Looking for an empty string is awkward.

And on the eleven version I did, I put the strings wrong.  Results are
the same than from query-migrate.


>
>> +     "report": error has been reported to the device
>> +     "stop": the VM is going to stop because of the error
>> +
>> +Example:
>> +
>> +{"timestamp": {"seconds": 1432121972, "microseconds": 744001},
>> + "event": "MIGRATION", "data": {"status": "completed"}}
>
> The example lists "completed", but the documentation does not mention
> it. Might be good to expand the docs to mention all states, and/or point
> to the enum definition.

I forget to do that when I changed it.

>
>
>> +++ b/qapi/event.json
>> @@ -243,6 +243,20 @@
>>  { 'event': 'SPICE_MIGRATE_COMPLETED' }
>> 
>>  ##
>> +# @MIGRATION
>> +#
>> +# Emitted when a migration event happens
>> +#
>> +# @status: @MigrationStatus describing the current migration status.
>> +#          If this field is not returned, no migration process
>> +#          has been initiated
>
> Rather than returning an empty string,...
>
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'event': 'MIGRATION',
>> +  'data': {'status': 'MigrationStatus'}}
>
> ...this field should be marked optional, as in '*status'.  Then in your
> callers, you'll have to pass true or false for has_status, so that you
> can omit status when there is none.  But really, when will this event
> ever be omitted if migration has not been initiated?  Maybe it is just
> bogus documentation that you can return an empty string, as I didn't see
> any addition of a call to qapi_event_send_migration() that would pass an
> empty string on the wire.  So it sounds to me like the interface is
> okay, but the documentation is wrong.

Bogus documentation, sorry.
This event it is only generated when we change the migration status to:
- SETUP, ACTIVE, COMPLETED, CANCELLING, CANCELLED and FAILED.

But let just say that this was my 1st big try with QMP events that have
arguments (previous version had one different event for each case, but
Jiri saw it and told me that it preffered it this way).

Thanks.

Re: [Qemu-devel] [PATCH 3/3] migration: Add migration events on target side

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/20/2015 09:35 AM, Juan Quintela wrote:
>> We reuse the migration events from the source side, sending them on the
>> appropiate place.
>
> s/appropiate/appropriate/
>
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/migration.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>
>> @@ -116,14 +117,17 @@ static void process_incoming_migration_co(void *opaque)
>>      Error *local_err = NULL;
>>      int ret;
>> 
>> +    qapi_event_send_migration(MIGRATION_STATUS_ACTIVE, &error_abort);
>>      ret = qemu_loadvm_state(f);
>>      qemu_fclose(f);
>>      free_xbzrle_decoded_buf();
>>      if (ret < 0) {
>> +        qapi_event_send_migration(MIGRATION_STATUS_FAILED, &error_abort);
>>          error_report("load of migration failed: %s", strerror(-ret));
>>          migrate_decompress_threads_join();
>>          exit(EXIT_FAILURE);
>
> Does qapi_event_send_* send an event right away, or merely just queue
> things up to be sent at a safe point later in the overall event loop?
> Or put another way, could the early exit() here prevent the actual error
> from being delivered, because we don't return control to the event loop
> to actually flush the pending event queue?

Too depth for me.


>
> But whether or not we have a design issue on the failure path, at least
> something got printed via error_report, and the real benefit of this
> patch is the event on success; so I'm fine with giving this patch:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks

Once that we are here

@@ -89,6 +89,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;

+    qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
     if (!strcmp(uri, "defer")) {
         deferred_incoming_migration(errp);
     } else if (strstart(uri, "tcp:", &p)) {


I am not able to see this event at all.  Not that it should matter for
destination.  My guess is that it happens too early for qapi, qmp or
whatever handles events is ready to consume them.  Any good idea?

Notice that in the big scheme of things, on destination, this event
should be very important/useful anyways.

Thanks again, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: create migration event

[Jiri Denemark]

On Wed, May 20, 2015 at 17:35:23 +0200, Juan Quintela wrote:
> We have one argument that tells us what event has happened.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  docs/qmp/qmp-events.txt | 16 ++++++++++++++++
>  migration/migration.c   | 12 ++++++++++++
>  qapi/event.json         | 14 ++++++++++++++
>  3 files changed, 42 insertions(+)

Hi Juan,

I patched libvirt to be able to consume this event and it all seems to
work fine except for events sent after migrate_cancel command:

{"execute":"migrate_cancel","id":"libvirt-33"}
{"timestamp": {"seconds": 1432899178, "microseconds": 844907}, "event":
"MIGRATION", "data": {"status": "cancelling"}}
{"return": {}, "id": "libvirt-33"}
{"timestamp": {"seconds": 1432899178, "microseconds": 845625}, "event":
"MIGRATION", "data": {"status": "failed"}}
{"timestamp": {"seconds": 1432899178, "microseconds": 846432}, "event":
"MIGRATION", "data": {"status": "cancelled"}}

In other words, the status first changes to "failed" and then it changes
correctly "cancelled". Can you fix this weird behavior in QEMU or do we
have to work around it in libvirt?

Jirka

Re: [Qemu-devel] [PATCH v4 0/3] Migration Events

[Jiri Denemark]

On Wed, May 20, 2015 at 17:35:21 +0200, Juan Quintela wrote:
> Hi
> 
> By popular demand (i.e. libvirt) we have another round of Migration events patches.
> 
> Notice that althought this is v4, code has changed enough since previous submission that was done from scratch.
> (Pervious submisinon was from Wed, 5 May 2010).
> 
> In this series:
> - We remove extra assignment of MIGRATION_STATUS_SETUP
> - Migration events for source (we recycle the state from query-migrate)
> - Migration events for target (we recycle previous ones)
> 
> As you can see, we win the greener price of the year.
> 
> This patches are on top of my previous series for optional sections.  You can get the whole bunch at:
> 
> 
> https://github.com/juanquintela/qemu/commits/migration-events

I compiled QEMU from Juan's github repository ^ and tested it with
libvirt patches I just sent to the list
(https://www.redhat.com/archives/libvir-list/2015-June/msg00064.html)
and it all works very well.

Thanks Juan, you can count this as

Tested-by: Jiri Denemark <jdenemar@redhat.com>

Jirka

Re: [Qemu-devel] [PATCH 1/3] migration: Remove duplicated assignement of SETUP status

[Juan Quintela]

John Snow <jsnow@redhat.com> wrote:
> On 05/20/2015 11:58 AM, Eric Blake wrote:
>> On 05/20/2015 09:35 AM, Juan Quintela wrote:
>>> We assign the MIGRATION_STATUS_SETUP status in two places.  Just
>>> in sucession.  Just remove the second one.
>> 
>> s/sucession/succession/
>> 
>
> Also s/assignement/assignment/ in the subject.
>
>> Where's the second place? Not in migrate_fd_connect...
>> 
>> okay, migrate_init() does the same thing, and I guess you're
>> arguing that it was always called either just before or just after 
>> migrate_fd_connect can be called. But mentioning it in the commit 
>> message would help.
>> 
>>> 
>>> Signed-off-by: Juan Quintela <quintela@redhat.com> --- 
>>> migration/migration.c | 3 --- 1 file changed, 3 deletions(-)
>>> 
>> 
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 

Thanks to both of you.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: create migration event

[Juan Quintela]

Jiri Denemark <jdenemar@redhat.com> wrote:
> On Wed, May 20, 2015 at 17:35:23 +0200, Juan Quintela wrote:
>> We have one argument that tells us what event has happened.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  docs/qmp/qmp-events.txt | 16 ++++++++++++++++
>>  migration/migration.c   | 12 ++++++++++++
>>  qapi/event.json         | 14 ++++++++++++++
>>  3 files changed, 42 insertions(+)
>
> Hi Juan,
>
> I patched libvirt to be able to consume this event and it all seems to
> work fine except for events sent after migrate_cancel command:
>
> {"execute":"migrate_cancel","id":"libvirt-33"}
> {"timestamp": {"seconds": 1432899178, "microseconds": 844907}, "event":
> "MIGRATION", "data": {"status": "cancelling"}}
> {"return": {}, "id": "libvirt-33"}
> {"timestamp": {"seconds": 1432899178, "microseconds": 845625}, "event":
> "MIGRATION", "data": {"status": "failed"}}
> {"timestamp": {"seconds": 1432899178, "microseconds": 846432}, "event":
> "MIGRATION", "data": {"status": "cancelled"}}
>
> In other words, the status first changes to "failed" and then it changes
> correctly "cancelled". Can you fix this weird behavior in QEMU or do we
> have to work around it in libvirt?
>
> Jirka

Found root cause of this, basically we had the code:

migrate_set_state(FOO, MIGRATION_STATUS_FAILED);
qapi_send_event(...., MIGRATION_STATUS_FAILED);

But the first call only change the state if the current state is FOO.
So, changed the event to inside migrate_set_state() and only sent it
when we change the state.

See new patch series, basically we were using incorrectly
atomic_cmpxchg() and that made things ... interesting.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: create migration event

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/20/2015 09:35 AM, Juan Quintela wrote:
>> We have one argument that tells us what event has happened.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  docs/qmp/qmp-events.txt | 16 ++++++++++++++++
>>  migration/migration.c   | 12 ++++++++++++
>>  qapi/event.json         | 14 ++++++++++++++
>>  3 files changed, 42 insertions(+)
>> 
>> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
>> index 4c13d48..3797709 100644
>> --- a/docs/qmp/qmp-events.txt
>> +++ b/docs/qmp/qmp-events.txt
>> @@ -473,6 +473,22 @@ Example:
>>  { "timestamp": {"seconds": 1290688046, "microseconds": 417172},
>>    "event": "SPICE_MIGRATE_COMPLETED" }
>> 
>> +MIGRATION
>> +---------
>> +
>> +Emitted when a migration event happens
>> +
>> +Data: None.
>> +
>> + - "status": migration status
>> +     "": error has been ignored
>
> Uggh. Looking for an empty string is awkward.

We are using MigrationStatus from qapi-schema.json, add the comment
stating that.

>
>> +     "report": error has been reported to the device
>> +     "stop": the VM is going to stop because of the error
>> +
>> +Example:
>> +
>> +{"timestamp": {"seconds": 1432121972, "microseconds": 744001},
>> + "event": "MIGRATION", "data": {"status": "completed"}}
>
> The example lists "completed", but the documentation does not mention
> it. Might be good to expand the docs to mention all states, and/or point
> to the enum definition.

See above.


>
>
>> +++ b/qapi/event.json
>> @@ -243,6 +243,20 @@
>>  { 'event': 'SPICE_MIGRATE_COMPLETED' }
>> 
>>  ##
>> +# @MIGRATION
>> +#
>> +# Emitted when a migration event happens
>> +#
>> +# @status: @MigrationStatus describing the current migration status.
>> +#          If this field is not returned, no migration process
>> +#          has been initiated
>
> Rather than returning an empty string,...
>
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'event': 'MIGRATION',
>> +  'data': {'status': 'MigrationStatus'}}
>
> ...this field should be marked optional, as in '*status'.  Then in your
> callers, you'll have to pass true or false for has_status, so that you
> can omit status when there is none.  But really, when will this event
> ever be omitted if migration has not been initiated?  Maybe it is just
> bogus documentation that you can return an empty string, as I didn't see
> any addition of a call to qapi_event_send_migration() that would pass an
> empty string on the wire.  So it sounds to me like the interface is
> okay, but the documentation is wrong.

It is wrong documentation, sorry for the inconvenience.

Later, Juan.