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

[Juan Quintela]

Steven Sistare <steven.sistare@oracle.com> wrote:
> 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

As a started, that sounds good.
When you add more features, please add tests acordingly.

Thanks again, Juan.