[Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Alon Levy]

Fix the ticket expiration on target vm for a spice connection without introducing
a race between the spice server switching the client to the new host itself and
the target libvirt setting the new expiration date, by adding an option to
client_migrate_info to not automatically switch the client on migration completion,
instead waiting for an explicit client_migrate_switch (new monitor command) from
libvirt.

The first patch is more of an RFC, it fixes the missing optional argument
support for boolean arguments, instead of doing it as a simple copy paste done
for all the other arguments it refactors that parsing before the main switch.

Alon Levy (3):
  monitor: refactor whitespace and optional argument parsing
  spice-core: client_migrate_info: add optional auto_switch parameter
    (RHBZ 725009)
  monitor: add client_migrate_switch command (RHBZ 725009)

 hmp-commands.hx |   21 +++++++++--
 monitor.c       |  106 +++++++++++++++++++++++++++++--------------------------
 qmp-commands.hx |   32 +++++++++++++++-
 ui/qemu-spice.h |    7 +++-
 ui/spice-core.c |   18 ++++++++-
 5 files changed, 126 insertions(+), 58 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/3] monitor: refactor whitespace and optional argument parsing

[Alon Levy]

Takes out the optional ('?') message parsing from the main switch loop
in monitor_parse_command. Adds optional argument option for boolean parameters.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 monitor.c |   79 +++++++++++++++++++++++-------------------------------------
 1 files changed, 30 insertions(+), 49 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6e3d970..baf46ba 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4122,6 +4122,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
             break;
         c = *typestr;
         typestr++;
+        while (qemu_isspace(*p)) {
+            p++;
+        }
+        /* take care of optional arguments */
+        switch (c) {
+        case 's':
+        case 'i':
+        case 'l':
+        case 'M':
+        case 'o':
+        case 'T':
+        case 'b':
+            if (*typestr == '?') {
+                typestr++;
+                if (*p == '\0') {
+                    /* missing optional argument: NULL argument */
+                    qemu_free(key);
+                    key = NULL;
+                    continue;
+                }
+            }
+            break;
+        }
         switch(c) {
         case 'F':
         case 'B':
@@ -4129,15 +4152,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
             {
                 int ret;
 
-                while (qemu_isspace(*p))
-                    p++;
-                if (*typestr == '?') {
-                    typestr++;
-                    if (*p == '\0') {
-                        /* no optional string: NULL argument */
-                        break;
-                    }
-                }
                 ret = get_str(buf, sizeof(buf), &p);
                 if (ret < 0) {
                     switch(c) {
@@ -4167,9 +4181,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 if (!opts_list || opts_list->desc->name) {
                     goto bad_type;
                 }
-                while (qemu_isspace(*p)) {
-                    p++;
-                }
                 if (!*p)
                     break;
                 if (get_str(buf, sizeof(buf), &p) < 0) {
@@ -4187,8 +4198,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
             {
                 int count, format, size;
 
-                while (qemu_isspace(*p))
-                    p++;
                 if (*p == '/') {
                     /* format found */
                     p++;
@@ -4268,23 +4277,15 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
             {
                 int64_t val;
 
-                while (qemu_isspace(*p))
-                    p++;
-                if (*typestr == '?' || *typestr == '.') {
-                    if (*typestr == '?') {
-                        if (*p == '\0') {
-                            typestr++;
-                            break;
-                        }
-                    } else {
-                        if (*p == '.') {
+                if (*typestr == '.') {
+                    if (*p == '.') {
+                        p++;
+                        while (qemu_isspace(*p)) {
                             p++;
-                            while (qemu_isspace(*p))
-                                p++;
-                        } else {
-                            typestr++;
-                            break;
                         }
+                    } else {
+                        typestr++;
+                        break;
                     }
                     typestr++;
                 }
@@ -4306,15 +4307,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 int64_t val;
                 char *end;
 
-                while (qemu_isspace(*p)) {
-                    p++;
-                }
-                if (*typestr == '?') {
-                    typestr++;
-                    if (*p == '\0') {
-                        break;
-                    }
-                }
                 val = strtosz(p, &end);
                 if (val < 0) {
                     monitor_printf(mon, "invalid size\n");
@@ -4328,14 +4320,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
             {
                 double val;
 
-                while (qemu_isspace(*p))
-                    p++;
-                if (*typestr == '?') {
-                    typestr++;
-                    if (*p == '\0') {
-                        break;
-                    }
-                }
                 if (get_double(mon, &val, &p) < 0) {
                     goto fail;
                 }
@@ -4361,9 +4345,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 const char *beg;
                 int val;
 
-                while (qemu_isspace(*p)) {
-                    p++;
-                }
                 beg = p;
                 while (qemu_isgraph(*p)) {
                     p++;
-- 
1.7.6

[Qemu-devel] [PATCH 2/3] spice-core: client_migrate_info: add optional auto_switch parameter (RHBZ 725009)

[Alon Levy]

Add an extra optional boolean parameter defaulting to true called auto_switch,
if auto_switch is true the client is requested to switch to the target qemu
upon migration completion. Only implemented for spice, ignored for vnc.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hmp-commands.hx |    6 +++---
 monitor.c       |    4 +++-
 qmp-commands.hx |    5 +++--
 ui/qemu-spice.h |    2 +-
 ui/spice-core.c |   14 ++++++++++++--
 5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 0ccfb28..fc2bf99 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -823,15 +823,15 @@ ETEXI
 
     {
         .name       = "client_migrate_info",
-        .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
-        .params     = "protocol hostname port tls-port cert-subject",
+        .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?,auto-switch:b?",
+        .params     = "protocol hostname port tls-port cert-subject auto-switch",
         .help       = "send migration info to spice/vnc client",
         .user_print = monitor_user_noop,
         .mhandler.cmd_new = client_migrate_info,
     },
 
 STEXI
-@item client_migrate_info @var{protocol} @var{hostname} @var{port} @var{tls-port} @var{cert-subject}
+@item client_migrate_info @var{protocol} @var{hostname} @var{port} @var{tls-port} @var{cert-subject} @var{auto-switch}
 @findex client_migrate_info
 Set the spice/vnc connection info for the migration target.  The spice/vnc
 server will ask the spice/vnc client to automatically reconnect using the
diff --git a/monitor.c b/monitor.c
index baf46ba..1daa283 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1226,6 +1226,7 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict, QObject **ret_d
     const char *subject  = qdict_get_try_str(qdict, "cert-subject");
     int port             = qdict_get_try_int(qdict, "port", -1);
     int tls_port         = qdict_get_try_int(qdict, "tls-port", -1);
+    int auto_switch      = qdict_get_try_bool(qdict, "auto-switch", 1);
     int ret;
 
     if (strcmp(protocol, "spice") == 0) {
@@ -1234,7 +1235,8 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict, QObject **ret_d
             return -1;
         }
 
-        ret = qemu_spice_migrate_info(hostname, port, tls_port, subject);
+        ret = qemu_spice_migrate_info(hostname, port, tls_port,
+                                      subject, auto_switch);
         if (ret != 0) {
             qerror_report(QERR_UNDEFINED_ERROR);
             return -1;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 03f67da..13b2bc6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -574,8 +574,8 @@ EQMP
 
     {
         .name       = "client_migrate_info",
-        .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
-        .params     = "protocol hostname port tls-port cert-subject",
+        .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?,auto-switch:b?",
+        .params     = "protocol hostname port tls-port cert-subject auto-switch",
         .help       = "send migration info to spice/vnc client",
         .user_print = monitor_user_noop,
         .mhandler.cmd_new = client_migrate_info,
@@ -596,6 +596,7 @@ Arguments:
 - "port":         spice/vnc tcp port for plaintext channels (json-int, optional)
 - "tls-port":     spice tcp port for tls-secured channels (json-int, optional)
 - "cert-subject": server certificate subject (json-string, optional)
+- "auto-switch":  automatically switch when migration is done (json-bool, optional)
 
 Example:
 
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index f34be69..e54f16e 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -37,7 +37,7 @@ int qemu_spice_set_passwd(const char *passwd,
                           bool fail_if_connected, bool disconnect_if_connected);
 int qemu_spice_set_pw_expire(time_t expires);
 int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
-                            const char *subject);
+                            const char *subject, int auto_switch);
 
 void do_info_spice_print(Monitor *mon, const QObject *data);
 void do_info_spice(Monitor *mon, QObject **ret_data);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 8bb62ea..3a1d851 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -42,6 +42,14 @@ static Notifier migration_state;
 static const char *auth = "spice";
 static char *auth_passwd;
 static time_t auth_expires = TIME_MAX;
+
+/* automatically switch host when migration is complete.
+ * This is on by default but we turn it off to address RHBZ #725009
+ * The longer term solution should be seamless migration by reconnecting
+ * on migration start instead of end, but that requires a harder change to
+ * all spice clients. */
+static int g_auto_switch = 1;
+
 int using_spice = 0;
 
 struct SpiceTimer {
@@ -428,7 +436,7 @@ static void migration_state_notifier(Notifier *notifier, void *data)
 {
     int state = get_migration_state();
 
-    if (state == MIG_STATE_COMPLETED) {
+    if (state == MIG_STATE_COMPLETED && g_auto_switch) {
 #if SPICE_SERVER_VERSION >= 0x000701 /* 0.7.1 */
         spice_server_migrate_switch(spice_server);
 #endif
@@ -436,12 +444,14 @@ static void migration_state_notifier(Notifier *notifier, void *data)
 }
 
 int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
-                            const char *subject)
+                            const char *subject, int auto_switch)
 {
+    g_auto_switch = auto_switch;
     return spice_server_migrate_info(spice_server, hostname,
                                      port, tls_port, subject);
 }
 
+
 static int add_channel(const char *name, const char *value, void *opaque)
 {
     int security = 0;
-- 
1.7.6

[Qemu-devel] [PATCH 3/3] monitor: add client_migrate_switch command (RHBZ 725009)

[Alon Levy]

Complementary to the auto_switch parameter of client_migrate_info, if that is
set to false the new command client_migrate_switch can be used to complete
the switch to the new client. This allows the vm manager (i.e. libvirt) to
first ensure the ticketing information in the target vm is valid before issuing
the switch.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hmp-commands.hx |   15 +++++++++++++++
 monitor.c       |   23 +++++++++++++++++++++++
 qmp-commands.hx |   27 +++++++++++++++++++++++++++
 ui/qemu-spice.h |    5 +++++
 ui/spice-core.c |    4 ++++
 5 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index fc2bf99..7979529 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -839,6 +839,21 @@ new parameters (if specified) once the vm migration finished successfully.
 ETEXI
 
     {
+        .name       = "client_migrate_switch",
+        .args_type  = "protocol:s",
+        .params     = "protocol",
+        .help       = "tell client to disconnect and reconnect to client_migrate_info set destination",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = client_migrate_switch,
+    },
+
+STEXI
+@item client_migrate_switch @var{protocol}
+@findex client_migrate_switch
+Tell the spice/vnc client to switch to the target set previously with client_migrate_info. Only has an effect if the last client_migrate_info used auto_switch=false.
+ETEXI
+
+    {
         .name       = "snapshot_blkdev",
         .args_type  = "device:B,snapshot-file:s?,format:s?",
         .params     = "device [new-image-file] [format]",
diff --git a/monitor.c b/monitor.c
index 1daa283..ae6f4ad 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1248,6 +1248,29 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict, QObject **ret_d
     return -1;
 }
 
+static int client_migrate_switch(Monitor *mon, const QDict *qdict,
+                                 QObject **ret_data)
+{
+    const char *protocol = qdict_get_str(qdict, "protocol");
+    int ret;
+
+    if (strcmp(protocol, "spice") == 0) {
+        if (!using_spice) {
+            qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
+            return -1;
+        }
+        ret = qemu_spice_migrate_switch();
+        if (ret != 0) {
+            qerror_report(QERR_UNDEFINED_ERROR);
+            return -1;
+        }
+        return 0;
+    }
+
+    qerror_report(QERR_INVALID_PARAMETER, "protocol");
+    return -1;
+}
+
 static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 13b2bc6..9e0fecf 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -609,6 +609,33 @@ Example:
 EQMP
 
     {
+        .name       = "client_migrate_switch",
+        .args_type  = "protocol:s",
+        .params     = "protocol",
+        .help       = "tell client to disconnect and reconnect to client_migrate_info set destination",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = client_migrate_switch,
+    },
+
+SQMP
+client_migrate_switch
+---------------------
+
+Tell the spice/vnc client to switch to the target set previously with client_migrate_info. Only has an effect if the last client_migrate_info used auto_switch=false.
+
+Arguments:
+
+- "protocol":     protocol: "spice" or "vnc" (json-string)
+
+Example:
+
+-> { "execute": "client_migrate_switch",
+     "arguments": { "protocol": "spice" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "netdev_add",
         .args_type  = "netdev:O",
         .params     = "[user|tap|socket],id=str[,prop=value][,...]",
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index e54f16e..113bd2f 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -38,6 +38,7 @@ int qemu_spice_set_passwd(const char *passwd,
 int qemu_spice_set_pw_expire(time_t expires);
 int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
                             const char *subject, int auto_switch);
+int qemu_spice_migrate_switch(void);
 
 void do_info_spice_print(Monitor *mon, const QObject *data);
 void do_info_spice(Monitor *mon, QObject **ret_data);
@@ -60,6 +61,10 @@ static inline int qemu_spice_set_pw_expire(time_t expires)
 static inline int qemu_spice_migrate_info(const char *h, int p, int t, const char *s)
 { return -1; }
 
+static inline int qemu_spice_migrate_switch(void)
+{ return -1; }
+
+
 #endif /* CONFIG_SPICE */
 
 #endif /* QEMU_SPICE_H */
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 3a1d851..83362c2 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -451,6 +451,10 @@ int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
                                      port, tls_port, subject);
 }
 
+int qemu_spice_migrate_switch(void)
+{
+    return spice_server_migrate_switch(spice_server);
+}
 
 static int add_channel(const char *name, const char *value, void *opaque)
 {
-- 
1.7.6

Re: [Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Gerd Hoffmann]

On 08/19/11 19:08, Alon Levy wrote:
> Fix the ticket expiration on target vm for a spice connection without introducing
> a race between the spice server switching the client to the new host itself and
> the target libvirt setting the new expiration date, by adding an option to
> client_migrate_info to not automatically switch the client on migration completion,
> instead waiting for an explicit client_migrate_switch (new monitor command) from
> libvirt.

Hmm.  Guess the fundamental issue is that libvirt wants to use the 
monitor to set the ticket instead of the command line for security 
reasons.  The qemu monitor doesn't accept commands while the incoming 
migration is running.  We also can't kick the incoming migration via 
monitor, so first setting the ticket then start migration doesn't work 
too.  Correct?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Daniel P. Berrange]

On Fri, Aug 26, 2011 at 11:54:49AM +0200, Gerd Hoffmann wrote:
> On 08/19/11 19:08, Alon Levy wrote:
> >Fix the ticket expiration on target vm for a spice connection without introducing
> >a race between the spice server switching the client to the new host itself and
> >the target libvirt setting the new expiration date, by adding an option to
> >client_migrate_info to not automatically switch the client on migration completion,
> >instead waiting for an explicit client_migrate_switch (new monitor command) from
> >libvirt.
> 
> Hmm.  Guess the fundamental issue is that libvirt wants to use the
> monitor to set the ticket instead of the command line for security
> reasons.  The qemu monitor doesn't accept commands while the
> incoming migration is running.  We also can't kick the incoming
> migration via monitor, so first setting the ticket then start
> migration doesn't work too.  Correct?

There is actually a reliable window where we can use the monitor
before incoming migration starts. Libvirt's migration is a 5 stage
handshake:

  1. Begin(src)
      - Gets current source VM XML config
  2. Prepare(dst)
      - Launches QEMU -incoming
      - Sets passwords, etc to monitor
  3. Perform(src)
      - Issue migrate_client_info
      - Issue migrate_set_speed
      - Issue migrate
      - Loop
          - Issue query-migrate
          - Break if finished/failed
  4. Finish(dst)
      - If success
          - start CPUs
        Else
          - Kill QEMU
  5. Confirm(src)
      - If success
          - Kill QEMU
        Else
          - Restart CPUs

Those stages are all serialized, so we can do anything we like
with the QEMU monitor at stage 2, before stage 3 will start
back on the src.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Gerd Hoffmann]

On 08/26/11 12:03, Daniel P. Berrange wrote:
> On Fri, Aug 26, 2011 at 11:54:49AM +0200, Gerd Hoffmann wrote:
>> On 08/19/11 19:08, Alon Levy wrote:
>>> Fix the ticket expiration on target vm for a spice connection without introducing
>>> a race between the spice server switching the client to the new host itself and
>>> the target libvirt setting the new expiration date, by adding an option to
>>> client_migrate_info to not automatically switch the client on migration completion,
>>> instead waiting for an explicit client_migrate_switch (new monitor command) from
>>> libvirt.
>>
>> Hmm.  Guess the fundamental issue is that libvirt wants to use the
>> monitor to set the ticket instead of the command line for security
>> reasons.  The qemu monitor doesn't accept commands while the
>> incoming migration is running.  We also can't kick the incoming
>> migration via monitor, so first setting the ticket then start
>> migration doesn't work too.  Correct?
>
> There is actually a reliable window where we can use the monitor
> before incoming migration starts. Libvirt's migration is a 5 stage
> handshake:
>
>    1. Begin(src)
>        - Gets current source VM XML config
>    2. Prepare(dst)
>        - Launches QEMU -incoming
>        - Sets passwords, etc to monitor
>    3. Perform(src)

> Those stages are all serialized, so we can do anything we like
> with the QEMU monitor at stage 2, before stage 3 will start
> back on the src.

Ok, so I think we should be able to fix the race outlined above without 
adding new monitor commands, just by letting libvirt set the spice 
ticket in stage 2.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Alon Levy]

On Fri, Aug 26, 2011 at 11:03:38AM +0100, Daniel P. Berrange wrote:
> On Fri, Aug 26, 2011 at 11:54:49AM +0200, Gerd Hoffmann wrote:
> > On 08/19/11 19:08, Alon Levy wrote:
> > >Fix the ticket expiration on target vm for a spice connection without introducing
> > >a race between the spice server switching the client to the new host itself and
> > >the target libvirt setting the new expiration date, by adding an option to
> > >client_migrate_info to not automatically switch the client on migration completion,
> > >instead waiting for an explicit client_migrate_switch (new monitor command) from
> > >libvirt.
> > 
> > Hmm.  Guess the fundamental issue is that libvirt wants to use the
> > monitor to set the ticket instead of the command line for security
> > reasons.  The qemu monitor doesn't accept commands while the
> > incoming migration is running.  We also can't kick the incoming
> > migration via monitor, so first setting the ticket then start
> > migration doesn't work too.  Correct?
> 
> There is actually a reliable window where we can use the monitor
> before incoming migration starts. Libvirt's migration is a 5 stage
> handshake:
> 

So the current suggestion would be to have:

>   1. Begin(src)
>       - Gets current source VM XML config
>   2. Prepare(dst)
>       - Launches QEMU -incoming
>       - Sets passwords, etc to monitor
>   3. Perform(src)
>       - Issue migrate_client_info

client_migrate_info implementation is changes to do:

spice-server sends switch command to spice client, and waits for
a success or error indication from it. Then the monitor command completes
and libvirt continues to the next one. Since this is before migrate
is issued the destination handles the spice client connection.

>       - Issue migrate_set_speed
>       - Issue migrate
>       - Loop
>           - Issue query-migrate
>           - Break if finished/failed
>   4. Finish(dst)
>       - If success
>           - start CPUs
>         Else
>           - Kill QEMU
>   5. Confirm(src)
>       - If success
>           - Kill QEMU
>         Else
>           - Restart CPUs
> 
> Those stages are all serialized, so we can do anything we like
> with the QEMU monitor at stage 2, before stage 3 will start
> back on the src.
> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Alon Levy]

On Fri, Aug 26, 2011 at 12:17:58PM +0200, Gerd Hoffmann wrote:
> On 08/26/11 12:03, Daniel P. Berrange wrote:
> >On Fri, Aug 26, 2011 at 11:54:49AM +0200, Gerd Hoffmann wrote:
> >>On 08/19/11 19:08, Alon Levy wrote:
> >>>Fix the ticket expiration on target vm for a spice connection without introducing
> >>>a race between the spice server switching the client to the new host itself and
> >>>the target libvirt setting the new expiration date, by adding an option to
> >>>client_migrate_info to not automatically switch the client on migration completion,
> >>>instead waiting for an explicit client_migrate_switch (new monitor command) from
> >>>libvirt.
> >>
> >>Hmm.  Guess the fundamental issue is that libvirt wants to use the
> >>monitor to set the ticket instead of the command line for security
> >>reasons.  The qemu monitor doesn't accept commands while the
> >>incoming migration is running.  We also can't kick the incoming
> >>migration via monitor, so first setting the ticket then start
> >>migration doesn't work too.  Correct?
> >
> >There is actually a reliable window where we can use the monitor
> >before incoming migration starts. Libvirt's migration is a 5 stage
> >handshake:
> >
> >   1. Begin(src)
> >       - Gets current source VM XML config
> >   2. Prepare(dst)
> >       - Launches QEMU -incoming
> >       - Sets passwords, etc to monitor
> >   3. Perform(src)
> 
> >Those stages are all serialized, so we can do anything we like
> >with the QEMU monitor at stage 2, before stage 3 will start
> >back on the src.
> 
> Ok, so I think we should be able to fix the race outlined above
> without adding new monitor commands, just by letting libvirt set the
> spice ticket in stage 2.

Is that different then what I suggested in my reply to Daniel's 5 stage outline?

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Gerd Hoffmann]

   Hi,

>> Ok, so I think we should be able to fix the race outlined above
>> without adding new monitor commands, just by letting libvirt set the
>> spice ticket in stage 2.
>
> Is that different then what I suggested in my reply to Daniel's 5 stage outline?

I think we have to care to not mix up switch-host and seamless spice 
client migration, your reply seems to refer top some seamless spice 
migration implementation ideas.

Today we seem to have this workflow:

    (1)  migration src->dst (stage 3)
    (2a) libvirt sets spice ticket at dst (not sure which stage)
    (2b) spice client switches connection to dst

2a and 2b are racing here.  I think we can fix the race by doing this 
instead:

   (1) libvirt sets spice ticket at dst (stage 2)
   (2) migration src->dst (stage 3)
   (3) spice client switches connection to dst

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Daniel P. Berrange]

On Fri, Aug 26, 2011 at 01:00:29PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >>Ok, so I think we should be able to fix the race outlined above
> >>without adding new monitor commands, just by letting libvirt set the
> >>spice ticket in stage 2.
> >
> >Is that different then what I suggested in my reply to Daniel's 5 stage outline?
> 
> I think we have to care to not mix up switch-host and seamless spice
> client migration, your reply seems to refer top some seamless spice
> migration implementation ideas.
> 
> Today we seem to have this workflow:
> 
>    (1)  migration src->dst (stage 3)
>    (2a) libvirt sets spice ticket at dst (not sure which stage)
>    (2b) spice client switches connection to dst

No, that's not what we do currently.

> 2a and 2b are racing here.  I think we can fix the race by doing
> this instead:
> 
>   (1) libvirt sets spice ticket at dst (stage 2)
>   (2) migration src->dst (stage 3)
>   (3) spice client switches connection to dst

This is actually what we have already. The problem is that
the 'migration src->dst' here can take an arbitrary amount
of time. So by the time the spice client switchs to dst,
the ticket will likely have already expired.

The only way to fix this AFAICT is to ensure the SPICE
client connects to dst before migration starts, but delays
display switch until the end. eg

    (1) libvirt sets spice ticket at dst (libvirt stage 2)
    (2) spice client connects to dst (start of libvirt stage 3)
    (3) migration src->dst (middle libvirt stage 3)
    (4) spice client switches to dst (end of libvirt stage 3 or stage 5)


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Gerd Hoffmann]

>>    (1) libvirt sets spice ticket at dst (stage 2)
>>    (2) migration src->dst (stage 3)
>>    (3) spice client switches connection to dst
>
> This is actually what we have already. The problem is that
> the 'migration src->dst' here can take an arbitrary amount
> of time. So by the time the spice client switchs to dst,
> the ticket will likely have already expired.

Ah, *that* is the bug.

> The only way to fix this AFAICT is to ensure the SPICE
> client connects to dst before migration starts, but delays
> display switch until the end. eg
>
>      (1) libvirt sets spice ticket at dst (libvirt stage 2)
>      (2) spice client connects to dst (start of libvirt stage 3)

That must be in stage 2.  Once the migration started qemu will not 
accept new connections and thus the client would not be able to connect.

Can qemu handle async monitor commands now?  We would need to wait until 
the client has actually connected, but without blocking the iothread 
because it can take a while.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 0/3] client_migrate_switch and auto_switch (RHBZ 725009)

[Daniel P. Berrange]

On Fri, Aug 26, 2011 at 02:39:59PM +0200, Gerd Hoffmann wrote:
> >>   (1) libvirt sets spice ticket at dst (stage 2)
> >>   (2) migration src->dst (stage 3)
> >>   (3) spice client switches connection to dst
> >
> >This is actually what we have already. The problem is that
> >the 'migration src->dst' here can take an arbitrary amount
> >of time. So by the time the spice client switchs to dst,
> >the ticket will likely have already expired.
> 
> Ah, *that* is the bug.
> 
> >The only way to fix this AFAICT is to ensure the SPICE
> >client connects to dst before migration starts, but delays
> >display switch until the end. eg
> >
> >     (1) libvirt sets spice ticket at dst (libvirt stage 2)
> >     (2) spice client connects to dst (start of libvirt stage 3)
> 
> That must be in stage 2.  Once the migration started qemu will not
> accept new connections and thus the client would not be able to
> connect.

NB, I said "start of libvirt stage 3" which has multi-steps:

  3. Perform(src)
      - Issue migrate_client_info
      - Issue migrate_set_speed
      - Issue migrate
      - Loop
          - Issue query-migrate
          - Break if finished/failed

ie, before step 3.3

> Can qemu handle async monitor commands now?  We would need to wait
> until the client has actually connected, but without blocking the
> iothread because it can take a while.

Not sure about that


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|