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

[Prasanna Kalever <pkalever@redhat.com>]

> On 09/21/2015 05:24 AM, Prasanna Kumar Kalever wrote:
> > This patch adds a way to specify multiple backup volfile servers to the
> > gluster
> > block backend of QEMU with tcp|rdma transport types and their port numbers.
> > 
> 
> > 
> > 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)
> > 
> > This patch depends on a recent fix in libgfapi raised as part of this work:
> > http://review.gluster.org/#/c/12114/ (not merged yet)
> > 
> 
> It would be nice to get that merged before we take this in qemu.
> 
> > Credits: Sincere thanks to Kevin Wolf <kwolf@redhat.com> and
> > "Deepak C Shetty" <deepakcs@redhat.com> for inputs and all their support
> > 
> 
> Up to here is good.

thank you

> 
> > v1:
> > multiple server addresses but common port number and transport type
> > pattern: URI syntax with query (?) delimitor
> > syntax:
> >     file=gluster[+transport-type]://server1:24007/testvol/a.img\
> >          ?backup-volfile-servers=server2&backup-volfile-servers=server3
> 
> But the changelog information here should instead occur...

Agree, I will correct myself

> 
> 
> > 
> > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> > ---
> 
> ...here, so that it is helpful to reviewers but doesn't clog qemu.git
> history (a year from now, we won't care how many iterations a patch went
> through, only what got committed).
> 
> 
> > +++ b/qapi/block-core.json
> > @@ -1382,7 +1382,7 @@
> >    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> >              'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> >              'host_floppy', 'http', 'https', 'null-aio', 'null-co',
> >              'parallels',
> > -            'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi',
> > 'vhdx',
> > +            'qcow', 'qcow2', 'qed', 'quorum', 'gluster', 'raw', 'tftp',
> > 'vdi', 'vhdx',
> >              'vmdk', 'vpc', 'vvfat' ] }
> 
> Please keep this list sorted.  Also, missing documentation that
> 'gluster' was added in 2.5.
> 

Agree, I will change it

> >  
> >  ##
> > @@ -1794,6 +1794,43 @@
> >              '*read-pattern': 'QuorumReadPattern' } }
> >  
> >  ##
> > +# @GlusterTuplePattern
> 
> Name is long. Something like 'GlusterHost' would be nicer.
>

Hmm, I think rather than length, readability is important, Even 'QuorumReadPattern' is also long, but gives  completeness.
Okay, If you still thing its better to have short name I can change it to "GlusterTuple"
 
> > +#
> > +# Gluster tuple pattern
> > +#
> > +# @transport:    #transport type used to connect to gluster management
> > daemon
> > +#                 it can be tcp|rdma (default 'tcp')
> > +#
> > +# @port:         port number on which glusterd is listening. (default
> > 24007)
> 
> To have a default, the parameter needs to be optional ('*port').
>

Thanks for correcting here, I really didn't knew this.
 
> > +#
> > +# @server:       server address (hostname/ipv4/ipv6 addresses)
> > +#
> > +# Since: 2.4+
> 
> s/2.4+/2.5/ (there is no 2.4+ release; the release that this will appear
> in is 2.5)
>

Okay.
 
> > +##
> > +{ 'struct': 'GlusterTuplePattern',
> > +  'data': { 'server': 'str',
> > +            'transport': 'str',
> 
> Yuck. Please don't open-code this as a 'str', but instead define it as
> an enum:
> 
> { 'enum': 'GlusterTransport', 'data': [ 'tcp', 'rdma' ] }
> 
> > +            'port': 'str' } }
> 
> Should port be an integer instead of a string? I guess we already allow
> named ports in other network-related interfaces, but it would still be
> nice to have an 'alternate' type that allows both integer and string.
> 
> It would be nice if your documentation of the parameters occurred in the
> same order as the parameters appear in the 'struct'.
> 

I expected, If I take it as an integer, while passing in json format
'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
             "image-path":"/path/a.qcow2","backup-volfile-servers":
             [{"server":"1.2.3.4","port":"24007","transport":"tcp"},
              {"server":"4.5.6.7","port":"24008","transport":"rdma"},] } }'
I should specify only "port":"24007" => "port":24007 (as integer)
Thanks for correcting me again.

I think the below should be enough

+##
+{ 'enum': 'GTransport', 'data': [ 'tcp', 'rdma' ] }

 ##
 # @GlusterTuplePattern
 #
@@ -1809,8 +1813,8 @@
 ##
 { 'struct': 'GlusterTuplePattern',
   'data': { 'server': 'str',
-            'transport': 'str',
-            'port': 'str' } }
+            '*transport': 'GTransport',
+            '*port': 'int' } }

Will do this.

> > +
> > +##
> > +# @BlockdevOptionsGluster
> > +#
> > +# Driver specific block device options for Gluster
> > +#
> > +# @volname:      name of gluster volume where our VM image resides
> > +#
> > +# @image-path:   absolute path to image file in gluster volume
> > +#
> > +# @backup-volfile-servers: holds multiple tuples of {server, transport,
> > port}
> > +#
> > +# Since: 2.4+
> 
> s/2.4+/2.5/
> 
Okay.
> > +##
> > +{ 'struct': 'BlockdevOptionsGluster',
> > +  'data': { 'volname': 'str',
> > +            'image-path': 'str',
> > +            'backup-volfile-servers': [ 'GlusterTuplePattern' ] } }
> 
> Shouldn't this be simply 'volfile-servers', as you are including the
> primary server in addition to the backup servers?
>

Again I want to maintain naming as mount.glusterfs do for fuse.
 
> > +
> > +##
> >  # @BlockdevOptions
> >  #
> >  # Options for creating a block device.
> > @@ -1814,6 +1851,7 @@
> >        'ftp':        'BlockdevOptionsFile',
> >        'ftps':       'BlockdevOptionsFile',
> >  # TODO gluster: Wait for structured options
> > +      'gluster':    'BlockdevOptionsGluster',
> >        'host_cdrom': 'BlockdevOptionsFile',
> >        'host_device':'BlockdevOptionsFile',
> >        'host_floppy':'BlockdevOptionsFile',
> > 
> 
> There's also work on the list to expose NBD in a structured fashion; I'm
> a bit worried that the two proposals might need to be sharing some
> common functionality, rather than independently inventing slightly
> different syntax.

I have gone through patch, Still NDB support a slight different transport types,
and naming say volname etc..
So I think for now lets make them independent.

> 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
> 

Thanks for your detailed comments Eric Blake :)

Best Regards,
Prasanna Kumar Kalever.