Re: [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers
On Tuesday, November 10, 2015 9:37:20 PM, Eric Blake wrote: > > On 11/10/2015 02:09 AM, Prasanna Kumar Kalever wrote: > > This patch adds a way to specify multiple volfile servers to the gluster > > block backend of QEMU with tcp|rdma transport types and their port numbers. > > > > [...] [...] > > Overall, I think we are probably on the right track for the QMP > interface; but since blockdev-add is NOT stable yet for 2.5, it won't > hurt to wait to get this in until 2.6, to make sure we have plenty of > time; and it would also be nice to make sure we get nbd, nfs, rbd, > sheepdog all supported in the same release; possibly by sharing common > types instead of introducing GlusterServer as a one-off type. We are hoping this to go in 2.5 which is really important for gluster hyper-convergence release (next Feb). Is there any possibility of getting exception for this patch ? Thanks, -Prasanna > > -- > Eric Blake eblake redhat com+1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
Re: [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers
On Tuesday, November 10, 2015 10:54:25 PM, Jeff Cody wrote:
>
> On Tue, Nov 10, 2015 at 02:39:16PM +0530, Prasanna Kumar Kalever wrote:
> > This patch adds a way to specify multiple volfile servers to the gluster
> > block backend of QEMU with tcp|rdma transport types and their port numbers.
> >
> > Problem:
> >
> > Currently VM Image on gluster volume is specified like this:
> >
> > file=gluster[+tcp]://host[:port]/testvol/a.img
> >
> > Assuming we have three hosts in trusted pool with replica 3 volume
> > in action and unfortunately host (mentioned in the command above) went down
> > for some reason, since the volume is replica 3 we now have other 2 hosts
> > active from which we can boot the VM.
> >
> > But currently there is no mechanism to pass the other 2 gluster host
> > addresses to qemu.
> >
> > Solution:
> >
> > New way of specifying VM Image on gluster volume with volfile servers:
> > (We still support old syntax to maintain backward compatibility)
> >
> > Basic command line syntax looks like:
> >
> > Pattern I:
> > -drive driver=gluster,
> > volume=testvol,path=/path/a.raw,
> > servers.0.host=1.2.3.4,
> >[servers.0.port=24007,]
> >[servers.0.transport=tcp,]
> > servers.1.host=5.6.7.8,
> >[servers.1.port=24008,]
> >[servers.1.transport=rdma,] ...
> >
> > Pattern II:
> > 'json:{"driver":"qcow2","file":{"driver":"gluster",
> >"volume":"testvol","path":"/path/a.qcow2",
> >"servers":[{tuple0},{tuple1}, ...{tupleN}]}}'
> >
> >driver => 'gluster' (protocol name)
> >volume => name of gluster volume where our VM image resides
> >path=> absolute path of image in gluster volume
> >
> > {tuple} => {"host":"1.2.3.4"[,"port":"24007","transport":"tcp"]}
> >
> >host=> host address (hostname/ipv4/ipv6 addresses)
> >port=> port number on which glusterd is listening. (default
> >24007)
> >transport => transport type used to connect to gluster management
> >daemon,
> >it can be tcp|rdma (default 'tcp')
> >
> > Examples:
> > 1.
> > -drive driver=qcow2,file.driver=gluster,
> > file.volume=testvol,file.path=/path/a.qcow2,
> > file.servers.0.host=1.2.3.4,
> > file.servers.0.port=24007,
> > file.servers.0.transport=tcp,
> > file.servers.1.host=5.6.7.8,
> > file.servers.1.port=24008,
> > file.servers.1.transport=rdma
> > 2.
> > 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
> > "path":"/path/a.qcow2","servers":
> > [{"host":"1.2.3.4","port":"24007","transport":"tcp"},
> > {"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'
> >
> > This patch gives a mechanism to provide all the server addresses, which are
> > in
> > replica set, so in case host1 is down VM can still boot from any of the
> > active hosts.
> >
> > This is equivalent to the backup-volfile-servers option supported by
> > mount.glusterfs (FUSE way of mounting gluster volume)
> >
> > Credits: Sincere thanks to Kevin Wolf and
> > "Deepak C Shetty" for inputs and all their support
> >
> > Signed-off-by: Prasanna Kumar Kalever
>
>
> Previous versions of this commit mentioned that the new functionality
> is dependent on a recent fix in libgfapi. This commit message is
> missing that line; does its absence mean that the new functionality is
> not dependent on any particular libgfapi version?
>
> What happens if the new functionality is tried on the last stable
> libgfapi release?
Sorry for not removing this since long, actually the libgfapi fix is for
defaults values
i.e. When glfs_set_volfile_server is invocated multiple times, only on the first
invocation gfapi code replace port 0 with 24007 and transport NULL with "tcp".
Any have to remove this dependency, I have put up code that will take care of
defaults.
Thanks,
-prasanna
Hence, replacing the parameters at the entry function is the right way.
>
> Thanks!
> Jeff
>
>
Re: [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers
On Thu, Nov 12, 2015 at 11:23:35AM -0500, Jeff Cody wrote: > On Thu, Nov 12, 2015 at 05:04:02AM -0500, Prasanna Kumar Kalever wrote: > > On Tuesday, November 10, 2015 9:37:20 PM, Eric Blake wrote: > > > > > > On 11/10/2015 02:09 AM, Prasanna Kumar Kalever wrote: > > > > This patch adds a way to specify multiple volfile servers to the gluster > > > > block backend of QEMU with tcp|rdma transport types and their port > > > > numbers. > > > > > > > > > > [...] > > > > [...] > > > > > > > > Overall, I think we are probably on the right track for the QMP > > > interface; but since blockdev-add is NOT stable yet for 2.5, it won't > > > hurt to wait to get this in until 2.6, to make sure we have plenty of > > > time; and it would also be nice to make sure we get nbd, nfs, rbd, > > > sheepdog all supported in the same release; possibly by sharing common > > > types instead of introducing GlusterServer as a one-off type. > > > > We are hoping this to go in 2.5 which is really important for gluster > > hyper-convergence release (next Feb). > > > > Is there any possibility of getting exception for this patch ? > > > > Thanks, > > -Prasanna > > > > Today is the hard freeze for 2.5 for new features. As the v14 patches > hit the list today, I will review them, and if they look good I can > pull patches into my tree. > > Whether there is an exception is really up to Peter Maydell > (added him to the cc) - if all looks good with the patches, I could > send a pull request out later today with them, but that will be late > in the evening for Peter. > I wanted to give a follow-up - as Eric pointed out in his reviews, the API interface could be made better by using some QAPI features that are not currently in qemu (discriminated unions). So I won't issue a pull request for this for 2.5, and we'll pick it up for 2.6 once the interface issues can be worked out. -Jeff
Re: [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers
On Thu, Nov 12, 2015 at 05:04:02AM -0500, Prasanna Kumar Kalever wrote: > On Tuesday, November 10, 2015 9:37:20 PM, Eric Blake wrote: > > > > On 11/10/2015 02:09 AM, Prasanna Kumar Kalever wrote: > > > This patch adds a way to specify multiple volfile servers to the gluster > > > block backend of QEMU with tcp|rdma transport types and their port > > > numbers. > > > > > > > [...] > > [...] > > > > > Overall, I think we are probably on the right track for the QMP > > interface; but since blockdev-add is NOT stable yet for 2.5, it won't > > hurt to wait to get this in until 2.6, to make sure we have plenty of > > time; and it would also be nice to make sure we get nbd, nfs, rbd, > > sheepdog all supported in the same release; possibly by sharing common > > types instead of introducing GlusterServer as a one-off type. > > We are hoping this to go in 2.5 which is really important for gluster > hyper-convergence release (next Feb). > > Is there any possibility of getting exception for this patch ? > > Thanks, > -Prasanna > Today is the hard freeze for 2.5 for new features. As the v14 patches hit the list today, I will review them, and if they look good I can pull patches into my tree. Whether there is an exception is really up to Peter Maydell (added him to the cc) - if all looks good with the patches, I could send a pull request out later today with them, but that will be late in the evening for Peter. -Jeff
Re: [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers
On 11/10/2015 02:09 AM, Prasanna Kumar Kalever wrote:
> This patch adds a way to specify multiple volfile servers to the gluster
> block backend of QEMU with tcp|rdma transport types and their port numbers.
>
[...]
> 2.
> 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
> "path":"/path/a.qcow2","servers":
> [{"host":"1.2.3.4","port":"24007","transport":"tcp"},
> {"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'
>
> This patch gives a mechanism to provide all the server addresses, which are in
> replica set, so in case host1 is down VM can still boot from any of the
> active hosts.
>
> This is equivalent to the backup-volfile-servers option supported by
> mount.glusterfs (FUSE way of mounting gluster volume)
>
> Credits: Sincere thanks to Kevin Wolf and
> "Deepak C Shetty" for inputs and all their support
>
> Signed-off-by: Prasanna Kumar Kalever
> ---
> v10:
> fix mem-leak as per Peter Krempa review comments
>
> v11:
> using qapi-types* defined structures as per "Eric Blake"
> review comments.
>
> v12:
> fix crash caused in qapi_free_BlockdevOptionsGluster
>
> v13:
> address comments from "Jeff Cody"
I had some other comments against v10 that I don't see addressed yet:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06377.html
> ---
> block/gluster.c | 468
> ---
> qapi/block-core.json | 60 ++-
> 2 files changed, 461 insertions(+), 67 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index ededda2..8939072 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -11,6 +11,19 @@
> #include "block/block_int.h"
> #include "qemu/uri.h"
>
> +#define GLUSTER_OPT_FILENAME"filename"
> +#define GLUSTER_OPT_VOLUME "volume"
> +#define GLUSTER_OPT_PATH"path"
> +#define GLUSTER_OPT_HOST"host"
> +#define GLUSTER_OPT_PORT"port"
> +#define GLUSTER_OPT_TRANSPORT "transport"
> +#define GLUSTER_OPT_SERVERS_PATTERN "servers."
> +
> +#define GLUSTER_DEFAULT_PORT24007
> +
> +#define MAX_SERVERS "1"
Why is this a string rather than an integer?
> +
> +
> typedef struct GlusterAIOCB {
> int64_t size;
> int ret;
> @@ -29,15 +42,6 @@ typedef struct BDRVGlusterReopenState {
> struct glfs_fd *fd;
> } BDRVGlusterReopenState;
>
> -typedef struct GlusterConf {
> -char *host;
> -int port;
> -char *volume;
> -char *path;
> -char *transport;
> -} GlusterConf;
> -
This patch feels pretty big. It may be smarter to break it into two
pieces - one that adds GlusterConf to qapi/block-core.json and replaces
existing uses of this definition to the qapi type but with no changes in
semantics; and the other that then extends things to add support for
multiple servers (so that we aren't trying to do too much in one patch).
> @@ -143,8 +176,11 @@ static int parse_volume_options(GlusterConf *gconf, char
> *path)
> * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
> * file=gluster+rdma://1.2.3.4:24007/testvol/a.img
> */
> -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> +static int qemu_gluster_parseuri(BlockdevOptionsGluster **pgconf,
> + const char *filename)
> {
> +BlockdevOptionsGluster *gconf;
> +GlusterServer *gsconf;
> URI *uri;
> QueryParams *qp = NULL;
> bool is_unix = false;
> @@ -155,20 +191,24 @@ static int qemu_gluster_parseuri(GlusterConf *gconf,
> const char *filename)
> return -EINVAL;
> }
>
> +gconf = g_new0(BlockdevOptionsGluster, 1);
> +gsconf = g_new0(GlusterServer, 1);
gconf and gsconf are both allocated here...
> +
> /* transport */
> if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> -gconf->transport = g_strdup("tcp");
> +gsconf->transport = GLUSTER_TRANSPORT_TCP;
> } else if (!strcmp(uri->scheme, "gluster+tcp")) {
> -gconf->transport = g_strdup("tcp");
> +gsconf->transport = GLUSTER_TRANSPORT_TCP;
> } else if (!strcmp(uri->scheme, "gluster+unix")) {
> -gconf->transport = g_strdup("unix");
> +gsconf->transport = GLUSTER_TRANSPORT_UNIX;
> is_unix = true;
> } else if (!strcmp(uri->scheme, "gluster+rdma")) {
> -gconf->transport = g_strdup("rdma");
> +gsconf->transport = GLUSTER_TRANSPORT_RDMA;
> } else {
> ret = -EINVAL;
> goto out;
...but you can error here...
> }
> +gsconf->has_transport = true;
>
> ret = parse_volume_options(gconf, uri->path);
> if (ret < 0) {
> @@ -190,13 +230,27 @@ static int qemu_gluster_parseuri(GlusterConf *gconf,
> const char *filename)
> ret = -EINVAL;
> goto out;
> }
>
Re: [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers
On Tue, Nov 10, 2015 at 09:07:20AM -0700, Eric Blake wrote: > On 11/10/2015 02:09 AM, Prasanna Kumar Kalever wrote: > > This patch adds a way to specify multiple volfile servers to the gluster > > block backend of QEMU with tcp|rdma transport types and their port numbers. > > [...] > > +/* > > +* > > +* Basic command line syntax looks like: > > +* > > You have 110 lines from /* to */. In v10, I already mentioned that this > comment is probably 100 lines too long. You do NOT need to repeat the > syntax, examples, or even more-readable example here; having them in the > commit body was enough. Someone that knows how to read qapi will be > able to deduce what this function is doing if you were to simplify to > just this: > > /* > * Convert the command line into qapi. > */ > Maybe a good compromise could be moving this into docs/gluster.txt (which doesn't exist, but could get its start this way!). That way, the info is there to read / find with grep without needing to look at commit logs. [...] -Jeff
Re: [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers
On Tue, Nov 10, 2015 at 02:39:16PM +0530, Prasanna Kumar Kalever wrote:
> This patch adds a way to specify multiple volfile servers to the gluster
> block backend of QEMU with tcp|rdma transport types and their port numbers.
>
> Problem:
>
> Currently VM Image on gluster volume is specified like this:
>
> file=gluster[+tcp]://host[:port]/testvol/a.img
>
> Assuming we have three hosts in trusted pool with replica 3 volume
> in action and unfortunately host (mentioned in the command above) went down
> for some reason, since the volume is replica 3 we now have other 2 hosts
> active from which we can boot the VM.
>
> But currently there is no mechanism to pass the other 2 gluster host
> addresses to qemu.
>
> Solution:
>
> New way of specifying VM Image on gluster volume with volfile servers:
> (We still support old syntax to maintain backward compatibility)
>
> Basic command line syntax looks like:
>
> Pattern I:
> -drive driver=gluster,
> volume=testvol,path=/path/a.raw,
> servers.0.host=1.2.3.4,
>[servers.0.port=24007,]
>[servers.0.transport=tcp,]
> servers.1.host=5.6.7.8,
>[servers.1.port=24008,]
>[servers.1.transport=rdma,] ...
>
> Pattern II:
> 'json:{"driver":"qcow2","file":{"driver":"gluster",
>"volume":"testvol","path":"/path/a.qcow2",
>"servers":[{tuple0},{tuple1}, ...{tupleN}]}}'
>
>driver => 'gluster' (protocol name)
>volume => name of gluster volume where our VM image resides
>path=> absolute path of image in gluster volume
>
> {tuple} => {"host":"1.2.3.4"[,"port":"24007","transport":"tcp"]}
>
>host=> host address (hostname/ipv4/ipv6 addresses)
>port=> port number on which glusterd is listening. (default 24007)
>transport => transport type used to connect to gluster management daemon,
>it can be tcp|rdma (default 'tcp')
>
> Examples:
> 1.
> -drive driver=qcow2,file.driver=gluster,
> file.volume=testvol,file.path=/path/a.qcow2,
> file.servers.0.host=1.2.3.4,
> file.servers.0.port=24007,
> file.servers.0.transport=tcp,
> file.servers.1.host=5.6.7.8,
> file.servers.1.port=24008,
> file.servers.1.transport=rdma
> 2.
> 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
> "path":"/path/a.qcow2","servers":
> [{"host":"1.2.3.4","port":"24007","transport":"tcp"},
> {"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'
>
> This patch gives a mechanism to provide all the server addresses, which are in
> replica set, so in case host1 is down VM can still boot from any of the
> active hosts.
>
> This is equivalent to the backup-volfile-servers option supported by
> mount.glusterfs (FUSE way of mounting gluster volume)
>
> Credits: Sincere thanks to Kevin Wolf and
> "Deepak C Shetty" for inputs and all their support
>
> Signed-off-by: Prasanna Kumar Kalever
Previous versions of this commit mentioned that the new functionality
is dependent on a recent fix in libgfapi. This commit message is
missing that line; does its absence mean that the new functionality is
not dependent on any particular libgfapi version?
What happens if the new functionality is tried on the last stable
libgfapi release?
Thanks!
Jeff
[Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers
This patch adds a way to specify multiple volfile servers to the gluster
block backend of QEMU with tcp|rdma transport types and their port numbers.
Problem:
Currently VM Image on gluster volume is specified like this:
file=gluster[+tcp]://host[:port]/testvol/a.img
Assuming we have three hosts in trusted pool with replica 3 volume
in action and unfortunately host (mentioned in the command above) went down
for some reason, since the volume is replica 3 we now have other 2 hosts
active from which we can boot the VM.
But currently there is no mechanism to pass the other 2 gluster host
addresses to qemu.
Solution:
New way of specifying VM Image on gluster volume with volfile servers:
(We still support old syntax to maintain backward compatibility)
Basic command line syntax looks like:
Pattern I:
-drive driver=gluster,
volume=testvol,path=/path/a.raw,
servers.0.host=1.2.3.4,
[servers.0.port=24007,]
[servers.0.transport=tcp,]
servers.1.host=5.6.7.8,
[servers.1.port=24008,]
[servers.1.transport=rdma,] ...
Pattern II:
'json:{"driver":"qcow2","file":{"driver":"gluster",
"volume":"testvol","path":"/path/a.qcow2",
"servers":[{tuple0},{tuple1}, ...{tupleN}]}}'
driver => 'gluster' (protocol name)
volume => name of gluster volume where our VM image resides
path=> absolute path of image in gluster volume
{tuple} => {"host":"1.2.3.4"[,"port":"24007","transport":"tcp"]}
host=> host address (hostname/ipv4/ipv6 addresses)
port=> port number on which glusterd is listening. (default 24007)
transport => transport type used to connect to gluster management daemon,
it can be tcp|rdma (default 'tcp')
Examples:
1.
-drive driver=qcow2,file.driver=gluster,
file.volume=testvol,file.path=/path/a.qcow2,
file.servers.0.host=1.2.3.4,
file.servers.0.port=24007,
file.servers.0.transport=tcp,
file.servers.1.host=5.6.7.8,
file.servers.1.port=24008,
file.servers.1.transport=rdma
2.
'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
"path":"/path/a.qcow2","servers":
[{"host":"1.2.3.4","port":"24007","transport":"tcp"},
{"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'
This patch gives a mechanism to provide all the server addresses, which are in
replica set, so in case host1 is down VM can still boot from any of the
active hosts.
This is equivalent to the backup-volfile-servers option supported by
mount.glusterfs (FUSE way of mounting gluster volume)
Credits: Sincere thanks to Kevin Wolf and
"Deepak C Shetty" for inputs and all their support
Signed-off-by: Prasanna Kumar Kalever
---
v1:
multiple host addresses but common port number and transport type
pattern: URI syntax with query (?) delimitor
syntax:
file=gluster[+transport-type]://host1:24007/testvol/a.img\
?servers=host2=host3
v2:
multiple host addresses each have their own port number, but all use
common transport type
pattern: URI syntax with query (?) delimiter
syntax:
file=gluster[+transport-type]://[host[:port]]/testvol/a.img\
[?servers=host1[:port]\
=host2[:port]]
v3:
multiple host addresses each have their own port number and transport type
pattern: changed to json
syntax:
'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
"path":"/path/a.qcow2","servers":
[{"host":"1.2.3.4","port":"24007","transport":"tcp"},
{"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'
v4, v5:
address comments from "Eric Blake"
renamed:
'backup-volfile-servers' -> 'volfile-servers'
v6:
address comments from Peter Krempa
renamed:
'volname'-> 'volume'
'image-path' -> 'path'
'server' -> 'host'
v7:
fix for v6 (initialize num_servers to 1 and other typos)
v8:
split patch set v7 into series of 3 as per Peter Krempa
review comments
v9:
reorder the series of patches addressing "Eric Blake"
review comments
v10:
fix mem-leak as per Peter Krempa review comments
v11:
using qapi-types* defined structures as per "Eric Blake"
review comments.
v12:
fix crash caused in qapi_free_BlockdevOptionsGluster
v13:
address comments from "Jeff Cody"
---
block/gluster.c | 468 ---
qapi/block-core.json | 60 ++-
2 files changed, 461 insertions(+), 67 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index ededda2..8939072 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -11,6 +11,19 @@
#include "block/block_int.h"
#include "qemu/uri.h"
+#define GLUSTER_OPT_FILENAME"filename"
+#define
