Re: [PATCH] block/rbd: Add support for ceph namespaces

[Stefano Garzarella <sgarzare@redhat.com>]

On Fri, Dec 20, 2019 at 09:56:51AM -0500, Jason Dillaman wrote:
> On Fri, Dec 20, 2019 at 9:11 AM Florian Florensa <fflorensa@online.net> wrote:
> >
> > Hello Stefano and Jason,
> >
> > First of all thanks for the quick reply,
> > Response inline belowe
> > > Hi Florian,
> > >
> > > I think we need to add (Since: 5.0).
> >
> > Are you implying by that (Since: 5.0) that we need to specify its
> > availability target is qemu 5.0 ?

Exactly, as Jason suggested is part of documentation,

Following the file, I mean something like that:

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0cf68fea14..9ebc020e93 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3657,6 +3657,8 @@
 #
 # @pool:               Ceph pool name.
 #
+# @namespace:          Rados namespace name in the Ceph pool. (Since: 5.0)
+#
 # @image:              Image name in the Ceph pool.
 #
 # @conf:               path to Ceph configuration file.  Values


> 
> FWIW, I took this as just a comment to add some documentation that the
> field is only valid starting w/ qemu v5.
> 
> > I guess that maybe a version check would be better ? Like try to do
> > namespaces stuff only if we have a recent enough librbd in the system ?
> > Using something like :
> >
> > int rbd_major;
> >
> > rbd_version(&rbd_major, NULL, NULL);
> > /*
> >  * Target only nautilus+ librbd for namespace support
> > */
> > if (rbd_major >= 14) // tar
> >  <process namespace>
> 
> Unfortunately, those versions weren't updated in the Mimic nor
> Nautilus release so it would still return 1/12 (whoops). I think that
> means you would need to add a probe in "configure" to test for librbd
> namespace support (e.g. test for the existence of the `rbd_list2`
> function or the `rbd_linked_image_spec_t` structure). I'll fix this
> before the forthcoming Octopus release.
> 
> > > The patch LGTM, but I'd like to use 'namespace' instead of cryptic
> > > 'nspace'. (as BlockdevOptionsNVMe did)
> > > What do you think?
> > >
> > Yes no worries, I can rename it to 'rbd_namespace' to avoid any possible
> > confusion, is this Ok for you ?
> 
> We use "pool_namespace" in the rbd CLI if you are trying to avoid the
> word "namespace".
> 

Agree, I'd avoid the 'rbd_' prefix.

Thanks,
Stefano