Re: [Qemu-devel] [PATCH 1/1] block/gluster: add support for multiple gluster backup volfile servers

[Daniel P. Berrange]

On Tue, Sep 08, 2015 at 06:34:09PM +0530, Prasanna Kumar Kalever wrote:
> This patch adds a way to specify multiple backup volfile servers to the 
> gluster
> block backend of QEMU with both tcp and rdma transport types.
> 
> Problem:
> 
> Currenly VM Image on gluster volume is specified like this:
> 
> file=gluster[+tcp]://server1:24007/testvol/a.img
> 
> Assuming we have have three servers in trustred pool with replica 3 volume
> in action and unfortunately server1 (mentioned in the command above) went down
> for some reason, since the volume is replica 3 we now have other 2 servers
> active from which we can boot the VM.
> 
> But currently there is no mechanism to pass the other 2 gluster server
> addresses to qemu.
> 
> Solution:
> 
> New way of specifying VM Image on gluster volume with backup volfile servers:
> 
> file=gluster[+transport-type]://server1:24007/testvol/a.img\
>      ?backup-volfile-servers=server2&backup-volfile-servers=server3

Comparison with RBD syntax:

  file=rbd:pool/image:auth_supported=none:\
    mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;\
    mon3.example.org\:6322,if=virtio,format=raw

As Peter already mentioned, you're missing port numbers.

It is slightly unpleasant to have different ways of specifying the first
vs second, third, etc hosts. I wonder if it would be nicer to keep all
the hostnames in the host part of the URI. eg

 
file=gluster[+transport-type]://server1:24007,server2:3553,server3:2423/testvol/a.img\
      ?backup-volfile-servers=server2&backup-volfile-servers=server3

Of course it ceases to be a wellformed URI at that point, so another option
would be to just allow the host part of the URI to be optional, and then
accept mutliple instances ofa  'server' arg, eg

 file=gluster[+transport-type]:///testvol/a.img\
      ?server=server1:2424&server=server2:2423&sever=server3:34222

I think I prefer this last syntax most.

> 
> This patch gives a mechanism to provide all the server addresses which are in
> replica set, so in case server1 is down VM can still boot from any of the
> active servers.
> 
> This is equivalent to the backup-volfile-servers option supported by
> mount.glusterfs (FUSE way of mounting gluster volume)
> 
> Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> ---
>  block/gluster.c | 118 
> +++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 83 insertions(+), 35 deletions(-)


>  static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
>  {
> -    URI *uri;
> -    QueryParams *qp = NULL;
> -    bool is_unix = false;
> -    int ret = 0;
> +    URI         *uri     = NULL;
> +    QueryParams *qp      = NULL;
> +    bool        is_unix  = false;
> +    bool        is_tcp   = false;
> +    bool        is_rdma  = false;
> +    int         i        = 0;
> +    int         ret      = 0;
> +    int         nservers = 0;

Aligning indentation like this is really not desirable, as it results
in huge whitespace diffs for existing code anytime someone adds/removes
a variable which changes the indent depth, so please don't do this.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|