Re: [PATCH 1/2] block/rbd: support selected key-value-pairs via QAPI

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jun 16, 2025 at 02:29:56PM +0200, Fiona Ebner wrote:
> Am 16.06.25 um 11:41 schrieb Daniel P. Berrangé:
> > On Thu, May 15, 2025 at 01:29:07PM +0200, Fiona Ebner wrote:
> >> @@ -4327,6 +4360,9 @@
> >>  #     authentication.  This maps to Ceph configuration option "key".
> >>  #     (Since 3.0)
> >>  #
> >> +# @key-value-pairs: Key-value pairs for additional Ceph configuraton.
> >> +#     (Since 10.1)
> >> +#
> >>  # @server: Monitor host address and port.  This maps to the "mon_host"
> >>  #     Ceph option.
> >>  #
> >> @@ -4342,6 +4378,7 @@
> >>              '*user': 'str',
> >>              '*auth-client-required': ['RbdAuthMode'],
> >>              '*key-secret': 'str',
> >> +            '*key-value-pairs' : 'RbdKeyValuePairs',
> >
> > I'm not seeing any point in this 'RbdKeyValuePairs' struct. Why isn't
> > the 'rbd-cache-policy' field just directly part of the BlockdevOptionsRbd
> > struct like all the other options are ?
> >
> > Also, 'rbd-' as a prefix in the field name is redundant when this is
> > already in an RBD specific struct.
> 
> Am 16.06.25 um 11:25 schrieb Ilya Dryomov:
> > Hi Fiona,
> > 
> > I'm not following the rationale for introducing RbdKeyValuePairs
> > struct.  If there is a desire to expose rbd_cache_policy option this
> > way, couldn't it just be added to BlockdevOptionsRbd struct?  The
> > existing auth_client_required option has a very similar pattern.
> 
> Hi Ilya and Daniel,
> 
> my intention was to not "pollute" the top-level struct with rather
> uncommon options, but collect them separately and also make it explicit
> that those are the key-value pairs passed along to Ceph.

If these are useful things that users of ceph need to be able to
control for their QEMU VMs, then this is not "pollution".

> > If exposing rbd_cache_policy option, rbd_cache option (enabled/disabled
> > bool) should likely be exposed as well.  It doesn't make much sense to
> > provide a built-in way to adjust the cache policy without also providing
> > a built-in way to disable the cache entirely.  Then the question of what
> > would be better from the QAPI perspective arises: a bool option to map
> > to Ceph as close as possible or perhaps an additional 'disabled' value
> > in RbdCachePolicy enum?  And regardless of that, the need to remember
> > to update QEMU if a new cache policy is ever added because even though
> > these are just strings, QEMU is going to be validating them...
> 
> Good point! If going with the key-value-pairs approach, it would be
> nicer to go with a direct mapping IMHO. If adding it to the top-level,
> I'd be in favor of the 'disabled' value.

If this is stuff that users expect/need to be able to configure for
their VMs, that would bias towards formal modelling in QAPI, not plain
passthrough.

NB, any features intended to be configurable via libvirt would need
to be formalling modelled, as libvirt won't introduce a dependency
on things that QEMU declares "unstable" in QAPI.

With 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 :|