[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/1] block/gluster: add support for multiple
From: |
Prasanna Kalever |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/1] block/gluster: add support for multiple gluster backup volfile servers |
Date: |
Tue, 22 Sep 2015 04:06:54 -0400 (EDT) |
> 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 <address@hidden> and
> > "Deepak C Shetty" <address@hidden> 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 <address@hidden>
> > ---
>
> ...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.