> 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 <kw...@redhat.com> and > > "Deepak C Shetty" <deepa...@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.kale...@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.