* [PATCH] block/rbd: Add support for ceph namespaces
@ 2019-12-19 13:34 Florian Florensa
2019-12-19 14:15 ` Jason Dillaman
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Florian Florensa @ 2019-12-19 13:34 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, dillaman, armbru, mreitz
Starting from ceph Nautilus, RBD has support for namespaces, allowing
for finer grain ACLs on images inside a pool, and tenant isolation.
In the rbd cli tool documentation, the new image-spec and snap-spec are :
- [pool-name/[namespace-name/]]image-name
- [pool-name/[namespace-name/]]image-name@snap-name
When using an non namespace's enabled qemu, it complains about not
finding the image called namespace-name/image-name, thus we only need to
parse the image once again to find if there is a '/' in its name, and if
there is, use what is before it as the name of the namespace to later
pass it to rados_ioctx_set_namespace.
rados_ioctx_set_namespace if called with en empty string or a null
pointer as the namespace parameters pretty much does nothing, as it then
defaults to the default namespace.
The namespace is extracted inside qemu_rbd_parse_filename, stored in the
qdict, and used in qemu_rbd_connect to make it work with both qemu-img,
and qemu itself.
Signed-off-by: Florian Florensa <fflorensa@online.net>
---
block/rbd.c | 30 ++++++++++++++++++++++++------
qapi/block-core.json | 3 +++
2 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 027cbcc695..e43099fc75 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -104,6 +104,7 @@ typedef struct BDRVRBDState {
rbd_image_t image;
char *image_name;
char *snap;
+ char *nspace;
uint64_t image_size;
} BDRVRBDState;
@@ -152,7 +153,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
const char *start;
char *p, *buf;
QList *keypairs = NULL;
- char *found_str;
+ char *found_str, *image_name;
if (!strstart(filename, "rbd:", &start)) {
error_setg(errp, "File name must start with 'rbd:'");
@@ -171,18 +172,24 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
qdict_put_str(options, "pool", found_str);
if (strchr(p, '@')) {
- found_str = qemu_rbd_next_tok(p, '@', &p);
- qemu_rbd_unescape(found_str);
- qdict_put_str(options, "image", found_str);
+ image_name = qemu_rbd_next_tok(p, '@', &p);
found_str = qemu_rbd_next_tok(p, ':', &p);
qemu_rbd_unescape(found_str);
qdict_put_str(options, "snapshot", found_str);
} else {
- found_str = qemu_rbd_next_tok(p, ':', &p);
+ image_name = qemu_rbd_next_tok(p, ':', &p);
+ }
+ /* Check for namespace in the image_name */
+ if (strchr(image_name, '/')) {
+ found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
qemu_rbd_unescape(found_str);
- qdict_put_str(options, "image", found_str);
+ qdict_put_str(options, "nspace", found_str);
+ } else {
+ qdict_put_str(options, "nspace", "");
}
+ qemu_rbd_unescape(image_name);
+ qdict_put_str(options, "image", image_name);
if (!p) {
goto done;
}
@@ -343,6 +350,11 @@ static QemuOptsList runtime_opts = {
.type = QEMU_OPT_STRING,
.help = "Rados pool name",
},
+ {
+ .name = "nspace",
+ .type = QEMU_OPT_STRING,
+ .help = "Rados namespace name in the pool",
+ },
{
.name = "image",
.type = QEMU_OPT_STRING,
@@ -472,6 +484,7 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
loc->has_conf = !!loc->conf;
loc->user = g_strdup(qdict_get_try_str(options, "user"));
loc->has_user = !!loc->user;
+ loc->nspace = g_strdup(qdict_get_try_str(options, "nspace"));
loc->image = g_strdup(qdict_get_try_str(options, "image"));
keypairs = qdict_get_try_str(options, "=keyvalue-pairs");
@@ -648,6 +661,11 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
error_setg_errno(errp, -r, "error opening pool %s", opts->pool);
goto failed_shutdown;
}
+ /*
+ * Set the namespace after opening the io context on the pool,
+ * if nspace == NULL or if nspace == "", it is just as we did nothing
+ */
+ rados_ioctx_set_namespace(*io_ctx, opts->nspace);
return 0;
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.
#
+# @nspace: Rados namespace name in the Ceph pool.
+#
# @image: Image name in the Ceph pool.
#
# @conf: path to Ceph configuration file. Values
@@ -3683,6 +3685,7 @@
##
{ 'struct': 'BlockdevOptionsRbd',
'data': { 'pool': 'str',
+ 'nspace': 'str',
'image': 'str',
'*conf': 'str',
'*snapshot': 'str',
--
2.24.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] block/rbd: Add support for ceph namespaces
2019-12-19 13:34 [PATCH] block/rbd: Add support for ceph namespaces Florian Florensa
@ 2019-12-19 14:15 ` Jason Dillaman
2019-12-19 14:51 ` Stefano Garzarella
2019-12-20 21:02 ` Eric Blake
2 siblings, 0 replies; 9+ messages in thread
From: Jason Dillaman @ 2019-12-19 14:15 UTC (permalink / raw)
To: Florian Florensa
Cc: Kevin Wolf, dillaman, qemu-devel, Markus Armbruster, Max Reitz
On Thu, Dec 19, 2019 at 8:44 AM Florian Florensa <fflorensa@online.net> wrote:
>
> Starting from ceph Nautilus, RBD has support for namespaces, allowing
> for finer grain ACLs on images inside a pool, and tenant isolation.
>
> In the rbd cli tool documentation, the new image-spec and snap-spec are :
> - [pool-name/[namespace-name/]]image-name
> - [pool-name/[namespace-name/]]image-name@snap-name
>
> When using an non namespace's enabled qemu, it complains about not
> finding the image called namespace-name/image-name, thus we only need to
> parse the image once again to find if there is a '/' in its name, and if
> there is, use what is before it as the name of the namespace to later
> pass it to rados_ioctx_set_namespace.
> rados_ioctx_set_namespace if called with en empty string or a null
> pointer as the namespace parameters pretty much does nothing, as it then
> defaults to the default namespace.
>
> The namespace is extracted inside qemu_rbd_parse_filename, stored in the
> qdict, and used in qemu_rbd_connect to make it work with both qemu-img,
> and qemu itself.
>
> Signed-off-by: Florian Florensa <fflorensa@online.net>
> ---
> block/rbd.c | 30 ++++++++++++++++++++++++------
> qapi/block-core.json | 3 +++
> 2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 027cbcc695..e43099fc75 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -104,6 +104,7 @@ typedef struct BDRVRBDState {
> rbd_image_t image;
> char *image_name;
> char *snap;
> + char *nspace;
> uint64_t image_size;
> } BDRVRBDState;
>
> @@ -152,7 +153,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
> const char *start;
> char *p, *buf;
> QList *keypairs = NULL;
> - char *found_str;
> + char *found_str, *image_name;
>
> if (!strstart(filename, "rbd:", &start)) {
> error_setg(errp, "File name must start with 'rbd:'");
> @@ -171,18 +172,24 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
> qdict_put_str(options, "pool", found_str);
>
> if (strchr(p, '@')) {
> - found_str = qemu_rbd_next_tok(p, '@', &p);
> - qemu_rbd_unescape(found_str);
> - qdict_put_str(options, "image", found_str);
> + image_name = qemu_rbd_next_tok(p, '@', &p);
>
> found_str = qemu_rbd_next_tok(p, ':', &p);
> qemu_rbd_unescape(found_str);
> qdict_put_str(options, "snapshot", found_str);
> } else {
> - found_str = qemu_rbd_next_tok(p, ':', &p);
> + image_name = qemu_rbd_next_tok(p, ':', &p);
> + }
> + /* Check for namespace in the image_name */
> + if (strchr(image_name, '/')) {
> + found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
> qemu_rbd_unescape(found_str);
> - qdict_put_str(options, "image", found_str);
> + qdict_put_str(options, "nspace", found_str);
> + } else {
> + qdict_put_str(options, "nspace", "");
> }
> + qemu_rbd_unescape(image_name);
> + qdict_put_str(options, "image", image_name);
> if (!p) {
> goto done;
> }
> @@ -343,6 +350,11 @@ static QemuOptsList runtime_opts = {
> .type = QEMU_OPT_STRING,
> .help = "Rados pool name",
> },
> + {
> + .name = "nspace",
> + .type = QEMU_OPT_STRING,
> + .help = "Rados namespace name in the pool",
> + },
> {
> .name = "image",
> .type = QEMU_OPT_STRING,
> @@ -472,6 +484,7 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
> loc->has_conf = !!loc->conf;
> loc->user = g_strdup(qdict_get_try_str(options, "user"));
> loc->has_user = !!loc->user;
> + loc->nspace = g_strdup(qdict_get_try_str(options, "nspace"));
> loc->image = g_strdup(qdict_get_try_str(options, "image"));
> keypairs = qdict_get_try_str(options, "=keyvalue-pairs");
>
> @@ -648,6 +661,11 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
> error_setg_errno(errp, -r, "error opening pool %s", opts->pool);
> goto failed_shutdown;
> }
> + /*
> + * Set the namespace after opening the io context on the pool,
> + * if nspace == NULL or if nspace == "", it is just as we did nothing
> + */
> + rados_ioctx_set_namespace(*io_ctx, opts->nspace);
>
> return 0;
>
> 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.
> #
> +# @nspace: Rados namespace name in the Ceph pool.
> +#
> # @image: Image name in the Ceph pool.
> #
> # @conf: path to Ceph configuration file. Values
> @@ -3683,6 +3685,7 @@
> ##
> { 'struct': 'BlockdevOptionsRbd',
> 'data': { 'pool': 'str',
> + 'nspace': 'str',
> 'image': 'str',
> '*conf': 'str',
> '*snapshot': 'str',
> --
> 2.24.1
>
Thanks for tackling this. I had this and msgr v2 support on my todo
list for QEMU but I haven't had a chance to work on them yet. The
changes look good to me and it works as expected during CLI
play-testing.
Reviewed-by: Jason Dillaman <dillaman@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block/rbd: Add support for ceph namespaces
2019-12-19 13:34 [PATCH] block/rbd: Add support for ceph namespaces Florian Florensa
2019-12-19 14:15 ` Jason Dillaman
@ 2019-12-19 14:51 ` Stefano Garzarella
2019-12-20 14:11 ` Florian Florensa
2019-12-20 21:02 ` Eric Blake
2 siblings, 1 reply; 9+ messages in thread
From: Stefano Garzarella @ 2019-12-19 14:51 UTC (permalink / raw)
To: Florian Florensa; +Cc: kwolf, mreitz, dillaman, qemu-devel, armbru
Hi Florian,
On Thu, Dec 19, 2019 at 02:34:16PM +0100, Florian Florensa wrote:
> Starting from ceph Nautilus, RBD has support for namespaces, allowing
> for finer grain ACLs on images inside a pool, and tenant isolation.
>
> In the rbd cli tool documentation, the new image-spec and snap-spec are :
> - [pool-name/[namespace-name/]]image-name
> - [pool-name/[namespace-name/]]image-name@snap-name
>
> When using an non namespace's enabled qemu, it complains about not
> finding the image called namespace-name/image-name, thus we only need to
> parse the image once again to find if there is a '/' in its name, and if
> there is, use what is before it as the name of the namespace to later
> pass it to rados_ioctx_set_namespace.
> rados_ioctx_set_namespace if called with en empty string or a null
> pointer as the namespace parameters pretty much does nothing, as it then
> defaults to the default namespace.
>
> The namespace is extracted inside qemu_rbd_parse_filename, stored in the
> qdict, and used in qemu_rbd_connect to make it work with both qemu-img,
> and qemu itself.
>
> Signed-off-by: Florian Florensa <fflorensa@online.net>
> ---
> block/rbd.c | 30 ++++++++++++++++++++++++------
> qapi/block-core.json | 3 +++
> 2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 027cbcc695..e43099fc75 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -104,6 +104,7 @@ typedef struct BDRVRBDState {
> rbd_image_t image;
> char *image_name;
> char *snap;
> + char *nspace;
> uint64_t image_size;
> } BDRVRBDState;
>
> @@ -152,7 +153,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
> const char *start;
> char *p, *buf;
> QList *keypairs = NULL;
> - char *found_str;
> + char *found_str, *image_name;
>
> if (!strstart(filename, "rbd:", &start)) {
> error_setg(errp, "File name must start with 'rbd:'");
> @@ -171,18 +172,24 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
> qdict_put_str(options, "pool", found_str);
>
> if (strchr(p, '@')) {
> - found_str = qemu_rbd_next_tok(p, '@', &p);
> - qemu_rbd_unescape(found_str);
> - qdict_put_str(options, "image", found_str);
> + image_name = qemu_rbd_next_tok(p, '@', &p);
>
> found_str = qemu_rbd_next_tok(p, ':', &p);
> qemu_rbd_unescape(found_str);
> qdict_put_str(options, "snapshot", found_str);
> } else {
> - found_str = qemu_rbd_next_tok(p, ':', &p);
> + image_name = qemu_rbd_next_tok(p, ':', &p);
> + }
> + /* Check for namespace in the image_name */
> + if (strchr(image_name, '/')) {
> + found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
> qemu_rbd_unescape(found_str);
> - qdict_put_str(options, "image", found_str);
> + qdict_put_str(options, "nspace", found_str);
> + } else {
> + qdict_put_str(options, "nspace", "");
> }
> + qemu_rbd_unescape(image_name);
> + qdict_put_str(options, "image", image_name);
> if (!p) {
> goto done;
> }
> @@ -343,6 +350,11 @@ static QemuOptsList runtime_opts = {
> .type = QEMU_OPT_STRING,
> .help = "Rados pool name",
> },
> + {
> + .name = "nspace",
> + .type = QEMU_OPT_STRING,
> + .help = "Rados namespace name in the pool",
> + },
> {
> .name = "image",
> .type = QEMU_OPT_STRING,
> @@ -472,6 +484,7 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
> loc->has_conf = !!loc->conf;
> loc->user = g_strdup(qdict_get_try_str(options, "user"));
> loc->has_user = !!loc->user;
> + loc->nspace = g_strdup(qdict_get_try_str(options, "nspace"));
> loc->image = g_strdup(qdict_get_try_str(options, "image"));
> keypairs = qdict_get_try_str(options, "=keyvalue-pairs");
>
> @@ -648,6 +661,11 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
> error_setg_errno(errp, -r, "error opening pool %s", opts->pool);
> goto failed_shutdown;
> }
> + /*
> + * Set the namespace after opening the io context on the pool,
> + * if nspace == NULL or if nspace == "", it is just as we did nothing
> + */
> + rados_ioctx_set_namespace(*io_ctx, opts->nspace);
>
> return 0;
>
> 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.
> #
> +# @nspace: Rados namespace name in the Ceph pool.
> +#
I think we need to add (Since: 5.0).
The patch LGTM, but I'd like to use 'namespace' instead of cryptic
'nspace'. (as BlockdevOptionsNVMe did)
What do you think?
With those fixed:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Thanks,
Stefano
> # @image: Image name in the Ceph pool.
> #
> # @conf: path to Ceph configuration file. Values
> @@ -3683,6 +3685,7 @@
> ##
> { 'struct': 'BlockdevOptionsRbd',
> 'data': { 'pool': 'str',
> + 'nspace': 'str',
> 'image': 'str',
> '*conf': 'str',
> '*snapshot': 'str',
> --
> 2.24.1
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block/rbd: Add support for ceph namespaces
2019-12-19 14:51 ` Stefano Garzarella
@ 2019-12-20 14:11 ` Florian Florensa
2019-12-20 14:56 ` Jason Dillaman
0 siblings, 1 reply; 9+ messages in thread
From: Florian Florensa @ 2019-12-20 14:11 UTC (permalink / raw)
To: Stefano Garzarella; +Cc: kwolf, mreitz, dillaman, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 979 bytes --]
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 ?
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>
>
> 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 ?
>
> With those fixed:
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
> Thanks,
> Stefano
Regards,
Florian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block/rbd: Add support for ceph namespaces
2019-12-20 14:11 ` Florian Florensa
@ 2019-12-20 14:56 ` Jason Dillaman
2019-12-20 15:09 ` Stefano Garzarella
2019-12-20 17:17 ` Florian Florensa
0 siblings, 2 replies; 9+ messages in thread
From: Jason Dillaman @ 2019-12-20 14:56 UTC (permalink / raw)
To: Florian Florensa
Cc: Kevin Wolf, qemu-devel, Markus Armbruster, Max Reitz, dillaman,
Stefano Garzarella
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 ?
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".
> > With those fixed:
> >
> > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >
> > Thanks,
> > Stefano
>
> Regards,
> Florian
--
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block/rbd: Add support for ceph namespaces
2019-12-20 14:56 ` Jason Dillaman
@ 2019-12-20 15:09 ` Stefano Garzarella
2019-12-20 17:17 ` Florian Florensa
1 sibling, 0 replies; 9+ messages in thread
From: Stefano Garzarella @ 2019-12-20 15:09 UTC (permalink / raw)
To: Florian Florensa
Cc: Kevin Wolf, Max Reitz, dillaman, qemu-devel, Markus Armbruster
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] block/rbd: Add support for ceph namespaces
2019-12-20 14:56 ` Jason Dillaman
2019-12-20 15:09 ` Stefano Garzarella
@ 2019-12-20 17:17 ` Florian Florensa
2019-12-20 21:05 ` Eric Blake
1 sibling, 1 reply; 9+ messages in thread
From: Florian Florensa @ 2019-12-20 17:17 UTC (permalink / raw)
To: dillaman
Cc: Kevin Wolf, Markus Armbruster, Max Reitz, qemu-devel, Stefano Garzarella
[-- Attachment #1: Type: text/plain, Size: 2478 bytes --]
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 ?
>
> FWIW, I took this as just a comment to add some documentation that the
> field is only valid starting w/ qemu v5.
>
Works for me, will add this in v2.
> > 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.
Will see to do this, I originally wanted to do this at runtime so a Qemu
built against an older librbd would work if the library was updated.
Else some dlopen + dlsym trickery would work by checking for the
existence of rbd_list2 in librbd.so, but I guess this might be a bad
idea, as it would add code that would be useless in sometime
>
> > > 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".
>
Yes I wanted to avoid namespace because it looks like the qapi generated
code changes the name to something like q_namespace, will use
pool_namespace in the v2.
> > > With those fixed:
> > >
> > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > >
> > > Thanks,
> > > Stefano
> >
> > Regards,
> > Florian
>
> --
> Jason
>
Regards,
Florian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block/rbd: Add support for ceph namespaces
2019-12-19 13:34 [PATCH] block/rbd: Add support for ceph namespaces Florian Florensa
2019-12-19 14:15 ` Jason Dillaman
2019-12-19 14:51 ` Stefano Garzarella
@ 2019-12-20 21:02 ` Eric Blake
2 siblings, 0 replies; 9+ messages in thread
From: Eric Blake @ 2019-12-20 21:02 UTC (permalink / raw)
To: Florian Florensa, qemu-devel; +Cc: kwolf, dillaman, armbru, mreitz
On 12/19/19 7:34 AM, Florian Florensa wrote:
> Starting from ceph Nautilus, RBD has support for namespaces, allowing
> for finer grain ACLs on images inside a pool, and tenant isolation.
>
> In the rbd cli tool documentation, the new image-spec and snap-spec are :
> - [pool-name/[namespace-name/]]image-name
> - [pool-name/[namespace-name/]]image-name@snap-name
>
> When using an non namespace's enabled qemu, it complains about not
> finding the image called namespace-name/image-name, thus we only need to
> parse the image once again to find if there is a '/' in its name, and if
> there is, use what is before it as the name of the namespace to later
> pass it to rados_ioctx_set_namespace.
> rados_ioctx_set_namespace if called with en empty string or a null
> pointer as the namespace parameters pretty much does nothing, as it then
> defaults to the default namespace.
>
> The namespace is extracted inside qemu_rbd_parse_filename, stored in the
> qdict, and used in qemu_rbd_connect to make it work with both qemu-img,
> and qemu itself.
>
> Signed-off-by: Florian Florensa <fflorensa@online.net>
> ---
> +++ b/qapi/block-core.json
> @@ -3657,6 +3657,8 @@
> #
> # @pool: Ceph pool name.
> #
> +# @nspace: Rados namespace name in the Ceph pool.
> +#
Needs a '(since 5.0)' tag.
> # @image: Image name in the Ceph pool.
> #
> # @conf: path to Ceph configuration file. Values
> @@ -3683,6 +3685,7 @@
> ##
> { 'struct': 'BlockdevOptionsRbd',
> 'data': { 'pool': 'str',
> + 'nspace': 'str',
This makes a new argument mandatory, which breaks expectations of older
clients that failed to provide it. You probably want to make it
'*nspace', and have a sane default when the argument is not present.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] block/rbd: Add support for ceph namespaces
2019-12-20 17:17 ` Florian Florensa
@ 2019-12-20 21:05 ` Eric Blake
0 siblings, 0 replies; 9+ messages in thread
From: Eric Blake @ 2019-12-20 21:05 UTC (permalink / raw)
To: Florian Florensa, dillaman
Cc: Kevin Wolf, qemu-devel, Stefano Garzarella, Markus Armbruster, Max Reitz
On 12/20/19 11:17 AM, Florian Florensa wrote:
>>>> 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".
>>
> Yes I wanted to avoid namespace because it looks like the qapi generated
> code changes the name to something like q_namespace, will use
> pool_namespace in the v2.
The whole point of the mangling of 'q_namespace' in the C code is so
that you can have a SANE name in the qapi, without tripping up
compilation in a C++ compiler where 'namespace' is a reserved word
(since we do have parts of qemu compiled by c++). I'd go with just
'namespace', rather than 'pool-namespace' (note that if you DO go with a
longer name, we prefer - over _ in qapi names).
>>>> With those fixed:
>>>>
>>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
But see my other comment upthread about making the new parameter
optional, to avoid breaks with older qapi clients.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-12-20 21:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-19 13:34 [PATCH] block/rbd: Add support for ceph namespaces Florian Florensa
2019-12-19 14:15 ` Jason Dillaman
2019-12-19 14:51 ` Stefano Garzarella
2019-12-20 14:11 ` Florian Florensa
2019-12-20 14:56 ` Jason Dillaman
2019-12-20 15:09 ` Stefano Garzarella
2019-12-20 17:17 ` Florian Florensa
2019-12-20 21:05 ` Eric Blake
2019-12-20 21:02 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.