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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Feb 22, 2023 at 03:21:42PM -0500, Michael S. Tsirkin wrote:
> On Wed, Feb 22, 2023 at 08:25:19PM +0200, Anton Kuchin wrote:
> > On 22/02/2023 19:12, Michael S. Tsirkin wrote:
> > > On Wed, Feb 22, 2023 at 07:05:47PM +0200, Anton Kuchin wrote:
> > > > On 22/02/2023 18:51, Michael S. Tsirkin wrote:
> > > > > On Wed, Feb 22, 2023 at 06:49:10PM +0200, Anton Kuchin wrote:
> > > > > > On 22/02/2023 17:14, Vladimir Sementsov-Ogievskiy wrote:
> > > > > > > On 22.02.23 17:25, Anton Kuchin wrote:
> > > > > > > > > > > +static int vhost_user_fs_pre_save(void *opaque)
> > > > > > > > > > > +{
> > > > > > > > > > > +    VHostUserFS *fs = opaque;
> > > > > > > > > > > +    g_autofree char *path = object_get_canonical_path(OBJECT(fs));
> > > > > > > > > > > +
> > > > > > > > > > > +    switch (fs->migration_type) {
> > > > > > > > > > > +    case VHOST_USER_MIGRATION_TYPE_NONE:
> > > > > > > > > > > +        error_report("Migration is blocked by device %s", path);
> > > > > > > > > > > +        break;
> > > > > > > > > > > +    case VHOST_USER_MIGRATION_TYPE_EXTERNAL:
> > > > > > > > > > > +        return 0;
> > > > > > > > > > > +    default:
> > > > > > > > > > > +        error_report("Migration type '%s' is not
> > > > > > > > > > > supported by device %s",
> > > > > > > > > > > + VhostUserMigrationType_str(fs->migration_type), path);
> > > > > > > > > > > +        break;
> > > > > > > > > > > +    }
> > > > > > > > > > > +
> > > > > > > > > > > +    return -1;
> > > > > > > > > > > +}
> > > > > > > > > > Should we also add this as .pre_load, to force user select
> > > > > > > > > > correct migration_type on target too?
> > > > > > > > > In fact, I would claim we only want pre_load.
> > > > > > > > > When qemu is started on destination we know where it's migrated
> > > > > > > > > from so this flag can be set.
> > > > > > > > > When qemu is started on source we generally do not yet know so
> > > > > > > > > we don't know whether it's safe to set this flag.
> > > > > > > But destination is a "source" for next migration, so there shouldn't be
> > > > > > > real difference.
> > > > > > > The new property has ".realized_set_allowed = true", so, as I understand
> > > > > > > it may be changed at any time, so that's not a problem.
> > > > > > Yes, exactly. So destination's property sets not how it will handle this
> > > > > > incoming
> > > > > > migration but the future outgoing one.
> > > > > How do you know where you are going to migrate though?
> > > > > I think you don't.
> > > > > Setting it on source is better since we know where we
> > > > > are migrating from.
> > > > Yes, I don't know where I'm going to migrate to. This is why property
> > > > affects only how source saves state on outgoing migration.
> > > Um. I don't get the logic.
> > 
> > For this feature to work we need orchestrator to manage the migration. And
> > we
> > generally assume that it is responsibility of orchestrator to ensure
> > matching
> > properties on source and destination.
> > As orchestrator manages both sides of migration it can set option (and we
> > can
> > check it) on either source or destination. Now it's not important which side
> > we
> > select, because now the option is essentially binary allow/deny (but IMHO it
> > is much better to refuse source to migrate than find later that state can't
> > be
> > loaded by destination, in case of file migration this becomes especially
> > painful).
> > 
> > But there are plans to add internal migration option (extract FUSE state
> > from
> > backend and transfer it in QEMU migration stream), and that's where
> > setting/checking
> > on source becomes important because it will rely on this property to decide
> > if
> > extra state form backend needs to be put in the migration stream subsection.
> 
> 
> If we do internal migration that will be a different property
> which has to match on source *and* destination.
> 
> 
> > If you are concerned about orchestrator breaking assumption of matching
> > properties
> > on source and destination this is not really supported AFAIK but I don't
> > think we
> > need to punish it for this, maybe it has its reasons: I can imagine scenario
> > where orchestrator could want to migrate from source with
> > 'migration=external'
> > to destination with 'migration=none' to ensure that destination can't be
> > migrated further.
> 
> No. I am concerned about a simple practical matter:
> - I decide to restart qemu on the same host - so I need to enable
>   migration
> - Later I decide to migrate qemu to another host - this should be
>   blocked
> 
> 
> Property on source does not satisfy both at the same time.
> Property on destination does.


Stefan what's your take on this? Should we move this from
save to load hook?

> 
> 
> > > 
> > > 
> > > > > > > > This property selects if VM can migrate and if it can what should
> > > > > > > > qemu put
> > > > > > > > to the migration stream. So we select on source what type of
> > > > > > > > migration is
> > > > > > > > allowed for this VM, destination can't check anything at load time.
> > > > > > > OK, so the new field "migration" regulates only outgoing migration and
> > > > > > > do nothing for incoming. On incoming migration the migration stream
> > > > > > > itself defines the type of device migration.
> > > > > > > Worth mentioning in doc?
> > > > > > Good point. I don't think this deserves a respin but if I have to send v4
> > > > > > I'll include
> > > > > > clarification in it.