Re: [PATCH v3 1/1] vhost-user-fs: add migration type property

[Anton Kuchin]

On 01/03/2023 16:46, Michael S. Tsirkin wrote:
> On Wed, Mar 01, 2023 at 05:03:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 01.03.23 00:24, Michael S. Tsirkin wrote:
> > > > Said that checking on destination would need another flag and the safe
> > > > way of using this feature would require managing two flags instead of one
> > > > making it even more fragile. So I'd prefer not to make it more complex.
> > > >
> > > > In my opinion the best way to use this property by orchestrator is to
> > > > leave default unmigratable behavior at start and just before migration when
> > > > destination is known enumerate all vhost-user-fs devices and set properties
> > > > according to their backends capability with QMP like you mentioned. This
> > > > gives us single point of making the decision for each device and avoids
> > > > guessing future at VM start.
> > > this means that you need to remember what the values were and then
> > > any failure on destination requires you to go back and set them
> > > to original values.
> > Why do we need to restore old values?
> To get back to where you were before you were starting migration.
>
> > For me, this new property is a kind of per-device migration
> > capability. Do we care to restore migration capabilities to the values
> > that they had before setting them for failed migration? We don't need
> > it, as we just always set capabilities as we want before each
> > migration. Same thing for this new property: just set it properly
> > before migration and you don't need to care about restoring it after
> > failed migration attempt.
> If you really trust your management then we can just remove the
> migration blocker and be done with it. All this song and dance
> with changing properties is to catch errors. If one has to
> carefully play with QOM to achieve the desired result then
> IMHO we failed in this.

To be honest I would prefer just removing blocker because if orchestrator
doesn't know what it is doing it has lots of different ways to break
things and we can't do anything about it.
Just like vhost-user-scsi always allows migration since the day it was
introduced without additional checks and relies on the orchestrator.
But migration was blocked for vhost-user-fs when it was initially merged
and it is bad to change this contract now.

This property here is not only to block migration by default and catch
errors but really to select migration type. External migration can be
sometimes preferred even after internal is implemented because it requires
less calls to backend to extract internal state, less code to execute in
order to save and restore daemon state.
And this also will allow compatibility with old VMs that support only
external migration to move to internal migration without reboot someday
when it is implemented.

So catching errors in not the only purpose of this property, but it 
definitely
allows us to catch some obvious ones.

> > > With possibility of crashes on the orchestrator
> > > you also need to recall the temporary values in some file ...
> > > This is huge complexity much worse than two flags.
> > >
> > > Assuming we need two let's see whether just reload on source is good
> > > enough.
> > --
> > Best regards,
> > Vladimir