| Message ID | 20231013104736.31722-4-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Migration deprecated parts | expand |
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. 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. In the former case, we want something like "parameter 'inc' is deprecated". In the latter case, we want something like "-i is deprecated". 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. If you decide a single message is good enough, use something like "parameter 'inc' / flag -i is deprecated". > + "'block-incremental' migration parameter to 'true'" > + " instead."); Again, fine now, becomes bad advice in PATCH 05. > + } > + > if (resume) { > if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) { > error_setg(errp, "Cannot resume if there is no "
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.
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 :) [...]
Markus Armbruster <armbru@redhat.com> wrote: > Juan Quintela <quintela@redhat.com> writes: > >> Markus Armbruster <armbru@redhat.com> wrote: >>> Juan Quintela <quintela@redhat.com> writes: >> 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 :) > > [...] Telling this in deprecated.rst is enough? or you want me to put it also in the error/warn messages and qapi? Later, Juan.
Juan Quintela <quintela@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> wrote: >> Juan Quintela <quintela@redhat.com> writes: >> >>> Markus Armbruster <armbru@redhat.com> wrote: >>>> Juan Quintela <quintela@redhat.com> writes: >>> 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 :) >> >> [...] > > Telling this in deprecated.rst is enough? or you want me to put it also > in the error/warn messages and qapi? Let's make all of them point to blockdev-mirror with NBD. If you think mentioning @block-incremental & friends would be useful in some or all places would be useful, go ahead, I don't mind.
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. +# # 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" + "'block-incremental' migration parameter to 'true'" + " instead."); + } + if (resume) { if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) { error_setg(errp, "Cannot resume if there is no "