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.
The options that are changed during the migration can still be set
with the existing commands.
The order of precedence is:
'config' argument > -global cmdline > defaults (migration_properties)
I.e. the config takes precedence over all, values not present in the
config assume the default values. The (debug) -global command line
option allows the defaults to be overridden.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/migration-hmp-cmds.c | 5 +++--
migration/migration.c | 29 ++++++++++++++++++++++++++---
migration/migration.h | 1 +
migration/options.c | 30 ++++++++++++++++++++++++++++++
migration/options.h | 3 +++
qapi/migration.json | 25 +++++++++++++++++++++++--
system/vl.c | 3 ++-
7 files changed, 88 insertions(+), 8 deletions(-)
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index a8c3515e9d..38b289e8d8 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -575,7 +575,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
}
QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
- qmp_migrate_incoming(NULL, true, caps, true, false, &err);
+ qmp_migrate_incoming(NULL, true, caps, NULL, true, false, &err);
qapi_free_MigrationChannelList(caps);
end:
@@ -952,7 +952,8 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
}
QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
- qmp_migrate(NULL, true, caps, false, false, true, resume, &err);
+ qmp_migrate(NULL, true, caps, NULL, false, false, true, resume,
+ &err);
if (hmp_handle_error(mon, err)) {
return;
}
diff --git a/migration/migration.c b/migration/migration.c
index 75c4ec9a95..7b450b8836 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -335,6 +335,7 @@ void migration_object_init(void)
current_incoming->exit_on_error = INMIGRATE_DEFAULT_EXIT_ON_ERROR;
migration_object_check(current_migration, &error_fatal);
+ migrate_params_store_defaults(current_migration);
ram_mig_init();
dirty_bitmap_mig_init();
@@ -1916,13 +1917,24 @@ void migrate_del_blocker(Error **reasonp)
void qmp_migrate_incoming(const char *uri, bool has_channels,
MigrationChannelList *channels,
- bool has_exit_on_error, bool exit_on_error,
- Error **errp)
+ MigrationParameters *config, bool has_exit_on_error,
+ bool exit_on_error, Error **errp)
{
Error *local_err = NULL;
static bool once = true;
+ MigrationState *s = migrate_get_current();
MigrationIncomingState *mis = migration_incoming_get_current();
+ if (config) {
+ /*
+ * If a config was provided, all options set previously get
+ * ignored.
+ */
+ if (!migrate_params_override(s, config, errp)) {
+ return;
+ }
+ }
+
if (!once) {
error_setg(errp, "The incoming migration has already been started");
return;
@@ -2182,7 +2194,8 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
}
void qmp_migrate(const char *uri, bool has_channels,
- MigrationChannelList *channels, bool has_detach, bool detach,
+ MigrationChannelList *channels,
+ bool has_detach, bool detach, MigrationParameters *config,
bool has_resume, bool resume, Error **errp)
{
bool resume_requested;
@@ -2193,6 +2206,16 @@ void qmp_migrate(const char *uri, bool has_channels,
MigrationChannel *channelv[MIGRATION_CHANNEL_TYPE__MAX] = { NULL };
MigrationChannel *cpr_channel = NULL;
+ if (config) {
+ /*
+ * If a config was provided, all options set previously get
+ * ignored.
+ */
+ if (!migrate_params_override(s, config, errp)) {
+ return;
+ }
+ }
+
/*
* Having preliminary checks for uri and channel
*/
diff --git a/migration/migration.h b/migration/migration.h
index 993d51aedd..49761f4699 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -319,6 +319,7 @@ struct MigrationState {
/* params from 'migrate-set-parameters' */
MigrationParameters parameters;
+ MigrationParameters defaults;
MigrationStatus state;
diff --git a/migration/options.c b/migration/options.c
index fa3f7035c8..dd2288187d 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1333,6 +1333,36 @@ static void migrate_params_apply(MigrationParameters *params)
params->block_bitmap_mapping);
}
+void migrate_params_store_defaults(MigrationState *s)
+{
+ /*
+ * The defaults set for each qdev property in migration_properties
+ * will be stored as the default values for each migration
+ * parameter. For debugging, using -global can override the
+ * defaults.
+ */
+ QAPI_CLONE_MEMBERS(MigrationParameters, &s->defaults, &s->parameters);
+}
+
+bool migrate_params_override(MigrationState *s, MigrationParameters *new,
+ Error **errp)
+{
+ ERRP_GUARD();
+
+ assert(bql_locked());
+
+ /* reset to default parameters */
+ migrate_params_apply(&s->defaults);
+
+ /* overwrite with the new ones */
+ qmp_migrate_set_parameters(new, errp);
+ if (*errp) {
+ return false;
+ }
+
+ return true;
+}
+
void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
{
MigrationParameters *tmp = g_new0(MigrationParameters, 1);
diff --git a/migration/options.h b/migration/options.h
index fcfd120cd7..3630c2a0dd 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -83,4 +83,7 @@ void migrate_capability_set_compat(MigrationParameters *params, int i,
void migrate_capabilities_set_compat(MigrationParameters *params,
MigrationCapabilityStatusList *caps);
bool migrate_caps_check(MigrationParameters *new, Error **errp);
+void migrate_params_store_defaults(MigrationState *s);
+bool migrate_params_override(MigrationState *s, MigrationParameters *new,
+ Error **errp);
#endif
diff --git a/qapi/migration.json b/qapi/migration.json
index 7282e4b9eb..64a92d8d28 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1474,9 +1474,16 @@
#
# @resume: resume one paused migration, default "off". (since 3.0)
#
+# @config: migration configuration options, previously set via
+# @migrate-set-parameters and @migrate-set-capabilities. (since
+# 10.1)
+#
# Features:
#
# @deprecated: Argument @detach is deprecated.
+# @config: Indicates this command can receive the entire migration
+# configuration via the @config field, dispensing the use of
+# @migrate-set-parameters.
#
# Since: 0.14
#
@@ -1538,7 +1545,9 @@
'data': {'*uri': 'str',
'*channels': [ 'MigrationChannel' ],
'*detach': { 'type': 'bool', 'features': [ 'deprecated' ] },
- '*resume': 'bool' } }
+ '*config': 'MigrationParameters',
+ '*resume': 'bool' },
+ 'features': [ 'config' ] }
##
# @migrate-incoming:
@@ -1557,6 +1566,16 @@
# error details could be retrieved with query-migrate.
# (since 9.1)
#
+# @config: migration configuration options, previously set via
+# @migrate-set-parameters and @migrate-set-capabilities. (since
+# 10.1)
+#
+# Features:
+#
+# @config: Indicates this command can receive the entire migration
+# configuration via the @config field, dispensing the use of
+# @migrate-set-parameters.
+#
# Since: 2.3
#
# .. admonition:: Notes
@@ -1610,7 +1629,9 @@
{ 'command': 'migrate-incoming',
'data': {'*uri': 'str',
'*channels': [ 'MigrationChannel' ],
- '*exit-on-error': 'bool' } }
+ '*config': 'MigrationParameters',
+ '*exit-on-error': 'bool' },
+ 'features': [ 'config' ] }
##
# @xen-save-devices-state:
diff --git a/system/vl.c b/system/vl.c
index 3b7057e6c6..b29fd24d08 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2823,7 +2823,8 @@ void qmp_x_exit_preconfig(Error **errp)
g_new0(MigrationChannelList, 1);
channels->value = incoming_channels[MIGRATION_CHANNEL_TYPE_MAIN];
- qmp_migrate_incoming(NULL, true, channels, true, true, &local_err);
+ qmp_migrate_incoming(NULL, true, channels, NULL, true, true,
+ &local_err);
if (local_err) {
error_reportf_err(local_err, "-incoming %s: ", incoming);
exit(1);
--
2.35.3On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
> 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.
>
> The options that are changed during the migration can still be set
> with the existing commands.
>
> The order of precedence is:
>
> 'config' argument > -global cmdline > defaults (migration_properties)
Could we still keep the QMP migrate-set-parameters values?
'config' argument > QMP setups using migrate-set-parameters >
-global cmdline > defaults (migration_properties)
I asked this before, maybe I forgot the answer..
>
> I.e. the config takes precedence over all, values not present in the
> config assume the default values. The (debug) -global command line
> option allows the defaults to be overridden.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> migration/migration-hmp-cmds.c | 5 +++--
> migration/migration.c | 29 ++++++++++++++++++++++++++---
> migration/migration.h | 1 +
> migration/options.c | 30 ++++++++++++++++++++++++++++++
> migration/options.h | 3 +++
> qapi/migration.json | 25 +++++++++++++++++++++++--
> system/vl.c | 3 ++-
> 7 files changed, 88 insertions(+), 8 deletions(-)
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index a8c3515e9d..38b289e8d8 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -575,7 +575,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
> }
> QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
>
> - qmp_migrate_incoming(NULL, true, caps, true, false, &err);
> + qmp_migrate_incoming(NULL, true, caps, NULL, true, false, &err);
> qapi_free_MigrationChannelList(caps);
>
> end:
> @@ -952,7 +952,8 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
> }
> QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
>
> - qmp_migrate(NULL, true, caps, false, false, true, resume, &err);
> + qmp_migrate(NULL, true, caps, NULL, false, false, true, resume,
> + &err);
> if (hmp_handle_error(mon, err)) {
> return;
> }
> diff --git a/migration/migration.c b/migration/migration.c
> index 75c4ec9a95..7b450b8836 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -335,6 +335,7 @@ void migration_object_init(void)
> current_incoming->exit_on_error = INMIGRATE_DEFAULT_EXIT_ON_ERROR;
>
> migration_object_check(current_migration, &error_fatal);
> + migrate_params_store_defaults(current_migration);
>
> ram_mig_init();
> dirty_bitmap_mig_init();
> @@ -1916,13 +1917,24 @@ void migrate_del_blocker(Error **reasonp)
>
> void qmp_migrate_incoming(const char *uri, bool has_channels,
> MigrationChannelList *channels,
> - bool has_exit_on_error, bool exit_on_error,
> - Error **errp)
> + MigrationParameters *config, bool has_exit_on_error,
> + bool exit_on_error, Error **errp)
> {
> Error *local_err = NULL;
> static bool once = true;
> + MigrationState *s = migrate_get_current();
> MigrationIncomingState *mis = migration_incoming_get_current();
>
> + if (config) {
> + /*
> + * If a config was provided, all options set previously get
> + * ignored.
> + */
> + if (!migrate_params_override(s, config, errp)) {
> + return;
> + }
> + }
> +
> if (!once) {
> error_setg(errp, "The incoming migration has already been started");
> return;
> @@ -2182,7 +2194,8 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
> }
>
> void qmp_migrate(const char *uri, bool has_channels,
> - MigrationChannelList *channels, bool has_detach, bool detach,
> + MigrationChannelList *channels,
> + bool has_detach, bool detach, MigrationParameters *config,
> bool has_resume, bool resume, Error **errp)
> {
> bool resume_requested;
> @@ -2193,6 +2206,16 @@ void qmp_migrate(const char *uri, bool has_channels,
> MigrationChannel *channelv[MIGRATION_CHANNEL_TYPE__MAX] = { NULL };
> MigrationChannel *cpr_channel = NULL;
>
> + if (config) {
> + /*
> + * If a config was provided, all options set previously get
> + * ignored.
> + */
> + if (!migrate_params_override(s, config, errp)) {
> + return;
> + }
> + }
> +
> /*
> * Having preliminary checks for uri and channel
> */
> diff --git a/migration/migration.h b/migration/migration.h
> index 993d51aedd..49761f4699 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -319,6 +319,7 @@ struct MigrationState {
>
> /* params from 'migrate-set-parameters' */
> MigrationParameters parameters;
> + MigrationParameters defaults;
This is also prone to be a pointer; I still think embeded qapi objects are
too error prone. Since it's new, make it a pointer from start?
>
> MigrationStatus state;
>
> diff --git a/migration/options.c b/migration/options.c
> index fa3f7035c8..dd2288187d 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1333,6 +1333,36 @@ static void migrate_params_apply(MigrationParameters *params)
> params->block_bitmap_mapping);
> }
>
> +void migrate_params_store_defaults(MigrationState *s)
> +{
> + /*
> + * The defaults set for each qdev property in migration_properties
> + * will be stored as the default values for each migration
> + * parameter. For debugging, using -global can override the
> + * defaults.
> + */
> + QAPI_CLONE_MEMBERS(MigrationParameters, &s->defaults, &s->parameters);
> +}
> +
> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
> + Error **errp)
> +{
> + ERRP_GUARD();
> +
> + assert(bql_locked());
> +
> + /* reset to default parameters */
> + migrate_params_apply(&s->defaults);
> +
> + /* overwrite with the new ones */
> + qmp_migrate_set_parameters(new, errp);
> + if (*errp) {
> + return false;
> + }
> +
> + return true;
> +}
> +
> void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
> {
> MigrationParameters *tmp = g_new0(MigrationParameters, 1);
> diff --git a/migration/options.h b/migration/options.h
> index fcfd120cd7..3630c2a0dd 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -83,4 +83,7 @@ void migrate_capability_set_compat(MigrationParameters *params, int i,
> void migrate_capabilities_set_compat(MigrationParameters *params,
> MigrationCapabilityStatusList *caps);
> bool migrate_caps_check(MigrationParameters *new, Error **errp);
> +void migrate_params_store_defaults(MigrationState *s);
> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
> + Error **errp);
> #endif
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 7282e4b9eb..64a92d8d28 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1474,9 +1474,16 @@
> #
> # @resume: resume one paused migration, default "off". (since 3.0)
> #
> +# @config: migration configuration options, previously set via
> +# @migrate-set-parameters and @migrate-set-capabilities. (since
> +# 10.1)
> +#
> # Features:
> #
> # @deprecated: Argument @detach is deprecated.
> +# @config: Indicates this command can receive the entire migration
> +# configuration via the @config field, dispensing the use of
> +# @migrate-set-parameters.
> #
> # Since: 0.14
> #
> @@ -1538,7 +1545,9 @@
> 'data': {'*uri': 'str',
> '*channels': [ 'MigrationChannel' ],
> '*detach': { 'type': 'bool', 'features': [ 'deprecated' ] },
> - '*resume': 'bool' } }
> + '*config': 'MigrationParameters',
> + '*resume': 'bool' },
> + 'features': [ 'config' ] }
>
> ##
> # @migrate-incoming:
> @@ -1557,6 +1566,16 @@
> # error details could be retrieved with query-migrate.
> # (since 9.1)
> #
> +# @config: migration configuration options, previously set via
> +# @migrate-set-parameters and @migrate-set-capabilities. (since
> +# 10.1)
> +#
> +# Features:
> +#
> +# @config: Indicates this command can receive the entire migration
> +# configuration via the @config field, dispensing the use of
> +# @migrate-set-parameters.
> +#
> # Since: 2.3
> #
> # .. admonition:: Notes
> @@ -1610,7 +1629,9 @@
> { 'command': 'migrate-incoming',
> 'data': {'*uri': 'str',
> '*channels': [ 'MigrationChannel' ],
> - '*exit-on-error': 'bool' } }
> + '*config': 'MigrationParameters',
> + '*exit-on-error': 'bool' },
> + 'features': [ 'config' ] }
>
> ##
> # @xen-save-devices-state:
> diff --git a/system/vl.c b/system/vl.c
> index 3b7057e6c6..b29fd24d08 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2823,7 +2823,8 @@ void qmp_x_exit_preconfig(Error **errp)
> g_new0(MigrationChannelList, 1);
>
> channels->value = incoming_channels[MIGRATION_CHANNEL_TYPE_MAIN];
> - qmp_migrate_incoming(NULL, true, channels, true, true, &local_err);
> + qmp_migrate_incoming(NULL, true, channels, NULL, true, true,
> + &local_err);
> if (local_err) {
> error_reportf_err(local_err, "-incoming %s: ", incoming);
> exit(1);
> --
> 2.35.3
>
--
Peter Xu
Peter Xu <peterx@redhat.com> writes:
> On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
>> 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.
>>
>> The options that are changed during the migration can still be set
>> with the existing commands.
>>
>> The order of precedence is:
>>
>> 'config' argument > -global cmdline > defaults (migration_properties)
>
> Could we still keep the QMP migrate-set-parameters values?
>
> 'config' argument > QMP setups using migrate-set-parameters >
> -global cmdline > defaults (migration_properties)
>
That's the case. I failed to mention it in the commit message. IOW it
behaves just like today, but the new 'config' way takes precedence over
all.
> I asked this before, maybe I forgot the answer..
>
>>
>> I.e. the config takes precedence over all, values not present in the
>> config assume the default values. The (debug) -global command line
>> option allows the defaults to be overridden.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> migration/migration-hmp-cmds.c | 5 +++--
>> migration/migration.c | 29 ++++++++++++++++++++++++++---
>> migration/migration.h | 1 +
>> migration/options.c | 30 ++++++++++++++++++++++++++++++
>> migration/options.h | 3 +++
>> qapi/migration.json | 25 +++++++++++++++++++++++--
>> system/vl.c | 3 ++-
>> 7 files changed, 88 insertions(+), 8 deletions(-)
>>
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index a8c3515e9d..38b289e8d8 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -575,7 +575,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>> }
>> QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
>>
>> - qmp_migrate_incoming(NULL, true, caps, true, false, &err);
>> + qmp_migrate_incoming(NULL, true, caps, NULL, true, false, &err);
>> qapi_free_MigrationChannelList(caps);
>>
>> end:
>> @@ -952,7 +952,8 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>> }
>> QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
>>
>> - qmp_migrate(NULL, true, caps, false, false, true, resume, &err);
>> + qmp_migrate(NULL, true, caps, NULL, false, false, true, resume,
>> + &err);
>> if (hmp_handle_error(mon, err)) {
>> return;
>> }
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 75c4ec9a95..7b450b8836 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -335,6 +335,7 @@ void migration_object_init(void)
>> current_incoming->exit_on_error = INMIGRATE_DEFAULT_EXIT_ON_ERROR;
>>
>> migration_object_check(current_migration, &error_fatal);
>> + migrate_params_store_defaults(current_migration);
>>
>> ram_mig_init();
>> dirty_bitmap_mig_init();
>> @@ -1916,13 +1917,24 @@ void migrate_del_blocker(Error **reasonp)
>>
>> void qmp_migrate_incoming(const char *uri, bool has_channels,
>> MigrationChannelList *channels,
>> - bool has_exit_on_error, bool exit_on_error,
>> - Error **errp)
>> + MigrationParameters *config, bool has_exit_on_error,
>> + bool exit_on_error, Error **errp)
>> {
>> Error *local_err = NULL;
>> static bool once = true;
>> + MigrationState *s = migrate_get_current();
>> MigrationIncomingState *mis = migration_incoming_get_current();
>>
>> + if (config) {
>> + /*
>> + * If a config was provided, all options set previously get
>> + * ignored.
>> + */
>> + if (!migrate_params_override(s, config, errp)) {
>> + return;
>> + }
>> + }
>> +
>> if (!once) {
>> error_setg(errp, "The incoming migration has already been started");
>> return;
>> @@ -2182,7 +2194,8 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
>> }
>>
>> void qmp_migrate(const char *uri, bool has_channels,
>> - MigrationChannelList *channels, bool has_detach, bool detach,
>> + MigrationChannelList *channels,
>> + bool has_detach, bool detach, MigrationParameters *config,
>> bool has_resume, bool resume, Error **errp)
>> {
>> bool resume_requested;
>> @@ -2193,6 +2206,16 @@ void qmp_migrate(const char *uri, bool has_channels,
>> MigrationChannel *channelv[MIGRATION_CHANNEL_TYPE__MAX] = { NULL };
>> MigrationChannel *cpr_channel = NULL;
>>
>> + if (config) {
>> + /*
>> + * If a config was provided, all options set previously get
>> + * ignored.
>> + */
>> + if (!migrate_params_override(s, config, errp)) {
>> + return;
>> + }
>> + }
>> +
>> /*
>> * Having preliminary checks for uri and channel
>> */
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 993d51aedd..49761f4699 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -319,6 +319,7 @@ struct MigrationState {
>>
>> /* params from 'migrate-set-parameters' */
>> MigrationParameters parameters;
>> + MigrationParameters defaults;
>
> This is also prone to be a pointer; I still think embeded qapi objects are
> too error prone. Since it's new, make it a pointer from start?
>
Ok.
>>
>> MigrationStatus state;
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index fa3f7035c8..dd2288187d 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -1333,6 +1333,36 @@ static void migrate_params_apply(MigrationParameters *params)
>> params->block_bitmap_mapping);
>> }
>>
>> +void migrate_params_store_defaults(MigrationState *s)
>> +{
>> + /*
>> + * The defaults set for each qdev property in migration_properties
>> + * will be stored as the default values for each migration
>> + * parameter. For debugging, using -global can override the
>> + * defaults.
>> + */
>> + QAPI_CLONE_MEMBERS(MigrationParameters, &s->defaults, &s->parameters);
>> +}
>> +
>> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
>> + Error **errp)
>> +{
>> + ERRP_GUARD();
>> +
>> + assert(bql_locked());
>> +
>> + /* reset to default parameters */
>> + migrate_params_apply(&s->defaults);
>> +
>> + /* overwrite with the new ones */
>> + qmp_migrate_set_parameters(new, errp);
>> + if (*errp) {
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>> {
>> MigrationParameters *tmp = g_new0(MigrationParameters, 1);
>> diff --git a/migration/options.h b/migration/options.h
>> index fcfd120cd7..3630c2a0dd 100644
>> --- a/migration/options.h
>> +++ b/migration/options.h
>> @@ -83,4 +83,7 @@ void migrate_capability_set_compat(MigrationParameters *params, int i,
>> void migrate_capabilities_set_compat(MigrationParameters *params,
>> MigrationCapabilityStatusList *caps);
>> bool migrate_caps_check(MigrationParameters *new, Error **errp);
>> +void migrate_params_store_defaults(MigrationState *s);
>> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
>> + Error **errp);
>> #endif
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 7282e4b9eb..64a92d8d28 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1474,9 +1474,16 @@
>> #
>> # @resume: resume one paused migration, default "off". (since 3.0)
>> #
>> +# @config: migration configuration options, previously set via
>> +# @migrate-set-parameters and @migrate-set-capabilities. (since
>> +# 10.1)
>> +#
>> # Features:
>> #
>> # @deprecated: Argument @detach is deprecated.
>> +# @config: Indicates this command can receive the entire migration
>> +# configuration via the @config field, dispensing the use of
>> +# @migrate-set-parameters.
>> #
>> # Since: 0.14
>> #
>> @@ -1538,7 +1545,9 @@
>> 'data': {'*uri': 'str',
>> '*channels': [ 'MigrationChannel' ],
>> '*detach': { 'type': 'bool', 'features': [ 'deprecated' ] },
>> - '*resume': 'bool' } }
>> + '*config': 'MigrationParameters',
>> + '*resume': 'bool' },
>> + 'features': [ 'config' ] }
>>
>> ##
>> # @migrate-incoming:
>> @@ -1557,6 +1566,16 @@
>> # error details could be retrieved with query-migrate.
>> # (since 9.1)
>> #
>> +# @config: migration configuration options, previously set via
>> +# @migrate-set-parameters and @migrate-set-capabilities. (since
>> +# 10.1)
>> +#
>> +# Features:
>> +#
>> +# @config: Indicates this command can receive the entire migration
>> +# configuration via the @config field, dispensing the use of
>> +# @migrate-set-parameters.
>> +#
>> # Since: 2.3
>> #
>> # .. admonition:: Notes
>> @@ -1610,7 +1629,9 @@
>> { 'command': 'migrate-incoming',
>> 'data': {'*uri': 'str',
>> '*channels': [ 'MigrationChannel' ],
>> - '*exit-on-error': 'bool' } }
>> + '*config': 'MigrationParameters',
>> + '*exit-on-error': 'bool' },
>> + 'features': [ 'config' ] }
>>
>> ##
>> # @xen-save-devices-state:
>> diff --git a/system/vl.c b/system/vl.c
>> index 3b7057e6c6..b29fd24d08 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2823,7 +2823,8 @@ void qmp_x_exit_preconfig(Error **errp)
>> g_new0(MigrationChannelList, 1);
>>
>> channels->value = incoming_channels[MIGRATION_CHANNEL_TYPE_MAIN];
>> - qmp_migrate_incoming(NULL, true, channels, true, true, &local_err);
>> + qmp_migrate_incoming(NULL, true, channels, NULL, true, true,
>> + &local_err);
>> if (local_err) {
>> error_reportf_err(local_err, "-incoming %s: ", incoming);
>> exit(1);
>> --
>> 2.35.3
>>
On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
> >> 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.
> >>
> >> The options that are changed during the migration can still be set
> >> with the existing commands.
> >>
> >> The order of precedence is:
> >>
> >> 'config' argument > -global cmdline > defaults (migration_properties)
> >
> > Could we still keep the QMP migrate-set-parameters values?
> >
> > 'config' argument > QMP setups using migrate-set-parameters >
> > -global cmdline > defaults (migration_properties)
> >
>
> That's the case. I failed to mention it in the commit message. IOW it
> behaves just like today, but the new 'config' way takes precedence over
> all.
Referring to below chunk of code:
[...]
> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
> >> + Error **errp)
> >> +{
> >> + ERRP_GUARD();
> >> +
> >> + assert(bql_locked());
> >> +
> >> + /* reset to default parameters */
> >> + migrate_params_apply(&s->defaults);
IIUC here it'll reset all global parameters using the initial defaults
first, then apply the "config" specified in "migrate" QMP command?
I think there're actually two separate questions to be asked, to make it
clearer, they are:
(1) Whether we should allow QMP "migrate" 'config' parameter to overwrite
global setup?
(2) Whether we should allow previous QMP global setup to be used even if
QMP "migrate" provided 'config' parameter?
So IIUC the patch does (1) YES (2) NO, while what I think might be more
intuitive is (1) NO (2) YES.
> >> +
> >> + /* overwrite with the new ones */
> >> + qmp_migrate_set_parameters(new, errp);
> >> + if (*errp) {
> >> + return false;
> >> + }
> >> +
> >> + return true;
> >> +}
--
Peter Xu
On Fri, Jun 06, 2025 at 04:50:54PM -0400, Peter Xu wrote:
> On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
> > Peter Xu <peterx@redhat.com> writes:
> >
> > > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
> > >> 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.
> > >>
> > >> The options that are changed during the migration can still be set
> > >> with the existing commands.
> > >>
> > >> The order of precedence is:
> > >>
> > >> 'config' argument > -global cmdline > defaults (migration_properties)
> > >
> > > Could we still keep the QMP migrate-set-parameters values?
> > >
> > > 'config' argument > QMP setups using migrate-set-parameters >
> > > -global cmdline > defaults (migration_properties)
> > >
> >
> > That's the case. I failed to mention it in the commit message. IOW it
> > behaves just like today, but the new 'config' way takes precedence over
> > all.
>
> Referring to below chunk of code:
>
> [...]
>
> > >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
> > >> + Error **errp)
> > >> +{
> > >> + ERRP_GUARD();
> > >> +
> > >> + assert(bql_locked());
> > >> +
> > >> + /* reset to default parameters */
> > >> + migrate_params_apply(&s->defaults);
>
> IIUC here it'll reset all global parameters using the initial defaults
> first, then apply the "config" specified in "migrate" QMP command?
>
> I think there're actually two separate questions to be asked, to make it
> clearer, they are:
>
> (1) Whether we should allow QMP "migrate" 'config' parameter to overwrite
> global setup?
>
> (2) Whether we should allow previous QMP global setup to be used even if
> QMP "migrate" provided 'config' parameter?
>
> So IIUC the patch does (1) YES (2) NO, while what I think might be more
> intuitive is (1) NO (2) YES.
The point of the 'config' parameter to the 'migrate' command is to
enable the mgmt app to fully specify what it wants the configuration
to be, such that there is no previously set state will will cause
it surprises. Allowing -global to have an effect undermines the
predictibility in the same way that migrate-set-parameter undermines
the predictibility.
With 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 :|
On Mon, Jun 09, 2025 at 04:03:01PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 06, 2025 at 04:50:54PM -0400, Peter Xu wrote:
> > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
> > > Peter Xu <peterx@redhat.com> writes:
> > >
> > > > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
> > > >> 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.
> > > >>
> > > >> The options that are changed during the migration can still be set
> > > >> with the existing commands.
> > > >>
> > > >> The order of precedence is:
> > > >>
> > > >> 'config' argument > -global cmdline > defaults (migration_properties)
> > > >
> > > > Could we still keep the QMP migrate-set-parameters values?
> > > >
> > > > 'config' argument > QMP setups using migrate-set-parameters >
> > > > -global cmdline > defaults (migration_properties)
> > > >
> > >
> > > That's the case. I failed to mention it in the commit message. IOW it
> > > behaves just like today, but the new 'config' way takes precedence over
> > > all.
> >
> > Referring to below chunk of code:
> >
> > [...]
> >
> > > >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
> > > >> + Error **errp)
> > > >> +{
> > > >> + ERRP_GUARD();
> > > >> +
> > > >> + assert(bql_locked());
> > > >> +
> > > >> + /* reset to default parameters */
> > > >> + migrate_params_apply(&s->defaults);
> >
> > IIUC here it'll reset all global parameters using the initial defaults
> > first, then apply the "config" specified in "migrate" QMP command?
> >
> > I think there're actually two separate questions to be asked, to make it
> > clearer, they are:
> >
> > (1) Whether we should allow QMP "migrate" 'config' parameter to overwrite
> > global setup?
> >
> > (2) Whether we should allow previous QMP global setup to be used even if
> > QMP "migrate" provided 'config' parameter?
> >
> > So IIUC the patch does (1) YES (2) NO, while what I think might be more
> > intuitive is (1) NO (2) YES.
>
> The point of the 'config' parameter to the 'migrate' command is to
> enable the mgmt app to fully specify what it wants the configuration
> to be, such that there is no previously set state will will cause
> it surprises. Allowing -global to have an effect undermines the
> predictibility in the same way that migrate-set-parameter undermines
> the predictibility.
Now I think I know part of what I've missed: I used to think the "config"
of per-QMP-migrate-command can be totally temporary for a specific
migration request, but then we need another MigrationState.parameters_2 to
cache the old or vice versa; that's probably not necessary. Now I think it
makes sense to overwrite any settings directly, hence I think I changed my
mind on question (1), YES is fine here.
For (2), why it would introduce any uncertainty for mgmt?
If the mgmt app can both: (1) query from qapi schema knowing all the
parameters supported, then (2) specify all the parameters in QMP migrate's
"option" parameter. Then it's literally overwritting all the parameters,
so it's predictable with or without completely removing global settings as
an idea? To me, the "option" is the key to make QMP migrate command and
parameter/cap setup in one "atomic-like" operation, and that provides the
predictability if the command succeeded and if all the parameters are
specified (otherwise it'll fail saying migration in progress, internally
protected by BQL or whatever lock QMP monitor holds).
Thanks,
--
Peter Xu
On Mon, Jun 09, 2025 at 11:33:14AM -0400, Peter Xu wrote: > > Now I think I know part of what I've missed: I used to think the "config" > of per-QMP-migrate-command can be totally temporary for a specific > migration request, but then we need another MigrationState.parameters_2 to > cache the old or vice versa; that's probably not necessary. Now I think it > makes sense to overwrite any settings directly, hence I think I changed my > mind on question (1), YES is fine here. > > For (2), why it would introduce any uncertainty for mgmt? > > If the mgmt app can both: (1) query from qapi schema knowing all the > parameters supported, then (2) specify all the parameters in QMP migrate's > "option" parameter. Then it's literally overwritting all the parameters, > so it's predictable with or without completely removing global settings as > an idea? That is relying on the mgmt app specifiying absolutely every config parameter that exists. If they miss anything, then the behaviour is not well defined, as external global state still affects things. This is the same situation we already have with migrate-set-parameter, where mgmt apps have to know to call migrate-set-parameter over & over with every possible parameter to get back to a well known starting point. The command needs to run with the parameters provided in 'config' and no external global state, whether from -global or any prior call of migrate-set-parameter With 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 :|
On Mon, Jun 09, 2025 at 04:43:40PM +0100, Daniel P. Berrangé wrote: > On Mon, Jun 09, 2025 at 11:33:14AM -0400, Peter Xu wrote: > > > > Now I think I know part of what I've missed: I used to think the "config" > > of per-QMP-migrate-command can be totally temporary for a specific > > migration request, but then we need another MigrationState.parameters_2 to > > cache the old or vice versa; that's probably not necessary. Now I think it > > makes sense to overwrite any settings directly, hence I think I changed my > > mind on question (1), YES is fine here. > > > > For (2), why it would introduce any uncertainty for mgmt? > > > > If the mgmt app can both: (1) query from qapi schema knowing all the > > parameters supported, then (2) specify all the parameters in QMP migrate's > > "option" parameter. Then it's literally overwritting all the parameters, > > so it's predictable with or without completely removing global settings as > > an idea? > > That is relying on the mgmt app specifiying absolutely every config > parameter that exists. If they miss anything, then the behaviour is > not well defined, as external global state still affects things. > > This is the same situation we already have with migrate-set-parameter, > where mgmt apps have to know to call migrate-set-parameter over & over > with every possible parameter to get back to a well known starting point. > > The command needs to run with the parameters provided in 'config' and > no external global state, whether from -global or any prior call of > migrate-set-parameter So libvirt does not probe the qapi schema for all possible parameters? Why not do that once on QEMU boot up, then when migration is needed use a sequence of commands to make sure everything will be setup before "migrate"? It'll definitely take a few rounds of QMP commands, but the core issue is whether there can be any real atomic issues of that. Just to say, I still think having "option" is a fine idea at least, but I'm really curious on whether there's any real issue even without it. Thanks, -- Peter Xu
On Mon, Jun 09, 2025 at 11:53:47AM -0400, Peter Xu wrote: > On Mon, Jun 09, 2025 at 04:43:40PM +0100, Daniel P. Berrangé wrote: > > On Mon, Jun 09, 2025 at 11:33:14AM -0400, Peter Xu wrote: > > > > > > Now I think I know part of what I've missed: I used to think the "config" > > > of per-QMP-migrate-command can be totally temporary for a specific > > > migration request, but then we need another MigrationState.parameters_2 to > > > cache the old or vice versa; that's probably not necessary. Now I think it > > > makes sense to overwrite any settings directly, hence I think I changed my > > > mind on question (1), YES is fine here. > > > > > > For (2), why it would introduce any uncertainty for mgmt? > > > > > > If the mgmt app can both: (1) query from qapi schema knowing all the > > > parameters supported, then (2) specify all the parameters in QMP migrate's > > > "option" parameter. Then it's literally overwritting all the parameters, > > > so it's predictable with or without completely removing global settings as > > > an idea? > > > > That is relying on the mgmt app specifiying absolutely every config > > parameter that exists. If they miss anything, then the behaviour is > > not well defined, as external global state still affects things. > > > > This is the same situation we already have with migrate-set-parameter, > > where mgmt apps have to know to call migrate-set-parameter over & over > > with every possible parameter to get back to a well known starting point. > > > > The command needs to run with the parameters provided in 'config' and > > no external global state, whether from -global or any prior call of > > migrate-set-parameter > > So libvirt does not probe the qapi schema for all possible parameters? Why > not do that once on QEMU boot up, then when migration is needed use a > sequence of commands to make sure everything will be setup before > "migrate"? It'll definitely take a few rounds of QMP commands, but the > core issue is whether there can be any real atomic issues of that. Probing the QAPI schema tells you what parameters exist. It does not tell you what values you should set for parameters, if you don't already know what the semantics of that parameter are. Such a requirement to probe all parameters & set them all manually is again making migration into a special case that is not following the normal QMP design, and there's no justification for that other than the historical design mistakes in migration QMP which were copied from HMP. With 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 :|
On Mon, Jun 09, 2025 at 05:15:43PM +0100, Daniel P. Berrangé wrote: > On Mon, Jun 09, 2025 at 11:53:47AM -0400, Peter Xu wrote: > > On Mon, Jun 09, 2025 at 04:43:40PM +0100, Daniel P. Berrangé wrote: > > > On Mon, Jun 09, 2025 at 11:33:14AM -0400, Peter Xu wrote: > > > > > > > > Now I think I know part of what I've missed: I used to think the "config" > > > > of per-QMP-migrate-command can be totally temporary for a specific > > > > migration request, but then we need another MigrationState.parameters_2 to > > > > cache the old or vice versa; that's probably not necessary. Now I think it > > > > makes sense to overwrite any settings directly, hence I think I changed my > > > > mind on question (1), YES is fine here. > > > > > > > > For (2), why it would introduce any uncertainty for mgmt? > > > > > > > > If the mgmt app can both: (1) query from qapi schema knowing all the > > > > parameters supported, then (2) specify all the parameters in QMP migrate's > > > > "option" parameter. Then it's literally overwritting all the parameters, > > > > so it's predictable with or without completely removing global settings as > > > > an idea? > > > > > > That is relying on the mgmt app specifiying absolutely every config > > > parameter that exists. If they miss anything, then the behaviour is > > > not well defined, as external global state still affects things. > > > > > > This is the same situation we already have with migrate-set-parameter, > > > where mgmt apps have to know to call migrate-set-parameter over & over > > > with every possible parameter to get back to a well known starting point. > > > > > > The command needs to run with the parameters provided in 'config' and > > > no external global state, whether from -global or any prior call of > > > migrate-set-parameter > > > > So libvirt does not probe the qapi schema for all possible parameters? Why > > not do that once on QEMU boot up, then when migration is needed use a > > sequence of commands to make sure everything will be setup before > > "migrate"? It'll definitely take a few rounds of QMP commands, but the > > core issue is whether there can be any real atomic issues of that. > > Probing the QAPI schema tells you what parameters exist. It does not tell > you what values you should set for parameters, if you don't already know If Libvirt is looking for some suggested value to set a parameter, it should just leave it empty, using the default provided by QEMU? I was expecting Libvirt to only specify anything it explicitly knows the answer. > what the semantics of that parameter are. Such a requirement to probe > all parameters & set them all manually is again making migration into a > special case that is not following the normal QMP design, and there's > no justification for that other than the historical design mistakes in > migration QMP which were copied from HMP. I agree migration is special cased.. I also agree if we design the interface today it may not be like that. I suppose it means it's only the "API cleaness" issue. That matches my understanding, even if I wished I missed something else.. That'll be a hassle for all mgmt for sure whenever an old libvirt might still have a chance to run on a newer QEMU. That'll also be a hassle for any downstream if some Y+1 branch starts to drop the global-set way completely then downstream might need to take care of keeping that instead for the major release until the last Y, otherwise if someone installs some X.Y+1 package on X.Y it might break similarly. All that for "let's make the interface look better". I sincerely could be wrong, but I keep my skeptical view of this whole effort; it's only about after this series (while this series still makes sense to me to have caps being able to set as params, and the "config" in general). I would say we could at least prioritize and invest other more important things, for example on handshakes, which could provide functional differences (removing src/dst param dependency, removing hackish channel establishments all over the places, early failure of device state mismatch rather than late failure on converge, etc.). Thanks, -- Peter Xu
On Mon, Jun 09, 2025 at 11:53:47AM -0400, Peter Xu wrote: > On Mon, Jun 09, 2025 at 04:43:40PM +0100, Daniel P. Berrangé wrote: > > On Mon, Jun 09, 2025 at 11:33:14AM -0400, Peter Xu wrote: > > > > > > Now I think I know part of what I've missed: I used to think the "config" > > > of per-QMP-migrate-command can be totally temporary for a specific > > > migration request, but then we need another MigrationState.parameters_2 to > > > cache the old or vice versa; that's probably not necessary. Now I think it > > > makes sense to overwrite any settings directly, hence I think I changed my > > > mind on question (1), YES is fine here. > > > > > > For (2), why it would introduce any uncertainty for mgmt? > > > > > > If the mgmt app can both: (1) query from qapi schema knowing all the > > > parameters supported, then (2) specify all the parameters in QMP migrate's > > > "option" parameter. Then it's literally overwritting all the parameters, > > > so it's predictable with or without completely removing global settings as > > > an idea? > > > > That is relying on the mgmt app specifiying absolutely every config > > parameter that exists. If they miss anything, then the behaviour is > > not well defined, as external global state still affects things. > > > > This is the same situation we already have with migrate-set-parameter, > > where mgmt apps have to know to call migrate-set-parameter over & over > > with every possible parameter to get back to a well known starting point. > > > > The command needs to run with the parameters provided in 'config' and > > no external global state, whether from -global or any prior call of > > migrate-set-parameter > > So libvirt does not probe the qapi schema for all possible parameters? Why > not do that once on QEMU boot up, then when migration is needed use a > sequence of commands to make sure everything will be setup before > "migrate"? It'll definitely take a few rounds of QMP commands, but the > core issue is whether there can be any real atomic issues of that. > > Just to say, I still think having "option" is a fine idea at least, but I'm > really curious on whether there's any real issue even without it. Btw, one more thing to mention: IIUC libvirt shouldn't need to specify all parameters either. We have default parameters provided, now with Fabiano's work we can even have default caps being enabled now, used to controversial when being a cap. Hence, I would expect Libvirt doesn't need to specify all parameters (with/without "config") but whatever parameters it doesn't want to use the default (e.g. being specified by the user). When we can have more sane default parameters (I do plan to turn on preempt and blocktime caps on by default for postcopy at some point, for example), I think it's better libvirt uses the default, or e.g. blocktime may have been disabled there. Thanks, -- Peter Xu
Peter Xu <peterx@redhat.com> writes:
> On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
>> >> 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.
>> >>
>> >> The options that are changed during the migration can still be set
>> >> with the existing commands.
>> >>
>> >> The order of precedence is:
>> >>
>> >> 'config' argument > -global cmdline > defaults (migration_properties)
>> >
>> > Could we still keep the QMP migrate-set-parameters values?
>> >
>> > 'config' argument > QMP setups using migrate-set-parameters >
>> > -global cmdline > defaults (migration_properties)
>> >
>>
>> That's the case. I failed to mention it in the commit message. IOW it
>> behaves just like today, but the new 'config' way takes precedence over
>> all.
>
> Referring to below chunk of code:
>
> [...]
>
>> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
>> >> + Error **errp)
>> >> +{
>> >> + ERRP_GUARD();
>> >> +
>> >> + assert(bql_locked());
>> >> +
>> >> + /* reset to default parameters */
>> >> + migrate_params_apply(&s->defaults);
>
> IIUC here it'll reset all global parameters using the initial defaults
> first, then apply the "config" specified in "migrate" QMP command?
>
Yes, this is so any previously set parameter via migrate-set-parameter
gets erased. I think what we want (but feel free to disagree) is to have
the migrate-set-parameter _eventually_ only handle parameters that need
to be modifed during migration runtime. Anything else can be done via
passing config to qmp_migrate.
For -global, I don't have a preference. Having -global take precedence
over all would require a way to know which options were present in the
command-line and which are just the defaults seet in
migration_properties. I currently don't know how to do that. If it is at
all possible (within reason) we could make the change, no worries.
> I think there're actually two separate questions to be asked, to make it
> clearer, they are:
Here it got ambiguous when you say "global", I've been using -global to
refer to the cmdline -global migration.foo, but others have used global
to mean s->parameters (which has an extended lifetime). Could you
clarify?
>
> (1) Whether we should allow QMP "migrate" 'config' parameter to overwrite
> global setup?
>
> (2) Whether we should allow previous QMP global setup to be used even if
> QMP "migrate" provided 'config' parameter?
>
> So IIUC the patch does (1) YES (2) NO, while what I think might be more
> intuitive is (1) NO (2) YES.
>
>> >> +
>> >> + /* overwrite with the new ones */
>> >> + qmp_migrate_set_parameters(new, errp);
>> >> + if (*errp) {
>> >> + return false;
>> >> + }
>> >> +
>> >> + return true;
>> >> +}
On Mon, Jun 09, 2025 at 11:37:02AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >>
> >> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
> >> >> 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.
> >> >>
> >> >> The options that are changed during the migration can still be set
> >> >> with the existing commands.
> >> >>
> >> >> The order of precedence is:
> >> >>
> >> >> 'config' argument > -global cmdline > defaults (migration_properties)
> >> >
> >> > Could we still keep the QMP migrate-set-parameters values?
> >> >
> >> > 'config' argument > QMP setups using migrate-set-parameters >
> >> > -global cmdline > defaults (migration_properties)
> >> >
> >>
> >> That's the case. I failed to mention it in the commit message. IOW it
> >> behaves just like today, but the new 'config' way takes precedence over
> >> all.
> >
> > Referring to below chunk of code:
> >
> > [...]
> >
> >> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
> >> >> + Error **errp)
> >> >> +{
> >> >> + ERRP_GUARD();
> >> >> +
> >> >> + assert(bql_locked());
> >> >> +
> >> >> + /* reset to default parameters */
> >> >> + migrate_params_apply(&s->defaults);
> >
> > IIUC here it'll reset all global parameters using the initial defaults
> > first, then apply the "config" specified in "migrate" QMP command?
> >
>
> Yes, this is so any previously set parameter via migrate-set-parameter
> gets erased. I think what we want (but feel free to disagree) is to have
> the migrate-set-parameter _eventually_ only handle parameters that need
> to be modifed during migration runtime. Anything else can be done via
> passing config to qmp_migrate.
>
> For -global, I don't have a preference. Having -global take precedence
> over all would require a way to know which options were present in the
> command-line and which are just the defaults seet in
> migration_properties. I currently don't know how to do that. If it is at
> all possible (within reason) we could make the change, no worries.
>
> > I think there're actually two separate questions to be asked, to make it
> > clearer, they are:
>
> Here it got ambiguous when you say "global", I've been using -global to
> refer to the cmdline -global migration.foo, but others have used global
> to mean s->parameters (which has an extended lifetime). Could you
> clarify?
I meant the -global, and the global setups via migrate-set-parameters.
As replied to Dan in the other email, I changed my mind on question (1); I
think it makes sense to have it YES. I left my pure question on (2) there
too.
Do we really want to disable migrate-set-parameters setting most of the
parameters, and only allow it to be set during migration on a few things
like bandwidth or so?
I just don't really see the major benefit of that yet. I would think it
make more sense if we don't need to change any parameters in migration,
then provide that in one shot in QMP migrate "config". Maybe making more
sense if migration is not heavily thread-based but having its aiocontext so
we could even move to Jobs.
Now after all we'll need to allow setting something like bandwidth even
during migration alive, and we have all the things ready allowing to set
before migration starts, I'm not 100% sure whether we need to bother even
if it does look cleaner, because we'll still break mgmt used to be working
for years.. I could be over-cautious on breaking things, but I still want
to understand better on the benefits.
One step back, on this "allow migrate to specify 'config'" request: I think
we can definitely do that as it still provides some kind of atomicity. But
frankly speaking I never see it a "real problem" - do we really have report
or use case showing that Libvirt can trigger "migrate" with some global
settings touched by other apps at all?
To me, it was yet an illutionary problem, I never know the answer of that.
If Libvirt is still the owner of QEMU instance via the QMP channel, I
actually don't really see why the atomicity would even help, even though we
can still provide that as it's pretty easy as something optional; like what
this patch does without too much hassle.
Then if to move one step further to remove all global settings, we face
breaking debugging scripts, and breaking of any old libvirt and non-libvirt
mgmt apps. Frankly I really don't yet know whether it's a good idea. I
could miss some important reasoning of why we want to do it - it needs to
be something not relevant to "making the code cleaner", IMHO..
--
Peter Xu
Peter Xu <peterx@redhat.com> writes:
> On Mon, Jun 09, 2025 at 11:37:02AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >>
>> >> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
>> >> >> 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.
>> >> >>
>> >> >> The options that are changed during the migration can still be set
>> >> >> with the existing commands.
>> >> >>
>> >> >> The order of precedence is:
>> >> >>
>> >> >> 'config' argument > -global cmdline > defaults (migration_properties)
>> >> >
>> >> > Could we still keep the QMP migrate-set-parameters values?
>> >> >
>> >> > 'config' argument > QMP setups using migrate-set-parameters >
>> >> > -global cmdline > defaults (migration_properties)
>> >> >
>> >>
>> >> That's the case. I failed to mention it in the commit message. IOW it
>> >> behaves just like today, but the new 'config' way takes precedence over
>> >> all.
>> >
>> > Referring to below chunk of code:
>> >
>> > [...]
>> >
>> >> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
>> >> >> + Error **errp)
>> >> >> +{
>> >> >> + ERRP_GUARD();
>> >> >> +
>> >> >> + assert(bql_locked());
>> >> >> +
>> >> >> + /* reset to default parameters */
>> >> >> + migrate_params_apply(&s->defaults);
>> >
>> > IIUC here it'll reset all global parameters using the initial defaults
>> > first, then apply the "config" specified in "migrate" QMP command?
>> >
>>
>> Yes, this is so any previously set parameter via migrate-set-parameter
>> gets erased. I think what we want (but feel free to disagree) is to have
>> the migrate-set-parameter _eventually_ only handle parameters that need
>> to be modifed during migration runtime. Anything else can be done via
>> passing config to qmp_migrate.
>>
>> For -global, I don't have a preference. Having -global take precedence
>> over all would require a way to know which options were present in the
>> command-line and which are just the defaults seet in
>> migration_properties. I currently don't know how to do that. If it is at
>> all possible (within reason) we could make the change, no worries.
>>
>> > I think there're actually two separate questions to be asked, to make it
>> > clearer, they are:
>>
>> Here it got ambiguous when you say "global", I've been using -global to
>> refer to the cmdline -global migration.foo, but others have used global
>> to mean s->parameters (which has an extended lifetime). Could you
>> clarify?
>
> I meant the -global, and the global setups via migrate-set-parameters.
>
> As replied to Dan in the other email, I changed my mind on question (1); I
> think it makes sense to have it YES. I left my pure question on (2) there
> too.
>
> Do we really want to disable migrate-set-parameters setting most of the
> parameters, and only allow it to be set during migration on a few things
> like bandwidth or so?
>
Well, if we decide we have reasons to introduce the "config" concept,
then I think we should not present two ways of doing the same
thing. User calls qmp_migrate with its arguments and that's the
migration. No other ways of setting parameters.
Since we do have parameters that are set in "runtime" I though of
keeping migrate-set-parameters around to minimize the interface
change. Maybe those should have been separate knobs on their own after
all... But in any case, we can't reject migrate-set-parameters because
it might happen way earlier than the actual migration command. So I
don't think anything changes regarding the API.
> I just don't really see the major benefit of that yet. I would think it
> make more sense if we don't need to change any parameters in migration,
> then provide that in one shot in QMP migrate "config". Maybe making more
> sense if migration is not heavily thread-based but having its aiocontext so
> we could even move to Jobs.
>
> Now after all we'll need to allow setting something like bandwidth even
> during migration alive, and we have all the things ready allowing to set
> before migration starts, I'm not 100% sure whether we need to bother even
> if it does look cleaner, because we'll still break mgmt used to be working
> for years.. I could be over-cautious on breaking things, but I still want
> to understand better on the benefits.
>
Makes sense. We'd say either use the old way or the new way. If both are
mixed, then the new way takes precedence. That keeps older apps working
and allows new code to transition into the new way.
> One step back, on this "allow migrate to specify 'config'" request: I
> think we can definitely do that as it still provides some kind of
> atomicity. But frankly speaking I never see it a "real problem" - do
> we really have report or use case showing that Libvirt can trigger
> "migrate" with some global settings touched by other apps at all?
>
I don't think other apps is the problem, but libvirt itself maybe
attempting two migrations in sequence after one of them fails.
There always the possibility that the user is poking around, which of
course is not advisable, but if a weird migration bug shows up it's
difficult to confirm that other app/user hasn't changed the parameters.
> To me, it was yet an illutionary problem, I never know the answer of that.
> If Libvirt is still the owner of QEMU instance via the QMP channel, I
> actually don't really see why the atomicity would even help, even though we
> can still provide that as it's pretty easy as something optional; like what
> this patch does without too much hassle.
>
We can provide it, but I'd rather not unless we agree that is the way
forward. We don't need another way of doing the same as existing
commands.
> Then if to move one step further to remove all global settings, we face
> breaking debugging scripts, and breaking of any old libvirt and non-libvirt
> mgmt apps. Frankly I really don't yet know whether it's a good idea. I
> could miss some important reasoning of why we want to do it - it needs to
> be something not relevant to "making the code cleaner", IMHO..
I don't see it as breaking the old stuff. Because any old users would
still be using migrate-set-parameters as usual. So I think your concern
is about calling migrate the new way and also keeping -global
working. As I said, personally I don't mind if put some ifs around to
keep -global working.
Could we add another parameter that says allow-globals (or w/e) and make
everyone happy?
On Mon, Jun 09, 2025 at 03:02:06PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Mon, Jun 09, 2025 at 11:37:02AM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >>
> >> > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >>
> >> >> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
> >> >> >> 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.
> >> >> >>
> >> >> >> The options that are changed during the migration can still be set
> >> >> >> with the existing commands.
> >> >> >>
> >> >> >> The order of precedence is:
> >> >> >>
> >> >> >> 'config' argument > -global cmdline > defaults (migration_properties)
> >> >> >
> >> >> > Could we still keep the QMP migrate-set-parameters values?
> >> >> >
> >> >> > 'config' argument > QMP setups using migrate-set-parameters >
> >> >> > -global cmdline > defaults (migration_properties)
> >> >> >
> >> >>
> >> >> That's the case. I failed to mention it in the commit message. IOW it
> >> >> behaves just like today, but the new 'config' way takes precedence over
> >> >> all.
> >> >
> >> > Referring to below chunk of code:
> >> >
> >> > [...]
> >> >
> >> >> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
> >> >> >> + Error **errp)
> >> >> >> +{
> >> >> >> + ERRP_GUARD();
> >> >> >> +
> >> >> >> + assert(bql_locked());
> >> >> >> +
> >> >> >> + /* reset to default parameters */
> >> >> >> + migrate_params_apply(&s->defaults);
> >> >
> >> > IIUC here it'll reset all global parameters using the initial defaults
> >> > first, then apply the "config" specified in "migrate" QMP command?
> >> >
> >>
> >> Yes, this is so any previously set parameter via migrate-set-parameter
> >> gets erased. I think what we want (but feel free to disagree) is to have
> >> the migrate-set-parameter _eventually_ only handle parameters that need
> >> to be modifed during migration runtime. Anything else can be done via
> >> passing config to qmp_migrate.
> >>
> >> For -global, I don't have a preference. Having -global take precedence
> >> over all would require a way to know which options were present in the
> >> command-line and which are just the defaults seet in
> >> migration_properties. I currently don't know how to do that. If it is at
> >> all possible (within reason) we could make the change, no worries.
> >>
> >> > I think there're actually two separate questions to be asked, to make it
> >> > clearer, they are:
> >>
> >> Here it got ambiguous when you say "global", I've been using -global to
> >> refer to the cmdline -global migration.foo, but others have used global
> >> to mean s->parameters (which has an extended lifetime). Could you
> >> clarify?
> >
> > I meant the -global, and the global setups via migrate-set-parameters.
> >
> > As replied to Dan in the other email, I changed my mind on question (1); I
> > think it makes sense to have it YES. I left my pure question on (2) there
> > too.
> >
> > Do we really want to disable migrate-set-parameters setting most of the
> > parameters, and only allow it to be set during migration on a few things
> > like bandwidth or so?
> >
>
> Well, if we decide we have reasons to introduce the "config" concept,
> then I think we should not present two ways of doing the same
> thing. User calls qmp_migrate with its arguments and that's the
> migration. No other ways of setting parameters.
>
> Since we do have parameters that are set in "runtime" I though of
> keeping migrate-set-parameters around to minimize the interface
> change. Maybe those should have been separate knobs on their own after
> all... But in any case, we can't reject migrate-set-parameters because
> it might happen way earlier than the actual migration command. So I
> don't think anything changes regarding the API.
>
> > I just don't really see the major benefit of that yet. I would think it
> > make more sense if we don't need to change any parameters in migration,
> > then provide that in one shot in QMP migrate "config". Maybe making more
> > sense if migration is not heavily thread-based but having its aiocontext so
> > we could even move to Jobs.
> >
> > Now after all we'll need to allow setting something like bandwidth even
> > during migration alive, and we have all the things ready allowing to set
> > before migration starts, I'm not 100% sure whether we need to bother even
> > if it does look cleaner, because we'll still break mgmt used to be working
> > for years.. I could be over-cautious on breaking things, but I still want
> > to understand better on the benefits.
> >
>
> Makes sense. We'd say either use the old way or the new way. If both are
> mixed, then the new way takes precedence. That keeps older apps working
> and allows new code to transition into the new way.
Fair enough. Yes whenever the new way is chosen it can work in anyway we
define it.
It's just that if the global list of parameters will still be around then
it seems to have no good reason to not build the migration parameters on
top of the global list of parameters. After all, anything can be
overwritten in the QMP migrate if needed.
>
> > One step back, on this "allow migrate to specify 'config'" request: I
> > think we can definitely do that as it still provides some kind of
> > atomicity. But frankly speaking I never see it a "real problem" - do
> > we really have report or use case showing that Libvirt can trigger
> > "migrate" with some global settings touched by other apps at all?
> >
>
> I don't think other apps is the problem, but libvirt itself maybe
> attempting two migrations in sequence after one of them fails.
>
> There always the possibility that the user is poking around, which of
> course is not advisable, but if a weird migration bug shows up it's
> difficult to confirm that other app/user hasn't changed the parameters.
That's almost what I think the current patch is useful on providing some
kind of atomicity.
If we want to make debugging easy, we could also consider returning the
finalized migration setup in the response of QMP "migrate" with all
parameters, by defining "returns" for QMP "migrate".
>
> > To me, it was yet an illutionary problem, I never know the answer of that.
> > If Libvirt is still the owner of QEMU instance via the QMP channel, I
> > actually don't really see why the atomicity would even help, even though we
> > can still provide that as it's pretty easy as something optional; like what
> > this patch does without too much hassle.
> >
>
> We can provide it, but I'd rather not unless we agree that is the way
> forward. We don't need another way of doing the same as existing
> commands.
OK, it might be me that misunderstood the request initially.
>
> > Then if to move one step further to remove all global settings, we face
> > breaking debugging scripts, and breaking of any old libvirt and non-libvirt
> > mgmt apps. Frankly I really don't yet know whether it's a good idea. I
> > could miss some important reasoning of why we want to do it - it needs to
> > be something not relevant to "making the code cleaner", IMHO..
>
> I don't see it as breaking the old stuff. Because any old users would
> still be using migrate-set-parameters as usual. So I think your concern
> is about calling migrate the new way and also keeping -global
> working. As I said, personally I don't mind if put some ifs around to
> keep -global working.
>
> Could we add another parameter that says allow-globals (or w/e) and make
> everyone happy?
That's not needed if it's about making me happy. :) My happiness alone
isn't that important, I can change any of my script, and I'm OK whatever
ABI changes, but as long as the downstream won't be a mess..
If we want to either do nothing or making it a bundle, then we can decide
what's the bundle now.
For example, do we plan to have this, then drop migrate-set-parameters &
capabilities finally (or at least failing non-runtime-modifi-able ones)?
How fast do we want to do this, and how do we manage downstream to not be
affected by having new QEMU's migrate-set-* commands completely gone, would
be the follow up questions..
Maybe I worked much enough on Linux so I pay a lot of attention trying to
think such trade-off, then if I see not much benefit normally I'll try to
not break any ABI. But if that's everyone's wish (except myself.. even if
it only makes the interface better..) then we can discuss before moving on.
--
Peter Xu
Peter Xu <peterx@redhat.com> writes:
> On Mon, Jun 09, 2025 at 03:02:06PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > On Mon, Jun 09, 2025 at 11:37:02AM -0300, Fabiano Rosas wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >>
>> >> > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
>> >> >> Peter Xu <peterx@redhat.com> writes:
>> >> >>
>> >> >> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
>> >> >> >> 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.
>> >> >> >>
>> >> >> >> The options that are changed during the migration can still be set
>> >> >> >> with the existing commands.
>> >> >> >>
>> >> >> >> The order of precedence is:
>> >> >> >>
>> >> >> >> 'config' argument > -global cmdline > defaults (migration_properties)
>> >> >> >
>> >> >> > Could we still keep the QMP migrate-set-parameters values?
>> >> >> >
>> >> >> > 'config' argument > QMP setups using migrate-set-parameters >
>> >> >> > -global cmdline > defaults (migration_properties)
>> >> >> >
>> >> >>
>> >> >> That's the case. I failed to mention it in the commit message. IOW it
>> >> >> behaves just like today, but the new 'config' way takes precedence over
>> >> >> all.
>> >> >
>> >> > Referring to below chunk of code:
>> >> >
>> >> > [...]
>> >> >
>> >> >> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
>> >> >> >> + Error **errp)
>> >> >> >> +{
>> >> >> >> + ERRP_GUARD();
>> >> >> >> +
>> >> >> >> + assert(bql_locked());
>> >> >> >> +
>> >> >> >> + /* reset to default parameters */
>> >> >> >> + migrate_params_apply(&s->defaults);
>> >> >
>> >> > IIUC here it'll reset all global parameters using the initial defaults
>> >> > first, then apply the "config" specified in "migrate" QMP command?
>> >> >
>> >>
>> >> Yes, this is so any previously set parameter via migrate-set-parameter
>> >> gets erased. I think what we want (but feel free to disagree) is to have
>> >> the migrate-set-parameter _eventually_ only handle parameters that need
>> >> to be modifed during migration runtime. Anything else can be done via
>> >> passing config to qmp_migrate.
>> >>
>> >> For -global, I don't have a preference. Having -global take precedence
>> >> over all would require a way to know which options were present in the
>> >> command-line and which are just the defaults seet in
>> >> migration_properties. I currently don't know how to do that. If it is at
>> >> all possible (within reason) we could make the change, no worries.
>> >>
>> >> > I think there're actually two separate questions to be asked, to make it
>> >> > clearer, they are:
>> >>
>> >> Here it got ambiguous when you say "global", I've been using -global to
>> >> refer to the cmdline -global migration.foo, but others have used global
>> >> to mean s->parameters (which has an extended lifetime). Could you
>> >> clarify?
>> >
>> > I meant the -global, and the global setups via migrate-set-parameters.
>> >
>> > As replied to Dan in the other email, I changed my mind on question (1); I
>> > think it makes sense to have it YES. I left my pure question on (2) there
>> > too.
>> >
>> > Do we really want to disable migrate-set-parameters setting most of the
>> > parameters, and only allow it to be set during migration on a few things
>> > like bandwidth or so?
>> >
>>
>> Well, if we decide we have reasons to introduce the "config" concept,
>> then I think we should not present two ways of doing the same
>> thing. User calls qmp_migrate with its arguments and that's the
>> migration. No other ways of setting parameters.
>>
>> Since we do have parameters that are set in "runtime" I though of
>> keeping migrate-set-parameters around to minimize the interface
>> change. Maybe those should have been separate knobs on their own after
>> all... But in any case, we can't reject migrate-set-parameters because
>> it might happen way earlier than the actual migration command. So I
>> don't think anything changes regarding the API.
>>
>> > I just don't really see the major benefit of that yet. I would think it
>> > make more sense if we don't need to change any parameters in migration,
>> > then provide that in one shot in QMP migrate "config". Maybe making more
>> > sense if migration is not heavily thread-based but having its aiocontext so
>> > we could even move to Jobs.
>> >
>> > Now after all we'll need to allow setting something like bandwidth even
>> > during migration alive, and we have all the things ready allowing to set
>> > before migration starts, I'm not 100% sure whether we need to bother even
>> > if it does look cleaner, because we'll still break mgmt used to be working
>> > for years.. I could be over-cautious on breaking things, but I still want
>> > to understand better on the benefits.
>> >
>>
>> Makes sense. We'd say either use the old way or the new way. If both are
>> mixed, then the new way takes precedence. That keeps older apps working
>> and allows new code to transition into the new way.
>
> Fair enough. Yes whenever the new way is chosen it can work in anyway we
> define it.
>
> It's just that if the global list of parameters will still be around then
> it seems to have no good reason to not build the migration parameters on
> top of the global list of parameters. After all, anything can be
> overwritten in the QMP migrate if needed.
>
If we had a way to detect that the user has modified some parameters via
the cmdline, then we could merge that with the s->defaults and restore
it before applying config, that would achieve what you want. I'm in
favor, -global should only be used for debugging, I think it's fine if
we let it go through. But anything set by migrate-set-parameters
definitely needs to be reset. I just need a way to differentiate between
"default parameter" vs. "default parameter that got overwritten by
-global". I'll try to figure something out.
>>
>> > One step back, on this "allow migrate to specify 'config'" request: I
>> > think we can definitely do that as it still provides some kind of
>> > atomicity. But frankly speaking I never see it a "real problem" - do
>> > we really have report or use case showing that Libvirt can trigger
>> > "migrate" with some global settings touched by other apps at all?
>> >
>>
>> I don't think other apps is the problem, but libvirt itself maybe
>> attempting two migrations in sequence after one of them fails.
>>
>> There always the possibility that the user is poking around, which of
>> course is not advisable, but if a weird migration bug shows up it's
>> difficult to confirm that other app/user hasn't changed the parameters.
>
> That's almost what I think the current patch is useful on providing some
> kind of atomicity.
>
> If we want to make debugging easy, we could also consider returning the
> finalized migration setup in the response of QMP "migrate" with all
> parameters, by defining "returns" for QMP "migrate".
>
Isn't query-migrate-parameters that already?
>>
>> > To me, it was yet an illutionary problem, I never know the answer of that.
>> > If Libvirt is still the owner of QEMU instance via the QMP channel, I
>> > actually don't really see why the atomicity would even help, even though we
>> > can still provide that as it's pretty easy as something optional; like what
>> > this patch does without too much hassle.
>> >
>>
>> We can provide it, but I'd rather not unless we agree that is the way
>> forward. We don't need another way of doing the same as existing
>> commands.
>
> OK, it might be me that misunderstood the request initially.
>
>>
>> > Then if to move one step further to remove all global settings, we face
>> > breaking debugging scripts, and breaking of any old libvirt and non-libvirt
>> > mgmt apps. Frankly I really don't yet know whether it's a good idea. I
>> > could miss some important reasoning of why we want to do it - it needs to
>> > be something not relevant to "making the code cleaner", IMHO..
>>
>> I don't see it as breaking the old stuff. Because any old users would
>> still be using migrate-set-parameters as usual. So I think your concern
>> is about calling migrate the new way and also keeping -global
>> working. As I said, personally I don't mind if put some ifs around to
>> keep -global working.
>>
>> Could we add another parameter that says allow-globals (or w/e) and make
>> everyone happy?
>
> That's not needed if it's about making me happy. :) My happiness alone
> isn't that important, I can change any of my script, and I'm OK whatever
> ABI changes, but as long as the downstream won't be a mess..
>
At this point you've probably done more migrations than any single
person. So of course your use-case is important. I don't think an extra
knob is too much to ask. Could even be -global only.
However, if we're going to keep both requirements working: 1) overwrite
migrate-set-params; 2) do not overwrite -global; as I said we need a way
to detect a parameter changed via -global... and then we don't need a
new knob because that would already tell us.
> If we want to either do nothing or making it a bundle, then we can decide
> what's the bundle now.
>
> For example, do we plan to have this, then drop migrate-set-parameters &
> capabilities finally (or at least failing non-runtime-modifi-able ones)?
>
Good question, I don't think we've decided. Those last few patches could
have kept the RFC tag. I say we:
- Merge params+caps and deprecate migrate-set-capabilities now.
One is an internal change and the other is a normal command
deprecation, AFAIK.
- Make sure we agree on how config is going to work and introduce it
then (possibly this release).
We also need to make sure this is the right thing for savevm, cpr,
etc. It would be good to have all of them uniformly prepared to take
(or not) the config parameter.
- Leave migrate-set-parameters as is.
(this wasn't my original intention, but this discussion changed my
mind)
As I said, we can't predict whether the user will call
migrate-set-parameters before calling migrate. So I don't think we can
say:
"Only use this if you're not using the new 'config' option OR if
you're using the new 'config' option plus setting runtime parameters"
It also gives us the ability to say that nothing changes with
migrate-set-parameter except that it now takes caps as well.
> How fast do we want to do this, and how do we manage downstream to not be
> affected by having new QEMU's migrate-set-* commands completely gone, would
> be the follow up questions..
>
- The migrate-set-parameters change is just an addition of options. We
do this anytime a new feature is added and I don't think there are
issues downstream. Are there?
- Dropping migrate-set-capabilities would be covered by the deprecation
period. We shouldn't have to think about it. But let's say we _are_
going to think about it: a libvirt from before the removal of the
command would have trouble with a QEMU from after the
removal. Converting between the two may not be trivial because caps
today take a MigrationCapabilityList which is a more complex data
structure than just key=value as MigrationParameters.
- The 'config' change is supported by keeping migrate-set-parameters
around, so it wouldn't affect anything. Just don't use the new API.
I think that's it? Am I being too simplistic?
> Maybe I worked much enough on Linux so I pay a lot of attention trying to
> think such trade-off, then if I see not much benefit normally I'll try to
> not break any ABI. But if that's everyone's wish (except myself.. even if
> it only makes the interface better..) then we can discuss before
> moving on.
I think we can do this without breaking anything. We could bring more
people into the discussion to double-check. Let's just agree between
ourselves on some of these other details.
I don't discard the possibility of simply dropping it. But I see Daniel
(and Markus on the previous version) making compelling points.
On Mon, Jun 09, 2025 at 04:41:06PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Mon, Jun 09, 2025 at 03:02:06PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >>
> >> > On Mon, Jun 09, 2025 at 11:37:02AM -0300, Fabiano Rosas wrote:
> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >>
> >> >> > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
> >> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >> >>
> >> >> >> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
> >> >> >> >> 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.
> >> >> >> >>
> >> >> >> >> The options that are changed during the migration can still be set
> >> >> >> >> with the existing commands.
> >> >> >> >>
> >> >> >> >> The order of precedence is:
> >> >> >> >>
> >> >> >> >> 'config' argument > -global cmdline > defaults (migration_properties)
> >> >> >> >
> >> >> >> > Could we still keep the QMP migrate-set-parameters values?
> >> >> >> >
> >> >> >> > 'config' argument > QMP setups using migrate-set-parameters >
> >> >> >> > -global cmdline > defaults (migration_properties)
> >> >> >> >
> >> >> >>
> >> >> >> That's the case. I failed to mention it in the commit message. IOW it
> >> >> >> behaves just like today, but the new 'config' way takes precedence over
> >> >> >> all.
> >> >> >
> >> >> > Referring to below chunk of code:
> >> >> >
> >> >> > [...]
> >> >> >
> >> >> >> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
> >> >> >> >> + Error **errp)
> >> >> >> >> +{
> >> >> >> >> + ERRP_GUARD();
> >> >> >> >> +
> >> >> >> >> + assert(bql_locked());
> >> >> >> >> +
> >> >> >> >> + /* reset to default parameters */
> >> >> >> >> + migrate_params_apply(&s->defaults);
> >> >> >
> >> >> > IIUC here it'll reset all global parameters using the initial defaults
> >> >> > first, then apply the "config" specified in "migrate" QMP command?
> >> >> >
> >> >>
> >> >> Yes, this is so any previously set parameter via migrate-set-parameter
> >> >> gets erased. I think what we want (but feel free to disagree) is to have
> >> >> the migrate-set-parameter _eventually_ only handle parameters that need
> >> >> to be modifed during migration runtime. Anything else can be done via
> >> >> passing config to qmp_migrate.
> >> >>
> >> >> For -global, I don't have a preference. Having -global take precedence
> >> >> over all would require a way to know which options were present in the
> >> >> command-line and which are just the defaults seet in
> >> >> migration_properties. I currently don't know how to do that. If it is at
> >> >> all possible (within reason) we could make the change, no worries.
> >> >>
> >> >> > I think there're actually two separate questions to be asked, to make it
> >> >> > clearer, they are:
> >> >>
> >> >> Here it got ambiguous when you say "global", I've been using -global to
> >> >> refer to the cmdline -global migration.foo, but others have used global
> >> >> to mean s->parameters (which has an extended lifetime). Could you
> >> >> clarify?
> >> >
> >> > I meant the -global, and the global setups via migrate-set-parameters.
> >> >
> >> > As replied to Dan in the other email, I changed my mind on question (1); I
> >> > think it makes sense to have it YES. I left my pure question on (2) there
> >> > too.
> >> >
> >> > Do we really want to disable migrate-set-parameters setting most of the
> >> > parameters, and only allow it to be set during migration on a few things
> >> > like bandwidth or so?
> >> >
> >>
> >> Well, if we decide we have reasons to introduce the "config" concept,
> >> then I think we should not present two ways of doing the same
> >> thing. User calls qmp_migrate with its arguments and that's the
> >> migration. No other ways of setting parameters.
> >>
> >> Since we do have parameters that are set in "runtime" I though of
> >> keeping migrate-set-parameters around to minimize the interface
> >> change. Maybe those should have been separate knobs on their own after
> >> all... But in any case, we can't reject migrate-set-parameters because
> >> it might happen way earlier than the actual migration command. So I
> >> don't think anything changes regarding the API.
> >>
> >> > I just don't really see the major benefit of that yet. I would think it
> >> > make more sense if we don't need to change any parameters in migration,
> >> > then provide that in one shot in QMP migrate "config". Maybe making more
> >> > sense if migration is not heavily thread-based but having its aiocontext so
> >> > we could even move to Jobs.
> >> >
> >> > Now after all we'll need to allow setting something like bandwidth even
> >> > during migration alive, and we have all the things ready allowing to set
> >> > before migration starts, I'm not 100% sure whether we need to bother even
> >> > if it does look cleaner, because we'll still break mgmt used to be working
> >> > for years.. I could be over-cautious on breaking things, but I still want
> >> > to understand better on the benefits.
> >> >
> >>
> >> Makes sense. We'd say either use the old way or the new way. If both are
> >> mixed, then the new way takes precedence. That keeps older apps working
> >> and allows new code to transition into the new way.
> >
> > Fair enough. Yes whenever the new way is chosen it can work in anyway we
> > define it.
> >
> > It's just that if the global list of parameters will still be around then
> > it seems to have no good reason to not build the migration parameters on
> > top of the global list of parameters. After all, anything can be
> > overwritten in the QMP migrate if needed.
> >
>
> If we had a way to detect that the user has modified some parameters via
> the cmdline, then we could merge that with the s->defaults and restore
> it before applying config, that would achieve what you want. I'm in
> favor, -global should only be used for debugging, I think it's fine if
> we let it go through. But anything set by migrate-set-parameters
> definitely needs to be reset. I just need a way to differentiate between
> "default parameter" vs. "default parameter that got overwritten by
> -global". I'll try to figure something out.
I think I see what you meant. Ignoring -global is ok. I agree with you
that should be pure debugging, and feel free to keep it like that if you
can't find anything to persist it - it may not justify your time spent if
it grows too much.
>
> >>
> >> > One step back, on this "allow migrate to specify 'config'" request: I
> >> > think we can definitely do that as it still provides some kind of
> >> > atomicity. But frankly speaking I never see it a "real problem" - do
> >> > we really have report or use case showing that Libvirt can trigger
> >> > "migrate" with some global settings touched by other apps at all?
> >> >
> >>
> >> I don't think other apps is the problem, but libvirt itself maybe
> >> attempting two migrations in sequence after one of them fails.
> >>
> >> There always the possibility that the user is poking around, which of
> >> course is not advisable, but if a weird migration bug shows up it's
> >> difficult to confirm that other app/user hasn't changed the parameters.
> >
> > That's almost what I think the current patch is useful on providing some
> > kind of atomicity.
> >
> > If we want to make debugging easy, we could also consider returning the
> > finalized migration setup in the response of QMP "migrate" with all
> > parameters, by defining "returns" for QMP "migrate".
> >
>
> Isn't query-migrate-parameters that already?
The important part is still "atomicity". Consider right after Libvirt
sends a "migrate" command someone quickly cancelled it and invoked another
one using another "config". Yes there will still be events generated to
Libvirt but I think that'll be asynchronous anyway so its arrival might
have been after the other one migrating VM again.
Attach to that to "returns" provides atomicity making sure if Libvirt
invoking a "migrate" command and get the returns, if that succeeded Libvirt
knows the returned setup is 100% the one that is running now. It might be
cancelled too but the finalized setup will match what Libvirt triggers.
Said that, not that I think any of such would ever happen.. but that idea
does match with atomicity provided by QMP "migrate" with "config".
>
> >>
> >> > To me, it was yet an illutionary problem, I never know the answer of that.
> >> > If Libvirt is still the owner of QEMU instance via the QMP channel, I
> >> > actually don't really see why the atomicity would even help, even though we
> >> > can still provide that as it's pretty easy as something optional; like what
> >> > this patch does without too much hassle.
> >> >
> >>
> >> We can provide it, but I'd rather not unless we agree that is the way
> >> forward. We don't need another way of doing the same as existing
> >> commands.
> >
> > OK, it might be me that misunderstood the request initially.
> >
> >>
> >> > Then if to move one step further to remove all global settings, we face
> >> > breaking debugging scripts, and breaking of any old libvirt and non-libvirt
> >> > mgmt apps. Frankly I really don't yet know whether it's a good idea. I
> >> > could miss some important reasoning of why we want to do it - it needs to
> >> > be something not relevant to "making the code cleaner", IMHO..
> >>
> >> I don't see it as breaking the old stuff. Because any old users would
> >> still be using migrate-set-parameters as usual. So I think your concern
> >> is about calling migrate the new way and also keeping -global
> >> working. As I said, personally I don't mind if put some ifs around to
> >> keep -global working.
> >>
> >> Could we add another parameter that says allow-globals (or w/e) and make
> >> everyone happy?
> >
> > That's not needed if it's about making me happy. :) My happiness alone
> > isn't that important, I can change any of my script, and I'm OK whatever
> > ABI changes, but as long as the downstream won't be a mess..
> >
>
> At this point you've probably done more migrations than any single
> person. So of course your use-case is important. I don't think an extra
> knob is too much to ask. Could even be -global only.
>
> However, if we're going to keep both requirements working: 1) overwrite
> migrate-set-params; 2) do not overwrite -global; as I said we need a way
> to detect a parameter changed via -global... and then we don't need a
> new knob because that would already tell us.
Let's not bother; I'm totally OK ignoring -global. When one's testing
manually, one won't be using "config" in "migrate" normally so it's fine.
>
> > If we want to either do nothing or making it a bundle, then we can decide
> > what's the bundle now.
> >
> > For example, do we plan to have this, then drop migrate-set-parameters &
> > capabilities finally (or at least failing non-runtime-modifi-able ones)?
> >
>
> Good question, I don't think we've decided. Those last few patches could
> have kept the RFC tag. I say we:
>
> - Merge params+caps and deprecate migrate-set-capabilities now.
>
> One is an internal change and the other is a normal command
> deprecation, AFAIK.
Agree. Maybe we could still keep the interface for more than two releases.
Maybe we don't need to rush removing the support, and keep it deprecate for
long enough, until we figure out when it's safe.
>
> - Make sure we agree on how config is going to work and introduce it
> then (possibly this release).
>
> We also need to make sure this is the right thing for savevm, cpr,
> etc. It would be good to have all of them uniformly prepared to take
> (or not) the config parameter.
CPR is still live migration, I hope we can reach consensus. It uses
exactly the caps/params it needs, but it's migrating the same as live
migration would do, except it migrates some more fds.
It could be more special if it was based on fork(), there're tons of
uncertainties over fork() with a multi-threaded app, but now we're going
scm rights, much better I'd say. Same reason, it is still live migration
when it's using generic unix sockets.
savevm - we don't plan to yet support any migration cap/param on it, right?
I remember the other use case for enabling mapped-ram, but per my memory
there is much better way to go for that use case rather than building it on
top of savevm, so I'd still think savevm doesn't need any extension, and it
should take zero parameters even in the near future.
What it can do is reset all parameters right before start, then "recover"
the parameters right after, taking BQL for the whole process. Logically if
we know it's a new libvirt we don't even need to bother on the "recover"
part, but we may still want to consider the old libvirts too as long as
there's compat concerns.
>
> - Leave migrate-set-parameters as is.
> (this wasn't my original intention, but this discussion changed my
> mind)
>
> As I said, we can't predict whether the user will call
> migrate-set-parameters before calling migrate. So I don't think we can
> say:
>
> "Only use this if you're not using the new 'config' option OR if
> you're using the new 'config' option plus setting runtime parameters"
>
> It also gives us the ability to say that nothing changes with
> migrate-set-parameter except that it now takes caps as well.
Yep. For this one keeping it as-is is simpler. We can wait for Dan/Markus
and others to chime in when there's other opinions.
>
> > How fast do we want to do this, and how do we manage downstream to not be
> > affected by having new QEMU's migrate-set-* commands completely gone, would
> > be the follow up questions..
> >
>
> - The migrate-set-parameters change is just an addition of options. We
> do this anytime a new feature is added and I don't think there are
> issues downstream. Are there?
Adding caps into it is all fine; I don't see anything would break.
>
> - Dropping migrate-set-capabilities would be covered by the deprecation
> period. We shouldn't have to think about it. But let's say we _are_
> going to think about it: a libvirt from before the removal of the
> command would have trouble with a QEMU from after the
> removal. Converting between the two may not be trivial because caps
> today take a MigrationCapabilityList which is a more complex data
> structure than just key=value as MigrationParameters.
Yes, this is discussed above as well. The worst case is we can keep the
deprecation for as long as how we deprecate machine types. Logically
that was defined partly as "max major release cycle on guaranteed ABI",
then we may assume whatever to be rebased on the current release in any
downstream would have new libvirt ready. Again, we can discuss this later,
marking deprecation can be done first.
>
> - The 'config' change is supported by keeping migrate-set-parameters
> around, so it wouldn't affect anything. Just don't use the new API.
>
> I think that's it? Am I being too simplistic?
Nop; so far so good to me.
>
> > Maybe I worked much enough on Linux so I pay a lot of attention trying to
> > think such trade-off, then if I see not much benefit normally I'll try to
> > not break any ABI. But if that's everyone's wish (except myself.. even if
> > it only makes the interface better..) then we can discuss before
> > moving on.
>
> I think we can do this without breaking anything. We could bring more
> people into the discussion to double-check. Let's just agree between
> ourselves on some of these other details.
>
> I don't discard the possibility of simply dropping it. But I see Daniel
> (and Markus on the previous version) making compelling points.
Yeah I was absent for quite a while, and I may have missed some points. We
can wait for some more inputs.
--
Peter Xu
Peter Xu <peterx@redhat.com> writes:
> On Mon, Jun 09, 2025 at 04:41:06PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > On Mon, Jun 09, 2025 at 03:02:06PM -0300, Fabiano Rosas wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >>
>> >> > On Mon, Jun 09, 2025 at 11:37:02AM -0300, Fabiano Rosas wrote:
>> >> >> Peter Xu <peterx@redhat.com> writes:
>> >> >>
>> >> >> > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
>> >> >> >> Peter Xu <peterx@redhat.com> writes:
>> >> >> >>
>> >> >> >> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
>> >> >> >> >> 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.
>> >> >> >> >>
>> >> >> >> >> The options that are changed during the migration can still be set
>> >> >> >> >> with the existing commands.
>> >> >> >> >>
>> >> >> >> >> The order of precedence is:
>> >> >> >> >>
>> >> >> >> >> 'config' argument > -global cmdline > defaults (migration_properties)
>> >> >> >> >
>> >> >> >> > Could we still keep the QMP migrate-set-parameters values?
>> >> >> >> >
>> >> >> >> > 'config' argument > QMP setups using migrate-set-parameters >
>> >> >> >> > -global cmdline > defaults (migration_properties)
>> >> >> >> >
>> >> >> >>
>> >> >> >> That's the case. I failed to mention it in the commit message. IOW it
>> >> >> >> behaves just like today, but the new 'config' way takes precedence over
>> >> >> >> all.
>> >> >> >
>> >> >> > Referring to below chunk of code:
>> >> >> >
>> >> >> > [...]
>> >> >> >
>> >> >> >> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
>> >> >> >> >> + Error **errp)
>> >> >> >> >> +{
>> >> >> >> >> + ERRP_GUARD();
>> >> >> >> >> +
>> >> >> >> >> + assert(bql_locked());
>> >> >> >> >> +
>> >> >> >> >> + /* reset to default parameters */
>> >> >> >> >> + migrate_params_apply(&s->defaults);
>> >> >> >
>> >> >> > IIUC here it'll reset all global parameters using the initial defaults
>> >> >> > first, then apply the "config" specified in "migrate" QMP command?
>> >> >> >
>> >> >>
>> >> >> Yes, this is so any previously set parameter via migrate-set-parameter
>> >> >> gets erased. I think what we want (but feel free to disagree) is to have
>> >> >> the migrate-set-parameter _eventually_ only handle parameters that need
>> >> >> to be modifed during migration runtime. Anything else can be done via
>> >> >> passing config to qmp_migrate.
>> >> >>
>> >> >> For -global, I don't have a preference. Having -global take precedence
>> >> >> over all would require a way to know which options were present in the
>> >> >> command-line and which are just the defaults seet in
>> >> >> migration_properties. I currently don't know how to do that. If it is at
>> >> >> all possible (within reason) we could make the change, no worries.
>> >> >>
>> >> >> > I think there're actually two separate questions to be asked, to make it
>> >> >> > clearer, they are:
>> >> >>
>> >> >> Here it got ambiguous when you say "global", I've been using -global to
>> >> >> refer to the cmdline -global migration.foo, but others have used global
>> >> >> to mean s->parameters (which has an extended lifetime). Could you
>> >> >> clarify?
>> >> >
>> >> > I meant the -global, and the global setups via migrate-set-parameters.
>> >> >
>> >> > As replied to Dan in the other email, I changed my mind on question (1); I
>> >> > think it makes sense to have it YES. I left my pure question on (2) there
>> >> > too.
>> >> >
>> >> > Do we really want to disable migrate-set-parameters setting most of the
>> >> > parameters, and only allow it to be set during migration on a few things
>> >> > like bandwidth or so?
>> >> >
>> >>
>> >> Well, if we decide we have reasons to introduce the "config" concept,
>> >> then I think we should not present two ways of doing the same
>> >> thing. User calls qmp_migrate with its arguments and that's the
>> >> migration. No other ways of setting parameters.
>> >>
>> >> Since we do have parameters that are set in "runtime" I though of
>> >> keeping migrate-set-parameters around to minimize the interface
>> >> change. Maybe those should have been separate knobs on their own after
>> >> all... But in any case, we can't reject migrate-set-parameters because
>> >> it might happen way earlier than the actual migration command. So I
>> >> don't think anything changes regarding the API.
>> >>
>> >> > I just don't really see the major benefit of that yet. I would think it
>> >> > make more sense if we don't need to change any parameters in migration,
>> >> > then provide that in one shot in QMP migrate "config". Maybe making more
>> >> > sense if migration is not heavily thread-based but having its aiocontext so
>> >> > we could even move to Jobs.
>> >> >
>> >> > Now after all we'll need to allow setting something like bandwidth even
>> >> > during migration alive, and we have all the things ready allowing to set
>> >> > before migration starts, I'm not 100% sure whether we need to bother even
>> >> > if it does look cleaner, because we'll still break mgmt used to be working
>> >> > for years.. I could be over-cautious on breaking things, but I still want
>> >> > to understand better on the benefits.
>> >> >
>> >>
>> >> Makes sense. We'd say either use the old way or the new way. If both are
>> >> mixed, then the new way takes precedence. That keeps older apps working
>> >> and allows new code to transition into the new way.
>> >
>> > Fair enough. Yes whenever the new way is chosen it can work in anyway we
>> > define it.
>> >
>> > It's just that if the global list of parameters will still be around then
>> > it seems to have no good reason to not build the migration parameters on
>> > top of the global list of parameters. After all, anything can be
>> > overwritten in the QMP migrate if needed.
>> >
>>
>> If we had a way to detect that the user has modified some parameters via
>> the cmdline, then we could merge that with the s->defaults and restore
>> it before applying config, that would achieve what you want. I'm in
>> favor, -global should only be used for debugging, I think it's fine if
>> we let it go through. But anything set by migrate-set-parameters
>> definitely needs to be reset. I just need a way to differentiate between
>> "default parameter" vs. "default parameter that got overwritten by
>> -global". I'll try to figure something out.
>
> I think I see what you meant. Ignoring -global is ok. I agree with you
> that should be pure debugging, and feel free to keep it like that if you
> can't find anything to persist it - it may not justify your time spent if
> it grows too much.
>
I think I caused some confusion here. I wrote migrate_params_override()
last thing on a friday and forgot it did the right thing from the
beginning:
migrate_params_apply(&s->defaults);
qmp_migrate_set_parameters(new, errp);
This s->defaults is poorly named and is actualy already the merge of
defaults + globals, because qdev does it for us. migrate_params_apply()
will then copy that to s->parameters and qmp_migrate_set_parameters()
will apply the 'new' params from 'config' on top s->parameters. An
example:
Setting multifd-channels (default 2) using various methods and querying
both QMP and HMP:
a) global overrides default:
$ ./qemu-system-x86_64 -global migration.multifd-channels=4 ...
=> QMP: "multifd-channels": 4, HMP: multifd-channels: 4
b) migrate-set-parameter overrides global:
{ 'execute': 'migrate-set-parameters', 'arguments': { 'multifd-channels': 8 } }
=> QMP: "multifd-channels": 8, HMP: multifd-channels: 8
c) config not touching the parameter, value is reset to global:
{ 'execute': 'migrate', 'arguments': { ..., 'config': { 'multifd': true } } }
=> QMP: "multifd-channels": 4, HMP: multifd-channels: 4
d) config overrides all:
{ 'execute': 'migrate', 'arguments': { ..., 'config': {'multifd-channels': 16 } } }
=> QMP: "multifd-channels": 16, HMP: multifd-channels: 16
Without global:
e) default is set initially
$ ./qemu-system-x86_64 ...
=> QMP: "multifd-channels": 2, HMP: multifd-channels: 2
f) migrate-set-parameter overrides default:
{ 'execute': 'migrate-set-parameters', 'arguments': { 'multifd-channels': 8 } }
=> QMP: "multifd-channels": 8, HMP: multifd-channels: 8
g) config not touching the parameter, value is reset to default:
{ 'execute': 'migrate', 'arguments': { ..., 'config': { 'multifd': true } } }
=> "multifd-channels": 2, HMP: multifd-channels: 2
h) config overrides all:
{ 'execute': 'migrate', 'arguments': { ..., 'config': {'multifd-channels': 16 } } }
=> QMP: "multifd-channels": 16, HMP: multifd-channels: 16
I'll update the variable names and code comments to be more
precise. Sorry for the noise.
On Tue, Jun 10, 2025 at 05:55:31PM -0300, Fabiano Rosas wrote:
> I think I caused some confusion here. I wrote migrate_params_override()
> last thing on a friday and forgot it did the right thing from the
> beginning:
>
> migrate_params_apply(&s->defaults);
> qmp_migrate_set_parameters(new, errp);
>
> This s->defaults is poorly named and is actualy already the merge of
> defaults + globals, because qdev does it for us. migrate_params_apply()
Ha! I didn't remember this part of details when reading, but then I
followed with that idea it won't apply to &defaults.
> will then copy that to s->parameters and qmp_migrate_set_parameters()
> will apply the 'new' params from 'config' on top s->parameters. An
> example:
>
> Setting multifd-channels (default 2) using various methods and querying
> both QMP and HMP:
>
> a) global overrides default:
>
> $ ./qemu-system-x86_64 -global migration.multifd-channels=4 ...
> => QMP: "multifd-channels": 4, HMP: multifd-channels: 4
>
> b) migrate-set-parameter overrides global:
>
> { 'execute': 'migrate-set-parameters', 'arguments': { 'multifd-channels': 8 } }
> => QMP: "multifd-channels": 8, HMP: multifd-channels: 8
>
> c) config not touching the parameter, value is reset to global:
>
> { 'execute': 'migrate', 'arguments': { ..., 'config': { 'multifd': true } } }
> => QMP: "multifd-channels": 4, HMP: multifd-channels: 4
>
> d) config overrides all:
>
> { 'execute': 'migrate', 'arguments': { ..., 'config': {'multifd-channels': 16 } } }
> => QMP: "multifd-channels": 16, HMP: multifd-channels: 16
>
> Without global:
>
> e) default is set initially
>
> $ ./qemu-system-x86_64 ...
> => QMP: "multifd-channels": 2, HMP: multifd-channels: 2
>
> f) migrate-set-parameter overrides default:
>
> { 'execute': 'migrate-set-parameters', 'arguments': { 'multifd-channels': 8 } }
> => QMP: "multifd-channels": 8, HMP: multifd-channels: 8
>
> g) config not touching the parameter, value is reset to default:
>
> { 'execute': 'migrate', 'arguments': { ..., 'config': { 'multifd': true } } }
> => "multifd-channels": 2, HMP: multifd-channels: 2
>
> h) config overrides all:
>
> { 'execute': 'migrate', 'arguments': { ..., 'config': {'multifd-channels': 16 } } }
> => QMP: "multifd-channels": 16, HMP: multifd-channels: 16
>
> I'll update the variable names and code comments to be more
> precise. Sorry for the noise.
Good to know it's even working. Thanks for digging it.
--
Peter Xu
On Mon, Jun 09, 2025 at 11:51:17AM -0400, Peter Xu wrote:
> On Mon, Jun 09, 2025 at 11:37:02AM -0300, Fabiano Rosas wrote:
> > Peter Xu <peterx@redhat.com> writes:
> >
> > > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
> > >> Peter Xu <peterx@redhat.com> writes:
> > >>
> > >> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
> > >> >> 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.
> > >> >>
> > >> >> The options that are changed during the migration can still be set
> > >> >> with the existing commands.
> > >> >>
> > >> >> The order of precedence is:
> > >> >>
> > >> >> 'config' argument > -global cmdline > defaults (migration_properties)
> > >> >
> > >> > Could we still keep the QMP migrate-set-parameters values?
> > >> >
> > >> > 'config' argument > QMP setups using migrate-set-parameters >
> > >> > -global cmdline > defaults (migration_properties)
> > >> >
> > >>
> > >> That's the case. I failed to mention it in the commit message. IOW it
> > >> behaves just like today, but the new 'config' way takes precedence over
> > >> all.
> > >
> > > Referring to below chunk of code:
> > >
> > > [...]
> > >
> > >> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
> > >> >> + Error **errp)
> > >> >> +{
> > >> >> + ERRP_GUARD();
> > >> >> +
> > >> >> + assert(bql_locked());
> > >> >> +
> > >> >> + /* reset to default parameters */
> > >> >> + migrate_params_apply(&s->defaults);
> > >
> > > IIUC here it'll reset all global parameters using the initial defaults
> > > first, then apply the "config" specified in "migrate" QMP command?
> > >
> >
> > Yes, this is so any previously set parameter via migrate-set-parameter
> > gets erased. I think what we want (but feel free to disagree) is to have
> > the migrate-set-parameter _eventually_ only handle parameters that need
> > to be modifed during migration runtime. Anything else can be done via
> > passing config to qmp_migrate.
> >
> > For -global, I don't have a preference. Having -global take precedence
> > over all would require a way to know which options were present in the
> > command-line and which are just the defaults seet in
> > migration_properties. I currently don't know how to do that. If it is at
> > all possible (within reason) we could make the change, no worries.
> >
> > > I think there're actually two separate questions to be asked, to make it
> > > clearer, they are:
> >
> > Here it got ambiguous when you say "global", I've been using -global to
> > refer to the cmdline -global migration.foo, but others have used global
> > to mean s->parameters (which has an extended lifetime). Could you
> > clarify?
>
> I meant the -global, and the global setups via migrate-set-parameters.
>
> As replied to Dan in the other email, I changed my mind on question (1); I
> think it makes sense to have it YES. I left my pure question on (2) there
> too.
>
> Do we really want to disable migrate-set-parameters setting most of the
> parameters, and only allow it to be set during migration on a few things
> like bandwidth or so?
Yes, that's the whole point of the exercise IMHO.
> I just don't really see the major benefit of that yet. I would think it
> make more sense if we don't need to change any parameters in migration,
> then provide that in one shot in QMP migrate "config". Maybe making more
> sense if migration is not heavily thread-based but having its aiocontext so
> we could even move to Jobs.
The benefit is that it brings 'migrate' into line with all other
QMP commands, such that the data provided with the command is
precisely what controls behaviour, giving predictable behaviour.
> One step back, on this "allow migrate to specify 'config'" request: I think
> we can definitely do that as it still provides some kind of atomicity. But
> frankly speaking I never see it a "real problem" - do we really have report
> or use case showing that Libvirt can trigger "migrate" with some global
> settings touched by other apps at all?
atomicity is only one of the goals - being free from side effects
of externally set global state is the more important aspect.
> To me, it was yet an illutionary problem, I never know the answer of that.
> If Libvirt is still the owner of QEMU instance via the QMP channel, I
> actually don't really see why the atomicity would even help, even though we
> can still provide that as it's pretty easy as something optional; like what
> this patch does without too much hassle.
Even if only a single mgmt app is involved this is still beneficial
because the migration infrastructure is used for distinct use cases
inside QEMU - live migration, CPR, save/restore, and savevm/loadvm.
Any time code any one of those uses cases starts using a new parameter,
apps have to make sure they don't inadvertantly have its effects apply
to the other use cases.
> Then if to move one step further to remove all global settings, we face
> breaking debugging scripts, and breaking of any old libvirt and non-libvirt
> mgmt apps. Frankly I really don't yet know whether it's a good idea. I
> could miss some important reasoning of why we want to do it - it needs to
> be something not relevant to "making the code cleaner", IMHO..
Again, it makes the behaviour predictable as the QMP command fully expresses
what action is going to be peformed, free with side effets of any previously
set state.
With 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 :|
(I had a reply in the other thread, that might have covered most of the points but maybe not this one..) On Mon, Jun 09, 2025 at 05:13:00PM +0100, Daniel P. Berrangé wrote: > Even if only a single mgmt app is involved this is still beneficial > because the migration infrastructure is used for distinct use cases > inside QEMU - live migration, CPR, save/restore, and savevm/loadvm. I assume CPR is save/restore, so indeed we have 3 ways to use migration core. > Any time code any one of those uses cases starts using a new parameter, > apps have to make sure they don't inadvertantly have its effects apply > to the other use cases. AFAICT, that's not affected by "whether we allow global settings", that is still a concern internally as long as they use migration core. One thing to mention is CPR is really a fine citizen here, AFAICT it is exactly live migration using all the proper caps/params. We _could_ split it as many things do not apply like postcopy, but we could still just reuse everything and ignoring the rest. It'll be again a cleaness issue to me, and even if CPR reuses everything it looks still clean enough, especially comparing to savevm/loadvm. savevm/loadvm is another story.. however afaiu if we want to decouple it, it should be done not from the interface level, but internally first. E.g., we should allow taking parameters as a temp pointer passing to migration core, so that will be passed over by savevm setting all caps off, for example, ignoring the global config. The interface alone should, IMHO, be done only later. Meanwhile, even if that, IMO we can't avoid the need to think any new param affecting savevm, as long as it's still using migration core. I don't know whether we need to do that one step even further to decouple savevm: I would think the other way round to obsolete savevm completely if necessary when we have fine "file:" migrations now, especially with mapped-ram. Thanks, -- Peter Xu
Peter Xu <peterx@redhat.com> writes: > (I had a reply in the other thread, that might have covered most of the > points but maybe not this one..) > > On Mon, Jun 09, 2025 at 05:13:00PM +0100, Daniel P. Berrangé wrote: >> Even if only a single mgmt app is involved this is still beneficial >> because the migration infrastructure is used for distinct use cases >> inside QEMU - live migration, CPR, save/restore, and savevm/loadvm. > > I assume CPR is save/restore, so indeed we have 3 ways to use migration > core. > >> Any time code any one of those uses cases starts using a new parameter, >> apps have to make sure they don't inadvertantly have its effects apply >> to the other use cases. > > AFAICT, that's not affected by "whether we allow global settings", that is > still a concern internally as long as they use migration core. > > One thing to mention is CPR is really a fine citizen here, AFAICT it is > exactly live migration using all the proper caps/params. We _could_ split > it as many things do not apply like postcopy, but we could still just reuse > everything and ignoring the rest. It'll be again a cleaness issue to me, > and even if CPR reuses everything it looks still clean enough, especially > comparing to savevm/loadvm. > > savevm/loadvm is another story.. however afaiu if we want to decouple it, > it should be done not from the interface level, but internally first. > E.g., we should allow taking parameters as a temp pointer passing to > migration core, so that will be passed over by savevm setting all caps off, > for example, ignoring the global config. The interface alone should, IMHO, > be done only later. > This is simple to do, just reset all of s->parameters to (the new) s->defaults. We never decided if any migration parameters do make sense to use with savevm. If some of them does or is added later, then snapshot_save would gain a "config" argument. > Meanwhile, even if that, IMO we can't avoid the need to think any new param > affecting savevm, as long as it's still using migration core. I don't know > whether we need to do that one step even further to decouple savevm: I > would think the other way round to obsolete savevm completely if necessary > when we have fine "file:" migrations now, especially with mapped-ram. savevm is a weird case. It supports a wider range of setups than regular migration. I don't know what to make of this. I would also like to make it "just migration" but it will need a bunch of special-casing. Anyway, we can discuss, but that's definitely for another day. > > Thanks,
On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
> 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.
>
> The options that are changed during the migration can still be set
> with the existing commands.
>
> The order of precedence is:
>
> 'config' argument > -global cmdline > defaults (migration_properties)
>
> I.e. the config takes precedence over all, values not present in the
> config assume the default values. The (debug) -global command line
> option allows the defaults to be overridden.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> migration/migration-hmp-cmds.c | 5 +++--
> migration/migration.c | 29 ++++++++++++++++++++++++++---
> migration/migration.h | 1 +
> migration/options.c | 30 ++++++++++++++++++++++++++++++
> migration/options.h | 3 +++
> qapi/migration.json | 25 +++++++++++++++++++++++--
> system/vl.c | 3 ++-
> 7 files changed, 88 insertions(+), 8 deletions(-)
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 7282e4b9eb..64a92d8d28 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1474,9 +1474,16 @@
> #
> # @resume: resume one paused migration, default "off". (since 3.0)
> #
> +# @config: migration configuration options, previously set via
> +# @migrate-set-parameters and @migrate-set-capabilities. (since
> +# 10.1)
> +#
> # Features:
> #
> # @deprecated: Argument @detach is deprecated.
> +# @config: Indicates this command can receive the entire migration
> +# configuration via the @config field, dispensing the use of
> +# @migrate-set-parameters.
There is no need to add this feature - mgmt apps can identify
this simply by the fact that the 'config' parameter now exists
in the QAPI schema.
> #
> # Since: 0.14
> #
> @@ -1538,7 +1545,9 @@
> 'data': {'*uri': 'str',
> '*channels': [ 'MigrationChannel' ],
> '*detach': { 'type': 'bool', 'features': [ 'deprecated' ] },
> - '*resume': 'bool' } }
> + '*config': 'MigrationParameters',
> + '*resume': 'bool' },
> + 'features': [ 'config' ] }
>
> ##
> # @migrate-incoming:
> @@ -1557,6 +1566,16 @@
> # error details could be retrieved with query-migrate.
> # (since 9.1)
> #
> +# @config: migration configuration options, previously set via
> +# @migrate-set-parameters and @migrate-set-capabilities. (since
> +# 10.1)
> +#
> +# Features:
> +#
> +# @config: Indicates this command can receive the entire migration
> +# configuration via the @config field, dispensing the use of
> +# @migrate-set-parameters.
Likewise redundant.
> +#
> # Since: 2.3
> #
> # .. admonition:: Notes
> @@ -1610,7 +1629,9 @@
> { 'command': 'migrate-incoming',
> 'data': {'*uri': 'str',
> '*channels': [ 'MigrationChannel' ],
> - '*exit-on-error': 'bool' } }
> + '*config': 'MigrationParameters',
> + '*exit-on-error': 'bool' },
> + 'features': [ 'config' ] }
>
> ##
> # @xen-save-devices-state:
With 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 :|
© 2016 - 2025 Red Hat, Inc.