Re: [RFC v2] migration: Add migrate-set-bitmap-node-mapping

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5489 bytes --]

On 18.05.20 18:26, Peter Krempa wrote:
> On Wed, May 13, 2020 at 16:56:10 +0200, Max Reitz wrote:
>> This command allows mapping block node names to aliases for the purpose
>> of block dirty bitmap migration.
>>
>> This way, management tools can use different node names on the source
>> and destination and pass the mapping of how bitmaps are to be
>> transferred to qemu (on the source, the destination, or even both with
>> arbitrary aliases in the migration stream).
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
> 
> [...]
> 
>> ---
>>  qapi/migration.json            | 36 ++++++++++++++++++++
>>  migration/block-dirty-bitmap.c | 60 ++++++++++++++++++++++++++++++++--
>>  2 files changed, 94 insertions(+), 2 deletions(-)
> 
> I just started to write some quick and dirty hacks to test use of this
> infrastructure in libvirt. I have 2 quick observations for now:
> 
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d5000558c6..97037ea635 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1621,3 +1621,39 @@
>>  ##
>>  { 'event': 'UNPLUG_PRIMARY',
>>    'data': { 'device-id': 'str' } }
>> +
>> +##
>> +# @MigrationBlockNodeMapping:
>> +#
>> +# Maps a block node name to an alias for migration.
>> +#
>> +# @node-name: A block node name.
>> +#
>> +# @alias: An alias name for migration (for example the node name on
>> +#         the opposite site).
>> +#
>> +# Since: 5.1
>> +##
>> +{ 'struct': 'MigrationBlockNodeMapping',
>> +  'data': {
>> +      'node-name': 'str',
>> +      'alias': 'str'
>> +  } }
> 
> We'd probably like a
> 'nodename:bitmapname' -> 'alias' mapping so that we can select which
> bitmaps to migrate and where to migrate them to.

Sure, entirely doable.

I think Vladimir is right that we’d want separate node and bitmap
aliases then, though, because the migration stream has both fields.
(Also, if we only had a single alias, you’d always need to call
migrate-set-bitmap-(node-)mapping on both ends to unpack that alias.
With separate node and bitmap aliases, it suffices to call it on one
end, just like the version in thie patch.)

> The specific use case
> is following:
> 
> Libvirt supports migration without shared storage (NFS/etc) where we
> copy the disk images prior to the memory migration using qemu's NBD
> server and the blockdev-mirror job. By default and the most simple way
> which doesn't require users fudging with the disk images and copying
> backing images themselves is that we flatten all the backing chain
> during the copy ("sync":"full"). In this mode we'll need to do some
> merging of the bitmaps prior to finalizing the copy.
> 
> It's not a problem to do it ourselves, but in the end we'll need to copy
> only certain bitmaps which will be created temporarily on the source to
> the destination where they'll be persisted.
> 
> For now (until I implement the use of the dirty-bitmap-populate blockjob
> which I'm also doing in parallel with this kind of) when creating a
> snapshot we create a new active bitmap in the overlay for every active
> bitmap in the snapshotted image.
> 
> When flattening we'll then need to merge the appropriate ones. As said
> it's not a problem to prepare all the bitmaps before but we then don't
> need to migrate all of them.
> 
> By the way, that brings me to another question:
> 
> Is there any difference of handling of persistent and non-persistent
> bitmaps? Specifically I'm curious what's the correct approach to do the
> shared storage migration case when the source and destination image is
> the same one. Should we also instruct to migrate the active ones? or are
> they migrated by writing them to the image and reloading them?

I hope Vladimir has answered your question sufficiently extensively. :)

>> +##
>> +# @migrate-set-bitmap-node-mapping:
> 
> qemu-5.0 deprecated a bunch of migrate-set- specific commands in favor
> of migrate-set-parameters. Is it worth/necessary to add a new command
> here?

I wasn’t aware of that.  It would probably indeed make sense from a
user’s perspective.

It would make the implementation rather different, though, because
instead of putting the mapping locally (and statically) into
migration/block-dirty-bitmap.c, it would require putting it into the
central MigrationState.  Which isn’t to say it’d be worse.  I suppose
it’d be better, actually, but I’ll have to try to say for sure.

You also suggested “setting nothing will clear the mapping” in your
second mail.  That would be a weird default.  Right now, the default for
all migration parameters is to leave them as-is, so it would be different.

The first question would be: What do you mean by “clear the mapping”?
Reset it to the original identity mapping?  Or actually clear it, so
that no bitmap is migrated?  I presume the former, because the latter
can easily be achieved by passing an empty array.

I understand that it seems to make sense to be able to return to the
original identity mapping, but is there actually a use for this?  After
you have started using a custom mapping, wouldn’t you always use custom
mappings?

If there is a use for it, I think the better way to do it would be to
use an alternate type where an explicit null resets the mapping to the
identity mapping.

Max


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