[v3,1/4] migration: migrate 'inc' command option is deprecated.

Set the 'block_incremental' migration parameter to 'true' instead.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 docs/about/deprecated.rst |  7 +++++++
 qapi/migration.json       | 12 ++++++++++--
 migration/migration.c     |  6 ++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

Juan Quintela <quintela@redhat.com> writes:

> Set the 'block_incremental' migration parameter to 'true' instead.
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  docs/about/deprecated.rst |  7 +++++++
>  qapi/migration.json       | 12 ++++++++++--
>  migration/migration.c     |  6 ++++++
>  3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 8b136320e2..eb0f326f00 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -417,3 +417,10 @@ Migration
>  ``skipped`` field in Migration stats has been deprecated.  It hasn't
>  been used for more than 10 years.
>  
> +``inc`` migrate command option (since 8.2)
> +''''''''''''''''''''''''''''''''''''''''''
> +
> +The new way to modify migration is using migration parameters.
> +``inc`` functionality can be achieved by setting the
> +``block-incremental`` migration parameter to ``true``.
> +
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d7dfaa5db9..7669c98c7a 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1486,13 +1486,20 @@
>  #
>  # @blk: do block migration (full disk copy)
>  #
> -# @inc: incremental disk copy migration
> +# @inc: incremental disk copy migration.  This option is deprecated.
> +#     Set the 'block-incremetantal' migration parameter to 'true'
> +#     instead.

'block-incremental'

>  #
>  # @detach: this argument exists only for compatibility reasons and is
>  #     ignored by QEMU
>  #
>  # @resume: resume one paused migration, default "off". (since 3.0)
>  #
> +# Features:
> +#
> +# @deprecated: option @inc should be enabled by setting the
> +#     'block-incremental' migration parameter to 'true'.
> +#

You add deprecation notices, one to the member documentation, and one to
the "Features:" section.  You should add just one, to the "Features:"
section.  Suggest:

   # @deprecated: Member @inc is deprecated.  Use migration parameter
   # @block-incremental instead.

>  # Returns: nothing on success
>  #
>  # Since: 0.14
> @@ -1514,7 +1521,8 @@
>  # <- { "return": {} }
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> +  'data': {'uri': 'str', '*blk': 'bool',
> +           '*inc': { 'type': 'bool', 'features': ['deprecated'] },

For better or worse, we format like [ 'deprecated' ].

>             '*detach': 'bool', '*resume': 'bool' } }
>  
>  ##
> diff --git a/migration/migration.c b/migration/migration.c
> index 1c6c81ad49..c7e4c37b8a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1601,6 +1601,12 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>  {
>      Error *local_err = NULL;
>  
> +    if (blk_inc) {
> +        warn_report("-inc migrate option is deprecated, set the "
> +                    "'block-incremental' migration parameter to 'true'"
> +                    " instead.");

There is no "-inc migrate option".  You're refering to QMP command
migrate's parameter @inc / HMP command migrate's flag -i.

> +    }
> +
>      if (resume) {
>          if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>              error_setg(errp, "Cannot resume if there is no "

As far as I can see, HMP command migrate still uses the deprecated
interface:

    qmp_migrate(uri, !!blk, blk, !!inc, inc,
                false, false, true, resume, &err);

Its use should be replaced before we deprecate it.

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Set the 'block_incremental' migration parameter to 'true' instead.
>>
>>  # @blk: do block migration (full disk copy)
>>  #
>> -# @inc: incremental disk copy migration
>> +# @inc: incremental disk copy migration.  This option is deprecated.
>> +#     Set the 'block-incremetantal' migration parameter to 'true'
>> +#     instead.
>
> 'block-incremental'

Done, thanks.

>>  #
>>  # @detach: this argument exists only for compatibility reasons and is
>>  #     ignored by QEMU
>>  #
>>  # @resume: resume one paused migration, default "off". (since 3.0)
>>  #
>> +# Features:
>> +#
>> +# @deprecated: option @inc should be enabled by setting the
>> +#     'block-incremental' migration parameter to 'true'.
>> +#
>
> You add deprecation notices, one to the member documentation, and one to
> the "Features:" section.  You should add just one, to the "Features:"
> section.  Suggest:
>
>    # @deprecated: Member @inc is deprecated.  Use migration parameter
>    # @block-incremental instead.

Done.

>>  # Returns: nothing on success
>>  #
>>  # Since: 0.14
>> @@ -1514,7 +1521,8 @@
>>  # <- { "return": {} }
>>  ##
>>  { 'command': 'migrate',
>> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>> +  'data': {'uri': 'str', '*blk': 'bool',
>> +           '*inc': { 'type': 'bool', 'features': ['deprecated'] },
>
> For better or worse, we format like [ 'deprecated' ].

Done.

>>             '*detach': 'bool', '*resume': 'bool' } }
>>  
>>  ##
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 1c6c81ad49..c7e4c37b8a 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1601,6 +1601,12 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>>  {
>>      Error *local_err = NULL;
>>  
>> +    if (blk_inc) {
>> +        warn_report("-inc migrate option is deprecated, set the "
>> +                    "'block-incremental' migration parameter to 'true'"
>> +                    " instead.");
>
> There is no "-inc migrate option".  You're refering to QMP command
> migrate's parameter @inc / HMP command migrate's flag -i.

Changed to:


s|-inc|@inc/-i|

>> +    }
>> +
>>      if (resume) {
>>          if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>>              error_setg(errp, "Cannot resume if there is no "
>
> As far as I can see, HMP command migrate still uses the deprecated
> interface:
>
>     qmp_migrate(uri, !!blk, blk, !!inc, inc,
>                 false, false, true, resume, &err);
>
> Its use should be replaced before we deprecate it.

We need to drop it.

Blockjobs are much more flexible.  We want to get rid of the whole
concept of block migration inside the migration protocol/machinery.

Block migration requires that one:

- migrate all devices, i.e. no way to select some shared some local.

- I think that incremental bit requires that you use qcow2 images, but I
  haven't even double checked them.

I just want to drop it in the near future, if 9.0 is too soon, for
10.0.

Later, Juan.