[Qemu-devel] [PATCH] migration: Introduce MIGRATION_STATUS_FINISHING

[Qemu-devel] [PATCH] migration: Introduce MIGRATION_STATUS_FINISHING

[Pavel Fedin]

This status is set after vm_stop_force_state(), and is telling us that all
CPUs are stopped, and we are finishing up with the migration.

Also, call notifier_list_notify() when this status is set. This will be
necessary for ITS live migration on ARM, which will have to dump its state
into guest RAM at this point.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hmp.c                         |  1 +
 include/migration/migration.h |  1 +
 migration/migration.c         | 19 ++++++++++++++++---
 qapi-schema.json              |  4 +++-
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/hmp.c b/hmp.c
index 5048eee..202768f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1467,6 +1467,7 @@ static void hmp_migrate_status_cb(void *opaque)
 
     info = qmp_query_migrate(NULL);
     if (!info->has_status || info->status == MIGRATION_STATUS_ACTIVE ||
+        info->status == MIGRATION_STATUS_FINISHING ||
         info->status == MIGRATION_STATUS_SETUP) {
         if (info->has_disk) {
             int progress;
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 8334621..ff4bfcb 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -117,6 +117,7 @@ int migrate_fd_close(MigrationState *s);
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
 bool migration_in_setup(MigrationState *);
+bool migration_in_finishing(MigrationState *);
 bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
 MigrationState *migrate_get_current(void);
diff --git a/migration/migration.c b/migration/migration.c
index b092f38..8f28585 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -427,6 +427,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->has_total_time = false;
         break;
     case MIGRATION_STATUS_ACTIVE:
+    case MIGRATION_STATUS_FINISHING:
     case MIGRATION_STATUS_CANCELLING:
         info->has_status = true;
         info->has_total_time = true;
@@ -507,6 +508,7 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     MigrationCapabilityStatusList *cap;
 
     if (s->state == MIGRATION_STATUS_ACTIVE ||
+        s->state == MIGRATION_STATUS_FINISHING ||
         s->state == MIGRATION_STATUS_SETUP) {
         error_setg(errp, QERR_MIGRATION_ACTIVE);
         return;
@@ -641,7 +643,8 @@ static void migrate_fd_cancel(MigrationState *s)
     do {
         old_state = s->state;
         if (old_state != MIGRATION_STATUS_SETUP &&
-            old_state != MIGRATION_STATUS_ACTIVE) {
+            old_state != MIGRATION_STATUS_ACTIVE &&
+            old_state != MIGRATION_STATUS_FINISHING) {
             break;
         }
         migrate_set_state(s, old_state, MIGRATION_STATUS_CANCELLING);
@@ -674,6 +677,11 @@ bool migration_in_setup(MigrationState *s)
     return s->state == MIGRATION_STATUS_SETUP;
 }
 
+bool migration_in_finishing(MigrationState *s)
+{
+    return s->state == MIGRATION_STATUS_FINISHING;
+}
+
 bool migration_has_finished(MigrationState *s)
 {
     return s->state == MIGRATION_STATUS_COMPLETED;
@@ -774,6 +782,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     params.shared = has_inc && inc;
 
     if (s->state == MIGRATION_STATUS_ACTIVE ||
+        s->state == MIGRATION_STATUS_FINISHING ||
         s->state == MIGRATION_STATUS_SETUP ||
         s->state == MIGRATION_STATUS_CANCELLING) {
         error_setg(errp, QERR_MIGRATION_ACTIVE);
@@ -985,6 +994,7 @@ int64_t migrate_xbzrle_cache_size(void)
 static void migration_completion(MigrationState *s, bool *old_vm_running,
                                  int64_t *start_time)
 {
+    MigrationStatus state = MIGRATION_STATUS_ACTIVE;
     int ret;
 
     qemu_mutex_lock_iothread();
@@ -996,6 +1006,9 @@ static void migration_completion(MigrationState *s, bool *old_vm_running,
     if (!ret) {
         ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
         if (ret >= 0) {
+            state = MIGRATION_STATUS_FINISHING;
+            migrate_set_state(s, MIGRATION_STATUS_ACTIVE, state);
+            notifier_list_notify(&migration_state_notifiers, s);
             qemu_file_set_rate_limit(s->file, INT64_MAX);
             qemu_savevm_state_complete(s->file);
         }
@@ -1011,11 +1024,11 @@ static void migration_completion(MigrationState *s, bool *old_vm_running,
         goto fail;
     }
 
-    migrate_set_state(s, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_COMPLETED);
+    migrate_set_state(s, state, MIGRATION_STATUS_COMPLETED);
     return;
 
 fail:
-    migrate_set_state(s, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_FAILED);
+    migrate_set_state(s, state, MIGRATION_STATUS_FAILED);
 }
 
 /* migration thread support */
diff --git a/qapi-schema.json b/qapi-schema.json
index f60be29..c6226da 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -430,6 +430,8 @@
 #
 # @active: in the process of doing migration.
 #
+# @finishing: migration is being finished, CPUs have been stopped.
+#
 # @completed: migration is finished.
 #
 # @failed: some error occurred during migration process.
@@ -439,7 +441,7 @@
 ##
 { 'enum': 'MigrationStatus',
   'data': [ 'none', 'setup', 'cancelling', 'cancelled',
-            'active', 'completed', 'failed' ] }
+            'active', 'finishing', 'completed', 'failed' ] }
 
 ##
 # @MigrationInfo
-- 
1.9.5.msysgit.0

Re: [Qemu-devel] [PATCH] migration: Introduce MIGRATION_STATUS_FINISHING

[Eric Blake]

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

On 10/26/2015 06:47 AM, Pavel Fedin wrote:
> This status is set after vm_stop_force_state(), and is telling us that all
> CPUs are stopped, and we are finishing up with the migration.
> 
> Also, call notifier_list_notify() when this status is set. This will be
> necessary for ITS live migration on ARM, which will have to dump its state
> into guest RAM at this point.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---

Interface review only:

> +++ b/qapi-schema.json
> @@ -430,6 +430,8 @@
>  #
>  # @active: in the process of doing migration.
>  #
> +# @finishing: migration is being finished, CPUs have been stopped.
> +#

Missing a '(since 2.5)' notation.  Also, adding new user-visible states
has a tendency to break existing clients that aren't prepared for
unexpected states (although technically such bugs are in the client - in
the past, libvirt used to be one such client, although we've tried to
fix it to gracefully ignore unknown states).  Are we sure no other
existing state works for this?  I'm not opposed to the addition, just
wanting to make sure we have good reason for it.

One design idea for adding new states is making sure the new state will
not be exposed unless the client specifies some new option to enable the
state, so that old clients will never see the state and new clients have
expressed their interest in the state by opting-in to it with the new
option.

>  # @completed: migration is finished.
>  #
>  # @failed: some error occurred during migration process.
> @@ -439,7 +441,7 @@
>  ##
>  { 'enum': 'MigrationStatus',
>    'data': [ 'none', 'setup', 'cancelling', 'cancelled',
> -            'active', 'completed', 'failed' ] }
> +            'active', 'finishing', 'completed', 'failed' ] }
>  
>  ##
>  # @MigrationInfo
> 

-- 
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] migration: Introduce MIGRATION_STATUS_FINISHING

[Pavel Fedin]

 Hello!

> adding new user-visible states
> has a tendency to break existing clients that aren't prepared for
> unexpected states (although technically such bugs are in the client - in
> the past, libvirt used to be one such client, although we've tried to
> fix it to gracefully ignore unknown states).

 Yes, i know this, my host uses libvirt v1.2.9.3 (i backport necessary patches to it) and it barfed. I didn't check master though...

>  Are we sure no other
> existing state works for this?  I'm not opposed to the addition, just
> wanting to make sure we have good reason for it.

 You know, actually, this is a thing only for qemu's internal use, we don't need a new state at all. Instead, we could introduce a 'bool cpus_stopped' to MigrationState structure and test for it in migration_in_finishing(), like:
--- cut ---
bool migration_in_finishing(MigrationState *s)
{
    return s->cpus_stopped;
}
--- cut ---
 What do you think about this solution? It is much less complicated than...

> One design idea for adding new states is making sure the new state will
> not be exposed unless the client specifies some new option to enable the
> state, so that old clients will never see the state and new clients have
> expressed their interest in the state

 With this kind of approach we would not be able to migrate ITS without this option. Because it's not external clients being interested in this state at the moment, but qemu itself.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] migration: Introduce MIGRATION_STATUS_FINISHING

[Eric Blake]

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

On 10/27/2015 01:08 AM, Pavel Fedin wrote:
>  Hello!
> 
>> adding new user-visible states
>> has a tendency to break existing clients that aren't prepared for
>> unexpected states (although technically such bugs are in the client - in
>> the past, libvirt used to be one such client, although we've tried to
>> fix it to gracefully ignore unknown states).
> 
>  Yes, i know this, my host uses libvirt v1.2.9.3 (i backport necessary patches to it) and it barfed. I didn't check master though...
> 
>>  Are we sure no other
>> existing state works for this?  I'm not opposed to the addition, just
>> wanting to make sure we have good reason for it.
> 
>  You know, actually, this is a thing only for qemu's internal use, we don't need a new state at all. Instead, we could introduce a 'bool cpus_stopped' to MigrationState structure and test for it in migration_in_finishing(), like:
> --- cut ---
> bool migration_in_finishing(MigrationState *s)
> {
>     return s->cpus_stopped;
> }
> --- cut ---
>  What do you think about this solution? It is much less complicated than...

If it is truly internal, then avoiding exposing the state to external
clients is indeed the way to go.

-- 
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] migration: Introduce MIGRATION_STATUS_FINISHING

[Pavel Fedin]

 Hello!

> If it is truly internal, then avoiding exposing the state to external
> clients is indeed the way to go.

 Already done: http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg06262.html, you should have received it too.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia