Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Apr 01, 2019 at 10:27:49AM +0100, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>> > This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
>> > This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
>> > 
>> > Command line option --only-migratable is for disallowing any
>> > configuration that can block migration.
>> > 
>> > Initially, --only-migratable set global variable @only_migratable.
>> > 
>> > Commit 3df663e575 "migration: move only_migratable to MigrationState"
>> > replaced it by MigrationState member @only_migratable.  That was a
>> > mistake.
>> > 
>> > First, it doesn't make sense on the design level.  MigrationState
>> > captures the state of an individual migration, but --only-migratable
>> > isn't a property of an individual migration, it's a restriction on
>> > QEMU configuration.  With fault tolerance, we could have several
>> > migrations at once.  --only-migratable would certainly protect all of
>> > them.  Storing it in MigrationState feels inappropriate.
>> > 
>> > Second, it contributes to a dependency cycle that manifests itself as
>> > a bug now.
>> > 
>> > Putting @only_migratable into MigrationState means its available only
>> > after migration_object_init().
>> > 
>> > We can't set it before migration_object_init(), so we delay setting it
>> > with a global property (this is fixup commit b605c47b57 "migration:
>> > fix handling for --only-migratable").
>> > 
>> > We can't get it before migration_object_init(), so anything that uses
>> > it can only run afterwards.
>> > 
>> > Since migrate_add_blocker() needs to obey --only-migratable, any code
>> > adding migration blockers can run only afterwards.  This contributes
>> > to the following dependency cycle:
>> > 
>> > * configure_blockdev() must run before machine_set_property()
>> >   so machine properties can refer to block backends
>> > 
>> > * machine_set_property() before configure_accelerator()
>> >   so machine properties like kvm-irqchip get applied
>> > 
>> > * configure_accelerator() before migration_object_init()
>> >   so that Xen's accelerator compat properties get applied.
>> > 
>> > * migration_object_init() before configure_blockdev()
>> >   so configure_blockdev() can add migration blockers
>> > 
>> > The cycle was closed when recent commit cda4aa9a5a0 "Create block
>> > backends before setting machine properties" added the first
>> > dependency, and satisfied it by violating the last one.  Broke block
>> > backends that add migration blockers.
>> > 
>> > Moving @only_migratable into MigrationState was a mistake.  Revert it.
>> > 
>> > This doesn't quite break the "migration_object_init() before
>> > configure_blockdev() dependency, since migrate_add_blocker() still has
>> > another dependency on migration_object_init().  To be addressed the
>> > next commit
>> > 
>> > Conflicts:
>> > 	include/migration/misc.h
>> > 	migration/migration.c
>> > 	migration/migration.h
>> > 	vl.c
>> > 
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  include/sysemu/sysemu.h | 1 +
>> >  migration/migration.c   | 5 ++---
>> >  migration/migration.h   | 3 ---
>> >  migration/savevm.c      | 2 +-
>> >  vl.c                    | 9 ++-------
>> >  5 files changed, 6 insertions(+), 14 deletions(-)
>> > 
>> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> > index 6065d9e420..5f133cae83 100644
>> > --- a/include/sysemu/sysemu.h
>> > +++ b/include/sysemu/sysemu.h
>> > @@ -14,6 +14,7 @@
>> >  /* vl.c */
>> >  
>> >  extern const char *bios_name;
>> > +extern int only_migratable;
>> >  extern const char *qemu_name;
>> >  extern QemuUUID qemu_uuid;
>> >  extern bool qemu_uuid_set;
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index 69f75124c9..f6076e5295 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -1707,7 +1707,7 @@ static GSList *migration_blockers;
>> >  
>> >  int migrate_add_blocker(Error *reason, Error **errp)
>> >  {
>> > -    if (migrate_get_current()->only_migratable) {
>> > +    if (only_migratable) {
>> >          error_propagate_prepend(errp, error_copy(reason),
>> >                                  "disallowing migration blocker "
>> >                                  "(--only_migratable) for: ");
>> > @@ -3337,7 +3337,7 @@ void migration_global_dump(Monitor *mon)
>> >      monitor_printf(mon, "store-global-state: %s\n",
>> >                     ms->store_global_state ? "on" : "off");
>> >      monitor_printf(mon, "only-migratable: %s\n",
>> > -                   ms->only_migratable ? "on" : "off");
>> > +                   only_migratable ? "on" : "off");
>> >      monitor_printf(mon, "send-configuration: %s\n",
>> >                     ms->send_configuration ? "on" : "off");
>> >      monitor_printf(mon, "send-section-footer: %s\n",
>> > @@ -3352,7 +3352,6 @@ void migration_global_dump(Monitor *mon)
>> >  static Property migration_properties[] = {
>> >      DEFINE_PROP_BOOL("store-global-state", MigrationState,
>> >                       store_global_state, true),
>> > -    DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false),
>> 
>> So I'm generally OK at this patch, but I'm worried whether this
>> is API?

Uh, forgot to discuss this in my commit message, sorry!

> IIUC, old code can use either
>
>   -global migration.only-migratable=true
>   -only-migratable
>
> After this patch only
>
>   -only-migratable
>
> is valid. So from that POV it is an API break. 
>
> From libvirt's POV this is a non-issue as we never use either of these options.
> We did have a BZ filed to support it, but never did.
>
> To avoid the API break I guess there would been to be a special case early
> loop iterating over argv[] looking for '-global migration.only-migratable=true',
> making that set the static 'bool only_migratable', instead of letting it get
> handled by the object infra.

Is this is worth the trouble?

-global migration.only-migratable=on is entirely undocumented.  The
property appears in -device migration,help and device-list-properties,
but that's all.

Why would any sane person risk using arcane & undocumented -global
migration.only-migratable=on instead of the obvious & documented
-only-migratable?

[...]