[PATCH 0/6] migration: Add 'no-ram' and 'ram-only' cpabilities

[PATCH 0/6] migration: Add 'no-ram' and 'ram-only' cpabilities

[Nikita Lapshin]

We want to implement exteranl bg-snapshot tool for saving RAM. For this it
is important to be able manage migration stream because tool has no idea
about non-RAM part and its size.

Possible solution is to send RAM separately. This can be done with
implemented RAM capabilities. These capabilities allow to send only RAM part
or non-RAM part so tool can save non-RAM part without special handlers.
RAM will be saved with special handlers for postcopy load.

Nikita Lapshin (6):
  migration: is_ram changed to is_iterable
  migration: should_skip() implemented
  migration: Add no-ram capability
  migration: Add ram-only capability
  migration: analyze-migration script changed
  migration: Test for ram capabilities

 migration/migration.c                         | 29 +++++-
 migration/migration.h                         |  2 +
 migration/ram.c                               |  6 ++
 migration/savevm.c                            | 63 ++++++------
 qapi/migration.json                           | 11 ++-
 scripts/analyze-migration.py                  | 19 ++--
 .../tests/migrate-ram-capabilities-test       | 96 +++++++++++++++++++
 .../tests/migrate-ram-capabilities-test.out   |  5 +
 8 files changed, 192 insertions(+), 39 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/migrate-ram-capabilities-test
 create mode 100644 tests/qemu-iotests/tests/migrate-ram-capabilities-test.out

-- 
2.27.0

[PATCH 1/6] migration: is_ram changed to is_iterable

[Nikita Lapshin]

For new migration capabilities upcoming we need to use something
like is_ram for this purpose. This member of struction is true
not only for RAM so it should be renamed.

Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
---
 migration/savevm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 0bef031acb..f90fdb2bdd 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -248,7 +248,7 @@ typedef struct SaveStateEntry {
     const VMStateDescription *vmsd;
     void *opaque;
     CompatEntry *compat;
-    int is_ram;
+    int is_iterable;
 } SaveStateEntry;
 
 typedef struct SaveState {
@@ -797,9 +797,9 @@ int register_savevm_live(const char *idstr,
     se->ops = ops;
     se->opaque = opaque;
     se->vmsd = NULL;
-    /* if this is a live_savem then set is_ram */
+    /* if this is a live_savem then set is_iterable */
     if (ops->save_setup != NULL) {
-        se->is_ram = 1;
+        se->is_iterable = 1;
     }
 
     pstrcat(se->idstr, sizeof(se->idstr), idstr);
@@ -1625,7 +1625,7 @@ int qemu_save_device_state(QEMUFile *f)
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         int ret;
 
-        if (se->is_ram) {
+        if (se->is_iterable) {
             continue;
         }
         if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
@@ -2428,7 +2428,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
     se->load_section_id = section_id;
 
     /* Validate if it is a device's state */
-    if (xen_enabled() && se->is_ram) {
+    if (xen_enabled() && se->is_iterable) {
         error_report("loadvm: %s RAM loading not allowed on Xen", idstr);
         return -EINVAL;
     }
-- 
2.27.0

[PATCH 2/6] migration: should_skip() implemented

[Nikita Lapshin]

For next changes it is convenient to make all decisions about
sections skipping in one function.

Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
---
 migration/savevm.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index f90fdb2bdd..ba644083ab 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -943,6 +943,15 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se,
     return vmstate_save_state(f, se->vmsd, se->opaque, vmdesc);
 }
 
+static bool should_skip(SaveStateEntry *se)
+{
+    if (se->ops->is_active && !se->ops->is_active(se->opaque)) {
+        return true;
+    }
+
+    return false;
+}
+
 /*
  * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL)
  */
@@ -1207,10 +1216,8 @@ void qemu_savevm_state_setup(QEMUFile *f)
         if (!se->ops || !se->ops->save_setup) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (should_skip(se)) {
+            continue;
         }
         save_section_header(f, se, QEMU_VM_SECTION_START);
 
@@ -1238,10 +1245,8 @@ int qemu_savevm_state_resume_prepare(MigrationState *s)
         if (!se->ops || !se->ops->resume_prepare) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (should_skip(se)) {
+            continue;
         }
         ret = se->ops->resume_prepare(s, se->opaque);
         if (ret < 0) {
@@ -1268,8 +1273,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
         if (!se->ops || !se->ops->save_live_iterate) {
             continue;
         }
-        if (se->ops->is_active &&
-            !se->ops->is_active(se->opaque)) {
+        if (should_skip(se)) {
             continue;
         }
         if (se->ops->is_active_iterate &&
@@ -1336,10 +1340,8 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
         if (!se->ops || !se->ops->save_live_complete_postcopy) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (should_skip(se)) {
+            continue;
         }
         trace_savevm_section_start(se->idstr, se->section_id);
         /* Section type */
@@ -1373,10 +1375,8 @@ int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy)
             continue;
         }
 
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (should_skip(se)) {
+            continue;
         }
         trace_savevm_section_start(se->idstr, se->section_id);
 
@@ -1521,10 +1521,8 @@ void qemu_savevm_state_pending(QEMUFile *f, uint64_t threshold_size,
         if (!se->ops || !se->ops->save_live_pending) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (should_skip(se)) {
+            continue;
         }
         se->ops->save_live_pending(f, se->opaque, threshold_size,
                                    res_precopy_only, res_compatible,
-- 
2.27.0

[PATCH 3/6] migration: Add no-ram capability

[Nikita Lapshin]

This capability disable RAM section in migration stream.

Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
---
 migration/migration.c | 9 +++++++++
 migration/migration.h | 1 +
 migration/ram.c       | 6 ++++++
 qapi/migration.json   | 8 +++++---
 4 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3de11ae921..006447d00a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2610,6 +2610,15 @@ bool migrate_background_snapshot(void)
     return s->enabled_capabilities[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT];
 }
 
+bool migrate_no_ram(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_NO_RAM];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/migration/migration.h b/migration/migration.h
index 8130b703eb..43f7bf8eba 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -358,6 +358,7 @@ int migrate_decompress_threads(void);
 bool migrate_use_events(void);
 bool migrate_postcopy_blocktime(void);
 bool migrate_background_snapshot(void);
+bool migrate_no_ram(void);
 
 /* Sending on the return path - generic and then for each message type */
 void migrate_send_rp_shut(MigrationIncomingState *mis,
diff --git a/migration/ram.c b/migration/ram.c
index 57efa67f20..aa3583d0bc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4339,6 +4339,11 @@ static int ram_resume_prepare(MigrationState *s, void *opaque)
     return 0;
 }
 
+static bool ram_is_active(void* opaque)
+{
+    return !migrate_no_ram();
+}
+
 static SaveVMHandlers savevm_ram_handlers = {
     .save_setup = ram_save_setup,
     .save_live_iterate = ram_save_iterate,
@@ -4351,6 +4356,7 @@ static SaveVMHandlers savevm_ram_handlers = {
     .load_setup = ram_load_setup,
     .load_cleanup = ram_load_cleanup,
     .resume_prepare = ram_resume_prepare,
+    .is_active = ram_is_active,
 };
 
 static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
diff --git a/qapi/migration.json b/qapi/migration.json
index bbfd48cf0b..d53956852c 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -452,6 +452,8 @@
 #                       procedure starts. The VM RAM is saved with running VM.
 #                       (since 6.0)
 #
+# @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0)
+#
 # Features:
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
 #
@@ -465,8 +467,7 @@
            'block', 'return-path', 'pause-before-switchover', 'multifd',
            'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
            { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
-           'validate-uuid', 'background-snapshot'] }
-
+           'validate-uuid', 'background-snapshot', 'no-ram'] }
 ##
 # @MigrationCapabilityStatus:
 #
@@ -519,7 +520,8 @@
 #       {"state": false, "capability": "compress"},
 #       {"state": true, "capability": "events"},
 #       {"state": false, "capability": "postcopy-ram"},
-#       {"state": false, "capability": "x-colo"}
+#       {"state": false, "capability": "x-colo"},
+#       {"state": false, "capability": "no-ram"}
 #    ]}
 #
 ##
-- 
2.27.0

[PATCH 4/6] migration: Add ram-only capability

[Nikita Lapshin]

If this capability is enabled migration stream
will have RAM section only.

Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
---
 migration/migration.c | 20 +++++++++++++++++++-
 migration/migration.h |  1 +
 migration/savevm.c    | 11 ++++++++++-
 qapi/migration.json   |  7 +++++--
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 006447d00a..4d7ef9d8dc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1257,6 +1257,14 @@ static bool migrate_caps_check(bool *cap_list,
         return false;
     }
 
+    if (cap_list[MIGRATION_CAPABILITY_NO_RAM] &&
+        cap_list[MIGRATION_CAPABILITY_RAM_ONLY]) {
+        error_setg(errp, "ram-only and no-ram aren't "
+                         "compatible with each other");
+
+        return false;
+    }
+
     return true;
 }
 
@@ -2619,6 +2627,15 @@ bool migrate_no_ram(void)
     return s->enabled_capabilities[MIGRATION_CAPABILITY_NO_RAM];
 }
 
+bool migrate_ram_only(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_RAM_ONLY];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
@@ -3973,7 +3990,8 @@ static void *bg_migration_thread(void *opaque)
      * save their state to channel-buffer along with devices.
      */
     cpu_synchronize_all_states();
-    if (qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) {
+    if (!migrate_ram_only() &&
+        qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) {
         goto fail;
     }
     /*
diff --git a/migration/migration.h b/migration/migration.h
index 43f7bf8eba..d5a077e00d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -359,6 +359,7 @@ bool migrate_use_events(void);
 bool migrate_postcopy_blocktime(void);
 bool migrate_background_snapshot(void);
 bool migrate_no_ram(void);
+bool migrate_ram_only(void);
 
 /* Sending on the return path - generic and then for each message type */
 void migrate_send_rp_shut(MigrationIncomingState *mis,
diff --git a/migration/savevm.c b/migration/savevm.c
index ba644083ab..e41ca76000 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -249,6 +249,7 @@ typedef struct SaveStateEntry {
     void *opaque;
     CompatEntry *compat;
     int is_iterable;
+    bool is_ram;
 } SaveStateEntry;
 
 typedef struct SaveState {
@@ -802,6 +803,10 @@ int register_savevm_live(const char *idstr,
         se->is_iterable = 1;
     }
 
+    if (!strcmp("ram", idstr)) {
+        se->is_ram = true;
+    }
+
     pstrcat(se->idstr, sizeof(se->idstr), idstr);
 
     if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
@@ -949,6 +954,10 @@ static bool should_skip(SaveStateEntry *se)
         return true;
     }
 
+    if (migrate_ram_only() && !se->is_ram) {
+        return true;
+    }
+
     return false;
 }
 
@@ -1486,7 +1495,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
         }
     }
 
-    if (iterable_only) {
+    if (iterable_only || migrate_ram_only()) {
         goto flush;
     }
 
diff --git a/qapi/migration.json b/qapi/migration.json
index d53956852c..626fc59d14 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -454,6 +454,8 @@
 #
 # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0)
 #
+# @ram-only: If enabled, only RAM sections will be sent. (since 7.0)
+#
 # Features:
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
 #
@@ -467,7 +469,7 @@
            'block', 'return-path', 'pause-before-switchover', 'multifd',
            'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
            { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
-           'validate-uuid', 'background-snapshot', 'no-ram'] }
+           'validate-uuid', 'background-snapshot', 'no-ram', 'ram-only'] }
 ##
 # @MigrationCapabilityStatus:
 #
@@ -521,7 +523,8 @@
 #       {"state": true, "capability": "events"},
 #       {"state": false, "capability": "postcopy-ram"},
 #       {"state": false, "capability": "x-colo"},
-#       {"state": false, "capability": "no-ram"}
+#       {"state": false, "capability": "no-ram"},
+#       {"state": false, "capability": "ram-only"}
 #    ]}
 #
 ##
-- 
2.27.0

[PATCH 5/6] migration: analyze-migration script changed

[Nikita Lapshin]

This script is used for RAM capabilities test. But it cannot work
in case of no vm description in migration stream.
So new flag is added to allow work this script with ram-only
migration stream.

Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
---
 scripts/analyze-migration.py | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index b82a1b0c58..80077a09bc 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -495,7 +495,7 @@ def __init__(self, filename):
         self.filename = filename
         self.vmsd_desc = None
 
-    def read(self, desc_only = False, dump_memory = False, write_memory = False):
+    def read(self, ram_only, desc_only = False, dump_memory = False, write_memory = False):
         # Read in the whole file
         file = MigrationFile(self.filename)
 
@@ -509,7 +509,8 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
         if data != self.QEMU_VM_FILE_VERSION:
             raise Exception("Invalid version number %d" % data)
 
-        self.load_vmsd_json(file)
+        if not ram_only:
+            self.load_vmsd_json(file)
 
         # Read sections
         self.sections = collections.OrderedDict()
@@ -518,7 +519,10 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
             return
 
         ramargs = {}
-        ramargs['page_size'] = self.vmsd_desc['page_size']
+        if ram_only:
+            ramargs['page_size'] = 4096
+        else:
+            ramargs['page_size'] = self.vmsd_desc['page_size']
         ramargs['dump_memory'] = dump_memory
         ramargs['write_memory'] = write_memory
         self.section_classes[('ram',0)][1] = ramargs
@@ -579,6 +583,7 @@ def default(self, o):
 parser.add_argument("-m", "--memory", help='dump RAM contents as well', action='store_true')
 parser.add_argument("-d", "--dump", help='what to dump ("state" or "desc")', default='state')
 parser.add_argument("-x", "--extract", help='extract contents into individual files', action='store_true')
+parser.add_argument("--ram-only", help='parse migration dump containing only RAM', action='store_true')
 args = parser.parse_args()
 
 jsonenc = JSONEncoder(indent=4, separators=(',', ': '))
@@ -586,14 +591,14 @@ def default(self, o):
 if args.extract:
     dump = MigrationDump(args.file)
 
-    dump.read(desc_only = True)
+    dump.read(desc_only = True, ram_only = args.ram_only)
     print("desc.json")
     f = open("desc.json", "w")
     f.truncate()
     f.write(jsonenc.encode(dump.vmsd_desc))
     f.close()
 
-    dump.read(write_memory = True)
+    dump.read(write_memory = True, ram_only = args.ram_only)
     dict = dump.getDict()
     print("state.json")
     f = open("state.json", "w")
@@ -602,12 +607,12 @@ def default(self, o):
     f.close()
 elif args.dump == "state":
     dump = MigrationDump(args.file)
-    dump.read(dump_memory = args.memory)
+    dump.read(dump_memory = args.memory, ram_only = args.ram_only)
     dict = dump.getDict()
     print(jsonenc.encode(dict))
 elif args.dump == "desc":
     dump = MigrationDump(args.file)
-    dump.read(desc_only = True)
+    dump.read(desc_only = True, ram_only = args.ram_only)
     print(jsonenc.encode(dump.vmsd_desc))
 else:
     raise Exception("Please specify either -x, -d state or -d desc")
-- 
2.27.0

[PATCH 6/6] migration: Test for ram capabilities

[Nikita Lapshin]

Use scripts/analyze-migration.py to split migration stream into
sections and analyze its output.

Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
---
 .../tests/migrate-ram-capabilities-test       | 96 +++++++++++++++++++
 .../tests/migrate-ram-capabilities-test.out   |  5 +
 2 files changed, 101 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/migrate-ram-capabilities-test
 create mode 100644 tests/qemu-iotests/tests/migrate-ram-capabilities-test.out

diff --git a/tests/qemu-iotests/tests/migrate-ram-capabilities-test b/tests/qemu-iotests/tests/migrate-ram-capabilities-test
new file mode 100755
index 0000000000..917f888340
--- /dev/null
+++ b/tests/qemu-iotests/tests/migrate-ram-capabilities-test
@@ -0,0 +1,96 @@
+#!/usr/bin/env python3
+# group: rw migration
+#
+# Tests for 'no-ram' and 'ram-only' capabilities
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import json
+import subprocess
+import iotests
+
+img = os.path.join(iotests.test_dir, 'disk.img')
+
+class TestRamCapabilities(iotests.QMPTestCase):
+    def setUp(self):
+        iotests.qemu_img('create', '-f', iotests.imgfmt, img, '10M')
+        self.vm = iotests.VM()
+        self.vm.launch()
+        self.vm.qmp('migrate-set-capabilities', capabilities=[
+            {
+                'capability': 'events',
+                'state': True
+            }
+        ])
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(img)
+
+    def check_ram_only(self, output):
+        str_json = output.decode()
+        json_obj = json.loads(str_json)
+
+        success = False
+        for key in json_obj:
+            self.assertTrue("ram" in key)
+            success = True
+        self.assertTrue(success)
+
+    def run_migration(self, capability, tmp_stream):
+        self.vm.qmp('migrate-set-capabilities', capabilities=[
+            {
+                'capability': capability,
+                'state': True
+            }
+        ])
+
+        self.vm.qmp('migrate', uri='exec:cat>' + tmp_stream)
+
+        while True:
+            event = self.vm.event_wait('MIGRATION')
+
+            if event['data']['status'] == 'completed':
+                break
+
+
+    def test_no_ram(self):
+        with iotests.FilePath('tmp_stream') as tmp_stream:
+            self.run_migration('no-ram', tmp_stream)
+            output = subprocess.run(
+                ['../../../scripts/analyze-migration.py', '-f', tmp_stream],
+                stdout=subprocess.PIPE,
+                stderr=subprocess.STDOUT,
+                check=False).stdout
+
+            self.assertFalse('ram' in output.decode())
+
+    def test_ram_only(self):
+        with iotests.FilePath('tmp_stream') as tmp_stream:
+            self.run_migration('ram-only', tmp_stream)
+            output = subprocess.run(
+                ['../../../scripts/analyze-migration.py', '-f', tmp_stream,
+                    '--ram-only'],
+                stdout=subprocess.PIPE,
+                stderr=subprocess.STDOUT,
+                check=False).stdout
+
+            self.check_ram_only(output)
+
+if __name__ == '__main__':
+    iotests.main(supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/migrate-ram-capabilities-test.out b/tests/qemu-iotests/tests/migrate-ram-capabilities-test.out
new file mode 100644
index 0000000000..fbc63e62f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/migrate-ram-capabilities-test.out
@@ -0,0 +1,5 @@
+..
+----------------------------------------------------------------------
+Ran 2 tests
+
+OK
-- 
2.27.0

Re: [PATCH 1/6] migration: is_ram changed to is_iterable

[Vladimir Sementsov-Ogievskiy]

24.12.2021 14:11, Nikita Lapshin wrote:
> For new migration capabilities upcoming we need to use something
> like is_ram for this purpose. This member of struction is true
> not only for RAM so it should be renamed.
> 
> Signed-off-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 2/6] migration: should_skip() implemented

[Vladimir Sementsov-Ogievskiy]

Better subject: "migration: implement should_skip()"


24.12.2021 14:11, Nikita Lapshin wrote:
> For next changes it is convenient to make all decisions about
> sections skipping in one function.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH 3/6] migration: Add no-ram capability

[Vladimir Sementsov-Ogievskiy]

24.12.2021 14:11, Nikita Lapshin wrote:
> This capability disable RAM section in migration stream.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>

Probably we need some checks that new capability is not used together with ram-related capabilities, but that could be a separate patch.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH 4/6] migration: Add ram-only capability

[Vladimir Sementsov-Ogievskiy]

24.12.2021 14:11, Nikita Lapshin wrote:
> If this capability is enabled migration stream
> will have RAM section only.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> ---
>   migration/migration.c | 20 +++++++++++++++++++-
>   migration/migration.h |  1 +
>   migration/savevm.c    | 11 ++++++++++-
>   qapi/migration.json   |  7 +++++--
>   4 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 006447d00a..4d7ef9d8dc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1257,6 +1257,14 @@ static bool migrate_caps_check(bool *cap_list,
>           return false;
>       }
>   
> +    if (cap_list[MIGRATION_CAPABILITY_NO_RAM] &&
> +        cap_list[MIGRATION_CAPABILITY_RAM_ONLY]) {
> +        error_setg(errp, "ram-only and no-ram aren't "
> +                         "compatible with each other");
> +
> +        return false;
> +    }
> +
>       return true;
>   }
>   
> @@ -2619,6 +2627,15 @@ bool migrate_no_ram(void)
>       return s->enabled_capabilities[MIGRATION_CAPABILITY_NO_RAM];
>   }
>   
> +bool migrate_ram_only(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_RAM_ONLY];
> +}
> +
>   /* migration thread support */
>   /*
>    * Something bad happened to the RP stream, mark an error
> @@ -3973,7 +3990,8 @@ static void *bg_migration_thread(void *opaque)
>        * save their state to channel-buffer along with devices.
>        */
>       cpu_synchronize_all_states();
> -    if (qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) {
> +    if (!migrate_ram_only() &&
> +        qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) {

Here... [*]

>           goto fail;
>       }
>       /*
> diff --git a/migration/migration.h b/migration/migration.h
> index 43f7bf8eba..d5a077e00d 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -359,6 +359,7 @@ bool migrate_use_events(void);
>   bool migrate_postcopy_blocktime(void);
>   bool migrate_background_snapshot(void);
>   bool migrate_no_ram(void);
> +bool migrate_ram_only(void);
>   
>   /* Sending on the return path - generic and then for each message type */
>   void migrate_send_rp_shut(MigrationIncomingState *mis,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index ba644083ab..e41ca76000 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -249,6 +249,7 @@ typedef struct SaveStateEntry {
>       void *opaque;
>       CompatEntry *compat;
>       int is_iterable;
> +    bool is_ram;
>   } SaveStateEntry;
>   
>   typedef struct SaveState {
> @@ -802,6 +803,10 @@ int register_savevm_live(const char *idstr,
>           se->is_iterable = 1;
>       }
>   
> +    if (!strcmp("ram", idstr)) {
> +        se->is_ram = true;
> +    }
> +
>       pstrcat(se->idstr, sizeof(se->idstr), idstr);
>   
>       if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
> @@ -949,6 +954,10 @@ static bool should_skip(SaveStateEntry *se)
>           return true;
>       }
>   
> +    if (migrate_ram_only() && !se->is_ram) {
> +        return true;
> +    }
> +
>       return false;
>   }
>   
> @@ -1486,7 +1495,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
>           }
>       }
>   
> -    if (iterable_only) {
> +    if (iterable_only || migrate_ram_only()) {
>           goto flush;
>       }

[*] ... and here you care to avoid calling same qemu_savevm_state_complete_precopy_non_iterable()

Seems better to check migrate_ram_only at start of qemu_savevm_state_complete_precopy_non_iterable() and do early return from it.

>   
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d53956852c..626fc59d14 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -454,6 +454,8 @@
>   #
>   # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0)
>   #
> +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0)
> +#
>   # Features:
>   # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>   #
> @@ -467,7 +469,7 @@
>              'block', 'return-path', 'pause-before-switchover', 'multifd',
>              'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>              { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
> -           'validate-uuid', 'background-snapshot', 'no-ram'] }
> +           'validate-uuid', 'background-snapshot', 'no-ram', 'ram-only'] }
>   ##
>   # @MigrationCapabilityStatus:
>   #
> @@ -521,7 +523,8 @@
>   #       {"state": true, "capability": "events"},
>   #       {"state": false, "capability": "postcopy-ram"},
>   #       {"state": false, "capability": "x-colo"},
> -#       {"state": false, "capability": "no-ram"}
> +#       {"state": false, "capability": "no-ram"},
> +#       {"state": false, "capability": "ram-only"}
>   #    ]}
>   #
>   ##
> 


-- 
Best regards,
Vladimir

Re: [PATCH 5/6] migration: analyze-migration script changed

[Vladimir Sementsov-Ogievskiy]

24.12.2021 14:11, Nikita Lapshin wrote:
> This script is used for RAM capabilities test. But it cannot work
> in case of no vm description in migration stream.
> So new flag is added to allow work this script with ram-only
> migration stream.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> ---
>   scripts/analyze-migration.py | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index b82a1b0c58..80077a09bc 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -495,7 +495,7 @@ def __init__(self, filename):
>           self.filename = filename
>           self.vmsd_desc = None
>   
> -    def read(self, desc_only = False, dump_memory = False, write_memory = False):
> +    def read(self, ram_only, desc_only = False, dump_memory = False, write_memory = False):
>           # Read in the whole file
>           file = MigrationFile(self.filename)
>   
> @@ -509,7 +509,8 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
>           if data != self.QEMU_VM_FILE_VERSION:
>               raise Exception("Invalid version number %d" % data)
>   
> -        self.load_vmsd_json(file)
> +        if not ram_only:
> +            self.load_vmsd_json(file)
>   
>           # Read sections
>           self.sections = collections.OrderedDict()
> @@ -518,7 +519,10 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
>               return
>   
>           ramargs = {}
> -        ramargs['page_size'] = self.vmsd_desc['page_size']
> +        if ram_only:
> +            ramargs['page_size'] = 4096
> +        else:
> +            ramargs['page_size'] = self.vmsd_desc['page_size']
>           ramargs['dump_memory'] = dump_memory
>           ramargs['write_memory'] = write_memory
>           self.section_classes[('ram',0)][1] = ramargs
> @@ -579,6 +583,7 @@ def default(self, o):
>   parser.add_argument("-m", "--memory", help='dump RAM contents as well', action='store_true')
>   parser.add_argument("-d", "--dump", help='what to dump ("state" or "desc")', default='state')
>   parser.add_argument("-x", "--extract", help='extract contents into individual files', action='store_true')
> +parser.add_argument("--ram-only", help='parse migration dump containing only RAM', action='store_true')
>   args = parser.parse_args()
>   
>   jsonenc = JSONEncoder(indent=4, separators=(',', ': '))
> @@ -586,14 +591,14 @@ def default(self, o):
>   if args.extract:
>       dump = MigrationDump(args.file)
>   
> -    dump.read(desc_only = True)
> +    dump.read(desc_only = True, ram_only = args.ram_only)
>       print("desc.json")
>       f = open("desc.json", "w")
>       f.truncate()
>       f.write(jsonenc.encode(dump.vmsd_desc))
>       f.close()
>   
> -    dump.read(write_memory = True)
> +    dump.read(write_memory = True, ram_only = args.ram_only)
>       dict = dump.getDict()
>       print("state.json")
>       f = open("state.json", "w")
> @@ -602,12 +607,12 @@ def default(self, o):
>       f.close()
>   elif args.dump == "state":
>       dump = MigrationDump(args.file)
> -    dump.read(dump_memory = args.memory)
> +    dump.read(dump_memory = args.memory, ram_only = args.ram_only)
>       dict = dump.getDict()
>       print(jsonenc.encode(dict))
>   elif args.dump == "desc":
>       dump = MigrationDump(args.file)
> -    dump.read(desc_only = True)
> +    dump.read(desc_only = True, ram_only = args.ram_only)

I think, ram_only can't be supperted when we want to dump description ?

Otherwise looks good to me.

>       print(jsonenc.encode(dump.vmsd_desc))
>   else:
>       raise Exception("Please specify either -x, -d state or -d desc")
> 


-- 
Best regards,
Vladimir

Re: [PATCH 6/6] migration: Test for ram capabilities

[Vladimir Sementsov-Ogievskiy]

24.12.2021 14:11, Nikita Lapshin wrote:
> Use scripts/analyze-migration.py to split migration stream into
> sections and analyze its output.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> ---
>   .../tests/migrate-ram-capabilities-test       | 96 +++++++++++++++++++
>   .../tests/migrate-ram-capabilities-test.out   |  5 +

Most probably we have to move it ti tests/avocado

(look at tests/avocado/migration.py for example).

>   2 files changed, 101 insertions(+)
>   create mode 100755 tests/qemu-iotests/tests/migrate-ram-capabilities-test
>   create mode 100644 tests/qemu-iotests/tests/migrate-ram-capabilities-test.out
> 
> diff --git a/tests/qemu-iotests/tests/migrate-ram-capabilities-test b/tests/qemu-iotests/tests/migrate-ram-capabilities-test
> new file mode 100755
> index 0000000000..917f888340
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/migrate-ram-capabilities-test
> @@ -0,0 +1,96 @@
> +#!/usr/bin/env python3
> +# group: rw migration
> +#
> +# Tests for 'no-ram' and 'ram-only' capabilities
> +#
> +# Copyright (c) 2021 Virtuozzo International GmbH.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import json
> +import subprocess
> +import iotests
> +
> +img = os.path.join(iotests.test_dir, 'disk.img')
> +
> +class TestRamCapabilities(iotests.QMPTestCase):
> +    def setUp(self):
> +        iotests.qemu_img('create', '-f', iotests.imgfmt, img, '10M')
> +        self.vm = iotests.VM()
> +        self.vm.launch()
> +        self.vm.qmp('migrate-set-capabilities', capabilities=[
> +            {
> +                'capability': 'events',
> +                'state': True
> +            }
> +        ])
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(img)
> +
> +    def check_ram_only(self, output):
> +        str_json = output.decode()
> +        json_obj = json.loads(str_json)
> +
> +        success = False
> +        for key in json_obj:
> +            self.assertTrue("ram" in key)
> +            success = True
> +        self.assertTrue(success)

without explicit loop and extra variable:

   self.assertTrue(json_obj)  # not empty
   self.assertTrue(all("ram" in key for key in json_obj))

> +
> +    def run_migration(self, capability, tmp_stream):
> +        self.vm.qmp('migrate-set-capabilities', capabilities=[
> +            {
> +                'capability': capability,
> +                'state': True
> +            }
> +        ])
> +
> +        self.vm.qmp('migrate', uri='exec:cat>' + tmp_stream)
> +
> +        while True:
> +            event = self.vm.event_wait('MIGRATION')
> +
> +            if event['data']['status'] == 'completed':
> +                break
> +
> +
> +    def test_no_ram(self):
> +        with iotests.FilePath('tmp_stream') as tmp_stream:

Hmm, you use same construction and same file-path for both tests. Let's instead just set the path variable at top (like you do for img) and remove it in tearDown().

> +            self.run_migration('no-ram', tmp_stream)

and you will not need the second argument of run_migration()

> +            output = subprocess.run(
> +                ['../../../scripts/analyze-migration.py', '-f', tmp_stream],
> +                stdout=subprocess.PIPE,
> +                stderr=subprocess.STDOUT,
> +                check=False).stdout
> +
> +            self.assertFalse('ram' in output.decode())
> +
> +    def test_ram_only(self):
> +        with iotests.FilePath('tmp_stream') as tmp_stream:
> +            self.run_migration('ram-only', tmp_stream)
> +            output = subprocess.run(
> +                ['../../../scripts/analyze-migration.py', '-f', tmp_stream,
> +                    '--ram-only'],
> +                stdout=subprocess.PIPE,
> +                stderr=subprocess.STDOUT,
> +                check=False).stdout
> +
> +            self.check_ram_only(output)
> +
> +if __name__ == '__main__':
> +    iotests.main(supported_protocols=['file'])
> diff --git a/tests/qemu-iotests/tests/migrate-ram-capabilities-test.out b/tests/qemu-iotests/tests/migrate-ram-capabilities-test.out
> new file mode 100644
> index 0000000000..fbc63e62f8
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/migrate-ram-capabilities-test.out
> @@ -0,0 +1,5 @@
> +..
> +----------------------------------------------------------------------
> +Ran 2 tests
> +
> +OK
> 


-- 
Best regards,
Vladimir

Re: [PATCH 4/6] migration: Add ram-only capability

[Markus Armbruster]

Nikita Lapshin <nikita.lapshin@virtuozzo.com> writes:

> If this capability is enabled migration stream
> will have RAM section only.
>
> Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>

[...]

> diff --git a/qapi/migration.json b/qapi/migration.json
> index d53956852c..626fc59d14 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -454,6 +454,8 @@
>  #
>  # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0)
>  #
> +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0)
> +#

What happens when I ask for 'no-ram': true, 'ram-only': true?

>  # Features:
>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>  #
> @@ -467,7 +469,7 @@
>             'block', 'return-path', 'pause-before-switchover', 'multifd',
>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>             { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
> -           'validate-uuid', 'background-snapshot', 'no-ram'] }
> +           'validate-uuid', 'background-snapshot', 'no-ram', 'ram-only'] }
>  ##
>  # @MigrationCapabilityStatus:
>  #
> @@ -521,7 +523,8 @@
>  #       {"state": true, "capability": "events"},
>  #       {"state": false, "capability": "postcopy-ram"},
>  #       {"state": false, "capability": "x-colo"},
> -#       {"state": false, "capability": "no-ram"}
> +#       {"state": false, "capability": "no-ram"},
> +#       {"state": false, "capability": "ram-only"}
>  #    ]}
>  #
>  ##

Re: [PATCH 4/6] migration: Add ram-only capability

[Daniel P. Berrangé]

On Fri, Jan 14, 2022 at 12:22:13PM +0100, Markus Armbruster wrote:
> Nikita Lapshin <nikita.lapshin@virtuozzo.com> writes:
> 
> > If this capability is enabled migration stream
> > will have RAM section only.
> >
> > Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> 
> [...]
> 
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index d53956852c..626fc59d14 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -454,6 +454,8 @@
> >  #
> >  # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0)
> >  #
> > +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0)
> > +#
> 
> What happens when I ask for 'no-ram': true, 'ram-only': true?

So IIUC

  no-ram=false, ram-only=false =>  RAM + vmstate
  no-ram=true, ram-only=false => vmstate
  no-ram=false, ram-only=true =>  RAM
  no-ram=true, ram-only=true => nothing to send ?

I find that the fact that one flag is a negative request and
the other flag is a positive request to be confusing.

If we must have two flags then could we at least use the same
style for both. ie either

  @no-ram
  @no-vmstate

Or

  @ram-only
  @vmstate-only

Since the code enforces these flags are mutually exclusive
though, it might point towards being handled by a enum

  { 'enum': 'MigrationStreamContent',
    'data': ['both', 'ram', 'vmstate'] }

none of these approaches are especially future proof if we ever
need fine grained control over sending a sub-set of the non-RAM
vmstate. Not sure if that matters in the end.


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: [PATCH 4/6] migration: Add ram-only capability

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Jan 14, 2022 at 12:22:13PM +0100, Markus Armbruster wrote:
>> Nikita Lapshin <nikita.lapshin@virtuozzo.com> writes:
>> 
>> > If this capability is enabled migration stream
>> > will have RAM section only.
>> >
>> > Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
>> 
>> [...]
>> 
>> > diff --git a/qapi/migration.json b/qapi/migration.json
>> > index d53956852c..626fc59d14 100644
>> > --- a/qapi/migration.json
>> > +++ b/qapi/migration.json
>> > @@ -454,6 +454,8 @@
>> >  #
>> >  # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0)
>> >  #
>> > +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0)
>> > +#
>> 
>> What happens when I ask for 'no-ram': true, 'ram-only': true?
>
> So IIUC
>
>   no-ram=false, ram-only=false =>  RAM + vmstate
>   no-ram=true, ram-only=false => vmstate
>   no-ram=false, ram-only=true =>  RAM
>   no-ram=true, ram-only=true => nothing to send ?
>
> I find that the fact that one flag is a negative request and
> the other flag is a positive request to be confusing.

Me too.

> If we must have two flags then could we at least use the same
> style for both. ie either
>
>   @no-ram
>   @no-vmstate
>
> Or
>
>   @ram-only
>   @vmstate-only

I strongly prefer "positive" names for booleans, avoiding double
negation.

> Since the code enforces these flags are mutually exclusive
> though, it might point towards being handled by a enum
>
>   { 'enum': 'MigrationStreamContent',
>     'data': ['both', 'ram', 'vmstate'] }

Enumerating the combinations that are actually valid is often nicer than
a set of flags that look independent, but aren't.

MigrationCapability can only do flags.  For an enum, we'd have to use
MigrationParameters & friends.  For an example, check out
@multifd-compression there.

> none of these approaches are especially future proof if we ever
> need fine grained control over sending a sub-set of the non-RAM
> vmstate. Not sure if that matters in the end.
>
>
> Regards,
> Daniel

Re: [PATCH 4/6] migration: Add ram-only capability

[Nikta Lapshin]

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


On 1/14/22 14:22, Markus Armbruster wrote:
> Nikita Lapshin<nikita.lapshin@virtuozzo.com>  writes:
>
>> If this capability is enabled migration stream
>> will have RAM section only.
>>
>> Signed-off-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com>
> [...]
>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d53956852c..626fc59d14 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -454,6 +454,8 @@
>>   #
>>   # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0)
>>   #
>> +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0)
>> +#
> What happens when I ask for 'no-ram': true, 'ram-only': true?


migrate_caps_check() will return false and print error message that these
capabilities are not compatible.


>
>>   # Features:
>>   # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>>   #
>> @@ -467,7 +469,7 @@
>>              'block', 'return-path', 'pause-before-switchover', 'multifd',
>>              'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>>              { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
>> -           'validate-uuid', 'background-snapshot', 'no-ram'] }
>> +           'validate-uuid', 'background-snapshot', 'no-ram', 'ram-only'] }
>>   ##
>>   # @MigrationCapabilityStatus:
>>   #
>> @@ -521,7 +523,8 @@
>>   #       {"state": true, "capability": "events"},
>>   #       {"state": false, "capability": "postcopy-ram"},
>>   #       {"state": false, "capability": "x-colo"},
>> -#       {"state": false, "capability": "no-ram"}
>> +#       {"state": false, "capability": "no-ram"},
>> +#       {"state": false, "capability": "ram-only"}
>>   #    ]}
>>   #
>>   ##

[-- Attachment #2: Type: text/html, Size: 2917 bytes --]

Re: [PATCH 4/6] migration: Add ram-only capability

[Vladimir Sementsov-Ogievskiy]

14.01.2022 18:55, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Fri, Jan 14, 2022 at 12:22:13PM +0100, Markus Armbruster wrote:
>>> Nikita Lapshin <nikita.lapshin@virtuozzo.com> writes:
>>>
>>>> If this capability is enabled migration stream
>>>> will have RAM section only.
>>>>
>>>> Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
>>>
>>> [...]
>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index d53956852c..626fc59d14 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -454,6 +454,8 @@
>>>>   #
>>>>   # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0)
>>>>   #
>>>> +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0)
>>>> +#
>>>
>>> What happens when I ask for 'no-ram': true, 'ram-only': true?
>>
>> So IIUC
>>
>>    no-ram=false, ram-only=false =>  RAM + vmstate
>>    no-ram=true, ram-only=false => vmstate
>>    no-ram=false, ram-only=true =>  RAM
>>    no-ram=true, ram-only=true => nothing to send ?
>>
>> I find that the fact that one flag is a negative request and
>> the other flag is a positive request to be confusing.
> 
> Me too.
> 
>> If we must have two flags then could we at least use the same
>> style for both. ie either
>>
>>    @no-ram
>>    @no-vmstate
>>
>> Or
>>
>>    @ram-only
>>    @vmstate-only
> 
> I strongly prefer "positive" names for booleans, avoiding double
> negation.
> 
>> Since the code enforces these flags are mutually exclusive
>> though, it might point towards being handled by a enum
>>
>>    { 'enum': 'MigrationStreamContent',
>>      'data': ['both', 'ram', 'vmstate'] }
> 
> Enumerating the combinations that are actually valid is often nicer than
> a set of flags that look independent, but aren't.
> 
> MigrationCapability can only do flags.  For an enum, we'd have to use
> MigrationParameters & friends.  For an example, check out
> @multifd-compression there.
> 

Remember, that we also have block-dirty-bitmaps in migration stream. And we also may have block devices data (should it be deprecated?).

So, what about an optional migration parameter stream-content, which would be a *list* of MigrationStreamContent enum values?

Than we can deprecate dirty-bitmaps and block capabilities in favor of new migration parameter.

{ 'enum': 'MigrationStreamContent', 'data': ['block-dirty-bitmpas', 'block', 'ram', 'vmstate'] }

...

Migration parameters:

...
'stream-content': ['MigrationStreamContent']


-- 
Best regards,
Vladimir