On 01/19/2017 09:21 PM, Ashish Mittal wrote:
> Sample XML for a vxhs vdisk is as follows:
>
> <disk type='network' device='disk'>
> <driver name='qemu' type='raw' cache='none'/>
> <source protocol='vxhs'
name='eb90327c-8302-4725-9e1b-4e85ed4dc251'>
> <host name='192.168.0.1' port='9999'/>
> </source>
> <backingStore/>
> <target dev='vda' bus='virtio'/>
> <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial>
> <alias name='virtio-disk0'/>
> <address type='pci' domain='0x0000' bus='0x00'
slot='0x04'
> function='0x0'/>
> </disk>
It's still not really clear how someone knows to use the name string.
IOW: How would someone know what to supply. Perhaps the libvirt wiki
(
http://wiki.libvirt.org/page/Main_Page) as opposed to the libvirt docs.
I assume that someone using VxHS knows how to get that, but still I find
it a "good thing" to be able to have that description somewhere as I can
only assume some day it'll come up.
But since we don't "require" this for iSCSI, Ceph/RBD, Gluster, etc.
it's not something 'required' for this patch. Still something for
someone's todo list to get a description on the libvirt wiki. Once you
have wiki write access, then you can always keep that data up to date
without requiring libvirt patches.
>
> Signed-off-by: Ashish Mittal <Ashish.Mittal(a)veritas.com>
> ---
> v2 changelog:
> (1) Added code for JSON parsing of a VxHS vdisk.
> (2) Added test case to verify JSON parsing.
> (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS.
> (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args.
>
> v3 changelog:
> (1) Implemented the modern syntax for VxHS disk specification.
> (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax.
> (3) Added a negative test case to check failure when multiple hosts
> are specified for a VxHS disk.
>
> docs/formatdomain.html.in | 15 ++++-
> docs/schemas/domaincommon.rng | 1 +
> src/libxl/libxl_conf.c | 1 +
> src/qemu/qemu_command.c | 68 ++++++++++++++++++++++
> src/qemu/qemu_driver.c | 3 +
> src/qemu/qemu_parse_command.c | 26 +++++++++
> src/util/virstoragefile.c | 63 +++++++++++++++++++-
> src/util/virstoragefile.h | 1 +
> src/xenconfig/xen_xl.c | 1 +
> ...xml2argv-disk-drive-network-vxhs-multi-host.xml | 35 +++++++++++
> .../qemuxml2argv-disk-drive-network-vxhs.args | 25 ++++++++
> .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 +++++++++++
> tests/qemuxml2argvtest.c | 2 +
> tests/virstoragetest.c | 16 +++++
> 14 files changed, 287 insertions(+), 4 deletions(-)
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
>
In general things are looking much better - a couple of nits below and 2
things to consider/rework...
#1. Based on Peter's v2 comments, we don't want to support the
older/legacy syntax for VxHS, so it's something that should be removed -
although we should check for it being present and fail if found.
#2. Is the desire to ever support more than 1 host? If not, then is the
"server" syntax you've borrowed from the Gluster code necessary? Could
you just go with the single "host" like NBD and SSH. As it relates to
the qemu command line - I'm not quite as clear. From the example I see
in commit id '7b7da9e28', the gluster syntax would have:
+file.server.0.type=tcp,file.server.0.host=example.org,file.server.0.port=6000,\
+file.server.1.type=tcp,file.server.1.host=example.org,file.server.1.port=24007,\
+file.server.2.type=unix,file.server.2.socket=/path/to/sock,format=qcow2,\
whereas, the VxHS syntax is:
+file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\
I think that all protocols while using the JSON or
JSON-converted-to-commandline syntax should use exactly the same syntax
so that we don't have to do custom generators for every single storage
transport somebody invents.