Re: [PATCH v4 03/10] migration: migrate 'inc' command option is deprecated.

[Juan Quintela <quintela@redhat.com>]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Set the 'block_incremental' migration parameter to 'true' instead.
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>
>> ---
>>
>> Improve documentation and style (thanks Markus)
>> ---
>>  docs/about/deprecated.rst | 7 +++++++
>>  qapi/migration.json       | 8 +++++++-
>>  migration/migration.c     | 6 ++++++
>>  3 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index 1c4d7f36f0..1b6b2870cf 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -452,3 +452,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 6865fea3c5..56bbd55b87 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1492,6 +1492,11 @@
>>  #
>>  # @resume: resume one paused migration, default "off". (since 3.0)
>>  #
>> +# Features:
>> +#
>> +# @deprecated: Member @inc is deprecated.  Use migration parameter
>> +#     @block-incremental instead.
>
> This is fine now.  It becomes bad advice in PATCH 05, which deprecates
> @block-incremental.  Two solutions:
>
> 1. Change this patch to point to an alternative that will *not* be
> deprecated.

Ok, clearly I am not explaining myself properly O:-)

History of block migration:
* In the beggining there was -b and -i migrate options
  There was the only way to do storage of migration.
* We moved to use parameters and capabilities for migration
  So we created @block-incremental and @block.
  But we didn't remove the command line options (for backward
  compatibility).
* We were asked to modify migration so some storaged was migrated and
  some was not migrated during migration.  But block people found that
  it was a good idea to allow storage migration without migrating the
  vm, so they created this blockdev-mirror mechanism that is shinny,
  funny, faster, <whatever> better.

So now we have old code that basically nobody uses (the last big user
was COLO, but now it can use multifd).  So we want to drop it, but we
don't care about a direct replacement.

So, why I am interested in removing this?
- @block and @block-incremental: If you don't use block migration, their
  existence don't bother you.  They are "quite" independent of the rest
  of the migration code (they could be better integrated, but not big
  trouble here).
- migrate options -i/-b: This ones hurt us each time that we need to
  do changing in options.  Notice that we have "perfect" replacements
  with @block and @block-incremental, exactly the same
  result/semantics/...
  You can see the trobles in the RFC patches

     * [PATCH v4 07/10] [RFC] migration: Make -i/-b an error for hmp and qmp
     * [PATCH v4 08/10] [RFC] migration: Remove helpers needed for -i/-b migrate options

So what I want, I want to remove -i/-b in the next version (9.0?).  For
the other, I want to remove it, but I don't care if the code is around
in "deprecated" state for another couple of years if there are still
people that feel that they want it.

This is the reason that I put a pointer for -i/-b to
@block/@block-incremental.  They are "perfect" replacements.

I can put here to use blockdev-mirror + NBD, but the replacement is not
so direct.

Does this make sense?


> 2. Change PATCH 05.
>
> Same end result.
>
>> +#
>>  # Returns: nothing on success
>>  #
>>  # Since: 0.14
>> @@ -1513,7 +1518,8 @@
>>  # <- { "return": {} }
>>  ##
>>  { 'command': 'migrate',
>> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>> +  'data': {'uri': 'str', '*blk': 'bool',
>> +           '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
>>             '*detach': 'bool', '*resume': 'bool' } }
>>  
>>  ##
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 1c6c81ad49..ac4897fe0d 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/-i migrate option is deprecated, set the"
>
> This is either about QMP migrate's parameter "inc", or HMP migrate's
> flags -i.

Needs to be @inc.  I want about the "-i" command option in other place.

> In the former case, we want something like "parameter 'inc' is
> deprecated".

This one.

> In the latter case, we want something like "-i is deprecated".

Ok, changing.

> Trying to do both in a single message results in a sub-par message.  If
> you want to do better, you have to check in hmp_migrate(), too.  Then
> hmp_migrate can refer to "-i", and migrate_prepare() to "parameter
> 'inc'".  Up to you.

That was the intention, but I forgot to update this message.

> If you decide a single message is good enough, use something like
> "parameter 'inc' / flag -i is deprecated".

Later, Juan.