Hi this series describe the migration parts that have to be deprecated. - It is an rfc because I doubt that I did the deprecation process right. Hello Markus O:-) - skipped field: It is older than me, I have never know what it stands for. As far as I know it has always been zero. - inc/blk migrate command options. They are only used by block migration (that I deprecate on the following patch). And they are really bad. grep must_remove_block_options. - block migration. block jobs, whatever they are called this week are way more flexible. Current code works, but we broke it here and there, and really nobody has stand up to maintain it. It is quite contained and can be left there. Is anyone really using it? - old compression method. It don't work. See last try from Lukas to make a test that works reliabely. I failed with the same task years ago. It is really slow, and if compression is good for you, multifd + zlib is going to perform/compress way more. I don't know what to do with this code, really. * Remove it for this release? It don't work, and haven't work reliabely in quite a few time. * Deprecate it and remove in another couple of releases, i.e. normal deprecation. * Ideas? - -incoming <uri> if you need to set parameters (multifd cames to mind, and preempt has the same problem), you really needs to use defer. So what should we do here? This part is not urget, because management apps have a working option that are already using "defer", and the code simplifacation if we remove it is not so big. So we can leave it until 9.0 or whatever we think fit. What do you think? Later, Juan. Juan Quintela (6): migration: skipped field is really obsolete. migration: migrate 'inc' command option is deprecated. migration: migrate 'blk' command option is deprecated. migration: Deprecate -incoming <uri> migration: Deprecate block migration migration: Deprecated old compression method docs/about/deprecated.rst | 45 ++++++++++++ qapi/migration.json | 141 +++++++++++++++++++++++++++----------- migration/block.c | 2 + migration/migration.c | 10 +++ migration/options.c | 20 ++++++ softmmu/vl.c | 2 + 6 files changed, 181 insertions(+), 39 deletions(-) base-commit: 5f9dd6a8ce3961db4ce47411ed2097ad88bdf5fc prerequisite-patch-id: 99c8bffa9428838925e330eb2881bab476122579 prerequisite-patch-id: 77ba427fd916aeb395e95aa0e7190f84e98e96ab prerequisite-patch-id: 9983d46fa438d7075a37be883529e37ae41e4228 prerequisite-patch-id: 207f7529924b12dcb57f6557d6db6f79ceb2d682 prerequisite-patch-id: 5ad1799a13845dbf893a28a202b51a6b50d95d90 prerequisite-patch-id: c51959aacd6d65ee84fcd4f1b2aed3dd6f6af879 prerequisite-patch-id: da9dbb6799b2da002c0896574334920097e4c50a prerequisite-patch-id: c1110ffafbaf5465fb277a20db809372291f7846 prerequisite-patch-id: 8307c92bedd07446214b35b40206eb6793a7384d prerequisite-patch-id: 0a6106cd4a508d5e700a7ff6c25edfdd03c8ca3d prerequisite-patch-id: 83205051de22382e75bf4acdf69e59315801fa0d prerequisite-patch-id: 8c9b3cba89d555c071a410041e6da41806106a7e prerequisite-patch-id: 0ff62a33b9a242226ccc1f5424a516de803c9fe5 prerequisite-patch-id: 25b8ae1ebe09ace14457c454cfcb23077c37346c prerequisite-patch-id: 466ea91d5be41fe345dacd4d17bbbe5ce13118c2 prerequisite-patch-id: d1045858f9729ac62eccf2e83ebf95cfebae2cb5 prerequisite-patch-id: 0276ec02073bda5426de39e2f2e81eef080b4f54 prerequisite-patch-id: 7afb4450a163cc1a63ea23831c50214966969131 prerequisite-patch-id: 06c053ce4f41db9675bd1778ae8f6a483641fcef prerequisite-patch-id: 13ea05d54d741ed08b3bfefa1fc8bedb9c81c782 prerequisite-patch-id: 99c4e2b7101bc8c4b9515129a1bbe6f068053dbf prerequisite-patch-id: 1e393a196dc7a1ee75f3cc3cebbb591c5422102f prerequisite-patch-id: 2cf497b41f5024ede0a224b1f5b172226067a534 prerequisite-patch-id: 2a70276ed61d33fc4f3b52560753c05d1cd413be prerequisite-patch-id: 17ec40f4388b62ba8bf3ac1546c6913f5d1f6079 prerequisite-patch-id: dba969ce9d6cf69c1319661a7d81b1c1c719804d prerequisite-patch-id: 8d800cda87167314f07320bdb3df936c323e4a40 prerequisite-patch-id: 25d4aaf54ea66f30e426fa38bdd4e0f47303c513 prerequisite-patch-id: 082c9d8584c1daff1e827e44ee3047178e7004a7 prerequisite-patch-id: 0ef73900899425ae2f00751347afdce3739aa954 prerequisite-patch-id: e7db4730b791b71aaf417ee0f65fb6304566aaf8 prerequisite-patch-id: 62d7f28f8196039507ffe362f97723395d7bb704 prerequisite-patch-id: ea8de47bcb54e33bcc67e59e9ed752a4d1fad703 prerequisite-patch-id: 497893ef92e1ea56bd8605e6990a05cb4c7f9293 prerequisite-patch-id: 3dc869c80ee568449bbfa2a9bc427524d0e8970b prerequisite-patch-id: 52c14b6fb14ed4ccd685385a9fbc6297b762c0ef prerequisite-patch-id: 23de8371e9e3277c374a47f9bd10de209a22fdd5 prerequisite-patch-id: d21f036dd106af3375fb920bf0a557fe2b86d98e prerequisite-patch-id: ca717076e9de83d6ce370f7374c4729a9f586fae prerequisite-patch-id: a235b6ab3237155f2b39e8e10d47ddd250f6b6cc prerequisite-patch-id: 6db2aa3d8a5804c85dd171864f5140fadf5f72da prerequisite-patch-id: a799b883f4cb41c34ad074220293f372c6e0a9c7 prerequisite-patch-id: 5e012c426aef7b2f07513cec68e7efa1cf85fe52 prerequisite-patch-id: 4e614e7e3395dda7bae5f9fa21257c57cce45a39 prerequisite-patch-id: 67f8e68622c9698280ff5c5dc7469f36daf9a012 prerequisite-patch-id: d86078331449a21499e3f955e27bc87294969346 prerequisite-patch-id: 3f30d10e0ac7f53307f6b462eaf5b47151b73631 prerequisite-patch-id: 0c7fb969e83ed1b01f15b54abb106026fadb7707 -- 2.40.1
Has return zero for more than 10 years. Just mark it deprecated. Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 10 ++++++++++ qapi/migration.json | 12 ++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 0743459862..e1aa0eafc8 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -423,3 +423,13 @@ both, older and future versions of QEMU. The ``blacklist`` config file option has been renamed to ``block-rpcs`` (to be in sync with the renaming of the corresponding command line option). + +Migration +--------- + +``skipped`` MigrationStats field (since 8.1) +'''''''''''''''''''''''''''''''''''''''''''' + +``skipped`` field in Migration stats has been deprecated. It hasn't +been used for more than 10 years. + diff --git a/qapi/migration.json b/qapi/migration.json index cb7cd3e578..bcae193733 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -23,7 +23,8 @@ # # @duplicate: number of duplicate (zero) pages (since 1.2) # -# @skipped: number of skipped zero pages (since 1.5) +# @skipped: number of skipped zero pages. Don't use, only provided for +# compatibility (since 1.5) # # @normal: number of normal pages (since 1.2) # @@ -62,11 +63,18 @@ # between 0 and @dirty-sync-count * @multifd-channels. (since # 7.1) # +# Features: +# +# @deprecated: Member @skipped has not been used for a long time. +# # Since: 0.14 +# ## { 'struct': 'MigrationStats', 'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' , - 'duplicate': 'int', 'skipped': 'int', 'normal': 'int', + 'duplicate': 'int', + 'skipped': { 'type': 'int', 'features': ['deprecated'] }, + 'normal': 'int', 'normal-bytes': 'int', 'dirty-pages-rate': 'int', 'mbps': 'number', 'dirty-sync-count': 'int', 'postcopy-requests': 'int', 'page-size': 'int', -- 2.40.1
On Mon, Jun 12, 2023 at 09:33:39PM +0200, Juan Quintela wrote:
Has return zero for more than 10 years. Just mark it deprecated.
Specifically we introduced the field in 1.5.0 commit f1c72795af573b24a7da5eb52375c9aba8a37972 Author: Peter Lieven <pl@kamp.de> Date: Tue Mar 26 10:58:37 2013 +0100 migration: do not sent zero pages in bulk stage during bulk stage of ram migration if a page is a zero page do not send it at all. the memory at the destination reads as zero anyway. even if there is an madvise with QEMU_MADV_DONTNEED at the target upon receipt of a zero page I have observed that the target starts swapping if the memory is overcommitted. it seems that the pages are dropped asynchronously. this patch also updates QMP to return the number of skipped pages in MigrationStats. but removed its usage in 1.5.3 commit 9ef051e5536b6368a1076046ec6c4ec4ac12b5c6 Author: Peter Lieven <pl@kamp.de> Date: Mon Jun 10 12:14:19 2013 +0200 Revert "migration: do not sent zero pages in bulk stage" Not sending zero pages breaks migration if a page is zero at the source but not at the destination. This can e.g. happen if different BIOS versions are used at source and destination. It has also been reported that migration on pseries is completely broken with this patch. This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 10 ++++++++++ qapi/migration.json | 12 ++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/qapi/migration.json b/qapi/migration.json index cb7cd3e578..bcae193733 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -23,7 +23,8 @@ # # @duplicate: number of duplicate (zero) pages (since 1.2) # -# @skipped: number of skipped zero pages (since 1.5) +# @skipped: number of skipped zero pages. Don't use, only provided for +# compatibility (since 1.5)
I'd say @skipped: number of skipped zero pages. Always zero, only provided for compatibility (since 1.5)
# # @normal: number of normal pages (since 1.2) # @@ -62,11 +63,18 @@ # between 0 and @dirty-sync-count * @multifd-channels. (since # 7.1) # +# Features: +# +# @deprecated: Member @skipped has not been used for a long time.
@deprecated: Member @skipped is always zero since 1.5.3 With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:39PM +0200, Juan Quintela wrote:
Has return zero for more than 10 years. Just mark it deprecated.
Specifically we introduced the field in 1.5.0
commit f1c72795af573b24a7da5eb52375c9aba8a37972 Author: Peter Lieven <pl@kamp.de> Date: Tue Mar 26 10:58:37 2013 +0100
migration: do not sent zero pages in bulk stage
during bulk stage of ram migration if a page is a zero page do not send it at all. the memory at the destination reads as zero anyway.
even if there is an madvise with QEMU_MADV_DONTNEED at the target upon receipt of a zero page I have observed that the target starts swapping if the memory is overcommitted. it seems that the pages are dropped asynchronously.
this patch also updates QMP to return the number of skipped pages in MigrationStats.
but removed its usage in 1.5.3
commit 9ef051e5536b6368a1076046ec6c4ec4ac12b5c6 Author: Peter Lieven <pl@kamp.de> Date: Mon Jun 10 12:14:19 2013 +0200
Revert "migration: do not sent zero pages in bulk stage"
Not sending zero pages breaks migration if a page is zero at the source but not at the destination. This can e.g. happen if different BIOS versions are used at source and destination. It has also been reported that migration on pseries is completely broken with this patch.
This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.
Thanks for the history O:-)
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 10 ++++++++++ qapi/migration.json | 12 ++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/qapi/migration.json b/qapi/migration.json index cb7cd3e578..bcae193733 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -23,7 +23,8 @@ # # @duplicate: number of duplicate (zero) pages (since 1.2) # -# @skipped: number of skipped zero pages (since 1.5) +# @skipped: number of skipped zero pages. Don't use, only provided for +# compatibility (since 1.5)
I'd say
@skipped: number of skipped zero pages. Always zero, only provided for compatibility (since 1.5)
Changed.
# # @normal: number of normal pages (since 1.2) # @@ -62,11 +63,18 @@ # between 0 and @dirty-sync-count * @multifd-channels. (since # 7.1) # +# Features: +# +# @deprecated: Member @skipped has not been used for a long time.
@deprecated: Member @skipped is always zero since 1.5.3
Changed. Thanks.
Use 'migrate_set_parameter block_incremental true' instead. Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ qapi/migration.json | 11 +++++++++-- migration/migration.c | 5 +++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index e1aa0eafc8..c75a3a8f5a 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -433,3 +433,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.1) +'''''''''''''''''''''''''''''''''''''''''' + +The new way to modify migration is using migration parameters. +``inc`` functionality can be acchieved using +``migrate_set_parameter block-incremental true``. + diff --git a/qapi/migration.json b/qapi/migration.json index bcae193733..4ee28df6da 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1424,13 +1424,19 @@ # # @blk: do block migration (full disk copy) # -# @inc: incremental disk copy migration +# @inc: incremental disk copy migration. This option is deprecated. +# Use 'migrate_set_parameter block-incremetantal true' instead. # # @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 is better set with +# 'migrate_set_parameter block-incremental true'. +# # Returns: nothing on success # # Since: 0.14 @@ -1452,7 +1458,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 dc05c6f6ea..7ebce7c7bf 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1544,6 +1544,11 @@ 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, use" + "'migrate_set_parameter block-incremental true' instead."); + } + if (resume) { if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) { error_setg(errp, "Cannot resume if there is no " -- 2.40.1
On Mon, Jun 12, 2023 at 09:33:40PM +0200, Juan Quintela wrote:
Use 'migrate_set_parameter block_incremental true' instead.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ qapi/migration.json | 11 +++++++++-- migration/migration.c | 5 +++++ 3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index e1aa0eafc8..c75a3a8f5a 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -433,3 +433,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.1) +'''''''''''''''''''''''''''''''''''''''''' + +The new way to modify migration is using migration parameters. +``inc`` functionality can be acchieved using +``migrate_set_parameter block-incremental true``.
This is a HMP command, but the change affects QMP too. I'd suggest ``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 bcae193733..4ee28df6da 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1424,13 +1424,19 @@ # # @blk: do block migration (full disk copy) # -# @inc: incremental disk copy migration +# @inc: incremental disk copy migration. This option is deprecated. +# Use 'migrate_set_parameter block-incremetantal true' instead.
@inc: incremental disk copy migration. This option is deprecated. Set the 'block-incremetantal' migration parameter to 'true' instead.
# # @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 is better set with +# 'migrate_set_parameter block-incremental true'.
# @deprecated: option @inc should be enabled by # setting the 'block-incremental' migration parameter to 'true'.
+# # Returns: nothing on success # # Since: 0.14 @@ -1452,7 +1458,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 dc05c6f6ea..7ebce7c7bf 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1544,6 +1544,11 @@ 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, use" + "'migrate_set_parameter block-incremental true' instead.");
warn_report("-inc migrate option is deprecated, set the" "'block-incremental' migration parameter to 'true' instead."); With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:40PM +0200, Juan Quintela wrote:
Use 'migrate_set_parameter block_incremental true' instead.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ qapi/migration.json | 11 +++++++++-- migration/migration.c | 5 +++++ 3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index e1aa0eafc8..c75a3a8f5a 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -433,3 +433,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.1) +'''''''''''''''''''''''''''''''''''''''''' + +The new way to modify migration is using migration parameters. +``inc`` functionality can be acchieved using +``migrate_set_parameter block-incremental true``.
This is a HMP command, but the change affects QMP too. I'd suggest
``inc`` functionality can be achieved by setting the ``block-incremental`` migration parameter to ``true``.
Applied all suggestions. Thanks.
Use 'migrate_set_capability block true' instead. Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ qapi/migration.json | 11 +++++++---- migration/migration.c | 5 +++++ 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index c75a3a8f5a..47e98dc95e 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -440,3 +440,10 @@ The new way to modify migration is using migration parameters. ``inc`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``. +``blk`` migrate command option (since 8.1) +'''''''''''''''''''''''''''''''''''''''''' + +The new way to modify migration is using migration parameters. +``blk`` functionality can be acchieved using +``migrate_set_parameter block-incremental true``. + diff --git a/qapi/migration.json b/qapi/migration.json index 4ee28df6da..b71e00737e 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1422,7 +1422,8 @@ # # @uri: the Uniform Resource Identifier of the destination VM # -# @blk: do block migration (full disk copy) +# @blk: do block migration (full disk copy). This option is deprecated. +# Use 'migrate_set_capability block true' instead. # # @inc: incremental disk copy migration. This option is deprecated. # Use 'migrate_set_parameter block-incremetantal true' instead. @@ -1434,8 +1435,9 @@ # # Features: # -# @deprecated: option @inc is better set with -# 'migrate_set_parameter block-incremental true'. +# @deprecated: options @inc and @blk are better set with +# 'migrate_set_parameter block-incremental true' and +# 'migrate_set_capability block true' respectively. # # Returns: nothing on success # @@ -1458,7 +1460,8 @@ # <- { "return": {} } ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', + 'data': {'uri': 'str', + '*blk': { 'type': 'bool', 'features': ['deprecated'] }, '*inc': { 'type': 'bool', 'features': ['deprecated'] }, '*detach': 'bool', '*resume': 'bool' } } diff --git a/migration/migration.c b/migration/migration.c index 7ebce7c7bf..b7d5f6b96c 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1549,6 +1549,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc, "'migrate_set_parameter block-incremental true' instead."); } + if (blk) { + warn_report("-blk migrate option is deprecated, use" + "'migrate_set_capability block true' instead."); + } + if (resume) { if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) { error_setg(errp, "Cannot resume if there is no " -- 2.40.1
On Mon, Jun 12, 2023 at 09:33:41PM +0200, Juan Quintela wrote:
Use 'migrate_set_capability block true' instead.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ qapi/migration.json | 11 +++++++---- migration/migration.c | 5 +++++ 3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index c75a3a8f5a..47e98dc95e 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -440,3 +440,10 @@ The new way to modify migration is using migration parameters. ``inc`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``.
+``blk`` migrate command option (since 8.1) +'''''''''''''''''''''''''''''''''''''''''' + +The new way to modify migration is using migration parameters. +``blk`` functionality can be acchieved using +``migrate_set_parameter block-incremental true``.
Same comments on rewording as the previous patch, so won't repeate them all. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:41PM +0200, Juan Quintela wrote:
Use 'migrate_set_capability block true' instead.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ qapi/migration.json | 11 +++++++---- migration/migration.c | 5 +++++ 3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index c75a3a8f5a..47e98dc95e 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -440,3 +440,10 @@ The new way to modify migration is using migration parameters. ``inc`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``.
+``blk`` migrate command option (since 8.1) +'''''''''''''''''''''''''''''''''''''''''' + +The new way to modify migration is using migration parameters. +``blk`` functionality can be acchieved using +``migrate_set_parameter block-incremental true``.
Same comments on rewording as the previous patch, so won't repeate them all.
Did the same than the previous one. Thanks.
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command. Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ softmmu/vl.c | 2 ++ 2 files changed, 9 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47e98dc95e..518672722d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -447,3 +447,10 @@ The new way to modify migration is using migration parameters. ``blk`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``. +``-incoming uri`` (since 8.1) +''''''''''''''''''''''''''''' + +Everything except ``-incoming defer`` are deprecated. This allows to +setup parameters before launching the proper migration with +``migrate-incoming uri``. + diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..7fe865ab59 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming); qmp_migrate_incoming(incoming, &local_err); if (local_err) { error_reportf_err(local_err, "-incoming %s: ", incoming); -- 2.40.1
On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ softmmu/vl.c | 2 ++ 2 files changed, 9 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47e98dc95e..518672722d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -447,3 +447,10 @@ The new way to modify migration is using migration parameters. ``blk`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``.
+``-incoming uri`` (since 8.1) +''''''''''''''''''''''''''''' + +Everything except ``-incoming defer`` are deprecated. This allows to +setup parameters before launching the proper migration with +``migrate-incoming uri``. + diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..7fe865ab59 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming);
I still use uri for all my scripts, alongside with "-global migration.xxx" and it works. Shall we just leave it there? Or is deprecating it helps us in any form? -- Peter Xu
Peter Xu <peterx@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ softmmu/vl.c | 2 ++ 2 files changed, 9 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47e98dc95e..518672722d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -447,3 +447,10 @@ The new way to modify migration is using migration parameters. ``blk`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``.
+``-incoming uri`` (since 8.1) +''''''''''''''''''''''''''''' + +Everything except ``-incoming defer`` are deprecated. This allows to +setup parameters before launching the proper migration with +``migrate-incoming uri``. + diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..7fe865ab59 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming);
I still use uri for all my scripts, alongside with "-global migration.xxx" and it works.
You know what you are doing (TM). And remember that we don't support -gobal migration.x-foo. Yes, I know, we should drop the "x-" prefixes.
Shall we just leave it there? Or is deprecating it helps us in any form?
See the patches two weeks ago when people complained that lisen(.., num) was too low. And there are other parameters that work the same way (that I convenientely had forgotten). So the easiest way to get things right is to use "defer" always. Using -incoming "uri" should only be for people that "know what they are doing", so we had to ways to do it: - review all migration options and see which ones work without defer and document it - deprecate everything that is not defer. Anything else is not going to be very user unfriendly. What do you think. Later, Juan. PD. This series are RFC for multiple reasons O:-)
On Mon, Jun 12, 2023 at 10:51:08PM +0200, Juan Quintela wrote:
Peter Xu <peterx@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ softmmu/vl.c | 2 ++ 2 files changed, 9 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47e98dc95e..518672722d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -447,3 +447,10 @@ The new way to modify migration is using migration parameters. ``blk`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``.
+``-incoming uri`` (since 8.1) +''''''''''''''''''''''''''''' + +Everything except ``-incoming defer`` are deprecated. This allows to +setup parameters before launching the proper migration with +``migrate-incoming uri``. + diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..7fe865ab59 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming);
I still use uri for all my scripts, alongside with "-global migration.xxx" and it works.
You know what you are doing (TM). And remember that we don't support -gobal migration.x-foo. Yes, I know, we should drop the "x-" prefixes.
I hope they'll always be there. :) They're pretty handy for tests, when we want to boot a VM without the need to script the sequences of qmp cmds. Yes, we probably should just always drop the x-. We can always declare debugging purpose for all -global migration.* fields.
Shall we just leave it there? Or is deprecating it helps us in any form?
See the patches two weeks ago when people complained that lisen(.., num) was too low. And there are other parameters that work the same way (that I convenientely had forgotten). So the easiest way to get things right is to use "defer" always. Using -incoming "uri" should only be for people that "know what they are doing", so we had to ways to do it: - review all migration options and see which ones work without defer and document it - deprecate everything that is not defer.
Anything else is not going to be very user unfriendly. What do you think.
IIRC Wei Wang had a series just for that, so after that patchset applied we should have fixed all issues cleanly? Is there one more thing that's not working right there?
Later, Juan.
PD. This series are RFC for multiple reasons O:-)
Happy to know the rest (besides which I know will break my script :). -- Peter Xu
On Mon, Jun 12, 2023 at 05:19:40PM -0400, Peter Xu wrote:
On Mon, Jun 12, 2023 at 10:51:08PM +0200, Juan Quintela wrote:
Peter Xu <peterx@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ softmmu/vl.c | 2 ++ 2 files changed, 9 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47e98dc95e..518672722d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -447,3 +447,10 @@ The new way to modify migration is using migration parameters. ``blk`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``.
+``-incoming uri`` (since 8.1) +''''''''''''''''''''''''''''' + +Everything except ``-incoming defer`` are deprecated. This allows to +setup parameters before launching the proper migration with +``migrate-incoming uri``. + diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..7fe865ab59 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming);
I still use uri for all my scripts, alongside with "-global migration.xxx" and it works.
You know what you are doing (TM). And remember that we don't support -gobal migration.x-foo. Yes, I know, we should drop the "x-" prefixes.
I hope they'll always be there. :) They're pretty handy for tests, when we want to boot a VM without the need to script the sequences of qmp cmds.
The long term vision for QEMU configuration is that everything will be done via QMP. This doesn't mean we'll need interactive scripts though. We ought to be able to just point QEMU as a config file containing the QMP bits to construct the VM. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Peter Xu <peterx@redhat.com> wrote:
On Mon, Jun 12, 2023 at 10:51:08PM +0200, Juan Quintela wrote:
Peter Xu <peterx@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ softmmu/vl.c | 2 ++ 2 files changed, 9 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47e98dc95e..518672722d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -447,3 +447,10 @@ The new way to modify migration is using migration parameters. ``blk`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``.
+``-incoming uri`` (since 8.1) +''''''''''''''''''''''''''''' + +Everything except ``-incoming defer`` are deprecated. This allows to +setup parameters before launching the proper migration with +``migrate-incoming uri``. + diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..7fe865ab59 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming);
I still use uri for all my scripts, alongside with "-global migration.xxx" and it works.
You know what you are doing (TM). And remember that we don't support -gobal migration.x-foo. Yes, I know, we should drop the "x-" prefixes.
I hope they'll always be there. :) They're pretty handy for tests, when we want to boot a VM without the need to script the sequences of qmp cmds.
Yes, we probably should just always drop the x-. We can always declare debugging purpose for all -global migration.* fields.
Shall we just leave it there? Or is deprecating it helps us in any form?
See the patches two weeks ago when people complained that lisen(.., num) was too low. And there are other parameters that work the same way (that I convenientely had forgotten). So the easiest way to get things right is to use "defer" always. Using -incoming "uri" should only be for people that "know what they are doing", so we had to ways to do it: - review all migration options and see which ones work without defer and document it - deprecate everything that is not defer.
Anything else is not going to be very user unfriendly. What do you think.
IIRC Wei Wang had a series just for that, so after that patchset applied we should have fixed all issues cleanly?
No, what he does is using always a very big value for listen. But that is it. Anyways, I don't know how to change the backlog listen value without restarting the listen call.
Is there one more thing that's not working right there?
Compression has other problems. But independentely of that, they have the problem that we need to set the parameters before we call incoming.
PD. This series are RFC for multiple reasons O:-)
Happy to know the rest (besides which I know will break my script :).
Thanks, Juan.
On Mon, Jun 12, 2023 at 10:51:08PM +0200, Juan Quintela wrote:
Peter Xu <peterx@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ softmmu/vl.c | 2 ++ 2 files changed, 9 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47e98dc95e..518672722d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -447,3 +447,10 @@ The new way to modify migration is using migration parameters. ``blk`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``.
+``-incoming uri`` (since 8.1) +''''''''''''''''''''''''''''' + +Everything except ``-incoming defer`` are deprecated. This allows to +setup parameters before launching the proper migration with +``migrate-incoming uri``. + diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..7fe865ab59 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming);
I still use uri for all my scripts, alongside with "-global migration.xxx" and it works.
You know what you are doing (TM). And remember that we don't support -gobal migration.x-foo. Yes, I know, we should drop the "x-" prefixes.
Shall we just leave it there? Or is deprecating it helps us in any form?
See the patches two weeks ago when people complained that lisen(.., num) was too low. And there are other parameters that work the same way (that I convenientely had forgotten). So the easiest way to get things right is to use "defer" always. Using -incoming "uri" should only be for people that "know what they are doing", so we had to ways to do it: - review all migration options and see which ones work without defer and document it - deprecate everything that is not defer.
Anything else is not going to be very user unfriendly. What do you think.
In some cases it is worth having a convenience option for user friendliness. In this case, however, users are already needing to use QMP/HMP on the source side to set migration parameters. I think it is reasonable to say that doing *exactly* the same with QMP/HMP on the destination side is actually more friendly than requiring use of -global on the dest side which is different syntax. We don't document the way to use -global with migration parameters so when people see '-incoming' I think we are steering them into a trap, making it look like -incoming is sufficient on its own. Hence that user's mistake recently about not setting migration parameters. Overall I agree with deprecating '-incoming' != 'defer', as I think it will better guide users to following the correct steps, as is not a usability burden as they're already using QMP/HMP on the source side. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Jun 20, 2023 at 01:10:55PM +0100, Daniel P. Berrangé wrote:
In some cases it is worth having a convenience option for user friendliness.
In this case, however, users are already needing to use QMP/HMP on the source side to set migration parameters. I think it is reasonable to say that doing *exactly* the same with QMP/HMP on the destination side is actually more friendly than requiring use of -global on the dest side which is different syntax.
The -global parameters also work for the src side for my scripts, so I only need to attach "-incoming tcp:XXX" for dst cmdline here.
We don't document the way to use -global with migration parameters so when people see '-incoming' I think we are steering them into a trap, making it look like -incoming is sufficient on its own. Hence that user's mistake recently about not setting migration parameters.
Overall I agree with deprecating '-incoming' != 'defer', as I think it will better guide users to following the correct steps, as is not a usability burden as they're already using QMP/HMP on the source side.
I'd say -global is definitely not for users but for developers. Just like we keep around HMP - I'm not sure whether it's used in productions, I thought it was only for developers and we don't deprecate it eagerly. No strong opinions here if everyone wants to get rid of that, but before that I hope we can have some kind of cmdline tool that can easily tunnel qmp commands so whoever developers using pure cmdlines can switch to the new way easily. Thanks, -- Peter Xu
On 6/12/23 22:51, Juan Quintela wrote:
>> Shall we just leave it there? Or is deprecating it helps us in any form?
> See the patches two weeks ago when people complained that lisen(.., num)
> was too low. And there are other parameters that work the same way
> (that I convenientely had forgotten). So the easiest way to get things
> right is to use "defer" always. Using -incoming "uri" should only be
> for people that "know what they are doing", so we had to ways to do it:
> - review all migration options and see which ones work without defer
> and document it
> - deprecate everything that is not defer.
"-incoming <uri>" is literally the same as running "migrate-incoming"
as the first thing on the monitor:
if (incoming) {
Error *local_err = NULL;
if (strcmp(incoming, "defer") != 0) {
qmp_migrate_incoming(incoming, &local_err);
if (local_err) {
error_reportf_err(local_err, "-incoming %s: ", incoming);
exit(1);
}
}
} else if (autostart) {
qmp_cont(NULL);
}
It's the only piece of code which distinguishes "-incoming defer" from
"-incoming <uri>".
So I'm not sure what the problem would be with keeping it? The issue is
not how many features the command line has, but how they're implemented.
If they're just QMP wrappers and as such they're self-contained in
softmmu/vl.c, that's fine.
In fact, even for parameters, we could use keyval to parse "-incoming" and
set the parameters in the same place as above. That would remove the need
for "-global migration".
Paolo
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 6/12/23 22:51, Juan Quintela wrote:
>>> Shall we just leave it there? Or is deprecating it helps us in any form?
>> See the patches two weeks ago when people complained that lisen(.., num)
>> was too low. And there are other parameters that work the same way
>> (that I convenientely had forgotten). So the easiest way to get things
>> right is to use "defer" always. Using -incoming "uri" should only be
>> for people that "know what they are doing", so we had to ways to do it:
>> - review all migration options and see which ones work without defer
>> and document it
>> - deprecate everything that is not defer.
>
> "-incoming <uri>" is literally the same as running "migrate-incoming"
> as the first thing on the monitor:
>
> if (incoming) {
> Error *local_err = NULL;
> if (strcmp(incoming, "defer") != 0) {
> qmp_migrate_incoming(incoming, &local_err);
> if (local_err) {
> error_reportf_err(local_err, "-incoming %s: ", incoming);
> exit(1);
> }
> }
> } else if (autostart) {
> qmp_cont(NULL);
> }
>
> It's the only piece of code which distinguishes "-incoming defer" from
> "-incoming <uri>".
>
> So I'm not sure what the problem would be with keeping it?
User friendliness.
First of all, I use it all the time. And I know that it is useful for
developers. I was the one asking peter to implement -global
migration.foo to be able to test multifd with it.
The problem is that if you use more than two channels with multifd, on
the incoming side, you need to do:
- migrate_set_parameter multifd-channels 16
- migrate_incoming <uri>
And people continue to do:
- qemu -incoming <uri>
- migrate_set_parameter multifd-channels 16 (on the command line)
And they complain that it fails, because we are calling listen with the
wrong value.
> The issue is
> not how many features the command line has, but how they're implemented.
Or if they are confusing for the user?
> If they're just QMP wrappers and as such they're self-contained in
> softmmu/vl.c, that's fine.
>
> In fact, even for parameters, we could use keyval to parse "-incoming"
What is keyval?
> and
> set the parameters in the same place as above. That would remove the need
> for "-global migration".
Could you elaborate?
The other option that I can think of is changing the error messages for
migrate_check_parameters() and give instructions that you can't set
multifd channels once that you have started incoming migration.
Explaining there to use migrate_incoming command?
Later, Juan.
On 22/06/2023 10.52, Juan Quintela wrote:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 6/12/23 22:51, Juan Quintela wrote:
>>>> Shall we just leave it there? Or is deprecating it helps us in any form?
>>> See the patches two weeks ago when people complained that lisen(.., num)
>>> was too low. And there are other parameters that work the same way
>>> (that I convenientely had forgotten). So the easiest way to get things
>>> right is to use "defer" always. Using -incoming "uri" should only be
>>> for people that "know what they are doing", so we had to ways to do it:
>>> - review all migration options and see which ones work without defer
>>> and document it
>>> - deprecate everything that is not defer.
>>
>> "-incoming <uri>" is literally the same as running "migrate-incoming"
>> as the first thing on the monitor:
>>
>> if (incoming) {
>> Error *local_err = NULL;
>> if (strcmp(incoming, "defer") != 0) {
>> qmp_migrate_incoming(incoming, &local_err);
>> if (local_err) {
>> error_reportf_err(local_err, "-incoming %s: ", incoming);
>> exit(1);
>> }
>> }
>> } else if (autostart) {
>> qmp_cont(NULL);
>> }
>>
>> It's the only piece of code which distinguishes "-incoming defer" from
>> "-incoming <uri>".
>>
>> So I'm not sure what the problem would be with keeping it?
>
> User friendliness.
>
> First of all, I use it all the time. And I know that it is useful for
> developers. I was the one asking peter to implement -global
> migration.foo to be able to test multifd with it.
>
> The problem is that if you use more than two channels with multifd, on
> the incoming side, you need to do:
>
> - migrate_set_parameter multifd-channels 16
> - migrate_incoming <uri>
>
> And people continue to do:
>
> - qemu -incoming <uri>
> - migrate_set_parameter multifd-channels 16 (on the command line)
>
> And they complain that it fails, because we are calling listen with the
> wrong value.
Then simply forbid "migrate_set_parameter multifd-channels ..." if the uri
has been specified on the command line?
Thomas
On Thu, Jun 22, 2023 at 11:22:56AM +0200, Thomas Huth wrote:
Then simply forbid "migrate_set_parameter multifd-channels ..." if the uri has been specified on the command line?
Yeah, actually already in a pull (even though the pr may need a new one..): https://lore.kernel.org/r/20230622021320.66124-23-quintela@redhat.com -- Peter Xu
Peter Xu <peterx@redhat.com> wrote:
On Thu, Jun 22, 2023 at 11:22:56AM +0200, Thomas Huth wrote:
Then simply forbid "migrate_set_parameter multifd-channels ..." if the uri has been specified on the command line?
Yeah, actually already in a pull (even though the pr may need a new one..):
https://lore.kernel.org/r/20230622021320.66124-23-quintela@redhat.com
That is a different problem, and different solution. It you try to set multifd_channels after migration has started, it just fails telling that you can't change it so late. Later, Juan.
On 6/22/23 10:52, Juan Quintela wrote:
User friendliness. The problem is that if you use more than two channels with multifd, on the incoming side, you need to do:
You're sacrificing user-friendliness for the 99.99% that don't use multifd, for an error (i.e. it's not even fixing the issue) for the 0.01% that use multifd. That's not user-friendly.
- migrate_set_parameter multifd-channels 16 - migrate_incoming <uri>
The issue is not how many features the command line has, but how they're implemented.
Or if they are confusing for the user?
Anyone using multifd is not a typical user anyway.
If they're just QMP wrappers and as such they're self-contained in softmmu/vl.c, that's fine.
In fact, even for parameters, we could use keyval to parse "-incoming"
What is keyval?
util/keyval.c and include/qemu/keyval.h. It parses a list of key=value pairs into a QDict. Once you have removed the "source" key from the QDict you can use a visitor to parse the rest into a MigrateSetParameters. See the handling of QEMU_OPTION_audio, it could be something like case QEMU_OPTION_incoing: { Visitor *v; MigrateSetParameters *incoming_params = NULL; QDict *dict = keyval_parse(optarg, "source", NULL, &error_fatal); if (incoming) { if (qdict_haskey(dict, "source")) { error_setg(&error_fatal, "Parameter 'source' is duplicate"); } } else { if (!qdict_haskey(dict, "source")) { error_setg(&error_fatal, "Parameter 'source' is missing"); } runstate_set(RUN_STATE_INMIGRATE); incoming = g_strdup(qdict_get_str(dict, "source")); qdict_del(dict, "source"); } v = qobject_input_visitor_new_keyval(QOBJECT(dict)); qobject_unref(dict); visit_type_MigrateSetParameters(v, NULL, &incoming_params, &error_fatal); visit_free(v); qmp_migration_set_parameters(incoming_params, &error_fatal); qapi_free_MigrateSetParameters(incoming_params); } For example "-incoming [source=]tcp:foo,multifd-channels=16" would desugar to migrate_set_parameter multifd-channels 16 migrate_incoming tcp:foo The only incompatibility is for people who are using "," in an URI, which is rare and only an issue for the "exec" protocol. Paolo
and set the parameters in the same place as above. That would remove the need for "-global migration".
Could you elaborate?
The other option that I can think of is changing the error messages for migrate_check_parameters() and give instructions that you can't set multifd channels once that you have started incoming migration. Explaining there to use migrate_incoming command?
Later, Juan.
Paolo Bonzini <pbonzini@redhat.com> wrote:
On 6/22/23 10:52, Juan Quintela wrote:
User friendliness. The problem is that if you use more than two channels with multifd, on the incoming side, you need to do:
You're sacrificing user-friendliness for the 99.99% that don't use multifd, for an error (i.e. it's not even fixing the issue) for the 0.01% that use multifd. That's not user-friendly.
You are forgeting of the 0.01% that uses postocopy preempt (that is easy just changing the default value to 2), and the 0.0000001% that uses compression (they have exactly the same problem with compress_threads, compress_zlib, etc).
- migrate_set_parameter multifd-channels 16 - migrate_incoming <uri>
The issue is not how many features the command line has, but how they're implemented. Or if they are confusing for the user?
Anyone using multifd is not a typical user anyway.
If they're just QMP wrappers and as such they're self-contained in softmmu/vl.c, that's fine.
In fact, even for parameters, we could use keyval to parse "-incoming" What is keyval?
util/keyval.c and include/qemu/keyval.h. It parses a list of key=value pairs into a QDict. Once you have removed the "source" key from the QDict you can use a visitor to parse the rest into a MigrateSetParameters. See the handling of QEMU_OPTION_audio, it could be something like
case QEMU_OPTION_incoing: { Visitor *v; MigrateSetParameters *incoming_params = NULL; QDict *dict = keyval_parse(optarg, "source", NULL, &error_fatal);
if (incoming) { if (qdict_haskey(dict, "source")) { error_setg(&error_fatal, "Parameter 'source' is duplicate"); } } else { if (!qdict_haskey(dict, "source")) { error_setg(&error_fatal, "Parameter 'source' is missing"); } runstate_set(RUN_STATE_INMIGRATE); incoming = g_strdup(qdict_get_str(dict, "source")); qdict_del(dict, "source"); }
v = qobject_input_visitor_new_keyval(QOBJECT(dict)); qobject_unref(dict); visit_type_MigrateSetParameters(v, NULL, &incoming_params, &error_fatal); visit_free(v); qmp_migration_set_parameters(incoming_params, &error_fatal); qapi_free_MigrateSetParameters(incoming_params); }
For example "-incoming [source=]tcp:foo,multifd-channels=16" would desugar to
migrate_set_parameter multifd-channels 16 migrate_incoming tcp:foo
The only incompatibility is for people who are using "," in an URI, which is rare and only an issue for the "exec" protocol.
Aha, that makes sense. And will allow us to deprecate/remove the --global migration.* stuff. Thanks very much. See why this was an RFC O:-) Later, Juan.
Paolo
and set the parameters in the same place as above. That would remove the need for "-global migration". Could you elaborate?
The other option that I can think of is changing the error messages for migrate_check_parameters() and give instructions that you can't set multifd channels once that you have started incoming migration. Explaining there to use migrate_incoming command? Later, Juan.
On Thu, Jun 22, 2023 at 12:01:55PM +0200, Juan Quintela wrote:
Paolo Bonzini <pbonzini@redhat.com> wrote:
On 6/22/23 10:52, Juan Quintela wrote:
User friendliness. The problem is that if you use more than two channels with multifd, on the incoming side, you need to do:
You're sacrificing user-friendliness for the 99.99% that don't use multifd, for an error (i.e. it's not even fixing the issue) for the 0.01% that use multifd. That's not user-friendly.
You are forgeting of the 0.01% that uses postocopy preempt (that is easy just changing the default value to 2), and the 0.0000001% that uses compression (they have exactly the same problem with compress_threads, compress_zlib, etc).
- migrate_set_parameter multifd-channels 16 - migrate_incoming <uri>
The issue is not how many features the command line has, but how they're implemented. Or if they are confusing for the user?
Anyone using multifd is not a typical user anyway.
If they're just QMP wrappers and as such they're self-contained in softmmu/vl.c, that's fine.
In fact, even for parameters, we could use keyval to parse "-incoming" What is keyval?
util/keyval.c and include/qemu/keyval.h. It parses a list of key=value pairs into a QDict. Once you have removed the "source" key from the QDict you can use a visitor to parse the rest into a MigrateSetParameters. See the handling of QEMU_OPTION_audio, it could be something like
case QEMU_OPTION_incoing: { Visitor *v; MigrateSetParameters *incoming_params = NULL; QDict *dict = keyval_parse(optarg, "source", NULL, &error_fatal);
if (incoming) { if (qdict_haskey(dict, "source")) { error_setg(&error_fatal, "Parameter 'source' is duplicate"); } } else { if (!qdict_haskey(dict, "source")) { error_setg(&error_fatal, "Parameter 'source' is missing"); } runstate_set(RUN_STATE_INMIGRATE); incoming = g_strdup(qdict_get_str(dict, "source")); qdict_del(dict, "source"); }
v = qobject_input_visitor_new_keyval(QOBJECT(dict)); qobject_unref(dict); visit_type_MigrateSetParameters(v, NULL, &incoming_params, &error_fatal); visit_free(v); qmp_migration_set_parameters(incoming_params,
PS: we may want to postpone this to be later than migration_object_init(), when/if there's a real patch.
&error_fatal); qapi_free_MigrateSetParameters(incoming_params); }
For example "-incoming [source=]tcp:foo,multifd-channels=16" would desugar to
migrate_set_parameter multifd-channels 16 migrate_incoming tcp:foo
The only incompatibility is for people who are using "," in an URI, which is rare and only an issue for the "exec" protocol.
If we worry on breaking anyone, we can apply the keyval parsing only when !exec. Not sure whether it matters a huge lot..
Aha, that makes sense. And will allow us to deprecate/remove the --global migration.* stuff.
We may still need a way to set the caps/params for src qemu?.. Thanks, -- Peter Xu
On Thu, Jun 22, 2023 at 5:26 PM Peter Xu <peterx@redhat.com> wrote:
PS: we may want to postpone this to be later than migration_object_init(), when/if there's a real patch.
Yes, that's true.
The only incompatibility is for people who are using "," in an URI, which is rare and only an issue for the "exec" protocol.
If we worry on breaking anyone, we can apply the keyval parsing only when !exec. Not sure whether it matters a huge lot..
No, I don't think it does.
Aha, that makes sense. And will allow us to deprecate/remove the --global migration.* stuff.
We may still need a way to set the caps/params for src qemu?..
The source can use migrate_set_parameters normally, since migration has to be started from the monitor anyway. Paolo
On Thu, Jun 22, 2023 at 10:52:12AM +0200, Juan Quintela wrote:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 6/12/23 22:51, Juan Quintela wrote:
> >>> Shall we just leave it there? Or is deprecating it helps us in any form?
> >> See the patches two weeks ago when people complained that lisen(.., num)
> >> was too low. And there are other parameters that work the same way
> >> (that I convenientely had forgotten). So the easiest way to get things
> >> right is to use "defer" always. Using -incoming "uri" should only be
> >> for people that "know what they are doing", so we had to ways to do it:
> >> - review all migration options and see which ones work without defer
> >> and document it
> >> - deprecate everything that is not defer.
> >
> > "-incoming <uri>" is literally the same as running "migrate-incoming"
> > as the first thing on the monitor:
> >
> > if (incoming) {
> > Error *local_err = NULL;
> > if (strcmp(incoming, "defer") != 0) {
> > qmp_migrate_incoming(incoming, &local_err);
> > if (local_err) {
> > error_reportf_err(local_err, "-incoming %s: ", incoming);
> > exit(1);
> > }
> > }
> > } else if (autostart) {
> > qmp_cont(NULL);
> > }
> >
> > It's the only piece of code which distinguishes "-incoming defer" from
> > "-incoming <uri>".
> >
> > So I'm not sure what the problem would be with keeping it?
>
> User friendliness.
>
> First of all, I use it all the time. And I know that it is useful for
> developers. I was the one asking peter to implement -global
> migration.foo to be able to test multifd with it.
>
> The problem is that if you use more than two channels with multifd, on
> the incoming side, you need to do:
>
> - migrate_set_parameter multifd-channels 16
> - migrate_incoming <uri>
>
> And people continue to do:
>
> - qemu -incoming <uri>
> - migrate_set_parameter multifd-channels 16 (on the command line)
>
> And they complain that it fails, because we are calling listen with the
> wrong value.
IMHO if we want to improve user friendliness then arguing about use
of the CLI vs QMP for migration is completely missing the bigger
picture IMHO.
I've mentioned several times before that the user should never need to
set this multifd-channels parameter (nor many other parameters) on the
destination in the first place.
The QEMU migration stream should be changed to add a full
bi-directional handshake, with negotiation of most parameters.
IOW, the src QEMU should be configured with 16 channels, and
it should connect the primary control channel, and then directly
tell the dest that it wants to use 16 multifd channels.
If we're expecting the user to pass this info across to the dest
manually we've already spectacularly failed wrt user friendliness.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jun 22, 2023 at 10:59:58AM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 22, 2023 at 10:52:12AM +0200, Juan Quintela wrote:
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > On 6/12/23 22:51, Juan Quintela wrote:
> > >>> Shall we just leave it there? Or is deprecating it helps us in any form?
> > >> See the patches two weeks ago when people complained that lisen(.., num)
> > >> was too low. And there are other parameters that work the same way
> > >> (that I convenientely had forgotten). So the easiest way to get things
> > >> right is to use "defer" always. Using -incoming "uri" should only be
> > >> for people that "know what they are doing", so we had to ways to do it:
> > >> - review all migration options and see which ones work without defer
> > >> and document it
> > >> - deprecate everything that is not defer.
> > >
> > > "-incoming <uri>" is literally the same as running "migrate-incoming"
> > > as the first thing on the monitor:
> > >
> > > if (incoming) {
> > > Error *local_err = NULL;
> > > if (strcmp(incoming, "defer") != 0) {
> > > qmp_migrate_incoming(incoming, &local_err);
> > > if (local_err) {
> > > error_reportf_err(local_err, "-incoming %s: ", incoming);
> > > exit(1);
> > > }
> > > }
> > > } else if (autostart) {
> > > qmp_cont(NULL);
> > > }
> > >
> > > It's the only piece of code which distinguishes "-incoming defer" from
> > > "-incoming <uri>".
> > >
> > > So I'm not sure what the problem would be with keeping it?
> >
> > User friendliness.
> >
> > First of all, I use it all the time. And I know that it is useful for
> > developers. I was the one asking peter to implement -global
> > migration.foo to be able to test multifd with it.
> >
> > The problem is that if you use more than two channels with multifd, on
> > the incoming side, you need to do:
> >
> > - migrate_set_parameter multifd-channels 16
> > - migrate_incoming <uri>
> >
> > And people continue to do:
> >
> > - qemu -incoming <uri>
> > - migrate_set_parameter multifd-channels 16 (on the command line)
> >
> > And they complain that it fails, because we are calling listen with the
> > wrong value.
>
> IMHO if we want to improve user friendliness then arguing about use
> of the CLI vs QMP for migration is completely missing the bigger
> picture IMHO.
>
> I've mentioned several times before that the user should never need to
> set this multifd-channels parameter (nor many other parameters) on the
> destination in the first place.
>
> The QEMU migration stream should be changed to add a full
> bi-directional handshake, with negotiation of most parameters.
> IOW, the src QEMU should be configured with 16 channels, and
> it should connect the primary control channel, and then directly
> tell the dest that it wants to use 16 multifd channels.
>
> If we're expecting the user to pass this info across to the dest
> manually we've already spectacularly failed wrt user friendliness.
I can try to move the todo even higher. Trying to list the initial goals
here:
- One extra phase of handshake between src/dst (maybe the time to boost
QEMU_VM_FILE_VERSION) before anything else happens.
- Dest shouldn't need to apply any cap/param, it should get all from src.
Dest still need to be setup with an URI and that should be all it needs.
- Src shouldn't need to worry on the binary version of dst anymore as long
as dest qemu supports handshake, because src can fetch it from dest.
- Handshake can always fail gracefully if anything wrong happened, it
normally should mean dest qemu is not compatible with src's setup (either
machine, device, or migration configs) for whatever reason. Src should
be able to get a solid error from dest if so.
- Handshake protocol should always be self-bootstrap-able, it means when we
change the handshake protocol it should always works with old binaries.
- When src is newer it should be able to know what's missing on dest and
skip the new bits.
- When dst is newer it should all rely on src (which is older) and it
should always understand src's language.
- All !main channels need to be established later than the handshake - if
we're going to do this anyway we probably should do it altogether to make
channels named, so each channel used in migration needs to have a common
header. Prepare to deprecate the old tricks of channel orderings.
Does it look reasonable? Anything to add/remove?
Thanks,
--
Peter Xu
On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote:
I can try to move the todo even higher. Trying to list the initial goals here:
- One extra phase of handshake between src/dst (maybe the time to boost QEMU_VM_FILE_VERSION) before anything else happens.
- Dest shouldn't need to apply any cap/param, it should get all from src. Dest still need to be setup with an URI and that should be all it needs.
- Src shouldn't need to worry on the binary version of dst anymore as long as dest qemu supports handshake, because src can fetch it from dest.
I'm not sure that works in general. Even if we have a handshake and bi-directional comms for live migration, we still haave the save/restore to file codepath to deal with. The dst QEMU doesn't exist at the time the save process is done, so we can't add logic to VMSate handling that assumes knowledge of the dst version at time of serialization.
- Handshake can always fail gracefully if anything wrong happened, it normally should mean dest qemu is not compatible with src's setup (either machine, device, or migration configs) for whatever reason. Src should be able to get a solid error from dest if so.
- Handshake protocol should always be self-bootstrap-able, it means when we change the handshake protocol it should always works with old binaries.
- When src is newer it should be able to know what's missing on dest and skip the new bits.
- When dst is newer it should all rely on src (which is older) and it should always understand src's language.
I'm not convinced it can reliably self-bootstrap in a backwards compatible manner, precisely because the current migration stream has no handshake and only requires a unidirectional channel. I don't think its possible for QEMU to validate that it has a fully bi-directional channel, without adding timeouts to its detection which I think we should strive to avoid. I don't think we actually need self-bootstrapping anyway. I think the mgmt app can just indicate the new v2 bi-directional protocol when issuing the 'migrate' and 'migrate-incoming' commands. This becomes trivial when Het's refactoring of the migrate address QAPI is accepted: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04851.html eg: { "execute": "migrate", "arguments": { "channels": [ { "channeltype": "main", "addr": { "transport": "socket", "type": "inet", "host": "10.12.34.9", "port": "1050" } } ] } } note the 'channeltype' parameter here. If we declare the 'main' refers to the existing migration protocol, then we merely need to define a new 'channeltype' to use as an indicator for the v2 migration handshake protocol.
- All !main channels need to be established later than the handshake - if we're going to do this anyway we probably should do it altogether to make channels named, so each channel used in migration needs to have a common header. Prepare to deprecate the old tricks of channel orderings.
Once the primary channel involves a bi-directional handshake, we'll trivially ensure ordering - similar to how the existing code worked fnie in TLS mode which had a bi-directional TLS handshake. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jun 22, 2023 at 05:33:29PM +0100, Daniel P. Berrangé wrote:
On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote:
I can try to move the todo even higher. Trying to list the initial goals here:
- One extra phase of handshake between src/dst (maybe the time to boost QEMU_VM_FILE_VERSION) before anything else happens.
- Dest shouldn't need to apply any cap/param, it should get all from src. Dest still need to be setup with an URI and that should be all it needs.
- Src shouldn't need to worry on the binary version of dst anymore as long as dest qemu supports handshake, because src can fetch it from dest.
I'm not sure that works in general. Even if we have a handshake and bi-directional comms for live migration, we still haave the save/restore to file codepath to deal with. The dst QEMU doesn't exist at the time the save process is done, so we can't add logic to VMSate handling that assumes knowledge of the dst version at time of serialization.
My current thought was still based on a new cap or anything the user would need to specify first on both sides (but hopefully the last cap to set on dest). E.g. if with a new handshake cap we shouldn't set it on a exec: or file: protocol migration, and it should just fail on qmp_migrate() telling that the URI is not supported if the cap is set. Return path is definitely required here.
- Handshake can always fail gracefully if anything wrong happened, it normally should mean dest qemu is not compatible with src's setup (either machine, device, or migration configs) for whatever reason. Src should be able to get a solid error from dest if so.
- Handshake protocol should always be self-bootstrap-able, it means when we change the handshake protocol it should always works with old binaries.
- When src is newer it should be able to know what's missing on dest and skip the new bits.
- When dst is newer it should all rely on src (which is older) and it should always understand src's language.
I'm not convinced it can reliably self-bootstrap in a backwards compatible manner, precisely because the current migration stream has no handshake and only requires a unidirectional channel.
Yes, please see above. I meant when we grow the handshake protocol we should make sure we don't need anything new to be setup either on src/dst of qemu. It won't apply to before-handshake binaries.
I don't think its possible for QEMU to validate that it has a fully bi-directional channel, without adding timeouts to its detection which I think we should strive to avoid.
I don't think we actually need self-bootstrapping anyway.
I think the mgmt app can just indicate the new v2 bi-directional protocol when issuing the 'migrate' and 'migrate-incoming' commands. This becomes trivial when Het's refactoring of the migrate address QAPI is accepted:
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04851.html
eg:
{ "execute": "migrate", "arguments": { "channels": [ { "channeltype": "main", "addr": { "transport": "socket", "type": "inet", "host": "10.12.34.9", "port": "1050" } } ] } }
note the 'channeltype' parameter here. If we declare the 'main' refers to the existing migration protocol, then we merely need to define a new 'channeltype' to use as an indicator for the v2 migration handshake protocol.
Using a new channeltype would also work at least on src qemu, but I'm not sure on how dest qemu would know that it needs a handshake in that case, because it knows nothing until the connection is established. Maybe we still need QEMU_VM_FILE_VERSION to be boosted at least in this case, so dest can read this at the very beginning, old binaries will fail immediately, new binaries will start to talk with v2 language.
- All !main channels need to be established later than the handshake - if we're going to do this anyway we probably should do it altogether to make channels named, so each channel used in migration needs to have a common header. Prepare to deprecate the old tricks of channel orderings.
Once the primary channel involves a bi-directional handshake, we'll trivially ensure ordering - similar to how the existing code worked fnie in TLS mode which had a bi-directional TLS handshake.
I'm not sure I fully get it here. IIUC tls handshake was mostly transparent to QEMU in this case while we're relying on gnutls_handshake(). Here IIUC we need to design the roundtrip messages to sync up two qemus well. The round trip messages can contain a lot of things that can be useful to us, besides knowing what features dest supports, what caps src use, we can e.g. also provide a device tree dump from dest and try to match it on src, failing the migration very early if we see any mismatch. Right now we fail too late, only until the device load (which is the last stage). For channel orders, I'd expect the v2 protocol contains a phase to talk on the channels and creation of named channels should be part of setup phase before anything will happen next. Thanks, -- Peter Xu
On Thu, Jun 22, 2023 at 03:20:01PM -0400, Peter Xu wrote:
On Thu, Jun 22, 2023 at 05:33:29PM +0100, Daniel P. Berrangé wrote:
On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote:
I can try to move the todo even higher. Trying to list the initial goals here:
- One extra phase of handshake between src/dst (maybe the time to boost QEMU_VM_FILE_VERSION) before anything else happens.
- Dest shouldn't need to apply any cap/param, it should get all from src. Dest still need to be setup with an URI and that should be all it needs.
- Src shouldn't need to worry on the binary version of dst anymore as long as dest qemu supports handshake, because src can fetch it from dest.
I'm not sure that works in general. Even if we have a handshake and bi-directional comms for live migration, we still haave the save/restore to file codepath to deal with. The dst QEMU doesn't exist at the time the save process is done, so we can't add logic to VMSate handling that assumes knowledge of the dst version at time of serialization.
My current thought was still based on a new cap or anything the user would need to specify first on both sides (but hopefully the last cap to set on dest).
E.g. if with a new handshake cap we shouldn't set it on a exec: or file: protocol migration, and it should just fail on qmp_migrate() telling that the URI is not supported if the cap is set. Return path is definitely required here.
exec can support bi-directional migration - we have both stdin + stdout for the command. For exec it is mostly a documentation problem - you can't merely use 'cat' for example, but if you used 'socat' that could be made to work bi-directionally.
I don't think its possible for QEMU to validate that it has a fully bi-directional channel, without adding timeouts to its detection which I think we should strive to avoid.
I don't think we actually need self-bootstrapping anyway.
I think the mgmt app can just indicate the new v2 bi-directional protocol when issuing the 'migrate' and 'migrate-incoming' commands. This becomes trivial when Het's refactoring of the migrate address QAPI is accepted:
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04851.html
eg:
{ "execute": "migrate", "arguments": { "channels": [ { "channeltype": "main", "addr": { "transport": "socket", "type": "inet", "host": "10.12.34.9", "port": "1050" } } ] } }
note the 'channeltype' parameter here. If we declare the 'main' refers to the existing migration protocol, then we merely need to define a new 'channeltype' to use as an indicator for the v2 migration handshake protocol.
Using a new channeltype would also work at least on src qemu, but I'm not sure on how dest qemu would know that it needs a handshake in that case, because it knows nothing until the connection is established.
In Het's series the 'migrate_incoming' command similarly has a chaneltype parameter. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jun 23, 2023 at 08:17:46AM +0100, Daniel P. Berrangé wrote:
On Thu, Jun 22, 2023 at 03:20:01PM -0400, Peter Xu wrote:
On Thu, Jun 22, 2023 at 05:33:29PM +0100, Daniel P. Berrangé wrote:
On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote:
I can try to move the todo even higher. Trying to list the initial goals here:
- One extra phase of handshake between src/dst (maybe the time to boost QEMU_VM_FILE_VERSION) before anything else happens.
- Dest shouldn't need to apply any cap/param, it should get all from src. Dest still need to be setup with an URI and that should be all it needs.
- Src shouldn't need to worry on the binary version of dst anymore as long as dest qemu supports handshake, because src can fetch it from dest.
I'm not sure that works in general. Even if we have a handshake and bi-directional comms for live migration, we still haave the save/restore to file codepath to deal with. The dst QEMU doesn't exist at the time the save process is done, so we can't add logic to VMSate handling that assumes knowledge of the dst version at time of serialization.
My current thought was still based on a new cap or anything the user would need to specify first on both sides (but hopefully the last cap to set on dest).
E.g. if with a new handshake cap we shouldn't set it on a exec: or file: protocol migration, and it should just fail on qmp_migrate() telling that the URI is not supported if the cap is set. Return path is definitely required here.
exec can support bi-directional migration - we have both stdin + stdout for the command. For exec it is mostly a documentation problem - you can't merely use 'cat' for example, but if you used 'socat' that could be made to work bi-directionally.
Okay. Just an example that the handshake just cannot work for all the cases, and it should always be able to fail. So when exec doesn't properly provide return path, I think with handshake=on we should get a timeout of not getting response properly and fail the migration after the timeout, then. There're a bunch of implications and details that need to be investigated around such a handshake if it'll be proposed for real, so I'm not yet sure whether there's something that may be surprising. For channeltypes it seems all fine for now. Hopefully nothing obvious overlooked.
I don't think its possible for QEMU to validate that it has a fully bi-directional channel, without adding timeouts to its detection which I think we should strive to avoid.
I don't think we actually need self-bootstrapping anyway.
I think the mgmt app can just indicate the new v2 bi-directional protocol when issuing the 'migrate' and 'migrate-incoming' commands. This becomes trivial when Het's refactoring of the migrate address QAPI is accepted:
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04851.html
eg:
{ "execute": "migrate", "arguments": { "channels": [ { "channeltype": "main", "addr": { "transport": "socket", "type": "inet", "host": "10.12.34.9", "port": "1050" } } ] } }
note the 'channeltype' parameter here. If we declare the 'main' refers to the existing migration protocol, then we merely need to define a new 'channeltype' to use as an indicator for the v2 migration handshake protocol.
Using a new channeltype would also work at least on src qemu, but I'm not sure on how dest qemu would know that it needs a handshake in that case, because it knows nothing until the connection is established.
In Het's series the 'migrate_incoming' command similarly has a chaneltype parameter.
Oh, yeah then that'll just work. Thanks. -- Peter Xu
On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote:
On Thu, Jun 22, 2023 at 10:59:58AM +0100, Daniel P. Berrangé wrote:
I've mentioned several times before that the user should never need to set this multifd-channels parameter (nor many other parameters) on the destination in the first place.
The QEMU migration stream should be changed to add a full bi-directional handshake, with negotiation of most parameters. IOW, the src QEMU should be configured with 16 channels, and it should connect the primary control channel, and then directly tell the dest that it wants to use 16 multifd channels.
If we're expecting the user to pass this info across to the dest manually we've already spectacularly failed wrt user friendliness.
I can try to move the todo even higher. Trying to list the initial goals here:
- One extra phase of handshake between src/dst (maybe the time to boost QEMU_VM_FILE_VERSION) before anything else happens.
- Dest shouldn't need to apply any cap/param, it should get all from src. Dest still need to be setup with an URI and that should be all it needs.
There are a few that the dest will still need set explicitly. Specifically the TLS parameters - tls-authz and tls-creds, because those are both related to --object parameters configured on the dst QEMU. Potentially there's an argument to be made for the TLS parameters to be part fo the initial 'migrate' and 'migrate-incoming' command data, as they're specifically related to the connection establishment, while (most) of the other params are related to the migration protocol running inside the connection. A few other parameters are also related to the connection establishment, most notably the enablement multifd, postcopy and postcopy-pre-empt. I think with those ones we don't need to set them on the src either. With the new migration handshake we should probably use multifd codepaths unconditionally, with a single channel. By matching with the introduction of new protocol, we have a nice point against which to deprecate the old non-multifd codepaths. We'll need to keep the non-multifd code around *alot* longer than the normal deprecation cycle though, as we need mig to/from very old QEMUs. The enablement of postcopy could be automatic too - src & dst can both detect if their host OS supports it. That would make all migrations post-copy capable. The mgmt app just needs to trigger the switch to post-copy mode *if* they want to use it. Likewise we can just always assume postcopy-pre-empt is available. I think 'return-path' becomes another one we can just assume too. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jun 23, 2023 at 09:23:18AM +0100, Daniel P. Berrangé wrote:
On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote:
On Thu, Jun 22, 2023 at 10:59:58AM +0100, Daniel P. Berrangé wrote:
I've mentioned several times before that the user should never need to set this multifd-channels parameter (nor many other parameters) on the destination in the first place.
The QEMU migration stream should be changed to add a full bi-directional handshake, with negotiation of most parameters. IOW, the src QEMU should be configured with 16 channels, and it should connect the primary control channel, and then directly tell the dest that it wants to use 16 multifd channels.
If we're expecting the user to pass this info across to the dest manually we've already spectacularly failed wrt user friendliness.
I can try to move the todo even higher. Trying to list the initial goals here:
- One extra phase of handshake between src/dst (maybe the time to boost QEMU_VM_FILE_VERSION) before anything else happens.
- Dest shouldn't need to apply any cap/param, it should get all from src. Dest still need to be setup with an URI and that should be all it needs.
There are a few that the dest will still need set explicitly. Specifically the TLS parameters - tls-authz and tls-creds, because those are both related to --object parameters configured on the dst QEMU. Potentially there's an argument to be made for the TLS parameters to be part fo the initial 'migrate' and 'migrate-incoming' command data, as they're specifically related to the connection establishment, while (most) of the other params are related to the migration protocol running inside the connection.
Ideally we can even make tls options to be after the main connection is established, IOW the gnutls handshake can be part of the generic handshake. But yeah I agree that may contain much more work, so we may start with assuming the v2 handshake just happen on the tls channel built for now. I think the new protocol should allow extension so when we want to move the tls handshake into it v2 protocol should be able to first detect src/dst binary support of that, and switch to that if we want - then we can even got a src qemu migration failure which tells "dest qemu forget to setup tls credentials in cmdlines", or anything wrong on dest during tls setup.
A few other parameters are also related to the connection establishment, most notably the enablement multifd, postcopy and postcopy-pre-empt.
As I mentioned in the list, I plan to make this part of the default v2 where v2 handshake will take care of managing the connections rather than relying on the old code. I'm not sure how complicated it'll be, but the v2 protocol just sounds a good fit for having such a major change on how we setup the channels, and chance we get all things alright from the start.
I think with those ones we don't need to set them on the src either. With the new migration handshake we should probably use multifd codepaths unconditionally, with a single channel.
The v2 handshake will be beneficial to !multifd as well. Right now I tend to make it also work for !multifd, e.g., it always makes sense to do a device tree comparision before migration, even if someone used special tunneling so multifd may not be able to be enabled for whatever reason, but as long as a return path is available so they can talk.
By matching with the introduction of new protocol, we have a nice point against which to deprecate the old non-multifd codepaths. We'll need to keep the non-multifd code around *alot* longer than the normal deprecation cycle though, as we need mig to/from very old QEMUs.
I actually had a feeling that we should always keep it.. I'm not sure whether we must combine a new handshake to "making multifd the default". I do think we can make the !multifd path very simple though, e.g., I'd think we should start considering deprecate things like !multifd+compressions etc earlier than that.
The enablement of postcopy could be automatic too - src & dst can both detect if their host OS supports it. That would make all migrations post-copy capable. The mgmt app just needs to trigger the switch to post-copy mode *if* they want to use it.
Sounds doable.
Likewise we can just always assume postcopy-pre-empt is available.
I think 'return-path' becomes another one we can just assume too.
Right, handshake cap (or with the new QAPI of URI replacement) should imply return path already. Thanks, -- Peter Xu
On Fri, Jun 23, 2023 at 10:51:53AM -0400, Peter Xu wrote:
On Fri, Jun 23, 2023 at 09:23:18AM +0100, Daniel P. Berrangé wrote:
On Thu, Jun 22, 2023 at 11:54:43AM -0400, Peter Xu wrote:
On Thu, Jun 22, 2023 at 10:59:58AM +0100, Daniel P. Berrangé wrote:
I've mentioned several times before that the user should never need to set this multifd-channels parameter (nor many other parameters) on the destination in the first place.
The QEMU migration stream should be changed to add a full bi-directional handshake, with negotiation of most parameters. IOW, the src QEMU should be configured with 16 channels, and it should connect the primary control channel, and then directly tell the dest that it wants to use 16 multifd channels.
If we're expecting the user to pass this info across to the dest manually we've already spectacularly failed wrt user friendliness.
I can try to move the todo even higher. Trying to list the initial goals here:
- One extra phase of handshake between src/dst (maybe the time to boost QEMU_VM_FILE_VERSION) before anything else happens.
- Dest shouldn't need to apply any cap/param, it should get all from src. Dest still need to be setup with an URI and that should be all it needs.
There are a few that the dest will still need set explicitly. Specifically the TLS parameters - tls-authz and tls-creds, because those are both related to --object parameters configured on the dst QEMU. Potentially there's an argument to be made for the TLS parameters to be part fo the initial 'migrate' and 'migrate-incoming' command data, as they're specifically related to the connection establishment, while (most) of the other params are related to the migration protocol running inside the connection.
Ideally we can even make tls options to be after the main connection is established, IOW the gnutls handshake can be part of the generic handshake. But yeah I agree that may contain much more work, so we may start with assuming the v2 handshake just happen on the tls channel built for now.
I think the new protocol should allow extension so when we want to move the tls handshake into it v2 protocol should be able to first detect src/dst binary support of that, and switch to that if we want - then we can even got a src qemu migration failure which tells "dest qemu forget to setup tls credentials in cmdlines", or anything wrong on dest during tls setup.
Doing negotiated "upgrades" from plain to TLS mode is generally frowned upon, as it opens up potentially dangerous attack routes which can prevent the upgrade from happening. If the user/app controlling the client and server side of a connection both know they want TLS, the best practice is for a connection to start in TLS mode *immediately*, never sending any data in the clear. We have this ability in QEMU right now, with libvirt explicitly enabling TLS mode on src + dst, and we should keep that in any v2 migration protocol.
A few other parameters are also related to the connection establishment, most notably the enablement multifd, postcopy and postcopy-pre-empt.
As I mentioned in the list, I plan to make this part of the default v2 where v2 handshake will take care of managing the connections rather than relying on the old code. I'm not sure how complicated it'll be, but the v2 protocol just sounds a good fit for having such a major change on how we setup the channels, and chance we get all things alright from the start.
I think with those ones we don't need to set them on the src either. With the new migration handshake we should probably use multifd codepaths unconditionally, with a single channel.
The v2 handshake will be beneficial to !multifd as well. Right now I tend to make it also work for !multifd, e.g., it always makes sense to do a device tree comparision before migration, even if someone used special tunneling so multifd may not be able to be enabled for whatever reason, but as long as a return path is available so they can talk.
By matching with the introduction of new protocol, we have a nice point against which to deprecate the old non-multifd codepaths. We'll need to keep the non-multifd code around *alot* longer than the normal deprecation cycle though, as we need mig to/from very old QEMUs.
I actually had a feeling that we should always keep it.. I'm not sure whether we must combine a new handshake to "making multifd the default". I do think we can make the !multifd path very simple though, e.g., I'd think we should start considering deprecate things like !multifd+compressions etc earlier than that.
My thought is that right now QEMU has effectively has two completely distinct migration protocols (even though they share the initial phase), and two distinct internal code paths, one for traditional single channel and one for the multifd. IIUC, Juan expressed a desire to get rid of non-multifd migration. The deprecation of compression, with the message that users should use compression on multifd just re-inforces this view the non-multifd is a dead end. If we go for a new v2 protocol, we're adding another network protocol to the testing matrix. If we support the new protocol for both non-multifd and multifd, then we've got two additions to the testing matrix instead of just one extra. I think that's a bad idea if we're intending to get rid of non-multifd codepaths in future. The deprecation period of getting rid of non-multifd will be long, so the sooner we start that the better. The deprecation period for getting rid of v1 protocol and more to exclusively v2, will be similarly long. Overall I think it is better for us to align the two and keep non-multifd strictly v1 only. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 12/06/2023 21.33, Juan Quintela wrote:
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ softmmu/vl.c | 2 ++ 2 files changed, 9 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47e98dc95e..518672722d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -447,3 +447,10 @@ The new way to modify migration is using migration parameters. ``blk`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``.
+``-incoming uri`` (since 8.1) +''''''''''''''''''''''''''''' + +Everything except ``-incoming defer`` are deprecated. This allows to +setup parameters before launching the proper migration with +``migrate-incoming uri``. + diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..7fe865ab59 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming); qmp_migrate_incoming(incoming, &local_err); if (local_err) { error_reportf_err(local_err, "-incoming %s: ", incoming);
Could we maybe keep at least the smallest set of necessary parameters around? I'm often doing a "-incoming tcp:0:1234" for doing quick sanity checks with migration, not caring about other migration parameters, so if that could continue to work, that would be very appreciated. Thomas
Thomas Huth <thuth@redhat.com> wrote:
On 12/06/2023 21.33, Juan Quintela wrote:
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command. Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 7 +++++++ softmmu/vl.c | 2 ++ 2 files changed, 9 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47e98dc95e..518672722d 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -447,3 +447,10 @@ The new way to modify migration is using migration parameters. ``blk`` functionality can be acchieved using ``migrate_set_parameter block-incremental true``. +``-incoming uri`` (since 8.1) +''''''''''''''''''''''''''''' + +Everything except ``-incoming defer`` are deprecated. This allows to +setup parameters before launching the proper migration with +``migrate-incoming uri``. + diff --git a/softmmu/vl.c b/softmmu/vl.c index b0b96f67fa..7fe865ab59 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming); qmp_migrate_incoming(incoming, &local_err); if (local_err) { error_reportf_err(local_err, "-incoming %s: ", incoming);
Could we maybe keep at least the smallest set of necessary parameters around? I'm often doing a "-incoming tcp:0:1234" for doing quick sanity checks with migration, not caring about other migration parameters, so if that could continue to work, that would be very appreciated.
I will try to explain myself here. I think that everything except tcp works. But when we have tcp, we have two cases where this is a trap: - multifd channels: * if we default to a big number, we underuse resources in a normal case * if we default to a small number, we have the problem that if the user set "later" multifd-channels to a bigger number, things can break. - postcopy+preempt: this case is also problematic, but easily fixable. Put a default of 2 instead of 1. The only other solution that I can think of is just fail if we set multifd without incoming defer. But more sooner than later we are going to have to default to multifd, so ... Later, Juan.
On 6/21/23 09:08, Thomas Huth wrote:
if (strcmp(incoming, "defer") != 0) { + warn_report("-incoming %s is deprecated, use -incoming defer and " + " set the uri with migrate-incoming.", incoming); qmp_migrate_incoming(incoming, &local_err); if (local_err) { error_reportf_err(local_err, "-incoming %s: ", incoming);
Could we maybe keep at least the smallest set of necessary parameters around? I'm often doing a "-incoming tcp:0:1234" for doing quick sanity checks with migration, not caring about other migration parameters, so if that could continue to work, that would be very appreciated.
Yeah, this is throwing the baby out with the bathwater. Paolo
Juan Quintela <quintela@redhat.com> wrote:
Only "defer" is recommended. After setting all migation parameters, start incoming migration with "migrate-incoming uri" command.
Signed-off-by: Juan Quintela <quintela@redhat.com>
Nack myself. Dropped on next submissiong. keyfile properties suggested by paolo is a much better suggestion. Thanks to everybody involved.
It is obsolete. It is better to use driver_mirror+NBD instead. CC: Kevin Wolf <kwolf@redhat.com> CC: Eric Blake <eblake@redhat.com> CC: Stefan Hajnoczi <stefanha@redhat.com> CC: Hanna Czenczek <hreitz@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> --- Can any of you give one example of how to use driver_mirror+NBD for deprecated.rst? Thanks, Juan. --- docs/about/deprecated.rst | 6 ++++++ qapi/migration.json | 29 +++++++++++++++++++++++++---- migration/block.c | 2 ++ migration/options.c | 7 +++++++ 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 518672722d..173c5ba5cb 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -454,3 +454,9 @@ Everything except ``-incoming defer`` are deprecated. This allows to setup parameters before launching the proper migration with ``migrate-incoming uri``. +block migration (since 8.1) +''''''''''''''''''''''''''' + +Block migration is too inflexible. It needs to migrate all block +devices or none. Use driver_mirror+NBD instead. + diff --git a/qapi/migration.json b/qapi/migration.json index b71e00737e..a8497de48d 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -258,11 +258,16 @@ # blocked. Present and non-empty when migration is blocked. # (since 6.0) # +# Features: +# +# @deprecated: @disk migration is deprecated. Use driver_mirror+NBD +# instead. +# # Since: 0.14 ## { 'struct': 'MigrationInfo', 'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats', - '*disk': 'MigrationStats', + '*disk': { 'type': 'MigrationStats', 'features': ['deprecated'] }, '*vfio': 'VfioStats', '*xbzrle-cache': 'XBZRLECacheStats', '*total-time': 'int', @@ -497,6 +502,9 @@ # # Features: # +# @deprecated: @block migration is deprecated. Use driver_mirror+NBD +# instead. +# # @unstable: Members @x-colo and @x-ignore-shared are experimental. # # Since: 1.2 @@ -506,7 +514,8 @@ 'compress', 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram', - 'block', 'return-path', 'pause-before-switchover', 'multifd', + { 'name': 'block', 'features': [ 'deprecated' ] }, + 'return-path', 'pause-before-switchover', 'multifd', 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, 'validate-uuid', 'background-snapshot', @@ -789,6 +798,9 @@ # # Features: # +# @deprecated: Member @block-incremental is obsolete. Use +# driver_mirror+NBD instead. +# # @unstable: Member @x-checkpoint-delay is experimental. # # Since: 2.4 @@ -803,7 +815,7 @@ 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', 'downtime-limit', { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, - 'block-incremental', + { 'name': 'block-incremental', 'features': [ 'deprecated' ] }, 'multifd-channels', 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-compression', @@ -945,6 +957,9 @@ # # Features: # +# @deprecated: Member @block-incremental is obsolete. Use +# driver_mirror+NBD instead. +# # @unstable: Member @x-checkpoint-delay is experimental. # # TODO: either fuse back into MigrationParameters, or make @@ -972,7 +987,8 @@ '*downtime-limit': 'uint64', '*x-checkpoint-delay': { 'type': 'uint32', 'features': [ 'unstable' ] }, - '*block-incremental': 'bool', + '*block-incremental': { 'type': 'bool', + 'features': [ 'deprecated' ] }, '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', @@ -1137,6 +1153,9 @@ # # Features: # +# @deprecated: Member @block-incremental is obsolete. Use +# driver_mirror+NBD instead. +# # @unstable: Member @x-checkpoint-delay is experimental. # # Since: 2.4 @@ -1161,6 +1180,8 @@ '*downtime-limit': 'uint64', '*x-checkpoint-delay': { 'type': 'uint32', 'features': [ 'unstable' ] }, + '*block-incremental': { 'type': 'bool', + 'features': [ 'deprecated' ] }, '*block-incremental': 'bool', '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', diff --git a/migration/block.c b/migration/block.c index b9580a6c7e..2521a22269 100644 --- a/migration/block.c +++ b/migration/block.c @@ -722,6 +722,8 @@ static int block_save_setup(QEMUFile *f, void *opaque) trace_migration_block_save("setup", block_mig_state.submitted, block_mig_state.transferred); + warn_report("block migration is deprecated. Use mirror jobs instead."); + qemu_mutex_lock_iothread(); ret = init_blk_migration(f); if (ret < 0) { diff --git a/migration/options.c b/migration/options.c index b62ab30cd5..374dc0767e 100644 --- a/migration/options.c +++ b/migration/options.c @@ -12,6 +12,7 @@ */ #include "qemu/osdep.h" +#include "qemu/error-report.h" #include "exec/target_page.h" #include "qapi/clone-visitor.h" #include "qapi/error.h" @@ -437,6 +438,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) return false; } #endif + if (new_caps[MIGRATION_CAPABILITY_BLOCK]) { + warn_report("Block migration is deprecated. " + "Use driver_mirror+NBD instead."); + } #ifndef CONFIG_REPLICATION if (new_caps[MIGRATION_CAPABILITY_X_COLO]) { @@ -1257,6 +1262,8 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) } if (params->has_block_incremental) { + warn_report("Block migration is deprecated. " + "Use driver_mirror+NBD instead."); s->parameters.block_incremental = params->block_incremental; } if (params->has_multifd_channels) { -- 2.40.1
On Mon, Jun 12, 2023 at 09:33:43PM +0200, Juan Quintela wrote:
It is obsolete. It is better to use driver_mirror+NBD instead.
CC: Kevin Wolf <kwolf@redhat.com> CC: Eric Blake <eblake@redhat.com> CC: Stefan Hajnoczi <stefanha@redhat.com> CC: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
Can any of you give one example of how to use driver_mirror+NBD for deprecated.rst?
Please see "QMP invocation for live storage migration with ``drive-mirror`` + NBD" in docs/interop/live-block-operations.rst for a detailed explanation.
Thanks, Juan. --- docs/about/deprecated.rst | 6 ++++++ qapi/migration.json | 29 +++++++++++++++++++++++++---- migration/block.c | 2 ++ migration/options.c | 7 +++++++ 4 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 518672722d..173c5ba5cb 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -454,3 +454,9 @@ Everything except ``-incoming defer`` are deprecated. This allows to setup parameters before launching the proper migration with ``migrate-incoming uri``.
+block migration (since 8.1) +''''''''''''''''''''''''''' + +Block migration is too inflexible. It needs to migrate all block +devices or none. Use driver_mirror+NBD instead.
blockdev-mirror with NBD
+ diff --git a/qapi/migration.json b/qapi/migration.json index b71e00737e..a8497de48d 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -258,11 +258,16 @@ # blocked. Present and non-empty when migration is blocked. # (since 6.0) # +# Features: +# +# @deprecated: @disk migration is deprecated. Use driver_mirror+NBD
blockdev-mirror with NBD
+# instead. +# # Since: 0.14 ## { 'struct': 'MigrationInfo', 'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats', - '*disk': 'MigrationStats', + '*disk': { 'type': 'MigrationStats', 'features': ['deprecated'] }, '*vfio': 'VfioStats', '*xbzrle-cache': 'XBZRLECacheStats', '*total-time': 'int', @@ -497,6 +502,9 @@ # # Features: # +# @deprecated: @block migration is deprecated. Use driver_mirror+NBD
blockdev-mirror with NBD
+# instead. +# # @unstable: Members @x-colo and @x-ignore-shared are experimental. # # Since: 1.2 @@ -506,7 +514,8 @@ 'compress', 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram', - 'block', 'return-path', 'pause-before-switchover', 'multifd', + { 'name': 'block', 'features': [ 'deprecated' ] }, + 'return-path', 'pause-before-switchover', 'multifd', 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, 'validate-uuid', 'background-snapshot', @@ -789,6 +798,9 @@ # # Features: # +# @deprecated: Member @block-incremental is obsolete. Use +# driver_mirror+NBD instead.
blockdev-mirror with NBD
+# # @unstable: Member @x-checkpoint-delay is experimental. # # Since: 2.4 @@ -803,7 +815,7 @@ 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', 'downtime-limit', { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, - 'block-incremental', + { 'name': 'block-incremental', 'features': [ 'deprecated' ] }, 'multifd-channels', 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-compression', @@ -945,6 +957,9 @@ # # Features: # +# @deprecated: Member @block-incremental is obsolete. Use +# driver_mirror+NBD instead.
blockdev-mirror with NBD
+# # @unstable: Member @x-checkpoint-delay is experimental. # # TODO: either fuse back into MigrationParameters, or make @@ -972,7 +987,8 @@ '*downtime-limit': 'uint64', '*x-checkpoint-delay': { 'type': 'uint32', 'features': [ 'unstable' ] }, - '*block-incremental': 'bool', + '*block-incremental': { 'type': 'bool', + 'features': [ 'deprecated' ] }, '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', @@ -1137,6 +1153,9 @@ # # Features: # +# @deprecated: Member @block-incremental is obsolete. Use +# driver_mirror+NBD instead.
blockdev-mirror with NBD
+# # @unstable: Member @x-checkpoint-delay is experimental. # # Since: 2.4 @@ -1161,6 +1180,8 @@ '*downtime-limit': 'uint64', '*x-checkpoint-delay': { 'type': 'uint32', 'features': [ 'unstable' ] }, + '*block-incremental': { 'type': 'bool', + 'features': [ 'deprecated' ] }, '*block-incremental': 'bool', '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', diff --git a/migration/block.c b/migration/block.c index b9580a6c7e..2521a22269 100644 --- a/migration/block.c +++ b/migration/block.c @@ -722,6 +722,8 @@ static int block_save_setup(QEMUFile *f, void *opaque) trace_migration_block_save("setup", block_mig_state.submitted, block_mig_state.transferred);
+ warn_report("block migration is deprecated. Use mirror jobs instead.");
blockdev-mirror with NBD
+ qemu_mutex_lock_iothread(); ret = init_blk_migration(f); if (ret < 0) { diff --git a/migration/options.c b/migration/options.c index b62ab30cd5..374dc0767e 100644 --- a/migration/options.c +++ b/migration/options.c @@ -12,6 +12,7 @@ */
#include "qemu/osdep.h" +#include "qemu/error-report.h" #include "exec/target_page.h" #include "qapi/clone-visitor.h" #include "qapi/error.h" @@ -437,6 +438,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) return false; } #endif + if (new_caps[MIGRATION_CAPABILITY_BLOCK]) { + warn_report("Block migration is deprecated. " + "Use driver_mirror+NBD instead.");
blockdev-mirror with NBD
+ }
#ifndef CONFIG_REPLICATION if (new_caps[MIGRATION_CAPABILITY_X_COLO]) { @@ -1257,6 +1262,8 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) }
if (params->has_block_incremental) { + warn_report("Block migration is deprecated. " + "Use driver_mirror+NBD instead.");
blockdev-mirror with NBD
s->parameters.block_incremental = params->block_incremental; } if (params->has_multifd_channels) { -- 2.40.1
Stefan Hajnoczi <stefanha@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:43PM +0200, Juan Quintela wrote:
It is obsolete. It is better to use driver_mirror+NBD instead.
CC: Kevin Wolf <kwolf@redhat.com> CC: Eric Blake <eblake@redhat.com> CC: Stefan Hajnoczi <stefanha@redhat.com> CC: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
Can any of you give one example of how to use driver_mirror+NBD for deprecated.rst?
Please see "QMP invocation for live storage migration with ``drive-mirror`` + NBD" in docs/interop/live-block-operations.rst for a detailed explanation.
You put here drive-mirror, and everything else blockdev-mirror. It appears that blockdev-mirror is the new name from driver-mirror, but as the documentation says driver-mirror + NBD, I am changing to driver-mirror everywhere? Thanks, Juan.
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 8 ++++ qapi/migration.json | 92 ++++++++++++++++++++++++--------------- migration/options.c | 13 ++++++ 3 files changed, 79 insertions(+), 34 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 173c5ba5cb..fe7f2bbde8 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -460,3 +460,11 @@ block migration (since 8.1) Block migration is too inflexible. It needs to migrate all block devices or none. Use driver_mirror+NBD instead. +old compression method (since 8.1) +'''''''''''''''''''''''''''''''''' + +Compression method fails too much. Too many races. We are going to +remove it if nobody fixes it. For starters, migration-test +compression tests are disabled becase they hand randomly. If you need +compression, use multifd compression methods. + diff --git a/qapi/migration.json b/qapi/migration.json index a8497de48d..40a8b5d124 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -244,6 +244,7 @@ # # @compression: migration compression statistics, only returned if # compression feature is on and status is 'active' or 'completed' +# It is obsolete and deprecated. Use multifd compression methods. # (Since 3.1) # # @socket-address: Only used for tcp, to know what the real port is @@ -261,7 +262,8 @@ # Features: # # @deprecated: @disk migration is deprecated. Use driver_mirror+NBD -# instead. +# instead. @compression is obsolete use multifd compression +# methods instead. # # Since: 0.14 ## @@ -279,7 +281,7 @@ '*blocked-reasons': ['str'], '*postcopy-blocktime': 'uint32', '*postcopy-vcpu-blocktime': ['uint32'], - '*compression': 'CompressionStats', + '*compression': { 'type': 'CompressionStats', 'features': ['deprecated'] }, '*socket-address': ['SocketAddress'] } } ## @@ -432,7 +434,8 @@ # compress and xbzrle are both on, compress only takes effect in # the ram bulk stage, after that, it will be disabled and only # xbzrle takes effect, this can help to minimize migration -# traffic. The feature is disabled by default. (since 2.4 ) +# traffic. The feature is disabled by default. Obsolete. Use +# multifd compression methods if needed. (since 2.4 ) # # @events: generate events for each migration state change (since 2.4 # ) @@ -503,6 +506,7 @@ # Features: # # @deprecated: @block migration is deprecated. Use driver_mirror+NBD +# instead. @compress is obsolete use multifd compression methods # instead. # # @unstable: Members @x-colo and @x-ignore-shared are experimental. @@ -511,7 +515,8 @@ ## { 'enum': 'MigrationCapability', 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', - 'compress', 'events', 'postcopy-ram', + { 'name': 'compress', 'features': [ 'deprecated' ] }, + 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram', { 'name': 'block', 'features': [ 'deprecated' ] }, @@ -671,22 +676,24 @@ # migration, the compression level is an integer between 0 and 9, # where 0 means no compression, 1 means the best compression # speed, and 9 means best compression ratio which will consume -# more CPU. +# more CPU. Obsolete, see multifd compression if needed. # # @compress-threads: Set compression thread count to be used in live # migration, the compression thread count is an integer between 1 -# and 255. +# and 255. Obsolete, see multifd compression if needed. # # @compress-wait-thread: Controls behavior when all compression # threads are currently busy. If true (default), wait for a free # compression thread to become available; otherwise, send the page -# uncompressed. (Since 3.1) +# uncompressed. Obsolete, see multifd compression if +# needed. (Since 3.1) # # @decompress-threads: Set decompression thread count to be used in # live migration, the decompression thread count is an integer # between 1 and 255. Usually, decompression is at least 4 times as # fast as compression, so set the decompress-threads to the number -# about 1/4 of compress-threads is adequate. +# about 1/4 of compress-threads is adequate. Obsolete, see multifd +# compression if needed. # # @throttle-trigger-threshold: The ratio of bytes_dirty_period and # bytes_xfer_period to trigger throttling. It is expressed as @@ -799,7 +806,9 @@ # Features: # # @deprecated: Member @block-incremental is obsolete. Use -# driver_mirror+NBD instead. +# driver_mirror+NBD instead. Compression is obsolete, so members +# @compress-level, @compress-threads, @decompress-threads and +# @compress-wait-thread should not be used. # # @unstable: Member @x-checkpoint-delay is experimental. # @@ -808,8 +817,11 @@ { 'enum': 'MigrationParameter', 'data': ['announce-initial', 'announce-max', 'announce-rounds', 'announce-step', - 'compress-level', 'compress-threads', 'decompress-threads', - 'compress-wait-thread', 'throttle-trigger-threshold', + { 'name': 'compress-level', 'features': [ 'deprecated' ] }, + { 'name': 'compress-threads', 'features': [ 'deprecated' ] }, + { 'name': 'decompress-threads', 'features': [ 'deprecated' ] }, + { 'name': 'compress-wait-thread', 'features': [ 'deprecated' ] }, + 'throttle-trigger-threshold', 'cpu-throttle-initial', 'cpu-throttle-increment', 'cpu-throttle-tailslow', 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', @@ -837,16 +849,17 @@ # @announce-step: Increase in delay (in milliseconds) between # subsequent packets in the announcement (Since 4.0) # -# @compress-level: compression level +# @compress-level: compression level. Obsolete and deprecated. # -# @compress-threads: compression thread count +# @compress-threads: compression thread count. Obsolete and deprecated. # # @compress-wait-thread: Controls behavior when all compression # threads are currently busy. If true (default), wait for a free # compression thread to become available; otherwise, send the page -# uncompressed. (Since 3.1) +# uncompressed. Obsolete and deprecated. (Since 3.1) # -# @decompress-threads: decompression thread count +# @decompress-threads: decompression thread count. Obsolete and +# deprecated. # # @throttle-trigger-threshold: The ratio of bytes_dirty_period and # bytes_xfer_period to trigger throttling. It is expressed as @@ -958,7 +971,9 @@ # Features: # # @deprecated: Member @block-incremental is obsolete. Use -# driver_mirror+NBD instead. +# driver_mirror+NBD instead. Compression is obsolete, so members +# @compress-level, @compress-threads, @decompress-threads and +# @compress-wait-thread should not be used. # # @unstable: Member @x-checkpoint-delay is experimental. # @@ -972,10 +987,14 @@ '*announce-max': 'size', '*announce-rounds': 'size', '*announce-step': 'size', - '*compress-level': 'uint8', - '*compress-threads': 'uint8', - '*compress-wait-thread': 'bool', - '*decompress-threads': 'uint8', + '*compress-level': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, + '*compress-threads': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, + '*compress-wait-thread': { 'type': 'bool', + 'features': [ 'deprecated' ] }, + '*decompress-threads': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, '*throttle-trigger-threshold': 'uint8', '*cpu-throttle-initial': 'uint8', '*cpu-throttle-increment': 'uint8', @@ -1008,7 +1027,7 @@ # Example: # # -> { "execute": "migrate-set-parameters" , -# "arguments": { "compress-level": 1 } } +# "arguments": { "multifd-channels": 5 } } # <- { "return": {} } ## { 'command': 'migrate-set-parameters', 'boxed': true, @@ -1031,16 +1050,18 @@ # @announce-step: Increase in delay (in milliseconds) between # subsequent packets in the announcement (Since 4.0) # -# @compress-level: compression level +# @compress-level: compression level. Obsolete and deprecated. # -# @compress-threads: compression thread count +# @compress-threads: compression thread count. Obsolote and +# deprecated. # # @compress-wait-thread: Controls behavior when all compression # threads are currently busy. If true (default), wait for a free # compression thread to become available; otherwise, send the page -# uncompressed. (Since 3.1) +# uncompressed. Obsolete and deprecated. (Since 3.1) # -# @decompress-threads: decompression thread count +# @decompress-threads: decompression thread count. Obsolete and +# deprecated. # # @throttle-trigger-threshold: The ratio of bytes_dirty_period and # bytes_xfer_period to trigger throttling. It is expressed as @@ -1154,7 +1175,9 @@ # Features: # # @deprecated: Member @block-incremental is obsolete. Use -# driver_mirror+NBD instead. +# driver_mirror+NBD instead. Compression is obsolete, so members +# @compress-level, @compress-threads, @decompress-threads and +# @compress-wait-thread should not be used. # # @unstable: Member @x-checkpoint-delay is experimental. # @@ -1165,10 +1188,14 @@ '*announce-max': 'size', '*announce-rounds': 'size', '*announce-step': 'size', - '*compress-level': 'uint8', - '*compress-threads': 'uint8', - '*compress-wait-thread': 'bool', - '*decompress-threads': 'uint8', + '*compress-level': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, + '*compress-threads': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, + '*compress-wait-thread': { 'type': 'bool', + 'features': [ 'deprecated' ] }, + '*decompress-threads': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, '*throttle-trigger-threshold': 'uint8', '*cpu-throttle-initial': 'uint8', '*cpu-throttle-increment': 'uint8', @@ -1182,7 +1209,6 @@ 'features': [ 'unstable' ] }, '*block-incremental': { 'type': 'bool', 'features': [ 'deprecated' ] }, - '*block-incremental': 'bool', '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', @@ -1205,10 +1231,8 @@ # # -> { "execute": "query-migrate-parameters" } # <- { "return": { -# "decompress-threads": 2, +# "multifd-channels": 2, # "cpu-throttle-increment": 10, -# "compress-threads": 8, -# "compress-level": 1, # "cpu-throttle-initial": 20, # "max-bandwidth": 33554432, # "downtime-limit": 300 diff --git a/migration/options.c b/migration/options.c index 374dc0767e..c04e62ba2d 100644 --- a/migration/options.c +++ b/migration/options.c @@ -443,6 +443,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) "Use driver_mirror+NBD instead."); } + if (new_caps[MIGRATION_CAPABILITY_BLOCK]) { + warn_report("Old compression method is deprecated. " + "Use multifd compression methods instead."); + } + #ifndef CONFIG_REPLICATION if (new_caps[MIGRATION_CAPABILITY_X_COLO]) { error_setg(errp, "QEMU compiled without replication module" @@ -1196,18 +1201,26 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) /* TODO use QAPI_CLONE() instead of duplicating it inline */ if (params->has_compress_level) { + warn_report("Old compression is deprecated. " + "Use multifd compression methods instead."); s->parameters.compress_level = params->compress_level; } if (params->has_compress_threads) { + warn_report("Old compression is deprecated. " + "Use multifd compression methods instead."); s->parameters.compress_threads = params->compress_threads; } if (params->has_compress_wait_thread) { + warn_report("Old compression is deprecated. " + "Use multifd compression methods instead."); s->parameters.compress_wait_thread = params->compress_wait_thread; } if (params->has_decompress_threads) { + warn_report("Old compression is deprecated. " + "Use multifd compression methods instead."); s->parameters.decompress_threads = params->decompress_threads; } -- 2.40.1
On 12/06/2023 21.33, Juan Quintela wrote:
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 8 ++++ qapi/migration.json | 92 ++++++++++++++++++++++++--------------- migration/options.c | 13 ++++++ 3 files changed, 79 insertions(+), 34 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 173c5ba5cb..fe7f2bbde8 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -460,3 +460,11 @@ block migration (since 8.1) Block migration is too inflexible. It needs to migrate all block devices or none. Use driver_mirror+NBD instead.
+old compression method (since 8.1) +'''''''''''''''''''''''''''''''''' + +Compression method fails too much. Too many races. We are going to +remove it if nobody fixes it. For starters, migration-test +compression tests are disabled becase they hand randomly. If you need
"because they fail randomly" ?
+compression, use multifd compression methods. + diff --git a/qapi/migration.json b/qapi/migration.json index a8497de48d..40a8b5d124 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -244,6 +244,7 @@ # # @compression: migration compression statistics, only returned if # compression feature is on and status is 'active' or 'completed' +# It is obsolete and deprecated. Use multifd compression methods. # (Since 3.1) # # @socket-address: Only used for tcp, to know what the real port is @@ -261,7 +262,8 @@ # Features: # # @deprecated: @disk migration is deprecated. Use driver_mirror+NBD -# instead. +# instead. @compression is obsolete use multifd compression
Use a dot or comma after "obsolete".
+# methods instead. # # Since: 0.14 ## @@ -279,7 +281,7 @@ '*blocked-reasons': ['str'], '*postcopy-blocktime': 'uint32', '*postcopy-vcpu-blocktime': ['uint32'], - '*compression': 'CompressionStats', + '*compression': { 'type': 'CompressionStats', 'features': ['deprecated'] }, '*socket-address': ['SocketAddress'] } }
## @@ -432,7 +434,8 @@ # compress and xbzrle are both on, compress only takes effect in # the ram bulk stage, after that, it will be disabled and only # xbzrle takes effect, this can help to minimize migration -# traffic. The feature is disabled by default. (since 2.4 ) +# traffic. The feature is disabled by default. Obsolete. Use +# multifd compression methods if needed. (since 2.4 ) # # @events: generate events for each migration state change (since 2.4 # ) @@ -503,6 +506,7 @@ # Features: # # @deprecated: @block migration is deprecated. Use driver_mirror+NBD +# instead. @compress is obsolete use multifd compression methods
dito
# instead. # # @unstable: Members @x-colo and @x-ignore-shared are experimental. @@ -511,7 +515,8 @@ ## { 'enum': 'MigrationCapability', 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', - 'compress', 'events', 'postcopy-ram', + { 'name': 'compress', 'features': [ 'deprecated' ] }, + 'events', 'postcopy-ram', { 'name': 'x-colo', 'features': [ 'unstable' ] }, 'release-ram', { 'name': 'block', 'features': [ 'deprecated' ] }, @@ -671,22 +676,24 @@ # migration, the compression level is an integer between 0 and 9, # where 0 means no compression, 1 means the best compression # speed, and 9 means best compression ratio which will consume -# more CPU. +# more CPU. Obsolete, see multifd compression if needed. # # @compress-threads: Set compression thread count to be used in live # migration, the compression thread count is an integer between 1 -# and 255. +# and 255. Obsolete, see multifd compression if needed. # # @compress-wait-thread: Controls behavior when all compression # threads are currently busy. If true (default), wait for a free # compression thread to become available; otherwise, send the page -# uncompressed. (Since 3.1) +# uncompressed. Obsolete, see multifd compression if +# needed. (Since 3.1) # # @decompress-threads: Set decompression thread count to be used in # live migration, the decompression thread count is an integer # between 1 and 255. Usually, decompression is at least 4 times as # fast as compression, so set the decompress-threads to the number -# about 1/4 of compress-threads is adequate. +# about 1/4 of compress-threads is adequate. Obsolete, see multifd +# compression if needed. # # @throttle-trigger-threshold: The ratio of bytes_dirty_period and # bytes_xfer_period to trigger throttling. It is expressed as @@ -799,7 +806,9 @@ # Features: # # @deprecated: Member @block-incremental is obsolete. Use -# driver_mirror+NBD instead. +# driver_mirror+NBD instead. Compression is obsolete, so members +# @compress-level, @compress-threads, @decompress-threads and +# @compress-wait-thread should not be used. # # @unstable: Member @x-checkpoint-delay is experimental. # @@ -808,8 +817,11 @@ { 'enum': 'MigrationParameter', 'data': ['announce-initial', 'announce-max', 'announce-rounds', 'announce-step', - 'compress-level', 'compress-threads', 'decompress-threads', - 'compress-wait-thread', 'throttle-trigger-threshold', + { 'name': 'compress-level', 'features': [ 'deprecated' ] }, + { 'name': 'compress-threads', 'features': [ 'deprecated' ] }, + { 'name': 'decompress-threads', 'features': [ 'deprecated' ] }, + { 'name': 'compress-wait-thread', 'features': [ 'deprecated' ] }, + 'throttle-trigger-threshold', 'cpu-throttle-initial', 'cpu-throttle-increment', 'cpu-throttle-tailslow', 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', @@ -837,16 +849,17 @@ # @announce-step: Increase in delay (in milliseconds) between # subsequent packets in the announcement (Since 4.0) # -# @compress-level: compression level +# @compress-level: compression level. Obsolete and deprecated. # -# @compress-threads: compression thread count +# @compress-threads: compression thread count. Obsolete and deprecated. # # @compress-wait-thread: Controls behavior when all compression # threads are currently busy. If true (default), wait for a free # compression thread to become available; otherwise, send the page -# uncompressed. (Since 3.1) +# uncompressed. Obsolete and deprecated. (Since 3.1) # -# @decompress-threads: decompression thread count +# @decompress-threads: decompression thread count. Obsolete and +# deprecated. # # @throttle-trigger-threshold: The ratio of bytes_dirty_period and # bytes_xfer_period to trigger throttling. It is expressed as @@ -958,7 +971,9 @@ # Features: # # @deprecated: Member @block-incremental is obsolete. Use -# driver_mirror+NBD instead. +# driver_mirror+NBD instead. Compression is obsolete, so members +# @compress-level, @compress-threads, @decompress-threads and +# @compress-wait-thread should not be used. # # @unstable: Member @x-checkpoint-delay is experimental. # @@ -972,10 +987,14 @@ '*announce-max': 'size', '*announce-rounds': 'size', '*announce-step': 'size', - '*compress-level': 'uint8', - '*compress-threads': 'uint8', - '*compress-wait-thread': 'bool', - '*decompress-threads': 'uint8', + '*compress-level': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, + '*compress-threads': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, + '*compress-wait-thread': { 'type': 'bool', + 'features': [ 'deprecated' ] }, + '*decompress-threads': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, '*throttle-trigger-threshold': 'uint8', '*cpu-throttle-initial': 'uint8', '*cpu-throttle-increment': 'uint8', @@ -1008,7 +1027,7 @@ # Example: # # -> { "execute": "migrate-set-parameters" , -# "arguments": { "compress-level": 1 } } +# "arguments": { "multifd-channels": 5 } } # <- { "return": {} } ## { 'command': 'migrate-set-parameters', 'boxed': true, @@ -1031,16 +1050,18 @@ # @announce-step: Increase in delay (in milliseconds) between # subsequent packets in the announcement (Since 4.0) # -# @compress-level: compression level +# @compress-level: compression level. Obsolete and deprecated. # -# @compress-threads: compression thread count +# @compress-threads: compression thread count. Obsolote and
Obsolete
+# deprecated. # # @compress-wait-thread: Controls behavior when all compression # threads are currently busy. If true (default), wait for a free # compression thread to become available; otherwise, send the page -# uncompressed. (Since 3.1) +# uncompressed. Obsolete and deprecated. (Since 3.1) # -# @decompress-threads: decompression thread count +# @decompress-threads: decompression thread count. Obsolete and +# deprecated. # # @throttle-trigger-threshold: The ratio of bytes_dirty_period and # bytes_xfer_period to trigger throttling. It is expressed as @@ -1154,7 +1175,9 @@ # Features: # # @deprecated: Member @block-incremental is obsolete. Use -# driver_mirror+NBD instead. +# driver_mirror+NBD instead. Compression is obsolete, so members +# @compress-level, @compress-threads, @decompress-threads and +# @compress-wait-thread should not be used. # # @unstable: Member @x-checkpoint-delay is experimental. # @@ -1165,10 +1188,14 @@ '*announce-max': 'size', '*announce-rounds': 'size', '*announce-step': 'size', - '*compress-level': 'uint8', - '*compress-threads': 'uint8', - '*compress-wait-thread': 'bool', - '*decompress-threads': 'uint8', + '*compress-level': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, + '*compress-threads': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, + '*compress-wait-thread': { 'type': 'bool', + 'features': [ 'deprecated' ] }, + '*decompress-threads': { 'type': 'uint8', + 'features': [ 'deprecated' ] }, '*throttle-trigger-threshold': 'uint8', '*cpu-throttle-initial': 'uint8', '*cpu-throttle-increment': 'uint8', @@ -1182,7 +1209,6 @@ 'features': [ 'unstable' ] }, '*block-incremental': { 'type': 'bool', 'features': [ 'deprecated' ] }, - '*block-incremental': 'bool',
That hunk should go into a previous patch, I think.
'*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', '*max-postcopy-bandwidth': 'size', @@ -1205,10 +1231,8 @@ # # -> { "execute": "query-migrate-parameters" } # <- { "return": { -# "decompress-threads": 2, +# "multifd-channels": 2, # "cpu-throttle-increment": 10, -# "compress-threads": 8, -# "compress-level": 1, # "cpu-throttle-initial": 20, # "max-bandwidth": 33554432, # "downtime-limit": 300
Thomas
Thomas Huth <thuth@redhat.com> wrote:
On 12/06/2023 21.33, Juan Quintela wrote:
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 8 ++++ qapi/migration.json | 92 ++++++++++++++++++++++++--------------- migration/options.c | 13 ++++++ 3 files changed, 79 insertions(+), 34 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 173c5ba5cb..fe7f2bbde8 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -460,3 +460,11 @@ block migration (since 8.1) Block migration is too inflexible. It needs to migrate all block devices or none. Use driver_mirror+NBD instead. +old compression method (since 8.1) +'''''''''''''''''''''''''''''''''' + +Compression method fails too much. Too many races. We are going to +remove it if nobody fixes it. For starters, migration-test +compression tests are disabled becase they hand randomly. If you need
"because they fail randomly" ?
yeap.
# @deprecated: @disk migration is deprecated. Use driver_mirror+NBD -# instead. +# instead. @compression is obsolete use multifd compression
Use a dot or comma after "obsolete".
fixed.
@@ -503,6 +506,7 @@ # Features: # # @deprecated: @block migration is deprecated. Use driver_mirror+NBD +# instead. @compress is obsolete use multifd compression methods
dito
fixed.
-# @compress-threads: compression thread count +# @compress-threads: compression thread count. Obsolote and
Obsolete
Fixed.
@@ -1182,7 +1209,6 @@ 'features': [ 'unstable' ] }, '*block-incremental': { 'type': 'bool', 'features': [ 'deprecated' ] }, - '*block-incremental': 'bool',
That hunk should go into a previous patch, I think.
Have found it already (it didn't compile). Thanks, Juan.
On Mon, Jun 12, 2023 at 09:33:44PM +0200, Juan Quintela wrote:
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 8 ++++ qapi/migration.json | 92 ++++++++++++++++++++++++--------------- migration/options.c | 13 ++++++ 3 files changed, 79 insertions(+), 34 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 173c5ba5cb..fe7f2bbde8 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -460,3 +460,11 @@ block migration (since 8.1) Block migration is too inflexible. It needs to migrate all block devices or none. Use driver_mirror+NBD instead.
+old compression method (since 8.1) +'''''''''''''''''''''''''''''''''' + +Compression method fails too much. Too many races. We are going to +remove it if nobody fixes it. For starters, migration-test +compression tests are disabled becase they hand randomly. If you need +compression, use multifd compression methods. + diff --git a/qapi/migration.json b/qapi/migration.json index a8497de48d..40a8b5d124 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -244,6 +244,7 @@ # # @compression: migration compression statistics, only returned if # compression feature is on and status is 'active' or 'completed' +# It is obsolete and deprecated. Use multifd compression methods. # (Since 3.1)
This doesn't give users an indication /why/ we're saying this. Instead I'd suggest This feature is unreliable and not tested. It is recommended to use multifd migration instead, which offers an alternative reliable and tested compression implementation.
# # @socket-address: Only used for tcp, to know what the real port is @@ -261,7 +262,8 @@ # Features: # # @deprecated: @disk migration is deprecated. Use driver_mirror+NBD -# instead. +# instead. @compression is obsolete use multifd compression +# methods instead.
For @deprecated, are we supposed to list multiple things at once, or use a separate @deprecated tag for each one ? Again I'd suggest rewording @compression is unreliable and untested. It is recommended to use multifd migration, which offers an alternative compression implementation that is reliable and tested.
diff --git a/migration/options.c b/migration/options.c index 374dc0767e..c04e62ba2d 100644 --- a/migration/options.c +++ b/migration/options.c @@ -443,6 +443,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) "Use driver_mirror+NBD instead."); }
+ if (new_caps[MIGRATION_CAPABILITY_BLOCK]) {
Surely MIGRATION_CAPABILITY_COMPRESS not BLOCK ?
+ warn_report("Old compression method is deprecated. " + "Use multifd compression methods instead."); + } + #ifndef CONFIG_REPLICATION if (new_caps[MIGRATION_CAPABILITY_X_COLO]) { error_setg(errp, "QEMU compiled without replication module" @@ -1196,18 +1201,26 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) /* TODO use QAPI_CLONE() instead of duplicating it inline */
if (params->has_compress_level) { + warn_report("Old compression is deprecated. " + "Use multifd compression methods instead."); s->parameters.compress_level = params->compress_level; }
if (params->has_compress_threads) { + warn_report("Old compression is deprecated. " + "Use multifd compression methods instead."); s->parameters.compress_threads = params->compress_threads; }
if (params->has_compress_wait_thread) { + warn_report("Old compression is deprecated. " + "Use multifd compression methods instead."); s->parameters.compress_wait_thread = params->compress_wait_thread; }
if (params->has_decompress_threads) { + warn_report("Old compression is deprecated. " + "Use multifd compression methods instead."); s->parameters.decompress_threads = params->decompress_threads; }
-- 2.40.1
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> wrote:
On Mon, Jun 12, 2023 at 09:33:44PM +0200, Juan Quintela wrote:
Signed-off-by: Juan Quintela <quintela@redhat.com> --- docs/about/deprecated.rst | 8 ++++ qapi/migration.json | 92 ++++++++++++++++++++++++--------------- migration/options.c | 13 ++++++ 3 files changed, 79 insertions(+), 34 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 173c5ba5cb..fe7f2bbde8 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -460,3 +460,11 @@ block migration (since 8.1) Block migration is too inflexible. It needs to migrate all block devices or none. Use driver_mirror+NBD instead.
+old compression method (since 8.1) +'''''''''''''''''''''''''''''''''' + +Compression method fails too much. Too many races. We are going to +remove it if nobody fixes it. For starters, migration-test +compression tests are disabled becase they hand randomly. If you need +compression, use multifd compression methods. + diff --git a/qapi/migration.json b/qapi/migration.json index a8497de48d..40a8b5d124 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -244,6 +244,7 @@ # # @compression: migration compression statistics, only returned if # compression feature is on and status is 'active' or 'completed' +# It is obsolete and deprecated. Use multifd compression methods. # (Since 3.1)
This doesn't give users an indication /why/ we're saying this. Instead I'd suggest
This feature is unreliable and not tested. It is recommended to use multifd migration instead, which offers an alternative reliable and tested compression implementation.
Much better. Done, thanks.
# @deprecated: @disk migration is deprecated. Use driver_mirror+NBD -# instead. +# instead. @compression is obsolete use multifd compression +# methods instead.
For @deprecated, are we supposed to list multiple things at once, or use a separate @deprecated tag for each one ?
# @unstable: Members @x-colo and @x-ignore-shared are experimental. This is the only example that I found that is similar. Only one example. Markus?
Again I'd suggest rewording
@compression is unreliable and untested. It is recommended to use multifd migration, which offers an alternative compression implementation that is reliable and tested.
Done.
@@ -443,6 +443,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) "Use driver_mirror+NBD instead."); }
+ if (new_caps[MIGRATION_CAPABILITY_BLOCK]) {
Surely MIGRATION_CAPABILITY_COMPRESS not BLOCK ?
Good catch. Copy & paste to its best. Thanks very much.
On Mon, Jun 12, 2023 at 21:33:38 +0200, Juan Quintela wrote:
Hi this series describe the migration parts that have to be deprecated.
- It is an rfc because I doubt that I did the deprecation process right. Hello Markus O:-)
- skipped field: It is older than me, I have never know what it stands for. As far as I know it has always been zero.
Libvirt doesn't use this.
- inc/blk migrate command options. They are only used by block migration (that I deprecate on the following patch). And they are really bad. grep must_remove_block_options.
- block migration. block jobs, whatever they are called this week are way more flexible. Current code works, but we broke it here and there, and really nobody has stand up to maintain it. It is quite contained and can be left there. Is anyone really using it?
We prefer nbd for storage migration if it is available.
- old compression method. It don't work. See last try from Lukas to make a test that works reliabely. I failed with the same task years ago. It is really slow, and if compression is good for you, multifd + zlib is going to perform/compress way more.
We do support these methods, but only enable them if a user explicitly requests so. In other words, the user will just get an error once these methods are removed, which is fine.
- -incoming <uri>
if you need to set parameters (multifd cames to mind, and preempt has the same problem), you really needs to use defer. So what should we do here?
This part is not urget, because management apps have a working option that are already using "defer", and the code simplifacation if we remove it is not so big. So we can leave it until 9.0 or whatever we think fit.
Right, libvirt already uses -incoming defer if supported. In other words, no objection to removing anything on this list and no additional work is needed on libvirt side. Jirka
participants (7)
-
Daniel P. Berrangé -
Jiri Denemark -
Juan Quintela -
Paolo Bonzini -
Peter Xu -
Stefan Hajnoczi -
Thomas Huth