[[RFC,WIP] v1] Adding feature to reconnect with -r option to migrate command

Commit Message

Md Haris Iqbal June 8, 2016, 6:13 p.m. UTC

---
 hmp-commands.hx               | 10 +++++---
 hmp.c                         |  4 ++-
 include/migration/migration.h |  1 +
 migration/migration.c         | 60 +++++++++++++++++++++++++++++++++----------
 qapi-schema.json              |  2 +-
 qmp-commands.hx               |  3 ++-
 6 files changed, 60 insertions(+), 20 deletions(-)

Comments

Dr. David Alan Gilbert June 15, 2016, 1:27 p.m. UTC | #1

* Md Haris Iqbal ([e-mail]) wrote:
> ---
>  hmp-commands.hx               | 10 +++++---
>  hmp.c                         |  4 ++-
>  include/migration/migration.h |  1 +
>  migration/migration.c         | 60 +++++++++++++++++++++++++++++++++----------
>  qapi-schema.json              |  2 +-
>  qmp-commands.hx               |  3 ++-
>  6 files changed, 60 insertions(+), 20 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 4f4f60a..2e7aed3 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -893,10 +893,11 @@ ETEXI
>  
>      {
>          .name       = "migrate",
> -        .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
> -        .params     = "[-d] [-b] [-i] uri",
> +        .args_type  = "detach:-d,recover:-r,blk:-b,inc:-i,uri:s",
> +        .params     = "[-d] [-r] [-b] [-i] uri",
>          .help       = "migrate to URI (using -d to not wait for completion)"
> -		      "\n\t\t\t -b for migration without shared storage with"
> +		      "\n\t\t\t -r to recover from a broken migration\n\t\t\t"
> +              " -b for migration without shared storage with"

Is that a tab/spce indent problem?

>  		      " full copy of disk\n\t\t\t -i for migration without "
>  		      "shared storage with incremental copy of disk "
>  		      "(base image shared between src and destination)",
> @@ -905,9 +906,10 @@ ETEXI
>  
>  
>  STEXI
> -@item migrate [-d] [-b] [-i] @var{uri}
> +@item migrate [-d] [-r] [-b] [-i] @var{uri}
>  @findex migrate
>  Migrate to @var{uri} (using -d to not wait for completion).
> +    -r to recover from a broken migration

Is that a tab/spce indent problem?

>  	-b for migration with full copy of disk
>  	-i for migration with incremental copy of disk (base image is shared)
>  ETEXI
> diff --git a/hmp.c b/hmp.c
> index d510236..ec8fab4 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1544,12 +1544,14 @@ static void hmp_migrate_status_cb(void *opaque)
>  void hmp_migrate(Monitor *mon, const QDict *qdict)
>  {
>      bool detach = qdict_get_try_bool(qdict, "detach", false);
> +    bool recover = qdict_get_try_bool(qdict, "recover", false);
>      bool blk = qdict_get_try_bool(qdict, "blk", false);
>      bool inc = qdict_get_try_bool(qdict, "inc", false);
>      const char *uri = qdict_get_str(qdict, "uri");
>      Error *err = NULL;
>  
> -    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> +    qmp_migrate(uri, !!recover, recover, !!blk, blk, !!inc, inc, false, false,
> +                &err);
>      if (err) {
>          error_report_err(err);
>          return;
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index ac2c12c..4a3201b 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -139,6 +139,7 @@ struct MigrationState
>  
>      int state;
>      MigrationParams params;
> +    bool in_recovery;
>  
>      /* State related to return path */
>      struct {
> diff --git a/migration/migration.c b/migration/migration.c
> index 991313a..a77f62e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -933,6 +933,7 @@ MigrationState *migrate_init(const MigrationParams *params)
>      s->xfer_limit = 0;
>      s->cleanup_bh = 0;
>      s->to_dst_file = NULL;
> +    s->in_recovery = false;
>      s->state = MIGRATION_STATUS_NONE;
>      s->params = *params;
>      s->rp_state.from_dst_file = NULL;
> @@ -992,13 +993,14 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
>      once = false;
>  }
>  
> -void qmp_migrate(const char *uri, bool has_blk, bool blk,
> -                 bool has_inc, bool inc, bool has_detach, bool detach,
> +void qmp_migrate(const char *uri, bool in_recover, bool recover, bool has_blk,
> +                 bool blk, bool has_inc, bool inc, bool has_detach, bool detach,
>                   Error **errp)
>  {
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      MigrationParams params;
> +    bool recovery = in_recover && recover;
>      const char *p;

You have an odd mix of things called recover and recovery.

>  
>      params.blk = has_blk && blk;
> @@ -1023,7 +1025,31 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          return;
>      }
>  
> -    s = migrate_init(&params);
> +    if (recovery ^ atomic_mb_read(&s->in_recovery)) {
> +        if (recovery) {
> +            /* No VM is waiting for recovery and
> +             * recovery option was set
> +             */
> +
> +            error_setg(errp, QERR_UNDEFINED_ERROR);
> +            return;
> +        } else {
> +            /* A VM is waiting for recovery and
> +             * no recovery option is set
> +             */
> +
> +            error_setg(errp, QERR_UNDEFINED_ERROR);
> +            return;

Use error messages that explain the problem; e.g.
   error_setg(errp, "Can't start a new migration while a migration is in recovery");

But I wonder; do you actually need an s->in_recovery?  Can't you just use the current
migration state - it's postcopy_recovery  then is that the same as your in_recovery
flag?   Do we change state once we start the recovery, at the moment I think you're
going:
    postcopy_active -> postmigrate_recovery
but should we go to :
    postcopy_active -> postmigrate_recovery -> postmigrate_recovery_active ?

> +        }
> +    } else {
> +        if (!recovery) {
> +            /* No VM is waiting for recovery and
> +             * no recovery option is set
> +             */
> +            fprintf(stderr, "hello1\n");
> +            s = migrate_init(&params);
> +        }
> +    }
>  
>      if (strstart(uri, "tcp:", &p)) {
>          tcp_start_outgoing_migration(s, p, &local_err);
> @@ -1767,16 +1793,6 @@ static void *migration_thread(void *opaque)
>  
>  void migrate_fd_connect(MigrationState *s)
>  {
> -    /* 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);
> -
> -    qemu_file_set_rate_limit(s->to_dst_file,
> -                             s->bandwidth_limit / XFER_LIMIT_RATIO);
> -
> -    /* Notify before starting migration thread */
> -    notifier_list_notify(&migration_state_notifiers, s);
> -
>      /*
>       * Open the return path; currently for postcopy but other things might
>       * also want it.
> @@ -1791,6 +1807,24 @@ void migrate_fd_connect(MigrationState *s)
>          }
>      }
>  
> +    if (atomic_mb_read(&s->in_recovery)) {
> +        atomic_mb_set(&s->in_recovery, 0);
> +
> +        fprintf(stderr, "recovered\n");
> +        return;
> +    }

Mark the places where you're going to add stuff with /* TODO: explanation */

> +
> +    /* 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);
> +
> +    qemu_file_set_rate_limit(s->to_dst_file,
> +                             s->bandwidth_limit / XFER_LIMIT_RATIO);
> +
> +    /* Notify before starting migration thread */
> +    notifier_list_notify(&migration_state_notifiers, s);
> +
> +
>      migrate_compress_threads_create();
>      qemu_thread_create(&s->thread, "migration", migration_thread, s,
>                         QEMU_THREAD_JOINABLE);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 54634c4..1b7b1e1 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2070,7 +2070,7 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
> +  'data': {'uri': 'str', '*recover': 'bool', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
>  
>  ##
>  # @migrate-incoming
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index de896a5..c8a1a93 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -612,7 +612,7 @@ EQMP
>  
>      {
>          .name       = "migrate",
> -        .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
> +        .args_type  = "detach:-d,recover:-r,blk:-b,inc:-i,uri:s",
>          .mhandler.cmd_new = qmp_marshal_migrate,
>      },
>  
> @@ -625,6 +625,7 @@ Migrate to URI.
>  Arguments:
>  
>  - "blk": block migration, full disk copy (json-bool, optional)
> +- "recover": recover migration (json-bool, optional)
>  - "inc": incremental disk copy (json-bool, optional)
>  - "uri": Destination URI (json-string)
>  
> -- 
> 2.7.4
> 

Dave

--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 4f4f60a..2e7aed3 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -893,10 +893,11 @@  ETEXI
 
     {
         .name       = "migrate",
-        .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
-        .params     = "[-d] [-b] [-i] uri",
+        .args_type  = "detach:-d,recover:-r,blk:-b,inc:-i,uri:s",
+        .params     = "[-d] [-r] [-b] [-i] uri",
         .help       = "migrate to URI (using -d to not wait for completion)"
-		      "\n\t\t\t -b for migration without shared storage with"
+		      "\n\t\t\t -r to recover from a broken migration\n\t\t\t"
+              " -b for migration without shared storage with"
 		      " full copy of disk\n\t\t\t -i for migration without "
 		      "shared storage with incremental copy of disk "
 		      "(base image shared between src and destination)",
@@ -905,9 +906,10 @@  ETEXI
 
 
 STEXI
-@item migrate [-d] [-b] [-i] @var{uri}
+@item migrate [-d] [-r] [-b] [-i] @var{uri}
 @findex migrate
 Migrate to @var{uri} (using -d to not wait for completion).
+    -r to recover from a broken migration
 	-b for migration with full copy of disk
 	-i for migration with incremental copy of disk (base image is shared)
 ETEXI
diff --git a/hmp.c b/hmp.c
index d510236..ec8fab4 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1544,12 +1544,14 @@  static void hmp_migrate_status_cb(void *opaque)
 void hmp_migrate(Monitor *mon, const QDict *qdict)
 {
     bool detach = qdict_get_try_bool(qdict, "detach", false);
+    bool recover = qdict_get_try_bool(qdict, "recover", false);
     bool blk = qdict_get_try_bool(qdict, "blk", false);
     bool inc = qdict_get_try_bool(qdict, "inc", false);
     const char *uri = qdict_get_str(qdict, "uri");
     Error *err = NULL;
 
-    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
+    qmp_migrate(uri, !!recover, recover, !!blk, blk, !!inc, inc, false, false,
+                &err);
     if (err) {
         error_report_err(err);
         return;
diff --git a/include/migration/migration.h b/include/migration/migration.h
index ac2c12c..4a3201b 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -139,6 +139,7 @@  struct MigrationState
 
     int state;
     MigrationParams params;
+    bool in_recovery;
 
     /* State related to return path */
     struct {
diff --git a/migration/migration.c b/migration/migration.c
index 991313a..a77f62e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -933,6 +933,7 @@  MigrationState *migrate_init(const MigrationParams *params)
     s->xfer_limit = 0;
     s->cleanup_bh = 0;
     s->to_dst_file = NULL;
+    s->in_recovery = false;
     s->state = MIGRATION_STATUS_NONE;
     s->params = *params;
     s->rp_state.from_dst_file = NULL;
@@ -992,13 +993,14 @@  void qmp_migrate_incoming(const char *uri, Error **errp)
     once = false;
 }
 
-void qmp_migrate(const char *uri, bool has_blk, bool blk,
-                 bool has_inc, bool inc, bool has_detach, bool detach,
+void qmp_migrate(const char *uri, bool in_recover, bool recover, bool has_blk,
+                 bool blk, bool has_inc, bool inc, bool has_detach, bool detach,
                  Error **errp)
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
     MigrationParams params;
+    bool recovery = in_recover && recover;
     const char *p;
 
     params.blk = has_blk && blk;
@@ -1023,7 +1025,31 @@  void qmp_migrate(const char *uri, bool has_blk, bool blk,
         return;
     }
 
-    s = migrate_init(&params);
+    if (recovery ^ atomic_mb_read(&s->in_recovery)) {
+        if (recovery) {
+            /* No VM is waiting for recovery and
+             * recovery option was set
+             */
+
+            error_setg(errp, QERR_UNDEFINED_ERROR);
+            return;
+        } else {
+            /* A VM is waiting for recovery and
+             * no recovery option is set
+             */
+
+            error_setg(errp, QERR_UNDEFINED_ERROR);
+            return;
+        }
+    } else {
+        if (!recovery) {
+            /* No VM is waiting for recovery and
+             * no recovery option is set
+             */
+            fprintf(stderr, "hello1\n");
+            s = migrate_init(&params);
+        }
+    }
 
     if (strstart(uri, "tcp:", &p)) {
         tcp_start_outgoing_migration(s, p, &local_err);
@@ -1767,16 +1793,6 @@  static void *migration_thread(void *opaque)
 
 void migrate_fd_connect(MigrationState *s)
 {
-    /* 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);
-
-    qemu_file_set_rate_limit(s->to_dst_file,
-                             s->bandwidth_limit / XFER_LIMIT_RATIO);
-
-    /* Notify before starting migration thread */
-    notifier_list_notify(&migration_state_notifiers, s);
-
     /*
      * Open the return path; currently for postcopy but other things might
      * also want it.
@@ -1791,6 +1807,24 @@  void migrate_fd_connect(MigrationState *s)
         }
     }
 
+    if (atomic_mb_read(&s->in_recovery)) {
+        atomic_mb_set(&s->in_recovery, 0);
+
+        fprintf(stderr, "recovered\n");
+        return;
+    }
+
+    /* 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);
+
+    qemu_file_set_rate_limit(s->to_dst_file,
+                             s->bandwidth_limit / XFER_LIMIT_RATIO);
+
+    /* Notify before starting migration thread */
+    notifier_list_notify(&migration_state_notifiers, s);
+
+
     migrate_compress_threads_create();
     qemu_thread_create(&s->thread, "migration", migration_thread, s,
                        QEMU_THREAD_JOINABLE);
diff --git a/qapi-schema.json b/qapi-schema.json
index 54634c4..1b7b1e1 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2070,7 +2070,7 @@ 
 # Since: 0.14.0
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
+  'data': {'uri': 'str', '*recover': 'bool', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
 
 ##
 # @migrate-incoming
diff --git a/qmp-commands.hx b/qmp-commands.hx
index de896a5..c8a1a93 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -612,7 +612,7 @@  EQMP
 
     {
         .name       = "migrate",
-        .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
+        .args_type  = "detach:-d,recover:-r,blk:-b,inc:-i,uri:s",
         .mhandler.cmd_new = qmp_marshal_migrate,
     },
 
@@ -625,6 +625,7 @@  Migrate to URI.
 Arguments:
 
 - "blk": block migration, full disk copy (json-bool, optional)
+- "recover": recover migration (json-bool, optional)
 - "inc": incremental disk copy (json-bool, optional)
 - "uri": Destination URI (json-string)