[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 <address@hidden> wrote:
> >  On 07/26/2010 10:23 AM, Luiz Capitulino wrote:
> >> On Sat, 24 Jul 2010 13:01:24 +0530
> >> Amit Shah<address@hidden>  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 :|