* Daniel P. Berrangé (berra...@redhat.com) wrote:
> On Mon, Apr 01, 2019 at 01:31:47PM +0200, Markus Armbruster wrote:
> > Daniel P. Berrangé <berra...@redhat.com> writes:
> >
> > > On Mon, Apr 01, 2019 at 10:27:49AM +0100, Dr. David Alan Gilbert wrote:
> > >> * Markus Armbruster (arm...@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 <arm...@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?
>
> Probably not.
>
> > -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?
>
> Given the combination of no known users, current broken impl, and its
> existance not documented, I think there's a reasonable case for just
> removing it.
Yes, agreed - it should be noted though.
Dave
> 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 :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
