[RFC 0/6] Migration deprecated parts

[RFC 0/6] Migration deprecated parts

[Juan Quintela]

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

[RFC 1/6] migration: skipped field is really obsolete.

[Juan Quintela]

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

[RFC 2/6] migration: migrate 'inc' command option is deprecated.

[Juan Quintela]

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

[RFC 3/6] migration: migrate 'blk' command option is deprecated.

[Juan Quintela]

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

[RFC 4/6] migration: Deprecate -incoming <uri>

[Juan Quintela]

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

[RFC 5/6] migration: Deprecate block migration

[Juan Quintela]

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

[RFC 6/6] migration: Deprecated old compression method

[Juan Quintela]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Peter Xu]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Juan Quintela]

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:-)

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Peter Xu]

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

Re: [RFC 0/6] Migration deprecated parts

[Jiri Denemark]

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

Re: [RFC 1/6] migration: skipped field is really obsolete.

[Daniel P. Berrangé]

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

Re: [RFC 2/6] migration: migrate 'inc' command option is deprecated.

[Daniel P. Berrangé]

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

Re: [RFC 3/6] migration: migrate 'blk' command option is deprecated.

[Daniel P. Berrangé]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Daniel P. Berrangé]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Daniel P. Berrangé]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Peter Xu]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Thomas Huth]

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

Re: [RFC 6/6] migration: Deprecated old compression method

[Thomas Huth]

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

Re: [RFC 6/6] migration: Deprecated old compression method

[Daniel P. Berrangé]

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

Re: [RFC 5/6] migration: Deprecate block migration

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 7300 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Juan Quintela]

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.

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Paolo Bonzini]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Paolo Bonzini]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Juan Quintela]

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.

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Thomas Huth]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Paolo Bonzini]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Daniel P. Berrangé]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Juan Quintela]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Peter Xu]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Peter Xu]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Peter Xu]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Paolo Bonzini]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Daniel P. Berrangé]

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

Re: [RFC 1/6] migration: skipped field is really obsolete.

[Juan Quintela]

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.

Re: [RFC 2/6] migration: migrate 'inc' command option is deprecated.

[Juan Quintela]

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.

Re: [RFC 3/6] migration: migrate 'blk' command option is deprecated.

[Juan Quintela]

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.

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Juan Quintela]

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.

Re: [RFC 5/6] migration: Deprecate block migration

[Juan Quintela]

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.

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Peter Xu]

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

Re: [RFC 6/6] migration: Deprecated old compression method

[Juan Quintela]

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.

Re: [RFC 6/6] migration: Deprecated old compression method

[Juan Quintela]

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.

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Juan Quintela]

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.

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Juan Quintela]

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.

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Daniel P. Berrangé]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Daniel P. Berrangé]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Peter Xu]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Peter Xu]

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

Re: [RFC 4/6] migration: Deprecate -incoming <uri>

[Daniel P. Berrangé]

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