On Thu, Apr 18, 2024 at 12:43:42PM -0400, Peter Xu wrote:
> On Thu, Apr 18, 2024 at 06:47:31PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 18.04.24 18:43, Daniel P. Berrangé wrote:
> > > On Thu, Apr 18, 2024 at 06:40:38PM +0300, Vladimir Sementsov-Ogievskiy
> > > wrote:
> > > > On 18.04.24 17:37, Daniel P. Berrangé wrote:
> > > > > On Thu, Apr 18, 2024 at 01:13:29AM +0300, Vladimir
> > > > > Sementsov-Ogievskiy wrote:
> > > > > > We do set MIGRATION_FAILED state, but don't give a chance to
> > > > > > orchestrator to query migration state and get the error.
> > > > > >
> > > > > > Let's report an error through QAPI like we do on outgoing migration.
> > > > > >
> > > > > > migration-test is updated correspondingly.
> > > > > >
> > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > > > > > <vsement...@yandex-team.ru>
> > > > > > ---
> > > > > >
> > > > > > Doubt: is exiting on failure a contract? Will this commit break
> > > > > > something in Libvirt? Finally, could we just change the logic, or I
> > > > > > need
> > > > > > and additional migration-parameter for new behavior?
> > > > >
> > > > > There's a decent risk that this could break apps, whether
> > > > > libvirt or something else, especially if the app is just
> > > > > launching QEMU with '-incoming URI', rather than using
> > > > > '-incoming defer' and then explicitly using QMP to start the
> > > > > incoming migration.
> > > > >
> > > > > I'd say that with '-incoming defer' we should *not* exit on
> > > > > migration error, because that arg implies the app explicitly
> > > > > wants to be using QMP to control migration.
> > > > >
> > > > > With the legacy '-incoming URI' it is probably best to keep
> > > > > exit on error, as that's comparatively more likely to be used
> > > > > in adhoc scenarios where the app/user is ignoring QMP on the
> > > > > dst side.
> > > > >
> > > > > None the less, I think we need to check how libvirt behaves
> > > > > with this patch to be sure of no surprises.
> > > > >
> > > >
> > > > Sounds reasonable, thanks! I'll rework it to behave the new
> > > > way only with "-incoming defer", and check how libvirt behave with it.
> > >
> > > If there are problems and/or we want to be super safe wrt
> > > backcompat, we could add a new '-incoming managed' as
> > > being equivalent to '-incoming defer' but without the
> > > implicit exit.
> > >
> >
> > Probably, that's the best variant. As I can check libvirt in some case, but
> > not at all cases. And libvirt is not the only vm manager finally.
> > And we can in the same time deprecate "-incoming defer" in favor of new
> > behavior.
>
> Or just make it a new migration parameter? Then we keep all existing
> interfaces untouched, no risk of breaking anyone, and then it'll also apply
> to anyone who uses things like -incoming tcp but still wants to keep the
> qemu instance alive?
True, or even more simply, an argument to the 'migrate-incoming' command
diff --git a/qapi/migration.json b/qapi/migration.json
index 8c65b90328..6882aef328 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1889,7 +1889,8 @@
##
{ 'command': 'migrate-incoming',
'data': {'*uri': 'str',
- '*channels': [ 'MigrationChannel' ] } }
+ '*channels': [ 'MigrationChannel' ],
+ '*exit-on-error': 'bool' } }
##
# @xen-save-devices-state:
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
