Re: [PATCH v5 06/12] nbd: Update qapi to support exporting multiple bitmaps

[Eric Blake]

On 10/26/20 5:50 AM, Peter Krempa wrote:
> On Fri, Oct 23, 2020 at 13:36:46 -0500, Eric Blake wrote:
>> Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to
>> 5.2, we can still tweak the interface.  Allowing 'bitmaps':['str'] is
>> nicer than 'bitmap':'str'.  This wires up the qapi and qemu-nbd
>> changes to permit passing multiple bitmaps as distinct metadata
>> contexts that the NBD client may request, but the actual support for
>> more than one will require a further patch to the server.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---

>> +++ b/qapi/block-export.json
>> @@ -74,10 +74,10 @@
>>  # @description: Free-form description of the export, up to 4096 bytes.
>>  #               (Since 5.0)
>>  #
>> -# @bitmap: Also export the dirty bitmap reachable from @device, so the
>> -#          NBD client can use NBD_OPT_SET_META_CONTEXT with the
>> -#          metadata context name "qemu:dirty-bitmap:NAME" to inspect the
>> -#          bitmap. (since 4.0)
>> +# @bitmaps: Also export each of the named dirty bitmaps reachable from
>> +#           @device, so the NBD client can use NBD_OPT_SET_META_CONTEXT with
>> +#           the metadata context name "qemu:dirty-bitmap:BITMAP" to inspect
>> +#           each bitmap. (since 5.2)
> 
> Given unsynchronised release cycles between qemu and management apps
> it's not cool to deprecate an interface without having at least one
> release where the replacement interface is already stable.
> 

That's why I'm trying as hard as possible to get the block-export-add
interface perfect in its 5.2 release; if we agree that allowing qemu to
expose more than one bitmap is beneficial (and I argue that it is), then
the new interface MUST support that from the get-go, and not something
where we release it with 5.2 having 'bitmap' and 6.0 adding 'bitmaps'.

> This means that any project wanting to stay up to date will either have
> to use deprecated interfaces for at least one release and develop the
> replacement only when there's a stable interface or hope that they don't
> have to change the interfaces too often.
> 
> This specifically impacts libvirt as we have validators which notify us
> that a deprecated interface is used and we want to stay in sync, so that
> there's one less group of users to worry about at the point qemu will
> want to delete the interface.

The deprecated interface is nbd-server-add; for _that_ interface, the
'bitmap' parameter will continue to work until nbd-server-add is removed
in 6.1 (so you have all of 5.2 and 6.0 to switch from nbd-server-add to
block-export-add).

What this patch does, then, is alter the deprecation from merely
changing the command from nbd-server-add to block-export-add with all
parameter names remaining the same, to instead changing both the command
name and the 'bitmap'=>'bitmaps' parameter.  But I agree that libvirt
wants to do an all-or-none conversion: there is no reason for libvirt to
use block-export-add until 5.2 is actually released, at which point we
have locked in the new interface; and this patch is a demonstration that
we are still debating about a tweak to that interface before it becomes
locked in.

> 
>>  # @allocation-depth: Also export the allocation depth map for @device, so
>>  #                    the NBD client can use NBD_OPT_SET_META_CONTEXT with
>> @@ -88,7 +88,7 @@
>>  ##
>>  { 'struct': 'BlockExportOptionsNbd',
>>    'data': { '*name': 'str', '*description': 'str',
>> -            '*bitmap': 'str', '*allocation-depth': 'bool' } }
>> +            '*bitmaps': ['str'], '*allocation-depth': 'bool' } }
> 
> This adds 'bitmaps' also to nbd-server-add, which should not happen. You
> probably want to stop using 'base' for 'NbdServerAddOptions' and just
> duplicate everything else.

I can respin this to NOT add 'bitmaps' to the legacy 'nbd-server-add',
if you think that would be better.  It is more complex in the QAPI code,
but not too much more difficulty in the glue code; and the glue code all
goes away in 6.1 when the deprecation cycle ends.

> 
>>  # @NbdServerAddOptions:
>> @@ -100,12 +100,18 @@
>>  # @writable: Whether clients should be able to write to the device via the
>>  #            NBD connection (default false).
>>  #
>> +# @bitmap: Also export a single dirty bitmap reachable from @device, so the
>> +#          NBD client can use NBD_OPT_SET_META_CONTEXT with the metadata
>> +#          context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap
>> +#          (since 4.0).  Mutually exclusive with @bitmaps, and newer
>> +#          clients should use that instead.
> 
> This doesn't make sense, nbd-server-add never had @bitmaps. Also adding
> features to a deprecated interface doesn't IMO make sense if you want to
> motivate users switch to thne new one.

Fair enough. v6 coming up later today.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org