Re: [PATCH v3 32/51] tests/qtest/migration: Adapt convergence routines to config

[Peter Xu <peterx@redhat.com>]

On Thu, Dec 18, 2025 at 08:28:28PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, Dec 18, 2025 at 04:47:47PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Mon, Dec 15, 2025 at 07:00:18PM -0300, Fabiano Rosas wrote:
> >> >> Adapt the convergence routines migrate_ensure_[non_]converge to set
> >> >> the convergence parameters in the config dict it instead of using
> >> >> migrate-set-parameters.
> >> >> 
> >> >> Some tests need to change the convergence parameters during the
> >> >> migration. The config object method is specific to configuration prior
> >> >> to starting a migration, so by design it's not suitable to effect
> >> >> migration-runtime changes. The existing routines will be kept for this
> >> >> purpose (renamed with 'ongoing' for clarity).
> >> >> 
> >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> >> ---
> >> >>  tests/qtest/migration/framework.c     | 10 ++++-----
> >> >>  tests/qtest/migration/migration-qmp.c | 32 +++++++++++++++++++++++++--
> >> >>  tests/qtest/migration/migration-qmp.h |  6 +++--
> >> >>  tests/qtest/migration/misc-tests.c    |  4 ++--
> >> >>  tests/qtest/migration/precopy-tests.c | 26 +++++++++-------------
> >> >>  5 files changed, 52 insertions(+), 26 deletions(-)
> >> >> 
> >> >> diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
> >> >> index fd15bd832e..df42a8a2c6 100644
> >> >> --- a/tests/qtest/migration/framework.c
> >> >> +++ b/tests/qtest/migration/framework.c
> >> >> @@ -583,7 +583,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
> >> >>          args->postcopy_data = args->start_hook(from, to);
> >> >>      }
> >> >>  
> >> >> -    migrate_ensure_non_converge(from);
> >> >> +    migrate_ensure_non_converge(from, args->start.config);
> >> >>      migrate_prepare_for_dirty_mem(from);
> >> >>      qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
> >> >>                               "  'arguments': { "
> >> >> @@ -872,7 +872,7 @@ int test_precopy_common(MigrateCommon *args)
> >> >>      }
> >> >>  
> >> >>      if (args->live) {
> >> >> -        migrate_ensure_non_converge(from);
> >> >> +        migrate_ensure_non_converge(from, args->start.config);
> >> >>          migrate_prepare_for_dirty_mem(from);
> >> >>      } else {
> >> >>          /*
> >> >> @@ -884,7 +884,7 @@ int test_precopy_common(MigrateCommon *args)
> >> >>          if (args->result == MIG_TEST_SUCCEED) {
> >> >>              qtest_qmp_assert_success(from, "{ 'execute' : 'stop'}");
> >> >>              wait_for_stop(from, &src_state);
> >> >> -            migrate_ensure_converge(from);
> >> >> +            migrate_ongoing_ensure_converge(from);
> >> >>          }
> >> >>      }
> >> >>  
> >> >> @@ -942,7 +942,7 @@ int test_precopy_common(MigrateCommon *args)
> >> >>              }
> >> >>              migrate_wait_for_dirty_mem(from, to);
> >> >>  
> >> >> -            migrate_ensure_converge(from);
> >> >> +            migrate_ongoing_ensure_converge(from);
> >> >>  
> >> >>              /*
> >> >>               * We do this first, as it has a timeout to stop us
> >> >> @@ -1047,7 +1047,7 @@ void test_file_common(MigrateCommon *args, bool stop_src)
> >> >>          data_hook = args->start_hook(from, to);
> >> >>      }
> >> >>  
> >> >> -    migrate_ensure_converge(from);
> >> >> +    migrate_ensure_converge(from, args->start.config);
> >> >>      wait_for_serial("src_serial");
> >> >>  
> >> >>      if (stop_src) {
> >> >> diff --git a/tests/qtest/migration/migration-qmp.c b/tests/qtest/migration/migration-qmp.c
> >> >> index 5c46ceb3e6..7fe47a5793 100644
> >> >> --- a/tests/qtest/migration/migration-qmp.c
> >> >> +++ b/tests/qtest/migration/migration-qmp.c
> >> >> @@ -499,20 +499,48 @@ void migrate_set_parameter_bool(QTestState *who, const char *parameter,
> >> >>      migrate_check_parameter_bool(who, parameter, value);
> >> >>  }
> >> >>  
> >> >> -void migrate_ensure_non_converge(QTestState *who)
> >> >> +void migrate_ongoing_ensure_non_converge(QTestState *who)
> >> >>  {
> >> >>      /* Can't converge with 1ms downtime + 3 mbs bandwidth limit */
> >> >>      migrate_set_parameter_int(who, "max-bandwidth", 3 * 1000 * 1000);
> >> >>      migrate_set_parameter_int(who, "downtime-limit", 1);
> >> >>  }
> >> >>  
> >> >> -void migrate_ensure_converge(QTestState *who)
> >> >> +void migrate_ongoing_ensure_converge(QTestState *who)
> >> >>  {
> >> >>      /* Should converge with 30s downtime + 1 gbs bandwidth limit */
> >> >>      migrate_set_parameter_int(who, "max-bandwidth", 1 * 1000 * 1000 * 1000);
> >> >>      migrate_set_parameter_int(who, "downtime-limit", 30 * 1000);
> >> >>  }
> >> >>  
> >> >> +void migrate_ensure_non_converge(QTestState *who, QDict *config)
> >> >> +{
> >> >> +    config = config_load(config);
> >> >> +    if (config) {
> >> >> +        /* Can't converge with 1ms downtime + 3 mbs bandwidth limit */
> >> >> +        qdict_put_int(config, "max-bandwidth", 3 * 1000 * 1000);
> >> >> +        qdict_put_int(config, "downtime-limit", 1);
> >> >> +    } else {
> >> >> +        assert(who);
> >> >> +        migrate_ongoing_ensure_non_converge(who);
> >> >> +    }
> >> >> +    config_put(config);
> >> >> +}
> >> >> +
> >> >> +void migrate_ensure_converge(QTestState *who, QDict *config)
> >> >> +{
> >> >> +    config = config_load(config);
> >> >> +    /* Should converge with 30s downtime + 1 gbs bandwidth limit */
> >> >> +    if (config) {
> >> >> +        qdict_put_int(config, "max-bandwidth", 1 * 1000 * 1000 * 1000);
> >> >> +        qdict_put_int(config, "downtime-limit", 30 * 1000);
> >> >> +    } else {
> >> >> +        assert(who);
> >> >> +        migrate_ongoing_ensure_converge(who);
> >> >> +    }
> >> >> +    config_put(config);
> >> >> +}
> >> >
> >> > It's slightly an overkill to me to have these converge helpers to provide
> >> > two versions.  Also a bit confusing on when should we use which.
> >> >
> >> > After all, parameters touched on convergence must be able to be dynamically
> >> > set..
> >> >
> >> > Can we always stick with the QMP set-parameters for all these?
> >> >
> >> 
> >> Well, QEMU ignores anything set with migrate-set-parameters once it sees
> >> the config, so we'd need to change that in the code.
> >> 
> >> Thinking about the design of "config", I think the point was to never
> >> configure a migration via migrate-set-parameters. Always pass the config
> >> to the migration commands.
> >> 
> >> These options are special in that they make sense both before and after
> >> starting the migration, so it's indeed confusing. I don't know what the
> >> best approach is.
> >
> > Hmm, now I start to question whether this is a good idea.  That's about
> > this patch of the series:
> >
> >     migration: Allow migrate commands to provide the migration config
> >     
> >     Allow the migrate and migrate_incoming commands to pass the migration
> >     configuration options all at once, dispensing the use of
> >     migrate-set-parameters and migrate-set-capabilities.
> >     
> >     The motivation of this is to simplify the interface with the
> >     management layer and avoid the usage of several command invocations to
> >     configure a migration. It also avoids stale parameters from a previous
> >     migration to influence the current migration.
> >
> > Logically speaking, if mgmt worries about a stale parameter leftover, the
> > mgmt should always overwrite it in the config of this QMP migrate command..
> > Now I don't see a real benefit that we need to ignore global setups.
> >
> > A mgmt should simply query all parameters when QEMU just started up, then
> > keep it, then whatever user changes should be applied on top,  Then when
> > any QMP migrate happens, it should always set all parameters.. no matter
> > what is the global.
> >
> 
> We can decide that QEMU will not force the mgmt app to do that work and
> will provide an API that doesn't require setting all parameters. I don't
> see an argument here.
> 
> > The problem is exactly here, that when some parameters can be dynamically
> > changed like max-bw, if it was set and throttled 10Gbps dynamically,
> > migration failed, someone re-started the migration expecting the 10Gbps was
> > still applied when QMP migrate didn't set max-bw this time, but it didn't
> > work like that.
> >
> 
> We need to think about what the QMP API exposes.
> 
> If we expose an API that says: QMP_MIGRATE might use a value that was
> set using MIGRATE-SET-PARAMETERS 6 months ago because QEMU uses global
> state, that's an API usability issue.
> 
> If we expose an API that says: QMP_MIGRATE runs the migration with
> whatever arguments were passed to it via CONFIG, that sounds like
> something sensible.
> 
> I don't think the API consumers would be surprised if we allow
> MIGRATE-SET-PARAMETERS to change runtime values for a migration and on
> the next migration that value is no longer the same.
> 
> > Do you think we should make "config" of migrate / migrate_incoming taking
> > global setting as base, rather than initial_params?  I hope we don't
> > introduce something for nobody at last, but only to make our lives slightly
> > harder. :(
> 
> One calls a function and it uses the arguments passed to it, that's
> it. New migration, new arguments. As you said, mgmt could just hold the
> dynamic parameter that their user changed and pass that along with the
> config for the new migration.

It's never about when mgmt is an app like libvirt, IMHO either way we
define it, libvirt will be fine.  It's only about human beings when using
this API e.g. via HMPs, or who start to write scripts manipulating QEMU
directly assuming the max-bw will persist.

That's almost why I thought remembering the globals should be the best
because it will work well for both a computer and a human being.  But I
might be the only one thinking so if you don't agree on this; it's fine.

> 
> But I don't think we fleshed out the usage regarding the dynamic
> parameters yet. There might be other issues that I'm overlooking. Maybe
> we'd need a whole new command with slightly different semantics from
> migrate-set-parameters that adjusts the dynamic options while migration
> is running, I'm not sure.
> 
> ---
> side note:
> 
> If you think the whole endevour of passing a config to qmp_migrate
> is a bad idea, please speak up. If mgmt code will require the amount of
> churn I had to do for our test suite, maybe it's not worth the effort
> after all.

Frankly, I never thought there was an real problem to solve..  the only
thing I can remember is that "libvirt used to control this qemu, but link
down, some admin set something, libvirt reconnected and migration caused
weird issues".

IMHO when reconnect libvirt can always apply all the parameters once
more.. then if link kept, globals are also kept for this session anyway.
IMHO it's fair here because migration is special and unique for a QEMU
instance.  I don't further worry on other weird setups, like QEMU exposing
two QMP monitors - libvirt started the QEMU instance after all.

I almost always thought having "config" in migrate QMP command is a bonus
and optional, since it's still easy to get.  Hence, there's no harm anyway
we support it.  Your other changes (dedup >1 migration parameters structs,
merging caps into parameters, etc.) are more solid changers to me.

I'd confess initially I didn't expect so - logically we can introduce one
or a few new cases to test "config" setups, because most of the code paths
should really be shared (unless we decide to obsolete set-parameters; I
guess we won't.. forever).  But since we moved this far, The test cases are
now also moving all towards configs after this series. I think it's fine,
and you can decide that.

> 
> My opinion is that, in general, the "config" changes are going in a good
> direction. I worry slightly about what the cost would be for the users
> to adhere to it. Migration has been quirky for a looong time, you make
> it less quirky people will find it strange =)

Yes, actually I won't be surprised that after your series lands maybe the
"config" in QMP_MIGRATE won't be used for a while.  And if it keeps going
like that, nobody will consume it.. but I don't really know.  We'll see!

What I know is, after your series lands, I'll start to play with changing
default values for capabilities. :)

-- 
Peter Xu