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

[Markus Armbruster <armbru@redhat.com>]

Juan Quintela <quintela@redhat.com> writes:

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

I see where you're coming from.  Now let's change perspective for a
minute: what's the purpose of deprecating stuff?

We normally deprecate with intent to remove.

We don't remove right away, because we promised to first deprecate for a
grace period, so users can adjust in an orderly manner.  The deprecation
serves as signal "you need to adjust".  The documentation that comes
with it should help with the adjustment.  It's commonly of the form "use
$alternative instead".  The alternative is often a direct replacement,
but not always.  There could even be no replacement at all.  We don't
promise replacements, we promise an orderly process, so users can
adjust.

Sometimes, we don't have firm plans to remove, but are more like "maybe
remove when gets in the way".  We could soften the "you need to adjust"
signal in documentation, but I doubt that's a good idea.  Regardless,
the need to help users adjust remains.

Back to your patches.  There are two separate interfaces to block
migration, and both are deprecated at the end of the series:

1. Migration parameter @block-incremental & friends

   Not in the way, content to keep around for longer if it helps users.

   The deprecation documentation advises to use block-mirror with NBD
   instead.  All good.

2. QMP migrate parameters @inc and @blk

   Firm intent to remove as soon as the grace period expires, because
   it's in the way.

   The deprecation documentation advises to use interface 1 instead.
   But that's deprecated, too!

   Insufficiently careful readers will miss that the replacement is
   deprecated, and just use it.  Risks surprise when the replacement
   goes away, too.

   More careful readers will realize that this advises to use something
   we elsewhere advise not to use.  Contradiction!  Confusion ensues.

   On further reflection, these readers might conclude that the
   *combined* advice is to use block-mirror with NBD instead.  This is
   correct.

   So why not tell them?

   Perhaps you'd like to give more nuanced advice, like "you should move
   to block-mirror with NBD, but if that's not practical for you, you
   should at least move to @block-incremental & friends, which will
   likely stick around for longer."  That's fine.  All I'm asking for is
   to not make things more confusing than they need to be :)

[...]