[PATCH]: Add a "migrate_incoming" monitor option

[PATCH]: Add a "migrate_incoming" monitor option

[Chris Lalancette]

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

We've been trying to plumb libvirt to do KVM migration.  One of the stumbling
blocks we are running into, however, is that libvirt expects to be able to use
the Qemu monitor both before and after migration has taken place, on both the
source and destination nodes.  After migration has taken place is no problem; we
return to the main qemu select() loop, and we can run monitor commands.
However, before migration, on the destination side, when we start qemu with a
command-line like:

qemu-kvm -M pc -S blah blah -incoming tcp://0:4444

we can't run any monitor commands since the migration code is synchronously
waiting for an incoming tcp connection.  To get around this, the following patch
adds a new monitor command called "migrate_incoming"; it takes all of the same
parameters as the command-line option, but just starts it later.  To make sure
it is safe, you actually have to start with "-incoming monitor"; if you run it
without that, it will just spit an error at you.  So with this in place, libvirt
can do the equivalent of:

qemu-kvm -M pc -S blah blah -incoming monitor
(qemu) info cpus
...other commands
(qemu) migrate_incoming tcp://0:4444
...wait for migration to start, and then complete
(qemu) info block
...etc.

Signed-off-by: Chris Lalancette <clalance@redhat.com>

[-- Attachment #2: qemu-monitor-migrate.patch --]
[-- Type: text/x-patch, Size: 2128 bytes --]

diff --git a/qemu/monitor.c b/qemu/monitor.c
index 20dcca6..c11b82c 100644
--- a/qemu/monitor.c
+++ b/qemu/monitor.c
@@ -504,6 +504,25 @@ static void do_cont(void)
     vm_start();
 }
 
+static void do_migrate_incoming(const char *incom)
+{
+    extern int incoming_monitor;
+
+    if (!incoming_monitor) {
+        term_printf("FAIL: Can only use the migrate-incoming command with -incoming monitor\n");
+    }
+    else {
+        int rc;
+
+        rc = migrate_incoming(incom);
+	if (rc != 0) {
+            fprintf(stderr, "Migration failed rc=%d\n", rc);
+            exit(rc);
+	}
+        vm_start();
+    }
+}
+
 #ifdef CONFIG_GDBSTUB
 static void do_gdbserver(const char *port)
 {
@@ -1486,6 +1505,7 @@ static term_cmd_t term_cmds[] = {
       "", "cancel the current VM migration" },
     { "migrate_set_speed", "s", do_migrate_set_speed,
       "value", "set maximum speed (in bytes) for migrations" },
+    { "migrate_incoming", "s", do_migrate_incoming, "incom", "incoming string" },
     { "cpu_set", "is", do_cpu_set_nr, "cpu [online|offline]", "change cpu state" },
 #if defined(TARGET_I386) || defined(TARGET_X86_64)
     { "drive_add", "iss", drive_hot_add, "pcibus pcidevfn [file=file][,if=type][,bus=n]\n"
diff --git a/qemu/vl.c b/qemu/vl.c
index e1762ee..9b5f113 100644
--- a/qemu/vl.c
+++ b/qemu/vl.c
@@ -229,6 +229,7 @@ int cursor_hide = 1;
 int graphic_rotate = 0;
 int daemonize = 0;
 const char *incoming;
+int incoming_monitor = 0;
 const char *option_rom[MAX_OPTION_ROMS];
 int nb_option_roms;
 int semihosting_enabled = 0;
@@ -9673,11 +9675,16 @@ int main(int argc, char **argv)
     if (incoming) {
         int rc;
 
-        rc = migrate_incoming(incoming);
-        if (rc != 0) {
-            fprintf(stderr, "Migration failed rc=%d\n", rc);
-            exit(rc);
-	}
+        if (strncmp(incoming, "monitor", 7) == 0) {
+            incoming_monitor = 1;
+        }
+        else {
+            rc = migrate_incoming(incoming);
+            if (rc != 0) {
+                fprintf(stderr, "Migration failed rc=%d\n", rc);
+                exit(rc);
+            }
+        }
     }
 
     {

Re: [PATCH]: Add a "migrate_incoming" monitor option

[Daniel P. Berrange]

On Thu, Jul 31, 2008 at 04:50:51PM +0200, Chris Lalancette wrote:
> We've been trying to plumb libvirt to do KVM migration.  One of the stumbling
> blocks we are running into, however, is that libvirt expects to be able to use
> the Qemu monitor both before and after migration has taken place, on both the
> source and destination nodes.  After migration has taken place is no problem; we
> return to the main qemu select() loop, and we can run monitor commands.
> However, before migration, on the destination side, when we start qemu with a
> command-line like:
> 
> qemu-kvm -M pc -S blah blah -incoming tcp://0:4444
> 
> we can't run any monitor commands since the migration code is synchronously
> waiting for an incoming tcp connection.  To get around this, the following patch
> adds a new monitor command called "migrate_incoming"; it takes all of the same
> parameters as the command-line option, but just starts it later.  To make sure
> it is safe, you actually have to start with "-incoming monitor"; if you run it
> without that, it will just spit an error at you.  So with this in place, libvirt
> can do the equivalent of:
> 
> qemu-kvm -M pc -S blah blah -incoming monitor
> (qemu) info cpus
> ...other commands
> (qemu) migrate_incoming tcp://0:4444
> ...wait for migration to start, and then complete
> (qemu) info block
> ...etc.
> 
> Signed-off-by: Chris Lalancette <clalance@redhat.com>

> diff --git a/qemu/monitor.c b/qemu/monitor.c
> index 20dcca6..c11b82c 100644
> --- a/qemu/monitor.c
> +++ b/qemu/monitor.c
> @@ -504,6 +504,25 @@ static void do_cont(void)
>      vm_start();
>  }
>  
> +static void do_migrate_incoming(const char *incom)
> +{
> +    extern int incoming_monitor;
> +
> +    if (!incoming_monitor) {
> +        term_printf("FAIL: Can only use the migrate-incoming command with -incoming monitor\n");
> +    }
> +    else {
> +        int rc;
> +
> +        rc = migrate_incoming(incom);

You probably want to set  'incoming_monitor = 0' here, to protect against
someone accidentially running it multiple times.

> +	if (rc != 0) {
> +            fprintf(stderr, "Migration failed rc=%d\n", rc);
> +            exit(rc);
> +	}

And some whitespace issues here..

> +        vm_start();
> +    }
> +}
> +
>  #ifdef CONFIG_GDBSTUB
>  static void do_gdbserver(const char *port)
>  {
> @@ -1486,6 +1505,7 @@ static term_cmd_t term_cmds[] = {
>        "", "cancel the current VM migration" },
>      { "migrate_set_speed", "s", do_migrate_set_speed,
>        "value", "set maximum speed (in bytes) for migrations" },
> +    { "migrate_incoming", "s", do_migrate_incoming, "incom", "incoming string" },
>      { "cpu_set", "is", do_cpu_set_nr, "cpu [online|offline]", "change cpu state" },
>  #if defined(TARGET_I386) || defined(TARGET_X86_64)
>      { "drive_add", "iss", drive_hot_add, "pcibus pcidevfn [file=file][,if=type][,bus=n]\n"
> diff --git a/qemu/vl.c b/qemu/vl.c
> index e1762ee..9b5f113 100644
> --- a/qemu/vl.c
> +++ b/qemu/vl.c
> @@ -229,6 +229,7 @@ int cursor_hide = 1;
>  int graphic_rotate = 0;
>  int daemonize = 0;
>  const char *incoming;
> +int incoming_monitor = 0;
>  const char *option_rom[MAX_OPTION_ROMS];
>  int nb_option_roms;
>  int semihosting_enabled = 0;
> @@ -9673,11 +9675,16 @@ int main(int argc, char **argv)
>      if (incoming) {
>          int rc;
>  
> -        rc = migrate_incoming(incoming);
> -        if (rc != 0) {
> -            fprintf(stderr, "Migration failed rc=%d\n", rc);
> -            exit(rc);
> -	}
> +        if (strncmp(incoming, "monitor", 7) == 0) {
> +            incoming_monitor = 1;
> +        }
> +        else {
> +            rc = migrate_incoming(incoming);
> +            if (rc != 0) {
> +                fprintf(stderr, "Migration failed rc=%d\n", rc);
> +                exit(rc);
> +            }
> +        }

Rather than putting the strncmp("monitor")  into vl.c, I'd just leave
this part as is.  Put the logic into the 'migrate_incoming()' method
so that it just sets the 'incoming_monitor' flag and then returns
immediately. That would allwo the 'incoming_Monitor' flag to be declared
static to the migrate.c file, instead of polluting vl.c

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [PATCH]: Add a "migrate_incoming" monitor option

[Anthony Liguori]

Chris Lalancette wrote:
> We've been trying to plumb libvirt to do KVM migration.  One of the stumbling
> blocks we are running into, however, is that libvirt expects to be able to use
> the Qemu monitor both before and after migration has taken place, on both the
> source and destination nodes.  After migration has taken place is no problem; we
> return to the main qemu select() loop, and we can run monitor commands.
> However, before migration, on the destination side, when we start qemu with a
> command-line like:
>
> qemu-kvm -M pc -S blah blah -incoming tcp://0:4444
>
> we can't run any monitor commands since the migration code is synchronously
> waiting for an incoming tcp connection.  To get around this, the following patch
> adds a new monitor command called "migrate_incoming"; it takes all of the same
> parameters as the command-line option, but just starts it later.  To make sure
> it is safe, you actually have to start with "-incoming monitor"; if you run it
> without that, it will just spit an error at you.  So with this in place, libvirt
> can do the equivalent of:
>
> qemu-kvm -M pc -S blah blah -incoming monitor
>   

I think adding a 'nowait' parameter to migration would make more sense 
than introducing a monitor command.

So:

qemu-kvm -M pc -S blah blah -incoming tcp://0:4444,nowait

 From an implementation perspective, it's just a matter of setting a 
callback for the accept fd I imagine.

Regards,

Anthony Liguori


> (qemu) info cpus
> ...other commands
> (qemu) migrate_incoming tcp://0:4444
> ...wait for migration to start, and then complete
> (qemu) info block
> ...etc.
>
> Signed-off-by: Chris Lalancette <clalance@redhat.com>
>   

Re: [PATCH]: Add a "migrate_incoming" monitor option

[Daniel P. Berrange]

On Thu, Jul 31, 2008 at 12:27:43PM -0500, Anthony Liguori wrote:
> Chris Lalancette wrote:
> >We've been trying to plumb libvirt to do KVM migration.  One of the 
> >stumbling
> >blocks we are running into, however, is that libvirt expects to be able to 
> >use
> >the Qemu monitor both before and after migration has taken place, on both 
> >the
> >source and destination nodes.  After migration has taken place is no 
> >problem; we
> >return to the main qemu select() loop, and we can run monitor commands.
> >However, before migration, on the destination side, when we start qemu 
> >with a
> >command-line like:
> >
> >qemu-kvm -M pc -S blah blah -incoming tcp://0:4444
> >
> >we can't run any monitor commands since the migration code is synchronously
> >waiting for an incoming tcp connection.  To get around this, the following 
> >patch
> >adds a new monitor command called "migrate_incoming"; it takes all of the 
> >same
> >parameters as the command-line option, but just starts it later.  To make 
> >sure
> >it is safe, you actually have to start with "-incoming monitor"; if you 
> >run it
> >without that, it will just spit an error at you.  So with this in place, 
> >libvirt
> >can do the equivalent of:
> >
> >qemu-kvm -M pc -S blah blah -incoming monitor
> >  
> 
> I think adding a 'nowait' parameter to migration would make more sense 
> than introducing a monitor command.
> 
> So:
> 
> qemu-kvm -M pc -S blah blah -incoming tcp://0:4444,nowait
> 
> From an implementation perspective, it's just a matter of setting a 
> callback for the accept fd I imagine.

An accept trick only handles the TCP case though. I know this was Chris'
example that we're currently using, but we intend to switch to passing 
an open file descriptor instead, and proxying the data via a secure 
channel instead of the plain tcp, or builtin SSH tunnelling. 

With an open FD we'll need another way - I guess just registering a
event on POLLIN might do the trick. It seems to me that just having
an explicit migrate incoming monitor command would be simpler than having
to code different triggers to implement 'nowait' for each transport
we have.
 
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [PATCH]: Add a "migrate_incoming" monitor option

[Chris Lalancette]

Daniel P. Berrange wrote:
> An accept trick only handles the TCP case though. I know this was Chris'
> example that we're currently using, but we intend to switch to passing 
> an open file descriptor instead, and proxying the data via a secure 
> channel instead of the plain tcp, or builtin SSH tunnelling. 
> 
> With an open FD we'll need another way - I guess just registering a
> event on POLLIN might do the trick. It seems to me that just having
> an explicit migrate incoming monitor command would be simpler than having
> to code different triggers to implement 'nowait' for each transport
> we have.

I just finished a patch to do the "nowait" for tcp as suggested by Anthony, and
it was actually far less code then I feared.  It just needed a little
re-factoring of the migrate_incoming_tcp() function.  I'll be posting that in a
couple of minutes.  Given how this code worked out, doing a "nowait" for the fd
style is really no big deal.

Both ways (explicit monitor command and "nowait") seem to have their benefits;
the monitor command has the benefit of being more flexible, while the nowait has
the benefit of being similar to already existing Qemu commands.  My preference
is for the monitor command since it seems a little more natural for a management
tool, but the nowait is clearly an option as well.

I'll also post a cleanup patch with Dan's suggestion for the monitor patch, so
both implementations will be available.

Chris Lalancette

Re: [PATCH]: Add a "migrate_incoming" monitor option

[Chris Lalancette]

Daniel P. Berrange wrote:
>> @@ -9673,11 +9675,16 @@ int main(int argc, char **argv)
>>      if (incoming) {
>>          int rc;
>>  
>> -        rc = migrate_incoming(incoming);
>> -        if (rc != 0) {
>> -            fprintf(stderr, "Migration failed rc=%d\n", rc);
>> -            exit(rc);
>> -	}
>> +        if (strncmp(incoming, "monitor", 7) == 0) {
>> +            incoming_monitor = 1;
>> +        }
>> +        else {
>> +            rc = migrate_incoming(incoming);
>> +            if (rc != 0) {
>> +                fprintf(stderr, "Migration failed rc=%d\n", rc);
>> +                exit(rc);
>> +            }
>> +        }
> 
> Rather than putting the strncmp("monitor")  into vl.c, I'd just leave
> this part as is.  Put the logic into the 'migrate_incoming()' method
> so that it just sets the 'incoming_monitor' flag and then returns
> immediately. That would allwo the 'incoming_Monitor' flag to be declared
> static to the migrate.c file, instead of polluting vl.c

Actually, that won't quite work.  We still need to share the incoming_monitor
flag between migration.c and monitor.c.  However, your suggestion is better in
that this is a "migration-specific" flag, so I'll move it over like you suggest.

Chris Lalancette