$SUBJ s/Change to support/Add support for s/ with qemu-kvm// On 11/12/2016 01:19 AM, 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> > > Signed-off-by: Ashish Mittal <Ashish.Mittal(a)veritas.com&gt; > --- > docs/formatdomain.html.in | 12 ++++++-- > docs/schemas/domaincommon.rng | 1 + > src/qemu/qemu_command.c | 4 +++ > src/qemu/qemu_driver.c | 3 ++ > src/qemu/qemu_parse_command.c | 25 ++++++++++++++++ > src/util/virstoragefile.c | 4 ++- > src/util/virstoragefile.h | 1 + > .../qemuxml2argv-disk-drive-network-vxhs.args | 23 +++++++++++++++ > .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 ++++++++++++++++++++++ > tests/qemuxml2argvtest.c | 1 + > 10 files changed, 104 insertions(+), 4 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml > Missing changes for src/libxl/libxl_conf.c (libxlMakeNetworkDiskSrcStr) and src/xenconfig/xen_xl.c (xenFormatXLDiskSrcNet) - need to add a "case VIR_STORAGE_NET_PROTOCOL_VXHS:" for the "switch (virStorageNetProtocol) src->protocol". Also running 'make syntax-check' would show that tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args has a too long line at the end: GEN test-wrap-argv --- tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args 2016-11-14 06:16:40.739509847 -0500 +++ - 2016-11-14 06:23:59.661334157 -0500 @@ -20,4 +20,5 @@ -usb \ -drive file=vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251,\ format=raw,if=none,id=drive-virtio-disk0,cache=none \ --device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ +id=virtio-disk0 Incorrect line wrapping in tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args Use test-wrap-argv.pl to wrap test data files cfg.mk:1075: recipe for target 'test-wrap-argv' failed make: *** [test-wrap-argv] Error 1 All those are easily fixable; however, you will also need to modify the virStorageSourceJSONDriverParser in src/util/virstoragefile.c in order to do the JSON parsing properly as well as tests/virstoragetest.c to add test case(s). If you use 'gitk' or some other git log -p browser, starting with commit id 'e91f767c743' and going forward through Peter's series you can see how this was done for other various protocols. From what I see from the QEMU list that'll work for the json you provided in your example: Sample command line using the JSON syntax: ./qemu-system-x86_64 -name instance-00000008 -S -vnc 0.0.0.0:0 -k en-us -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 -msg timestamp=on 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410", "server":{"host":"172.172.17.4","port":"9999"}}'

Other questions/concerns... These may have been answered previously, but I haven't followed closely along. 1. It appears to me that the 'name' field is a UUID for the VxHS volume, but it's not clear that's all it can be. You'll note in virDomainDiskSourceParse a specific gluster check. So that we don't get surprised at some point in the future - are there any specific things in the name that need to be addressed?

Beyond that, how does one know what to provide for that 'name=' field? For VxHS, it seems to me that the path must be a UUID of some sort. So would anyone know which UUID to magically pick in order to add the disk to a guest? How does one get a list?

2. Does/will VxHS ever require authentication similar to iSCSI and RBD?

3. Will there be (or is there) support for LUKS volumes on the VxHS server?

4. There's no VxHS Storage Pool support in this patch (OK actually an additional set of patches in order to support). That would be expected especially for a networked storage environment. You'll note there are src/storage/storage_backend_{iscsi|gluster|rbd}.{c|h} files that manage iSCSI, Gluster, and RBD protocol specific things. For starters, create, delete, and refresh - especially things that a stat() wouldn't necessarily return (capacity, allocation, type a/k/a target.format). Perhaps even the ability to upload/download and wipe volumes in the pool. Having a pool is a bit more work, but look at the genesis of existing storage_backend_*.c files to get a sense for what needs to change.

5. Along the same lines - virStorageFileBackendForTypeInternal handles getting details/metadata about the file format in order to determine block sizes for inactive domains. See the path qemuDomainGetBlockInfo calling into qemuStorageLimitsRefresh which calls virStorageFileInitAs. These would be for the backend of a 'virsh domblkinfo' type command when the domain isn't running. The gluster backend has a set of them (search on _virStorageFileBackend and virStorageFileBackendGluster).

6. Use something like cscope to be sure to check all the places where VIR_STORAGE_NET_PROTOCOL_ is used in the code and made sure there isn't a special case for VXHS (RBD, GLUSTER, and ISCSI have a few). Similarly check usage of VIR_STORAGE_TYPE_NETWORK. Some checks are not within switch/case statements and some involve specific disk configuration options that are(n't) supported. What about any need for migration? (see qemuMigrationIsSafe)

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 11b3330..ade35a3 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -2276,9 +2276,9 @@ > <dd> > The <code>protocol</code> attribute specifies the protocol to > access to the requested image. Possible values are "nbd", > - "iscsi", "rbd", "sheepdog" or "gluster". If the > - <code>protocol</code> attribute is "rbd", "sheepdog" or > - "gluster", an additional attribute <code>name</code> is > + "iscsi", "rbd", "sheepdog", "gluster" or "vxhs". If the > + <code>protocol</code> attribute is "rbd", "sheepdog", "gluster" > + or "vxhs", an additional attribute <code>name</code> is > mandatory to specify which volume/image will be used. For "nbd", > the <code>name</code> attribute is optional. For "iscsi" > (<span class="since">since 1.0.4</span>), the <code>name</code> Just before the closing </dd> add something to describe what the 'name' will be for vxfs like is done for iscsi, such as: For "vxhs" (<span class="since">since 2.5.0</span>), the <code>name</code> is ... [it looks like a UUID to me - although there's nothing that validates that hypothesis].

> @@ -2388,6 +2388,12 @@ > <td> one or more (<span class="since">Since 2.1.0</span>), just one prior to that </td> > <td> 24007 </td> > </tr> > + <tr> > + <td> vxhs </td> > + <td> a server running Veritas HyperScale daemon </td> > + <td> only one </td> > + <td> 9999 </td> > + </tr> > </table> > <p> > gluster supports "tcp", "rdma", "unix" as valid values for the > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 19d45fd..e569e0a 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -1464,6 +1464,7 @@ > <value>ftp</value> > <value>ftps</value> > <value>tftp</value> > + <value>vxhs</value> > </choice> > </attribute> > <optional> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 4a5fce3..c7b95aa 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -491,6 +491,9 @@ qemuNetworkDriveGetPort(int protocol, > /* no default port specified */ > return 0; > > + case VIR_STORAGE_NET_PROTOCOL_VXHS: > + return 9999; > + > case VIR_STORAGE_NET_PROTOCOL_RBD: > case VIR_STORAGE_NET_PROTOCOL_LAST: > case VIR_STORAGE_NET_PROTOCOL_NONE: > @@ -1034,6 +1037,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, > case VIR_STORAGE_NET_PROTOCOL_TFTP: > case VIR_STORAGE_NET_PROTOCOL_ISCSI: > case VIR_STORAGE_NET_PROTOCOL_GLUSTER: > + case VIR_STORAGE_NET_PROTOCOL_VXHS: > ret = qemuBuildNetworkDriveURI(src, secinfo); > break; > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index a82e58b..4910004 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -13724,6 +13724,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) > case VIR_STORAGE_NET_PROTOCOL_FTPS: > case VIR_STORAGE_NET_PROTOCOL_TFTP: > case VIR_STORAGE_NET_PROTOCOL_SSH: > + case VIR_STORAGE_NET_PROTOCOL_VXHS: > case VIR_STORAGE_NET_PROTOCOL_LAST: > virReportError(VIR_ERR_INTERNAL_ERROR, > _("external inactive snapshots are not supported on " > @@ -13787,6 +13788,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d > case VIR_STORAGE_NET_PROTOCOL_FTPS: > case VIR_STORAGE_NET_PROTOCOL_TFTP: > case VIR_STORAGE_NET_PROTOCOL_SSH: > + case VIR_STORAGE_NET_PROTOCOL_VXHS: > case VIR_STORAGE_NET_PROTOCOL_LAST: > virReportError(VIR_ERR_INTERNAL_ERROR, > _("external active snapshots are not supported on " > @@ -13932,6 +13934,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, > case VIR_STORAGE_NET_PROTOCOL_FTPS: > case VIR_STORAGE_NET_PROTOCOL_TFTP: > case VIR_STORAGE_NET_PROTOCOL_SSH: > + case VIR_STORAGE_NET_PROTOCOL_VXHS: > case VIR_STORAGE_NET_PROTOCOL_LAST: > virReportError(VIR_ERR_INTERNAL_ERROR, > _("internal inactive snapshots are not supported on " > diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c > index c3b27aa..f2edc28 100644 > --- a/src/qemu/qemu_parse_command.c > +++ b/src/qemu/qemu_parse_command.c > @@ -263,6 +263,16 @@ qemuParseNBDString(virDomainDiskDefPtr disk) > return -1; > } > > +static int > +qemuParseVxHSString(virDomainDiskDefPtr def) > +{ > + virURIPtr uri = NULL; > + > + if (!(uri = virURIParse(def->src->path))) > + return -1; > + Should there be validation of the name? Something like a call to virUUIDIsValid? This is code for the path of parsing a qemu command line of a guest not started by libvirt in order to add it to libvirt (virsh qemu-attach), which is used "less often", so I'm less concerned about this, but mainly curious.

John > + return qemuParseDriveURIString(def, uri, "vxhs"); > +} > > /* > * This method takes a string representing a QEMU command line ARGV set > @@ -737,6 +747,12 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, > if (VIR_STRDUP(def->src->path, vdi) < 0) > goto error; > } > + } else if (STRPREFIX(def->src->path, "vxhs:")) { > + def->src->type = VIR_STORAGE_TYPE_NETWORK; > + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; > + > + if (qemuParseVxHSString(def) < 0) > + goto error; > } else { > def->src->type = VIR_STORAGE_TYPE_FILE; > } > @@ -1933,6 +1949,10 @@ qemuParseCommandLine(virCapsPtr caps, > disk->src->type = VIR_STORAGE_TYPE_NETWORK; > disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG; > val += strlen("sheepdog:"); > + } else if (STRPREFIX(val, "vxhs:")) { > + disk->src->type = VIR_STORAGE_TYPE_NETWORK; > + disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; > + val += strlen("vxhs:"); > } else { > disk->src->type = VIR_STORAGE_TYPE_FILE; > } > @@ -2009,6 +2029,11 @@ qemuParseCommandLine(virCapsPtr caps, > goto error; > > break; > + case VIR_STORAGE_NET_PROTOCOL_VXHS: > + if (qemuParseVxHSString(disk) < 0) > + goto error; > + > + break; > case VIR_STORAGE_NET_PROTOCOL_HTTP: > case VIR_STORAGE_NET_PROTOCOL_HTTPS: > case VIR_STORAGE_NET_PROTOCOL_FTP: > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c > index 272db67..a730a01 100644 > --- a/src/util/virstoragefile.c > +++ b/src/util/virstoragefile.c > @@ -86,7 +86,8 @@ VIR_ENUM_IMPL(virStorageNetProtocol, VIR_STORAGE_NET_PROTOCOL_LAST, > "ftp", > "ftps", > "tftp", > - "ssh") > + "ssh", > + "vxhs") > > VIR_ENUM_IMPL(virStorageNetHostTransport, VIR_STORAGE_NET_HOST_TRANS_LAST, > "tcp", > @@ -2634,6 +2635,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, > case VIR_STORAGE_NET_PROTOCOL_ISCSI: > case VIR_STORAGE_NET_PROTOCOL_GLUSTER: > case VIR_STORAGE_NET_PROTOCOL_SSH: > + case VIR_STORAGE_NET_PROTOCOL_VXHS: > virReportError(VIR_ERR_INTERNAL_ERROR, > _("malformed backing store path for protocol %s"), > protocol); > diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h > index 3d09468..88dff36 100644 > --- a/src/util/virstoragefile.h > +++ b/src/util/virstoragefile.h > @@ -132,6 +132,7 @@ typedef enum { > VIR_STORAGE_NET_PROTOCOL_FTPS, > VIR_STORAGE_NET_PROTOCOL_TFTP, > VIR_STORAGE_NET_PROTOCOL_SSH, > + VIR_STORAGE_NET_PROTOCOL_VXHS, > > VIR_STORAGE_NET_PROTOCOL_LAST > } virStorageNetProtocol; > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args > new file mode 100644 > index 0000000..3d37b00 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args > @@ -0,0 +1,23 @@ > +LC_ALL=C \ > +PATH=/bin \ > +HOME=/home/test \ > +USER=test \ > +LOGNAME=test \ > +QEMU_AUDIO_DRV=none \ > +/usr/libexec/qemu-kvm \ > +-name QEMUGuest1 \ > +-S \ > +-M pc \ > +-cpu qemu32 \ > +-m 214 \ > +-smp 1,sockets=1,cores=1,threads=1 \ > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > +-nographic \ > +-nodefaults \ > +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ > +-no-acpi \ > +-boot c \ > +-usb \ > +-drive file=vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251,\ > +format=raw,if=none,id=drive-virtio-disk0,cache=none \ > +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml > new file mode 100644 > index 0000000..45c807f > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml > @@ -0,0 +1,34 @@ > +<domain type='qemu'> > + <name>QEMUGuest1</name> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> > + <memory unit='KiB'>219136</memory> > + <currentMemory unit='KiB'>219136</currentMemory> > + <vcpu placement='static'>1</vcpu> > + <os> > + <type arch='i686' machine='pc'>hvm</type> > + <boot dev='hd'/> > + </os> > + <clock offset='utc'/> > + <on_poweroff>destroy</on_poweroff> > + <on_reboot>restart</on_reboot> > + <on_crash>destroy</on_crash> > + <devices> > + <emulator>/usr/libexec/qemu-kvm</emulator> > + <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> > + <controller type='usb' index='0'/> > + <controller type='pci' index='0' model='pci-root'/> > + <input type='mouse' bus='ps2'/> > + <input type='keyboard' bus='ps2'/> > + <memballoon model='none'/> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 1ee8402..d94e3f2 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -869,6 +869,7 @@ mymain(void) > # endif > DO_TEST("disk-drive-network-rbd-ipv6", NONE); > DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE); > + DO_TEST("disk-drive-network-vxhs", NONE); > DO_TEST("disk-drive-no-boot", > QEMU_CAPS_BOOTINDEX); > DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", >

On 12/16/2016 10:26 PM, ashish mittal wrote: > Hi John, > > Thanks for the review and your comments. Sorry about the delay in > reply. Our qemu patch has been undergoing a lot of changes, and we > were hoping to finalize some things before the next libvirt patch. > I've been following the qemu changes from a distance - they're on the radar, but I don't follow the details! I'm deep in the middle of other work. I think some of the qemu changes will alter how some of the details work here. > > > On Mon, Nov 14, 2016 at 12:44 PM, John Ferlan <jferlan(a)redhat.com&gt; wrote: >> >> $SUBJ >> >> s/Change to support/Add support for >> >> s/ with qemu-kvm// >> >> On 11/12/2016 01:19 AM, 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> >>> >>> Signed-off-by: Ashish Mittal <Ashish.Mittal(a)veritas.com&gt; >>> --- >>> docs/formatdomain.html.in | 12 ++++++-- >>> docs/schemas/domaincommon.rng | 1 + >>> src/qemu/qemu_command.c | 4 +++ >>> src/qemu/qemu_driver.c | 3 ++ >>> src/qemu/qemu_parse_command.c | 25 ++++++++++++++++ >>> src/util/virstoragefile.c | 4 ++- >>> src/util/virstoragefile.h | 1 + >>> .../qemuxml2argv-disk-drive-network-vxhs.args | 23 +++++++++++++++ >>> .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 ++++++++++++++++++++++ >>> tests/qemuxml2argvtest.c | 1 + >>> 10 files changed, 104 insertions(+), 4 deletions(-) >>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args >>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml >>> >> >> Missing changes for src/libxl/libxl_conf.c (libxlMakeNetworkDiskSrcStr) >> and src/xenconfig/xen_xl.c (xenFormatXLDiskSrcNet) - need to add a "case >> VIR_STORAGE_NET_PROTOCOL_VXHS:" for the "switch (virStorageNetProtocol) >> src->protocol". >> >> Also running 'make syntax-check' would show that >> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args has a >> too long line at the end: >> >> GEN test-wrap-argv >> --- tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args >> 2016-11-14 06:16:40.739509847 -0500 >> +++ - 2016-11-14 06:23:59.661334157 -0500 >> @@ -20,4 +20,5 @@ >> -usb \ >> -drive file=vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251,\ >> format=raw,if=none,id=drive-virtio-disk0,cache=none \ >> --device >> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 >> +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ >> +id=virtio-disk0 >> Incorrect line wrapping in >> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args >> Use test-wrap-argv.pl to wrap test data files >> cfg.mk:1075: recipe for target 'test-wrap-argv' failed >> make: *** [test-wrap-argv] Error 1 >> >> >> All those are easily fixable; however, you will also need to modify the >> virStorageSourceJSONDriverParser in src/util/virstoragefile.c in order >> to do the JSON parsing properly as well as tests/virstoragetest.c to add >> test case(s). If you use 'gitk' or some other git log -p browser, >> starting with commit id 'e91f767c743' and going forward through Peter's >> series you can see how this was done for other various protocols. >> >> From what I see from the QEMU list that'll work for the json you >> provided in your example: >> >> Sample command line using the JSON syntax: >> ./qemu-system-x86_64 -name instance-00000008 -S -vnc 0.0.0.0:0 -k en-us >> -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 >> -msg timestamp=on >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410", >> "server":{"host":"172.172.17.4","port":"9999"}}' >> >> > > Will fix above issues. > >> Other questions/concerns... These may have been answered previously, but >> I haven't followed closely along. >> >> 1. It appears to me that the 'name' field is a UUID for the VxHS >> volume, but it's not clear that's all it can be. You'll note in >> virDomainDiskSourceParse a specific gluster check. So that we don't get >> surprised at some point in the future - are there any specific things in >> the name that need to be addressed? >> > > That's correct. Name is in fact a UUID and nothing else. The "name" > tag in XML name='eb90327c-8302-4725-9e1b-4e85ed4dc251' is misleading. > I will change it to "vdisk-id" so there's no confusion. > > We don't think there is a need to validate the name/vdisk-id format. > In case of incorrect vdisk ID, qemu vxhs code will fail the vdisk > open. > > >> Beyond that, how does one know what to provide for that 'name=' field? >> For VxHS, it seems to me that the path must be a UUID of some sort. So >> would anyone know which UUID to magically pick in order to add the disk >> to a guest? How does one get a list? >> > > The UUID allocation and vdisk creation is done out-of-band by the VxHS > server. The UUIDs for disks are populated in the XML file at the time > of VM creation. User does not (need to) know what UUID to open or the > list of all UUIDs. > Now I'm confused - maybe I asked the wrong question. You show the domain XML: <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'> <host name='192.168.0.1' port='9999'/> </source> How does anyone know what to put in 'name'? That's a UUID. That's what I'm looking to determine. How do I know which volume I'd be choosing? Maybe there's something I'm missing about the implementation. There has to be something to allow someone to make an intelligent choice about which vxhs volume/disk/storage is being used by the domain. If I compare that to iSCSI: <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/2'> <host name='example.com' port='3260'/> </source> There are some iSCSI tools and built up knowledge that show you how to generate the value for the "name" attribute. But it's a lot easier to do this if one creates an iSCSI pool and then adds the disk/volume/lun to the domain via pool source XML: <source pool='iscsi-pool' volume='unit:0:0:1' mode='host'/> or <source pool='iscsi-pool' volume='unit:0:0:2' mode='direct'/> where 'volume' is filled in from the output of a 'virsh vol-list iscsi-pool'... >> 2. Does/will VxHS ever require authentication similar to iSCSI and RBD? >> > > This is under active discussion with the qemu community. > Yes - I've seen.. The secrets concept is available in libvirt and it works with the qemu secrets... The RBD code uses the model I think you'll need to use. >> 3. Will there be (or is there) support for LUKS volumes on the VxHS server? >> > > We don't anticipate a need for this. > >> 4. There's no VxHS Storage Pool support in this patch (OK actually an >> additional set of patches in order to support). That would be expected >> especially for a networked storage environment. You'll note there are >> src/storage/storage_backend_{iscsi|gluster|rbd}.{c|h} files that manage >> iSCSI, Gluster, and RBD protocol specific things. For starters, create, >> delete, and refresh - especially things that a stat() wouldn't >> necessarily return (capacity, allocation, type a/k/a target.format). >> Perhaps even the ability to upload/download and wipe volumes in the >> pool. Having a pool is a bit more work, but look at the genesis of >> existing storage_backend_*.c files to get a sense for what needs to change. >> > > VxHS does not need the Storage Pool functionality. Do we still need to > implement this? > It's something that's expected. See my reasoning above.

>> 5. Along the same lines - virStorageFileBackendForTypeInternal handles >> getting details/metadata about the file format in order to determine >> block sizes for inactive domains. See the path qemuDomainGetBlockInfo >> calling into qemuStorageLimitsRefresh which calls virStorageFileInitAs. >> These would be for the backend of a 'virsh domblkinfo' type command when >> the domain isn't running. The gluster backend has a set of them (search >> on _virStorageFileBackend and virStorageFileBackendGluster). >> > > Would supporting the above functionality be optional? As mentioned > above, vxhs disk creation happens outside of qemu/libvirt by the vxhs > server in response to external messages. The XML config for the guest > VM is then populated with correct UUID + server IP/port info, which is > picked up by libvirt. > How would the libvirt code find out anything it needs about the target volume such as allocation, capacity, and physical usage. I think these are details we'll work through once the qemu connection is made. Of course without a storage pool and need for volume def, then this would be unnecessary. >> 6. Use something like cscope to be sure to check all the places where >> VIR_STORAGE_NET_PROTOCOL_ is used in the code and made sure there isn't >> a special case for VXHS (RBD, GLUSTER, and ISCSI have a few). Similarly >> check usage of VIR_STORAGE_TYPE_NETWORK. Some checks are not within >> switch/case statements and some involve specific disk configuration >> options that are(n't) supported. What about any need for migration? >> (see qemuMigrationIsSafe) >> > > Will check for other special cases. We do support live migration add > cache='none' to the config by default. Do I need to make additional > changes for this? > At this point - I have no idea. You have networked storage and there are interactions with libvirt/qemu and the nbd server which I don't have fresh in mind right now. >> >>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >>> index 11b3330..ade35a3 100644 >>> --- a/docs/formatdomain.html.in >>> +++ b/docs/formatdomain.html.in >>> @@ -2276,9 +2276,9 @@ >>> <dd> >>> The <code>protocol</code> attribute specifies the protocol to >>> access to the requested image. Possible values are "nbd", >>> - "iscsi", "rbd", "sheepdog" or "gluster". If the >>> - <code>protocol</code> attribute is "rbd", "sheepdog" or >>> - "gluster", an additional attribute <code>name</code> is >>> + "iscsi", "rbd", "sheepdog", "gluster" or "vxhs". If the >>> + <code>protocol</code> attribute is "rbd", "sheepdog", "gluster" >>> + or "vxhs", an additional attribute <code>name</code> is >>> mandatory to specify which volume/image will be used. For "nbd", >>> the <code>name</code> attribute is optional. For "iscsi" >>> (<span class="since">since 1.0.4</span>), the <code>name</code> >> >> Just before the closing </dd> add something to describe what the 'name' >> will be for vxfs like is done for iscsi, such as: >> >> For "vxhs" (<span class="since">since 2.5.0</span>), the >> <code>name</code> is ... [it looks like a UUID to me - although there's >> nothing that validates that hypothesis]. >> > > Will do. > >> >>> @@ -2388,6 +2388,12 @@ >>> <td> one or more (<span class="since">Since 2.1.0</span>), just one prior to that </td> >>> <td> 24007 </td> >>> </tr> >>> + <tr> >>> + <td> vxhs </td> >>> + <td> a server running Veritas HyperScale daemon </td> >>> + <td> only one </td> >>> + <td> 9999 </td> >>> + </tr> >>> </table> >>> <p> >>> gluster supports "tcp", "rdma", "unix" as valid values for the >>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >>> index 19d45fd..e569e0a 100644 >>> --- a/docs/schemas/domaincommon.rng >>> +++ b/docs/schemas/domaincommon.rng >>> @@ -1464,6 +1464,7 @@ >>> <value>ftp</value> >>> <value>ftps</value> >>> <value>tftp</value> >>> + <value>vxhs</value> >>> </choice> >>> </attribute> >>> <optional> >>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >>> index 4a5fce3..c7b95aa 100644 >>> --- a/src/qemu/qemu_command.c >>> +++ b/src/qemu/qemu_command.c >>> @@ -491,6 +491,9 @@ qemuNetworkDriveGetPort(int protocol, >>> /* no default port specified */ >>> return 0; >>> >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> + return 9999; >>> + >>> case VIR_STORAGE_NET_PROTOCOL_RBD: >>> case VIR_STORAGE_NET_PROTOCOL_LAST: >>> case VIR_STORAGE_NET_PROTOCOL_NONE: >>> @@ -1034,6 +1037,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, >>> case VIR_STORAGE_NET_PROTOCOL_TFTP: >>> case VIR_STORAGE_NET_PROTOCOL_ISCSI: >>> case VIR_STORAGE_NET_PROTOCOL_GLUSTER: >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> ret = qemuBuildNetworkDriveURI(src, secinfo); >>> break; >>> >>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >>> index a82e58b..4910004 100644 >>> --- a/src/qemu/qemu_driver.c >>> +++ b/src/qemu/qemu_driver.c >>> @@ -13724,6 +13724,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) >>> case VIR_STORAGE_NET_PROTOCOL_FTPS: >>> case VIR_STORAGE_NET_PROTOCOL_TFTP: >>> case VIR_STORAGE_NET_PROTOCOL_SSH: >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> case VIR_STORAGE_NET_PROTOCOL_LAST: >>> virReportError(VIR_ERR_INTERNAL_ERROR, >>> _("external inactive snapshots are not supported on " >>> @@ -13787,6 +13788,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d >>> case VIR_STORAGE_NET_PROTOCOL_FTPS: >>> case VIR_STORAGE_NET_PROTOCOL_TFTP: >>> case VIR_STORAGE_NET_PROTOCOL_SSH: >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> case VIR_STORAGE_NET_PROTOCOL_LAST: >>> virReportError(VIR_ERR_INTERNAL_ERROR, >>> _("external active snapshots are not supported on " >>> @@ -13932,6 +13934,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, >>> case VIR_STORAGE_NET_PROTOCOL_FTPS: >>> case VIR_STORAGE_NET_PROTOCOL_TFTP: >>> case VIR_STORAGE_NET_PROTOCOL_SSH: >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> case VIR_STORAGE_NET_PROTOCOL_LAST: >>> virReportError(VIR_ERR_INTERNAL_ERROR, >>> _("internal inactive snapshots are not supported on " >>> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c >>> index c3b27aa..f2edc28 100644 >>> --- a/src/qemu/qemu_parse_command.c >>> +++ b/src/qemu/qemu_parse_command.c >>> @@ -263,6 +263,16 @@ qemuParseNBDString(virDomainDiskDefPtr disk) >>> return -1; >>> } >>> >>> +static int >>> +qemuParseVxHSString(virDomainDiskDefPtr def) >>> +{ >>> + virURIPtr uri = NULL; >>> + >>> + if (!(uri = virURIParse(def->src->path))) >>> + return -1; >>> + >> >> Should there be validation of the name? Something like a call to >> virUUIDIsValid? >> >> This is code for the path of parsing a qemu command line of a guest not >> started by libvirt in order to add it to libvirt (virsh qemu-attach), >> which is used "less often", so I'm less concerned about this, but mainly >> curious. >> > > An incorrect UUID will only cause vxhs code in qemu to fail the disk > open. So we don't think there is a need for additional checks. Please > let us know if this sounds OK! > I'll wait for the qemu code to be done before I page these changes fully back in. John >> >> John >>> + return qemuParseDriveURIString(def, uri, "vxhs"); >>> +} >>> >>> /* >>> * This method takes a string representing a QEMU command line ARGV set >>> @@ -737,6 +747,12 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, >>> if (VIR_STRDUP(def->src->path, vdi) < 0) >>> goto error; >>> } >>> + } else if (STRPREFIX(def->src->path, "vxhs:")) { >>> + def->src->type = VIR_STORAGE_TYPE_NETWORK; >>> + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; >>> + >>> + if (qemuParseVxHSString(def) < 0) >>> + goto error; >>> } else { >>> def->src->type = VIR_STORAGE_TYPE_FILE; >>> } >>> @@ -1933,6 +1949,10 @@ qemuParseCommandLine(virCapsPtr caps, >>> disk->src->type = VIR_STORAGE_TYPE_NETWORK; >>> disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG; >>> val += strlen("sheepdog:"); >>> + } else if (STRPREFIX(val, "vxhs:")) { >>> + disk->src->type = VIR_STORAGE_TYPE_NETWORK; >>> + disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; >>> + val += strlen("vxhs:"); >>> } else { >>> disk->src->type = VIR_STORAGE_TYPE_FILE; >>> } >>> @@ -2009,6 +2029,11 @@ qemuParseCommandLine(virCapsPtr caps, >>> goto error; >>> >>> break; >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> + if (qemuParseVxHSString(disk) < 0) >>> + goto error; >>> + >>> + break; >>> case VIR_STORAGE_NET_PROTOCOL_HTTP: >>> case VIR_STORAGE_NET_PROTOCOL_HTTPS: >>> case VIR_STORAGE_NET_PROTOCOL_FTP: >>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c >>> index 272db67..a730a01 100644 >>> --- a/src/util/virstoragefile.c >>> +++ b/src/util/virstoragefile.c >>> @@ -86,7 +86,8 @@ VIR_ENUM_IMPL(virStorageNetProtocol, VIR_STORAGE_NET_PROTOCOL_LAST, >>> "ftp", >>> "ftps", >>> "tftp", >>> - "ssh") >>> + "ssh", >>> + "vxhs") >>> >>> VIR_ENUM_IMPL(virStorageNetHostTransport, VIR_STORAGE_NET_HOST_TRANS_LAST, >>> "tcp", >>> @@ -2634,6 +2635,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, >>> case VIR_STORAGE_NET_PROTOCOL_ISCSI: >>> case VIR_STORAGE_NET_PROTOCOL_GLUSTER: >>> case VIR_STORAGE_NET_PROTOCOL_SSH: >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> virReportError(VIR_ERR_INTERNAL_ERROR, >>> _("malformed backing store path for protocol %s"), >>> protocol); >>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h >>> index 3d09468..88dff36 100644 >>> --- a/src/util/virstoragefile.h >>> +++ b/src/util/virstoragefile.h >>> @@ -132,6 +132,7 @@ typedef enum { >>> VIR_STORAGE_NET_PROTOCOL_FTPS, >>> VIR_STORAGE_NET_PROTOCOL_TFTP, >>> VIR_STORAGE_NET_PROTOCOL_SSH, >>> + VIR_STORAGE_NET_PROTOCOL_VXHS, >>> >>> VIR_STORAGE_NET_PROTOCOL_LAST >>> } virStorageNetProtocol; >>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args >>> new file mode 100644 >>> index 0000000..3d37b00 >>> --- /dev/null >>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args >>> @@ -0,0 +1,23 @@ >>> +LC_ALL=C \ >>> +PATH=/bin \ >>> +HOME=/home/test \ >>> +USER=test \ >>> +LOGNAME=test \ >>> +QEMU_AUDIO_DRV=none \ >>> +/usr/libexec/qemu-kvm \ >>> +-name QEMUGuest1 \ >>> +-S \ >>> +-M pc \ >>> +-cpu qemu32 \ >>> +-m 214 \ >>> +-smp 1,sockets=1,cores=1,threads=1 \ >>> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ >>> +-nographic \ >>> +-nodefaults \ >>> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ >>> +-no-acpi \ >>> +-boot c \ >>> +-usb \ >>> +-drive file=vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251,\ >>> +format=raw,if=none,id=drive-virtio-disk0,cache=none \ >>> +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 >>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml >>> new file mode 100644 >>> index 0000000..45c807f >>> --- /dev/null >>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml >>> @@ -0,0 +1,34 @@ >>> +<domain type='qemu'> >>> + <name>QEMUGuest1</name> >>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> >>> + <memory unit='KiB'>219136</memory> >>> + <currentMemory unit='KiB'>219136</currentMemory> >>> + <vcpu placement='static'>1</vcpu> >>> + <os> >>> + <type arch='i686' machine='pc'>hvm</type> >>> + <boot dev='hd'/> >>> + </os> >>> + <clock offset='utc'/> >>> + <on_poweroff>destroy</on_poweroff> >>> + <on_reboot>restart</on_reboot> >>> + <on_crash>destroy</on_crash> >>> + <devices> >>> + <emulator>/usr/libexec/qemu-kvm</emulator> >>> + <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> >>> + <controller type='usb' index='0'/> >>> + <controller type='pci' index='0' model='pci-root'/> >>> + <input type='mouse' bus='ps2'/> >>> + <input type='keyboard' bus='ps2'/> >>> + <memballoon model='none'/> >>> + </devices> >>> +</domain> >>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >>> index 1ee8402..d94e3f2 100644 >>> --- a/tests/qemuxml2argvtest.c >>> +++ b/tests/qemuxml2argvtest.c >>> @@ -869,6 +869,7 @@ mymain(void) >>> # endif >>> DO_TEST("disk-drive-network-rbd-ipv6", NONE); >>> DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE); >>> + DO_TEST("disk-drive-network-vxhs", NONE); >>> DO_TEST("disk-drive-no-boot", >>> QEMU_CAPS_BOOTINDEX); >>> DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", >>> > > Thanks, > Ashish >