| Message ID | 1432136124-24572-3-git-send-email-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> 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.
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
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.
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.
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.
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(+)