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

[Juan Quintela]

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.