On Wed, Jan 25, 2017 at 10:59:59 -0500, John Ferlan wrote:
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@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.