Re: [RFC 6/6] migration: Deprecated old compression method

[Daniel P . Berrangé]

On Mon, Jun 12, 2023 at 09:33:44PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  docs/about/deprecated.rst |  8 ++++
>  qapi/migration.json       | 92 ++++++++++++++++++++++++---------------
>  migration/options.c       | 13 ++++++
>  3 files changed, 79 insertions(+), 34 deletions(-)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 173c5ba5cb..fe7f2bbde8 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -460,3 +460,11 @@ block migration (since 8.1)
>  Block migration is too inflexible.  It needs to migrate all block
>  devices or none.  Use driver_mirror+NBD instead.
>  
> +old compression method (since 8.1)
> +''''''''''''''''''''''''''''''''''
> +
> +Compression method fails too much.  Too many races.  We are going to
> +remove it if nobody fixes it.  For starters, migration-test
> +compression tests are disabled becase they hand randomly.  If you need
> +compression, use multifd compression methods.
> +
> diff --git a/qapi/migration.json b/qapi/migration.json
> index a8497de48d..40a8b5d124 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -244,6 +244,7 @@
>  #
>  # @compression: migration compression statistics, only returned if
>  #     compression feature is on and status is 'active' or 'completed'
> +#     It is obsolete and deprecated.  Use multifd compression methods.
>  #     (Since 3.1)

This doesn't give users an indication /why/ we're saying this. Instead
I'd suggest

  This feature is unreliable and not tested. It is recommended to
  use multifd migration instead, which offers an alternative reliable
  and tested compression implementation.
  

>  #
>  # @socket-address: Only used for tcp, to know what the real port is
> @@ -261,7 +262,8 @@
>  # Features:
>  #
>  # @deprecated: @disk migration is deprecated.  Use driver_mirror+NBD
> -#     instead.
> +#     instead. @compression is obsolete use multifd compression
> +#     methods instead.

For @deprecated, are we supposed to list multiple things at once, or
use a separate @deprecated tag for each one ?

Again I'd suggest rewording

    @compression is unreliable and untested. It is recommended to
    use multifd migration, which offers an alternative compression
    implementation that is reliable and tested.


> diff --git a/migration/options.c b/migration/options.c
> index 374dc0767e..c04e62ba2d 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -443,6 +443,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, 
> Error **errp)
>                      "Use driver_mirror+NBD instead.");
>      }
>  
> +    if (new_caps[MIGRATION_CAPABILITY_BLOCK]) {

Surely MIGRATION_CAPABILITY_COMPRESS not BLOCK ?

> +        warn_report("Old compression method is deprecated. "
> +                    "Use multifd compression methods instead.");
> +    }
> +
>  #ifndef CONFIG_REPLICATION
>      if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
>          error_setg(errp, "QEMU compiled without replication module"
> @@ -1196,18 +1201,26 @@ static void migrate_params_apply(MigrateSetParameters 
> *params, Error **errp)
>      /* TODO use QAPI_CLONE() instead of duplicating it inline */
>  
>      if (params->has_compress_level) {
> +        warn_report("Old compression is deprecated. "
> +                    "Use multifd compression methods instead.");
>          s->parameters.compress_level = params->compress_level;
>      }
>  
>      if (params->has_compress_threads) {
> +        warn_report("Old compression is deprecated. "
> +                    "Use multifd compression methods instead.");
>          s->parameters.compress_threads = params->compress_threads;
>      }
>  
>      if (params->has_compress_wait_thread) {
> +        warn_report("Old compression is deprecated. "
> +                    "Use multifd compression methods instead.");
>          s->parameters.compress_wait_thread = params->compress_wait_thread;
>      }
>  
>      if (params->has_decompress_threads) {
> +        warn_report("Old compression is deprecated. "
> +                    "Use multifd compression methods instead.");
>          s->parameters.decompress_threads = params->decompress_threads;
>      }
>  
> -- 
> 2.40.1
> 

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 :|