[Qemu-devel] [PATCH] migration: Issue 'cont' only on successful incoming migration

[Qemu-devel] [PATCH] migration: Issue 'cont' only on successful incoming migration

[Amit Shah]

When a 'cont' is issued on a VM that's just waiting for an incoming
migration, the VM reboots and boots into the guest, possibly corrupting
its storage since it could be shared with another VM running elsewhere.

Ensure that a VM started with '-incoming' is only run when an incoming
migration successfully completes.

Reported-by: Laine Stump <laine@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 migration.c |    3 +++
 monitor.c   |    4 ++++
 sysemu.h    |    1 +
 vl.c        |    3 +++
 4 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/migration.c b/migration.c
index 650eb78..84d7e4a 100644
--- a/migration.c
+++ b/migration.c
@@ -67,6 +67,9 @@ void process_incoming_migration(QEMUFile *f)
     qemu_announce_self();
     DPRINTF("successfully loaded vm state\n");
 
+    incoming_expected = false;
+    incoming_done = true;
+
     if (autostart)
         vm_start();
 }
diff --git a/monitor.c b/monitor.c
index 45fd482..d12a7b5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1056,6 +1056,10 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     struct bdrv_iterate_context context = { mon, 0 };
 
+    if (incoming_expected && !incoming_done) {
+        autostart = 1;
+        return 1; /* Waiting for incoming migration */
+    }
     bdrv_iterate(encrypted_bdrv_it, &context);
     /* only resume the vm if all keys are set and valid */
     if (!context.err) {
diff --git a/sysemu.h b/sysemu.h
index 9c988bb..f9c1962 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -99,6 +99,7 @@ typedef enum DisplayType
 } DisplayType;
 
 extern int autostart;
+extern int incoming_expected, incoming_done;
 extern int bios_size;
 
 typedef enum {
diff --git a/vl.c b/vl.c
index ba6ee11..b69c11b 100644
--- a/vl.c
+++ b/vl.c
@@ -182,6 +182,8 @@ int nb_nics;
 NICInfo nd_table[MAX_NICS];
 int vm_running;
 int autostart;
+int incoming_expected; /* Started with -incoming and waiting for incoming */
+int incoming_done; /* Incoming migration successful */
 static int rtc_utc = 1;
 static int rtc_date_offset = -1; /* -1 means no change */
 QEMUClock *rtc_clock;
@@ -2557,6 +2559,7 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_incoming:
                 incoming = optarg;
+                incoming_expected = true;
                 break;
             case QEMU_OPTION_nodefaults:
                 default_serial = 0;
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH] migration: Issue 'cont' only on successful incoming migration

[Luiz Capitulino]

On Fri, 23 Jul 2010 16:15:15 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> When a 'cont' is issued on a VM that's just waiting for an incoming
> migration, the VM reboots and boots into the guest, possibly corrupting
> its storage since it could be shared with another VM running elsewhere.
> 
> Ensure that a VM started with '-incoming' is only run when an incoming
> migration successfully completes.
> 
> Reported-by: Laine Stump <laine@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  migration.c |    3 +++
>  monitor.c   |    4 ++++
>  sysemu.h    |    1 +
>  vl.c        |    3 +++
>  4 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 650eb78..84d7e4a 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -67,6 +67,9 @@ void process_incoming_migration(QEMUFile *f)
>      qemu_announce_self();
>      DPRINTF("successfully loaded vm state\n");
>  
> +    incoming_expected = false;
> +    incoming_done = true;
> +
>      if (autostart)
>          vm_start();
>  }
> diff --git a/monitor.c b/monitor.c
> index 45fd482..d12a7b5 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1056,6 +1056,10 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>      struct bdrv_iterate_context context = { mon, 0 };
>  
> +    if (incoming_expected && !incoming_done) {
> +        autostart = 1;

Why do we need to set autostart? We should just fail if we're unable to run.

> +        return 1; /* Waiting for incoming migration */

You should return -1 and use qerror_report(), so that we have a meaningful
error in the user Monitor and QMP (otherwise we'll get UndefinedError).

And incomding_done is not needed.

> +    }
>      bdrv_iterate(encrypted_bdrv_it, &context);
>      /* only resume the vm if all keys are set and valid */
>      if (!context.err) {
> diff --git a/sysemu.h b/sysemu.h
> index 9c988bb..f9c1962 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -99,6 +99,7 @@ typedef enum DisplayType
>  } DisplayType;
>  
>  extern int autostart;
> +extern int incoming_expected, incoming_done;
>  extern int bios_size;
>  
>  typedef enum {
> diff --git a/vl.c b/vl.c
> index ba6ee11..b69c11b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -182,6 +182,8 @@ int nb_nics;
>  NICInfo nd_table[MAX_NICS];
>  int vm_running;
>  int autostart;
> +int incoming_expected; /* Started with -incoming and waiting for incoming */
> +int incoming_done; /* Incoming migration successful */
>  static int rtc_utc = 1;
>  static int rtc_date_offset = -1; /* -1 means no change */
>  QEMUClock *rtc_clock;
> @@ -2557,6 +2559,7 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_incoming:
>                  incoming = optarg;
> +                incoming_expected = true;
>                  break;
>              case QEMU_OPTION_nodefaults:
>                  default_serial = 0;

[Qemu-devel] Re: [PATCH] migration: Issue 'cont' only on successful incoming migration

[Amit Shah]

On (Fri) Jul 23 2010 [15:08:18], Luiz Capitulino wrote:
> > diff --git a/monitor.c b/monitor.c
> > index 45fd482..d12a7b5 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1056,6 +1056,10 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >  {
> >      struct bdrv_iterate_context context = { mon, 0 };
> >  
> > +    if (incoming_expected && !incoming_done) {
> > +        autostart = 1;
> 
> Why do we need to set autostart? We should just fail if we're unable to run.
> 
> > +        return 1; /* Waiting for incoming migration */
> 
> You should return -1 and use qerror_report(), so that we have a meaningful
> error in the user Monitor and QMP (otherwise we'll get UndefinedError).

That would mean old/existing libvirt will be confused on why guests
wouldn't start even though it issued cont.

If it's not a problem for the libvirt folks, I can do that.

> And incomding_done is not needed.

Yes, not in this version.

		Amit

[Qemu-devel] Re: [PATCH] migration: Issue 'cont' only on successful incoming migration

[Luiz Capitulino]

On Sat, 24 Jul 2010 13:01:24 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) Jul 23 2010 [15:08:18], Luiz Capitulino wrote:
> > > diff --git a/monitor.c b/monitor.c
> > > index 45fd482..d12a7b5 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -1056,6 +1056,10 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > >  {
> > >      struct bdrv_iterate_context context = { mon, 0 };
> > >  
> > > +    if (incoming_expected && !incoming_done) {
> > > +        autostart = 1;
> > 
> > Why do we need to set autostart? We should just fail if we're unable to run.
> > 
> > > +        return 1; /* Waiting for incoming migration */
> > 
> > You should return -1 and use qerror_report(), so that we have a meaningful
> > error in the user Monitor and QMP (otherwise we'll get UndefinedError).
> 
> That would mean old/existing libvirt will be confused on why guests
> wouldn't start even though it issued cont.

Yes, although delaying to start could cause a problem too and this is
also introducing an new error in QMP already.

I really would like to avoid adding weird semantics, specially in QMP where
cont will return an error but will put the VM to run later. We could fix this
there only, but then it will get complex w/o reason.

We should fix it properly right now, IMO.

> If it's not a problem for the libvirt folks, I can do that.

Laine, could you please check that?

> 
> > And incomding_done is not needed.
> 
> Yes, not in this version.
> 
> 		Amit
> 

[Qemu-devel] Re: [PATCH] migration: Issue 'cont' only on successful incoming migration

[Laine Stump]

  On 07/26/2010 10:23 AM, Luiz Capitulino wrote:
> On Sat, 24 Jul 2010 13:01:24 +0530
> Amit Shah<amit.shah@redhat.com>  wrote:
>
>> On (Fri) Jul 23 2010 [15:08:18], Luiz Capitulino wrote:
>>>> diff --git a/monitor.c b/monitor.c
>>>> index 45fd482..d12a7b5 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -1056,6 +1056,10 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>>   {
>>>>       struct bdrv_iterate_context context = { mon, 0 };
>>>>
>>>> +    if (incoming_expected&&  !incoming_done) {
>>>> +        autostart = 1;
>>> Why do we need to set autostart? We should just fail if we're unable to run.
>>>
>>>> +        return 1; /* Waiting for incoming migration */
>>> You should return -1 and use qerror_report(), so that we have a meaningful
>>> error in the user Monitor and QMP (otherwise we'll get UndefinedError).
>> That would mean old/existing libvirt will be confused on why guests
>> wouldn't start even though it issued cont.
> Yes, although delaying to start could cause a problem too and this is
> also introducing an new error in QMP already.
>
> I really would like to avoid adding weird semantics, specially in QMP where
> cont will return an error but will put the VM to run later. We could fix this
> there only, but then it will get complex w/o reason.
>
> We should fix it properly right now, IMO.
>
>> If it's not a problem for the libvirt folks, I can do that.
> Laine, could you please check that?

That should really be answered by someone who better understands the 
implications (I'm a newcomer to that part of the code). Dan Berrange or 
Chris Lalancette maybe?

(I am setting up to test the current version of the patch on the system 
where I can reproduce the problem. Haven't flipped the switch yet, though.)

[Qemu-devel] Re: [PATCH] migration: Issue 'cont' only on successful incoming migration

[Juan Quintela]

Laine Stump <laine@redhat.com> wrote:
>  On 07/26/2010 10:23 AM, Luiz Capitulino wrote:
>> On Sat, 24 Jul 2010 13:01:24 +0530
>> Amit Shah<amit.shah@redhat.com>  wrote:
>>
>>> On (Fri) Jul 23 2010 [15:08:18], Luiz Capitulino wrote:
>>>>> diff --git a/monitor.c b/monitor.c
>>>>> index 45fd482..d12a7b5 100644
>>>>> --- a/monitor.c
>>>>> +++ b/monitor.c
>>>>> @@ -1056,6 +1056,10 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>>>   {
>>>>>       struct bdrv_iterate_context context = { mon, 0 };
>>>>>
>>>>> +    if (incoming_expected&&  !incoming_done) {
>>>>> +        autostart = 1;
>>>> Why do we need to set autostart? We should just fail if we're unable to run.
>>>>
>>>>> +        return 1; /* Waiting for incoming migration */
>>>> You should return -1 and use qerror_report(), so that we have a meaningful
>>>> error in the user Monitor and QMP (otherwise we'll get UndefinedError).
>>> That would mean old/existing libvirt will be confused on why guests
>>> wouldn't start even though it issued cont.
>> Yes, although delaying to start could cause a problem too and this is
>> also introducing an new error in QMP already.
>>
>> I really would like to avoid adding weird semantics, specially in QMP where
>> cont will return an error but will put the VM to run later. We could fix this
>> there only, but then it will get complex w/o reason.
>>
>> We should fix it properly right now, IMO.
>>
>>> If it's not a problem for the libvirt folks, I can do that.
>> Laine, could you please check that?
>
> That should really be answered by someone who better understands the
> implications (I'm a newcomer to that part of the code). Dan Berrange
> or Chris Lalancette maybe?
>
> (I am setting up to test the current version of the patch on the
> system where I can reproduce the problem. Haven't flipped the switch
> yet, though.)

Just to be sure, what do you want "cont" to return if we are in the
middle of a migration?

This patch just sets autostart=1 (i.e. user wants to start guest as soon
as possible), is that ok, or a warning/error is better?

Later, Juan.

[Qemu-devel] Re: [PATCH] migration: Issue 'cont' only on successful incoming migration

[Daniel P. Berrange]

On Mon, Jul 26, 2010 at 09:49:12PM +0200, Juan Quintela wrote:
> Laine Stump <laine@redhat.com> wrote:
> >  On 07/26/2010 10:23 AM, Luiz Capitulino wrote:
> >> On Sat, 24 Jul 2010 13:01:24 +0530
> >> Amit Shah<amit.shah@redhat.com>  wrote:
> >>
> >>> On (Fri) Jul 23 2010 [15:08:18], Luiz Capitulino wrote:
> >>>>> diff --git a/monitor.c b/monitor.c
> >>>>> index 45fd482..d12a7b5 100644
> >>>>> --- a/monitor.c
> >>>>> +++ b/monitor.c
> >>>>> @@ -1056,6 +1056,10 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >>>>>   {
> >>>>>       struct bdrv_iterate_context context = { mon, 0 };
> >>>>>
> >>>>> +    if (incoming_expected&&  !incoming_done) {
> >>>>> +        autostart = 1;
> >>>> Why do we need to set autostart? We should just fail if we're unable to run.
> >>>>
> >>>>> +        return 1; /* Waiting for incoming migration */
> >>>> You should return -1 and use qerror_report(), so that we have a meaningful
> >>>> error in the user Monitor and QMP (otherwise we'll get UndefinedError).
> >>> That would mean old/existing libvirt will be confused on why guests
> >>> wouldn't start even though it issued cont.
> >> Yes, although delaying to start could cause a problem too and this is
> >> also introducing an new error in QMP already.
> >>
> >> I really would like to avoid adding weird semantics, specially in QMP where
> >> cont will return an error but will put the VM to run later. We could fix this
> >> there only, but then it will get complex w/o reason.
> >>
> >> We should fix it properly right now, IMO.
> >>
> >>> If it's not a problem for the libvirt folks, I can do that.
> >> Laine, could you please check that?
> >
> > That should really be answered by someone who better understands the
> > implications (I'm a newcomer to that part of the code). Dan Berrange
> > or Chris Lalancette maybe?
> >
> > (I am setting up to test the current version of the patch on the
> > system where I can reproduce the problem. Haven't flipped the switch
> > yet, though.)
> 
> Just to be sure, what do you want "cont" to return if we are in the
> middle of a migration?

NB, this is only making a difference if 'cont' is run between the time
QEMU starts with -incoming and the time migration starts.

Once migration starts, the entire QEMU monitor is blocked until it 
completes, so you'll never see 'cont' in the middle of migration.
libvirt currently relies on that behaviour but as per previous
discussions on the subject, we'd prefer to have some kind of async
notification whether via async events, or via an async QMP command.

> This patch just sets autostart=1 (i.e. user wants to start guest as soon
> as possible), is that ok, or a warning/error is better?

I agree with Luiz that having such 'magical' behaviour for commands is not
very desirable. If 'cont' isn't valid based on the current QEMU execution
state, then it should return an error. The downside is that while migration
will still complete successfully, the current libvirt will unfortunately
report an error in this scenario. If there is an explicit QMP error code
associated with this condition though, we can catch the error and handle
it appropriately in future.

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