Re: [PATCH V1 4/4] cpr: reboot mode

[Steven Sistare <steven.sistare@oracle.com>]

On 10/20/2023 3:40 PM, Juan Quintela wrote:
> Steven Sistare <steven.sistare@oracle.com> wrote:
>> On 10/20/2023 5:45 AM, Juan Quintela wrote:
>>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>>> Add the cpr-reboot migration mode.  Usage:
>>>>
>>>> $ qemu-system-$arch -monitor stdio ...
>>>> QEMU 8.1.50 monitor - type 'help' for more information
>>>> (qemu) migrate_set_capability x-ignore-shared on
>>>> (qemu) migrate_set_parameter mode cpr-reboot
>>>> (qemu) migrate -d file:vm.state
>>>> (qemu) info status
>>>> VM status: paused (postmigrate)
>>>> (qemu) quit
>>>>
>>>> $ qemu-system-$arch -monitor stdio -incoming defer ...
>>>> QEMU 8.1.50 monitor - type 'help' for more information
>>>> (qemu) migrate_set_capability x-ignore-shared on
>>>> (qemu) migrate_set_parameter mode cpr-reboot
>>>> (qemu) migrate_incoming file:vm.state
>>>> (qemu) info status
>>>> VM status: running
>>>>
>>>> In this mode, the migrate command saves state to a file, allowing one
>>>> to quit qemu, reboot to an updated kernel, and restart an updated version
>>>> of qemu.  The caller must specify a migration URI that writes to and reads
>>>> from a file.  Unlike normal mode, the use of certain local storage options
>>>> does not block the migration, but the caller must not modify guest block
>>>> devices between the quit and restart.  The guest RAM memory-backend must
>>>> be shared, and the @x-ignore-shared migration capability must be set,
>>>> to avoid saving RAM to the file.  Guest RAM must be non-volatile across
>>>> reboot, such as by backing it with a dax device, but this is not enforced.
>>>> The restarted qemu arguments must match those used to initially start qemu,
>>>> plus the -incoming option.
>>>
>>> Please, add this message to doc/<somewhere> instead (or additionally) to
>>> the commit log.
>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>  qapi/migration.json | 16 +++++++++++++++-
>>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index 184fb78..2d862fa 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -620,9 +620,23 @@
>>>>  #
>>>>  # @normal: the original form of migration. (since 8.2)
>>>>  #
>>>> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
>>>> +#              quit qemu, reboot to an updated kernel, and restart an updated
>>>> +#              version of qemu.  The caller must specify a migration URI
>>>> +#              that writes to and reads from a file.  Unlike normal mode,
>>>> +#              the use of certain local storage options does not block the
>>>> +#              migration, but the caller must not modify guest block devices
>>>> +#              between the quit and restart.  The guest RAM memory-backend
>>>> +#              must be shared, and the @x-ignore-shared migration capability
>>>> +#              must be set, to avoid saving it to the file.  Guest RAM must
>>>> +#              be non-volatile across reboot, such as by backing it with
>>>> +#              a dax device, but this is not enforced.  The restarted qemu
>>>> +#              arguments must match those used to initially start qemu, plus
>>>> +#              the -incoming option. (since 8.2)
>>>> +#
>>>>  ##
>>>>  { 'enum': 'MigMode',
>>>> -  'data': [ 'normal' ] }
>>>> +  'data': [ 'normal', 'cpr-reboot' ] }
>>>>  
>>>>  ##
>>>>  # @BitmapMigrationBitmapAliasTransform:
>>>
>>> It only works with file backend, and we don't have any check for that.
>>> Wondering how to add that check.
>>
>> Actually, it works for other backends, but the ram contents are saved in the
>> state file, which is slower. I should spell that out in the json comment and
>> in the commit message.
> 
> Thanks.
>>
>>> Additionally, you are not adding a migration test that does exactly what
>>> you put there in the comment.
>>
>> I provide tests/avocado/cpr.py in the original long series.  Would you
>> like me to add it to this series, or post it later?  Would you prefer I
>> add a test to tests/qtest/migration-test.c?
> 
> test/qtest/migration-test.c
> 
> please.
> 
> Something simple like what you say in the commit should be a good start.

I wrote a new test which I will submit with V2 of this series.  Turned out to be
easy after Fabiano provided test_file_common.

+static void *test_mode_reboot_start(QTestState *from, QTestState *to)
+{
+    migrate_set_parameter_str(from, "mode", "cpr-reboot");
+    migrate_set_parameter_str(to, "mode", "cpr-reboot");
+
+    migrate_set_capability(from, "x-ignore-shared", true);
+    migrate_set_capability(to, "x-ignore-shared", true);
+
+    return NULL;
+}
+
+static void test_mode_reboot(void)
+{
+    g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
+                                           FILE_TEST_FILENAME);
+    MigrateCommon args = {
+        .start.use_shmem = true,
+        .connect_uri = uri,
+        .listen_uri = "defer",
+        .start_hook = test_mode_reboot_start
+    };
+
+    test_file_common(&args, true);
+}

- Steve