Re: [PATCH v1 1/8] migration: Implemented new parameter stream_content

[Vladimir Sementsov-Ogievskiy]

23.03.2022 13:53, Nikita Lapshin wrote:
> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>

You'd better change author of commit, to keep signed-off-by email and author 
email to be the same thing.

> This new optional parameter contains inormation about migration
> stream parts to be sent (such as RAM, block, bitmap). This looks
> better than using capabilities to solve problem of dividing
> migration stream.
>
> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>   migration/migration.c | 47 ++++++++++++++++++++++++++++++++++++++++++-
>   migration/migration.h |  2 ++
>   qapi/migration.json   | 21 ++++++++++++++++---
>   3 files changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 695f0f2900..4adcc87d1d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1334,6 +1334,12 @@ void 
> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>       }
>   }
>   
> +static bool check_stream_parts(strList *stream_content_list)
> +{
> +    /* To be implemented in ext commits */
> +    return true;
> +}
> +
>   /*
>    * Check whether the parameters are valid. Error will be put into errp
>    * (if provided). Return true if valid, otherwise false.
> @@ -1482,7 +1488,12 @@ static bool migrate_params_check(MigrationParameters 
> *params, Error **errp)
>           return false;
>       }
>   
> -    return true;
> +    if (params->has_stream_content_list &&
> +        !check_stream_parts(params->stream_content_list)) {
> +        error_setg(errp, "Invalid parts of stream given for stream-content");
> +    }
> +
> +   return true;
>   }
>   
>   static void migrate_params_test_apply(MigrateSetParameters *params,
> @@ -1581,6 +1592,11 @@ static void 
> migrate_params_test_apply(MigrateSetParameters *params,
>           dest->has_block_bitmap_mapping = true;
>           dest->block_bitmap_mapping = params->block_bitmap_mapping;
>       }
> +
> +    if (params->has_stream_content_list) {
> +        dest->has_stream_content_list = true;
> +        dest->stream_content_list = params->stream_content_list;
> +    }
>   }
>   
>   static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> @@ -1703,6 +1719,13 @@ static void migrate_params_apply(MigrateSetParameters 
> *params, Error **errp)
>               QAPI_CLONE(BitmapMigrationNodeAliasList,
>                          params->block_bitmap_mapping);
>       }
> +
> +    if (params->has_stream_content_list) {
> +        qapi_free_strList(s->parameters.stream_content_list);
> +        s->parameters.has_stream_content_list = true;
> +        s->parameters.stream_content_list =
> +            QAPI_CLONE(strList, params->stream_content_list);
> +    }
>   }
>   
>   void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> @@ -2620,6 +2643,28 @@ bool migrate_background_snapshot(void)
>       return s->enabled_capabilities[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT];
>   }
>   
> +/* Checks if stream-content parameter has section_name in list */
> +bool migrate_find_stream_content(const char *section_name)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    if (!s->parameters.has_stream_content_list) {
> +        return false;
> +    }
> +
> +    strList *list = s->parameters.stream_content_list;
> +
> +    for (; list; list = list->next) {
> +        if (!strcmp(list->value, section_name)) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>   /* migration thread support */
>   /*
>    * Something bad happened to the RP stream, mark an error
> diff --git a/migration/migration.h b/migration/migration.h
> index 2de861df01..411c58e919 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -396,6 +396,8 @@ bool migrate_use_events(void);
>   bool migrate_postcopy_blocktime(void);
>   bool migrate_background_snapshot(void);
>   
> +bool migrate_find_stream_content(const char *section_name);
> +
>   /* Sending on the return path - generic and then for each message type */
>   void migrate_send_rp_shut(MigrationIncomingState *mis,
>                             uint32_t value);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 18e2610e88..80acf6dbc3 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -760,6 +760,12 @@
>   #                        block device name if there is one, and to their node 
> name
>   #                        otherwise. (Since 5.2)
>   #
> +# @stream-content-list: Parameter control content of migration stream such as 
> RAM,
> +#                       vmstate, block and dirty-bitmaps. This is optional 
> parameter
> +#                       so migration will work correctly without it.
> +#                       This parameter takes string list as description of 
> content
> +#                       and include that part of migration stream. (Since 7.0)

Should be 7.1.

I also think that simpler "stream-content" name would be enough

> +#
>   # Features:
>   # @unstable: Member @x-checkpoint-delay is experimental.
>   #
> @@ -780,7 +786,8 @@
>              'xbzrle-cache-size', 'max-postcopy-bandwidth',
>              'max-cpu-throttle', 'multifd-compression',
>              'multifd-zlib-level' ,'multifd-zstd-level',
> -           'block-bitmap-mapping' ] }
> +           'block-bitmap-mapping',
> +           'stream-content-list' ] }
>   
>   ##
>   # @MigrateSetParameters:
> @@ -925,6 +932,12 @@
>   #                        block device name if there is one, and to their node 
> name
>   #                        otherwise. (Since 5.2)
>   #
> +# @stream-content-list: Parameter control content of migration stream such as 
> RAM,
> +#                       vmstate, block and dirty-bitmaps. This is optional 
> parameter
> +#                       so migration will work correctly without it.
> +#                       This parameter takes string list as description of 
> content
> +#                       and include that part of migration stream. (Since 7.0)
> +#
>   # Features:
>   # @unstable: Member @x-checkpoint-delay is experimental.
>   #
> @@ -960,7 +973,8 @@
>               '*multifd-compression': 'MultiFDCompression',
>               '*multifd-zlib-level': 'uint8',
>               '*multifd-zstd-level': 'uint8',
> -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> +            '*stream-content-list': [ 'str' ] } }

Don't make it a list of strings. Better define enum, like

{ 'enum': 'StreamContentType',
  'data': [ 'block-dirty-bitmaps', 'ram', .. ] }

And make stream-content to be list of this new enum type

>   
>   ##
>   # @migrate-set-parameters:
> @@ -1158,7 +1172,8 @@
>               '*multifd-compression': 'MultiFDCompression',
>               '*multifd-zlib-level': 'uint8',
>               '*multifd-zstd-level': 'uint8',
> -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> +            '*stream-content-list': [ 'str' ] } }
>   
>   ##
>   # @query-migrate-parameters:


That's not a real problem, but IMHO it's better to add QAPI interface together 
with the feature itself (although some functions may be implemented in earlier 
patches). Otherwise, starting from this commit we do have a new documented QAPI 
interface, but it actually doesn't work until some future commit.


--
Best regards,
Vladimir