| Message ID | 12a16b28300f871052a49897ec7875a082fc63e3.1488220970.git.jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 27, 2017 at 01:58:48PM -0500, Jeff Cody wrote: > This adds support for two additional options that may be specified > by QAPI in blockdev-add: > > mon_host: servername and port > auth_supported: either 'cephx' or 'none' > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/rbd.c | 39 +++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 8 ++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/block/rbd.c b/block/rbd.c > index e04a5e1..51e971e 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = { > .name = "keyvalue-pairs", > .type = QEMU_OPT_STRING, > }, > + { > + .name = "server.host", > + .type = QEMU_OPT_STRING, > + }, > + { > + .name = "server.port", > + .type = QEMU_OPT_STRING, > + }, > + { > + .name = "auth_supported", > + .type = QEMU_OPT_STRING, > + }, > { /* end of list */ } > }, > }; > @@ -559,6 +571,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > { > BDRVRBDState *s = bs->opaque; > const char *pool, *snap, *conf, *clientname, *name, *keypairs; > + const char *host, *port, *auth_supported; > const char *secretid; > QemuOpts *opts; > Error *local_err = NULL; > @@ -580,6 +593,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > clientname = qemu_opt_get(opts, "user"); > name = qemu_opt_get(opts, "image"); > keypairs = qemu_opt_get(opts, "keyvalue-pairs"); > + host = qemu_opt_get(opts, "server.host"); > + port = qemu_opt_get(opts, "server.port"); > + auth_supported = qemu_opt_get(opts, "auth_supported"); > > r = rados_create(&s->cluster, clientname); > if (r < 0) { > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > goto failed_shutdown; > } > > + /* if mon_host was specified */ > + if (host) { > + const char *hostname = host; > + char *mon_host = NULL; > + > + if (port) { > + mon_host = g_strdup_printf("%s:%s", host, port); > + hostname = mon_host; > + } > + r = rados_conf_set(s->cluster, "mon_host", hostname); > + g_free(mon_host); > + if (r < 0) { > + goto failed_shutdown; > + } > + } > + > + if (auth_supported) { > + r = rados_conf_set(s->cluster, "auth_supported", auth_supported); > + if (r < 0) { > + goto failed_shutdown; > + } > + } > + > if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) { > r = -EIO; > goto failed_shutdown; > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 5b311ff..376512c 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2680,6 +2680,12 @@ > # > # @user: #optional Ceph id name. > # > +# @server: #optional Monitor host address and port. This maps > +# to the "mon_host" Ceph option. > +# > +# @auth_supported: #optional Authentication supported. > +# Either "cephx" or"none". > +# > # @password-secret: #optional The ID of a QCryptoSecret object providing > # the password for the login. > # > @@ -2691,6 +2697,8 @@ > '*conf': 'str', > '*snapshot': 'str', > '*user': 'str', > + '*server': 'InetSocketAddress', This needs to be an array > + '*auth_supported': 'str', IIUC, you're allowed to list multiple auth options too > '*password-secret': 'str' } } > Regards, Daniel
On 02/27/2017 12:58 PM, Jeff Cody wrote: > This adds support for two additional options that may be specified > by QAPI in blockdev-add: > > mon_host: servername and port > auth_supported: either 'cephx' or 'none' Please spell new options with '-' > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/rbd.c | 39 +++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 8 ++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/block/rbd.c b/block/rbd.c > index e04a5e1..51e971e 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = { > .name = "keyvalue-pairs", > .type = QEMU_OPT_STRING, > }, > + { > + .name = "server.host", > + .type = QEMU_OPT_STRING, > + }, > + { > + .name = "server.port", > + .type = QEMU_OPT_STRING, > + }, See Dan's comment about supporting more than one server via an array in QAPI. > + { > + .name = "auth_supported", Should be auth-supported in QAPI. > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > goto failed_shutdown; > } > > + /* if mon_host was specified */ > + if (host) { > + const char *hostname = host; > + char *mon_host = NULL; > + > + if (port) { > + mon_host = g_strdup_printf("%s:%s", host, port); Does Ceph care about IPv6 (in which case you may need [host]:port when host itself includes ':')? > + hostname = mon_host; > + } > + r = rados_conf_set(s->cluster, "mon_host", hostname); > + g_free(mon_host); > + if (r < 0) { > + goto failed_shutdown; > + } > + } > + > + if (auth_supported) { > + r = rados_conf_set(s->cluster, "auth_supported", auth_supported); Translating QAPI auth-supported to rados auth_supported is fine. > +++ b/qapi/block-core.json > @@ -2680,6 +2680,12 @@ > # > # @user: #optional Ceph id name. > # > +# @server: #optional Monitor host address and port. This maps > +# to the "mon_host" Ceph option. > +# > +# @auth_supported: #optional Authentication supported. > +# Either "cephx" or"none". Missing a space. If you're going to support only a finite set of strings, this should be a QAPI enum type, not 'str'. > +# > # @password-secret: #optional The ID of a QCryptoSecret object providing > # the password for the login. > # > @@ -2691,6 +2697,8 @@ > '*conf': 'str', > '*snapshot': 'str', > '*user': 'str', > + '*server': 'InetSocketAddress', > + '*auth_supported': 'str', > '*password-secret': 'str' } } > > ## > Looks like we'll need a v3 to tweak this one.
On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote: > On 02/27/2017 12:58 PM, Jeff Cody wrote: > > This adds support for two additional options that may be specified > > by QAPI in blockdev-add: > > > > mon_host: servername and port > > auth_supported: either 'cephx' or 'none' > > Please spell new options with '-' > OK, thanks. > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > block/rbd.c | 39 +++++++++++++++++++++++++++++++++++++++ > > qapi/block-core.json | 8 ++++++++ > > 2 files changed, 47 insertions(+) > > > > diff --git a/block/rbd.c b/block/rbd.c > > index e04a5e1..51e971e 100644 > > --- a/block/rbd.c > > +++ b/block/rbd.c > > @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = { > > .name = "keyvalue-pairs", > > .type = QEMU_OPT_STRING, > > }, > > + { > > + .name = "server.host", > > + .type = QEMU_OPT_STRING, > > + }, > > + { > > + .name = "server.port", > > + .type = QEMU_OPT_STRING, > > + }, > > See Dan's comment about supporting more than one server via an array in > QAPI. > > > + { > > + .name = "auth_supported", > > Should be auth-supported in QAPI. > > OK > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > > goto failed_shutdown; > > } > > > > + /* if mon_host was specified */ > > + if (host) { > > + const char *hostname = host; > > + char *mon_host = NULL; > > + > > + if (port) { > > + mon_host = g_strdup_printf("%s:%s", host, port); > > Does Ceph care about IPv6 (in which case you may need [host]:port when > host itself includes ':')? > Good question - it looks like it can be enabled in the conf file at least, from this: http://docs.ceph.com/docs/master/install/manual-deployment/ > > + hostname = mon_host; > > + } > > + r = rados_conf_set(s->cluster, "mon_host", hostname); > > + g_free(mon_host); > > + if (r < 0) { > > + goto failed_shutdown; > > + } > > + } > > + > > + if (auth_supported) { > > + r = rados_conf_set(s->cluster, "auth_supported", auth_supported); > > Translating QAPI auth-supported to rados auth_supported is fine. > > > > +++ b/qapi/block-core.json > > @@ -2680,6 +2680,12 @@ > > # > > # @user: #optional Ceph id name. > > # > > +# @server: #optional Monitor host address and port. This maps > > +# to the "mon_host" Ceph option. > > +# > > +# @auth_supported: #optional Authentication supported. > > +# Either "cephx" or"none". > > Missing a space. > > If you're going to support only a finite set of strings, this should be > a QAPI enum type, not 'str'. > OK, I'll make it an enum (and fix the space, of course). > > +# > > # @password-secret: #optional The ID of a QCryptoSecret object providing > > # the password for the login. > > # > > @@ -2691,6 +2697,8 @@ > > '*conf': 'str', > > '*snapshot': 'str', > > '*user': 'str', > > + '*server': 'InetSocketAddress', > > + '*auth_supported': 'str', > > '*password-secret': 'str' } } > > > > ## > > > > Looks like we'll need a v3 to tweak this one. > Yep - working on that now. Thanks! -Jeff
On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote: > On 02/27/2017 12:58 PM, Jeff Cody wrote: > > This adds support for two additional options that may be specified > > by QAPI in blockdev-add: > > > > mon_host: servername and port > > auth_supported: either 'cephx' or 'none' > > Please spell new options with '-' > > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > block/rbd.c | 39 +++++++++++++++++++++++++++++++++++++++ > > qapi/block-core.json | 8 ++++++++ > > 2 files changed, 47 insertions(+) > > > > diff --git a/block/rbd.c b/block/rbd.c > > index e04a5e1..51e971e 100644 > > --- a/block/rbd.c > > +++ b/block/rbd.c > > @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = { > > .name = "keyvalue-pairs", > > .type = QEMU_OPT_STRING, > > }, > > + { > > + .name = "server.host", > > + .type = QEMU_OPT_STRING, > > + }, > > + { > > + .name = "server.port", > > + .type = QEMU_OPT_STRING, > > + }, > > See Dan's comment about supporting more than one server via an array in > QAPI. > > > + { > > + .name = "auth_supported", > > Should be auth-supported in QAPI. > > > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > > goto failed_shutdown; > > } > > > > + /* if mon_host was specified */ > > + if (host) { > > + const char *hostname = host; > > + char *mon_host = NULL; > > + > > + if (port) { > > + mon_host = g_strdup_printf("%s:%s", host, port); > > Does Ceph care about IPv6 (in which case you may need [host]:port when > host itself includes ':')? > Some quick sanity testing seems to show that it does not need [] for ipv6 addresses, which is nice. > > + hostname = mon_host; > > + } > > + r = rados_conf_set(s->cluster, "mon_host", hostname); > > + g_free(mon_host); > > + if (r < 0) { > > + goto failed_shutdown; > > + } > > + } > > + > > + if (auth_supported) { > > + r = rados_conf_set(s->cluster, "auth_supported", auth_supported); > > Translating QAPI auth-supported to rados auth_supported is fine. > > > > +++ b/qapi/block-core.json > > @@ -2680,6 +2680,12 @@ > > # > > # @user: #optional Ceph id name. > > # > > +# @server: #optional Monitor host address and port. This maps > > +# to the "mon_host" Ceph option. > > +# > > +# @auth_supported: #optional Authentication supported. > > +# Either "cephx" or"none". > > Missing a space. > > If you're going to support only a finite set of strings, this should be > a QAPI enum type, not 'str'. > > > +# > > # @password-secret: #optional The ID of a QCryptoSecret object providing > > # the password for the login. > > # > > @@ -2691,6 +2697,8 @@ > > '*conf': 'str', > > '*snapshot': 'str', > > '*user': 'str', > > + '*server': 'InetSocketAddress', > > + '*auth_supported': 'str', > > '*password-secret': 'str' } } > > > > ## > > > > Looks like we'll need a v3 to tweak this one. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On Mon, Feb 27, 2017 at 10:57:44PM -0500, Jeff Cody wrote: > On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote: > > On 02/27/2017 12:58 PM, Jeff Cody wrote: > > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > > > goto failed_shutdown; > > > } > > > > > > + /* if mon_host was specified */ > > > + if (host) { > > > + const char *hostname = host; > > > + char *mon_host = NULL; > > > + > > > + if (port) { > > > + mon_host = g_strdup_printf("%s:%s", host, port); > > > > Does Ceph care about IPv6 (in which case you may need [host]:port when > > host itself includes ':')? > > > > Some quick sanity testing seems to show that it does not need [] for ipv6 > addresses, which is nice. Hmm, that is very odd to me, as that means parsing is ambiguous. The ceph 'mon_host' option allows the port to be omitted, so given a string 2001:242:24:23 there's no way of knowing whether it is a IPv6 addr 2001:242:24:23, with no port, or an addr 2001:242:24 with port of 23 Regards, Daniel
On Tue, Feb 28, 2017 at 10:16:51AM +0000, Daniel P. Berrange wrote: > On Mon, Feb 27, 2017 at 10:57:44PM -0500, Jeff Cody wrote: > > On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote: > > > On 02/27/2017 12:58 PM, Jeff Cody wrote: > > > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > > > > goto failed_shutdown; > > > > } > > > > > > > > + /* if mon_host was specified */ > > > > + if (host) { > > > > + const char *hostname = host; > > > > + char *mon_host = NULL; > > > > + > > > > + if (port) { > > > > + mon_host = g_strdup_printf("%s:%s", host, port); > > > > > > Does Ceph care about IPv6 (in which case you may need [host]:port when > > > host itself includes ':')? > > > > > > > Some quick sanity testing seems to show that it does not need [] for ipv6 > > addresses, which is nice. > > Hmm, that is very odd to me, as that means parsing is ambiguous. The > ceph 'mon_host' option allows the port to be omitted, so given a > string 2001:242:24:23 there's no way of knowing whether it is > a IPv6 addr 2001:242:24:23, with no port, or an addr 2001:242:24 with > port of 23 Looking at the source code to ceph it appears that if you omit the use of [], then it will treat the entire string as being the address without port. It does look to support use of [], so we should use that IIUC. https://blog.widodh.nl/2014/11/ceph-with-a-cluster-and-public-network-on-ipv6/ See also entity_addr_t::parse in $CEPH/src/msg/msg_types.cc which is what I think parses the mon addr. NB, I've not tested this yet, so if you have a live ceph cluster with ipv6, it'd be good to verify that using [] is correct. Regards, Daniel
On Tue, Feb 28, 2017 at 10:28:49AM +0000, Daniel P. Berrange wrote: > On Tue, Feb 28, 2017 at 10:16:51AM +0000, Daniel P. Berrange wrote: > > On Mon, Feb 27, 2017 at 10:57:44PM -0500, Jeff Cody wrote: > > > On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote: > > > > On 02/27/2017 12:58 PM, Jeff Cody wrote: > > > > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > > > > > goto failed_shutdown; > > > > > } > > > > > > > > > > + /* if mon_host was specified */ > > > > > + if (host) { > > > > > + const char *hostname = host; > > > > > + char *mon_host = NULL; > > > > > + > > > > > + if (port) { > > > > > + mon_host = g_strdup_printf("%s:%s", host, port); > > > > > > > > Does Ceph care about IPv6 (in which case you may need [host]:port when > > > > host itself includes ':')? > > > > > > > > > > Some quick sanity testing seems to show that it does not need [] for ipv6 > > > addresses, which is nice. > > > > Hmm, that is very odd to me, as that means parsing is ambiguous. The > > ceph 'mon_host' option allows the port to be omitted, so given a > > string 2001:242:24:23 there's no way of knowing whether it is > > a IPv6 addr 2001:242:24:23, with no port, or an addr 2001:242:24 with > > port of 23 > > Looking at the source code to ceph it appears that if you omit the > use of [], then it will treat the entire string as being the address > without port. It does look to support use of [], so we should use > that IIUC. For the sake of clarity, when you say we should use [] for ipv6, you mean QEMU (rather than doing it in libvirt)? > > https://blog.widodh.nl/2014/11/ceph-with-a-cluster-and-public-network-on-ipv6/ > > See also entity_addr_t::parse in $CEPH/src/msg/msg_types.cc which is > what I think parses the mon addr. NB, I've not tested this yet, so > if you have a live ceph cluster with ipv6, it'd be good to verify that > using [] is correct. > Do you think this needs to be in place before either A) we pull the series for softfreeze, or B) 2.10? I.e., is this something we can fix up later? It is OK if not, but I have a flight leaving soon and can't work on the series anymore -- so depending on this and how v3 review goes, we may just need to slip the series to 2.10.
On Tue, Feb 28, 2017 at 07:34:52AM -0500, Jeff Cody wrote: > On Tue, Feb 28, 2017 at 10:28:49AM +0000, Daniel P. Berrange wrote: > > On Tue, Feb 28, 2017 at 10:16:51AM +0000, Daniel P. Berrange wrote: > > > On Mon, Feb 27, 2017 at 10:57:44PM -0500, Jeff Cody wrote: > > > > On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote: > > > > > On 02/27/2017 12:58 PM, Jeff Cody wrote: > > > > > > @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, > > > > > > goto failed_shutdown; > > > > > > } > > > > > > > > > > > > + /* if mon_host was specified */ > > > > > > + if (host) { > > > > > > + const char *hostname = host; > > > > > > + char *mon_host = NULL; > > > > > > + > > > > > > + if (port) { > > > > > > + mon_host = g_strdup_printf("%s:%s", host, port); > > > > > > > > > > Does Ceph care about IPv6 (in which case you may need [host]:port when > > > > > host itself includes ':')? > > > > > > > > > > > > > Some quick sanity testing seems to show that it does not need [] for ipv6 > > > > addresses, which is nice. > > > > > > Hmm, that is very odd to me, as that means parsing is ambiguous. The > > > ceph 'mon_host' option allows the port to be omitted, so given a > > > string 2001:242:24:23 there's no way of knowing whether it is > > > a IPv6 addr 2001:242:24:23, with no port, or an addr 2001:242:24 with > > > port of 23 > > > > Looking at the source code to ceph it appears that if you omit the > > use of [], then it will treat the entire string as being the address > > without port. It does look to support use of [], so we should use > > that IIUC. > > For the sake of clarity, when you say we should use [] for ipv6, you mean > QEMU (rather than doing it in libvirt)? Yes, in QAPI JSON, it should be the bare IPv6 address in the server host field. When QEMU conbines the host + port to form the mon_host string, it must add [] if it sees the host from QAPI is a raw IPv6 address > > https://blog.widodh.nl/2014/11/ceph-with-a-cluster-and-public-network-on-ipv6/ > > > > See also entity_addr_t::parse in $CEPH/src/msg/msg_types.cc which is > > what I think parses the mon addr. NB, I've not tested this yet, so > > if you have a live ceph cluster with ipv6, it'd be good to verify that > > using [] is correct. > > > > Do you think this needs to be in place before either A) we pull the series > for softfreeze, or B) 2.10? I.e., is this something we can fix up later? > It is OK if not, but I have a flight leaving soon and can't work on the > series anymore -- so depending on this and how v3 review goes, we may just > need to slip the series to 2.10. The fix doesn't affect QAPI protocol spec, so as long as it is fixed before 2.9 release it'd be ok. Regards, Daniel
diff --git a/block/rbd.c b/block/rbd.c index e04a5e1..51e971e 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = { .name = "keyvalue-pairs", .type = QEMU_OPT_STRING, }, + { + .name = "server.host", + .type = QEMU_OPT_STRING, + }, + { + .name = "server.port", + .type = QEMU_OPT_STRING, + }, + { + .name = "auth_supported", + .type = QEMU_OPT_STRING, + }, { /* end of list */ } }, }; @@ -559,6 +571,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, { BDRVRBDState *s = bs->opaque; const char *pool, *snap, *conf, *clientname, *name, *keypairs; + const char *host, *port, *auth_supported; const char *secretid; QemuOpts *opts; Error *local_err = NULL; @@ -580,6 +593,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, clientname = qemu_opt_get(opts, "user"); name = qemu_opt_get(opts, "image"); keypairs = qemu_opt_get(opts, "keyvalue-pairs"); + host = qemu_opt_get(opts, "server.host"); + port = qemu_opt_get(opts, "server.port"); + auth_supported = qemu_opt_get(opts, "auth_supported"); r = rados_create(&s->cluster, clientname); if (r < 0) { @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_shutdown; } + /* if mon_host was specified */ + if (host) { + const char *hostname = host; + char *mon_host = NULL; + + if (port) { + mon_host = g_strdup_printf("%s:%s", host, port); + hostname = mon_host; + } + r = rados_conf_set(s->cluster, "mon_host", hostname); + g_free(mon_host); + if (r < 0) { + goto failed_shutdown; + } + } + + if (auth_supported) { + r = rados_conf_set(s->cluster, "auth_supported", auth_supported); + if (r < 0) { + goto failed_shutdown; + } + } + if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) { r = -EIO; goto failed_shutdown; diff --git a/qapi/block-core.json b/qapi/block-core.json index 5b311ff..376512c 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2680,6 +2680,12 @@ # # @user: #optional Ceph id name. # +# @server: #optional Monitor host address and port. This maps +# to the "mon_host" Ceph option. +# +# @auth_supported: #optional Authentication supported. +# Either "cephx" or"none". +# # @password-secret: #optional The ID of a QCryptoSecret object providing # the password for the login. # @@ -2691,6 +2697,8 @@ '*conf': 'str', '*snapshot': 'str', '*user': 'str', + '*server': 'InetSocketAddress', + '*auth_supported': 'str', '*password-secret': 'str' } } ##
This adds support for two additional options that may be specified by QAPI in blockdev-add: mon_host: servername and port auth_supported: either 'cephx' or 'none' Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/rbd.c | 39 +++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 8 ++++++++ 2 files changed, 47 insertions(+)