[libvirt] [PATCH v3] Add support for Veritas HyperScale (VxHS) block device protocol
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@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 diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f7bef51..2a071c9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2319,9 +2319,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> @@ -2329,6 +2329,9 @@ target's name by a slash (e.g., <code>iqn.2013-07.com.example:iscsi-pool/1</code>). If not specified, the default LUN is zero. + For "vxhs" (<span class="since">since 2.5.0</span>), the + <code>name</code> is the UUID of the volume, assigned by the + HyperScale sever. <span class="since">Since 0.8.7</span> </dd> <dt><code>volume</code></dt> @@ -2431,6 +2434,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 4d76315..cdc39ca 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1470,6 +1470,7 @@ <value>ftp</value> <value>ftps</value> <value>tftp</value> + <value>vxhs</value> </choice> </attribute> <optional> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b569dda..7e12d32 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -604,6 +604,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_GLUSTER: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8e48d2..08bad8e 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: @@ -899,6 +902,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) } +#define QEMU_DEFAULT_VXHS_PORT "9999" + +/* Build the VxHS host object */ +static virJSONValuePtr +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src) +{ + virJSONValuePtr server = NULL; + virStorageNetHostDefPtr host; + const char *portstr; + + if (src->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("VxHS supports only one server")); + goto cleanup; + } + + host = src->hosts; + portstr = host->port; + + if (!portstr) + portstr = QEMU_DEFAULT_VXHS_PORT; + + if (virJSONValueObjectCreate(&server, + "s:host", host->name, + "s:port", portstr, + NULL) < 0) + server = NULL; + + cleanup: + return server; +} + + +static virJSONValuePtr +qemuBuildVxHSDriveJSON(virStorageSourcePtr src) +{ + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); + virJSONValuePtr server = NULL; + virJSONValuePtr ret = NULL; + + if (!(server = qemuBuildVxHSDriveJSONHost(src))) + return NULL; + + /* VxHS disk sepecification example: + * { driver:"vxhs", + * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251", + * server.host:"1.2.3.4", + * server.port:1234} + */ + if (virJSONValueObjectCreate(&ret, + "s:driver", protocol, + "s:vdisk-id", src->path, + "a:server", server, NULL) < 0) + virJSONValueFree(server); + + return ret; +} + + static char * qemuBuildNetworkDriveURI(virStorageSourcePtr src, qemuDomainSecretInfoPtr secinfo) @@ -1034,6 +1096,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; @@ -1148,6 +1211,11 @@ qemuGetDriveSourceProps(virStorageSourcePtr src, if (!(fileprops = qemuBuildGlusterDriveJSON(src))) return -1; } + + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) { + if (!(fileprops = qemuBuildVxHSDriveJSON(src))) + return -1; + } break; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b359e77..c76e9d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13788,6 +13788,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 " @@ -13851,6 +13852,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 " @@ -13996,6 +13998,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 405e655..aab287a 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk) return -1; } +static int +qemuParseVxHSString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src->path))) + return -1; + + return qemuParseDriveURIString(def, uri, "vxhs"); +} + /* * This method takes a string representing a QEMU command line ARGV set @@ -737,6 +748,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; } @@ -1947,6 +1964,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; } @@ -2023,6 +2044,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 ce6d213..e62f4e6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -85,7 +85,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", @@ -2633,6 +2634,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); @@ -2964,6 +2966,64 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, } +static int +virStorageSourceParseBackingJSONVXHS(virStorageSourcePtr src, + virJSONValuePtr json, + int opaque ATTRIBUTE_UNUSED) +{ + virJSONValuePtr server; + const char *hostname, *port; + const char *uri = virJSONValueObjectGetString(json, "filename"); + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id"); + + src->type = VIR_STORAGE_TYPE_NETWORK; + src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; + + /* legacy URI based syntax passed via 'filename' option */ + if (uri) + return virStorageSourceParseBackingJSONUriStr(src, uri, + VIR_STORAGE_NET_PROTOCOL_VXHS); + + server = virJSONValueObjectGetObject(json, "server"); + hostname = virJSONValueObjectGetString(server, "host"); + port = virJSONValueObjectGetString(server, "port"); + + if (!vdisk_id) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'vdisk-id' attribute in " + "JSON backing definition for VxHS volume")); + return -1; + } + if (!server) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'server' attribute in " + "JSON backing definition for VxHS volume")); + return -1; + } + if (!hostname) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing hostname for tcp backing server in " + "JSON backing definition for VxHS volume")); + return -1; + } + + + if (VIR_STRDUP(src->path, vdisk_id) < 0) + return -1; + + if (VIR_ALLOC_N(src->hosts, 1) < 0) + return -1; + src->nhosts = 1; + + src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + + if (VIR_STRDUP(src->hosts[0].name, hostname) < 0 || + VIR_STRDUP(src->hosts[0].port, port) < 0) + return -1; + + return 0; +} + struct virStorageSourceJSONDriverParser { const char *drvname; int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque); @@ -2985,6 +3045,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = { {"sheepdog", virStorageSourceParseBackingJSONSheepdog, 0}, {"ssh", virStorageSourceParseBackingJSONSSH, 0}, {"rbd", virStorageSourceParseBackingJSONRBD, 0}, + {"vxhs", virStorageSourceParseBackingJSONVXHS, 0}, }; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1f62244..e60b6e9 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -134,6 +134,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/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 18d9fe3..9fe24d6 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -942,6 +942,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src) case VIR_STORAGE_NET_PROTOCOL_GLUSTER: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml new file mode 100644 index 0000000..660bcde --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml @@ -0,0 +1,35 @@ +<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'/> + <host name='192.168.0.2' 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/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args new file mode 100644 index 0000000..23045e1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args @@ -0,0 +1,25 @@ +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.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ +file.server.host=192.168.0.1,file.server.port=9999,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 ab3ad08..f751ec9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -892,6 +892,8 @@ 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_FAILURE("disk-drive-network-vxhs-multi-host", NONE); DO_TEST("disk-drive-no-boot", QEMU_CAPS_BOOTINDEX); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f766df1..a28fbd9 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1492,6 +1492,22 @@ mymain(void) "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\"," + "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\"" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='192.168.0.1' port='9999'/>\n" + "</source>\n"); + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\"," + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\"," + "\"server\": { \"host\":\"example.com\"," + "\"port\":\"1234\"" + "}" + "}" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='example.com' port='1234'/>\n" + "</source>\n"); cleanup: /* Final cleanup */ -- 2.5.5
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,\ FWIW: I also note there is no ".type=tcp" in your output - so perhaps the "default" is tcp unless otherwise specified, but I'm sure of the qemu syntax requirements in this area. I assume that since there's only 1 server, the ".0, .1, .2" become unnecessary (something added by commit id 'f1bbc7df4' for multiple gluster hosts). I haven't closedly followed the qemu syntax discussion, but it would it would be possible to use: +file.host=192.168.0.1,file.port=9999 Similar to how NBD (see commit id 'a1674fd9') and SSH (see commit id 'bc225b1b5') are handled.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f7bef51..2a071c9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2319,9 +2319,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> @@ -2329,6 +2329,9 @@ target's name by a slash (e.g., <code>iqn.2013-07.com.example:iscsi-pool/1</code>). If not specified, the default LUN is zero. + For "vxhs" (<span class="since">since 2.5.0</span>), the
Next release is 3.1.0
+ <code>name</code> is the UUID of the volume, assigned by the + HyperScale sever. <span class="since">Since 0.8.7</span> </dd> <dt><code>volume</code></dt> @@ -2431,6 +2434,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 4d76315..cdc39ca 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1470,6 +1470,7 @@ <value>ftp</value> <value>ftps</value> <value>tftp</value> + <value>vxhs</value> </choice> </attribute> <optional> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b569dda..7e12d32 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -604,6 +604,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_GLUSTER: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8e48d2..08bad8e 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: @@ -899,6 +902,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) }
+#define QEMU_DEFAULT_VXHS_PORT "9999" + +/* Build the VxHS host object */ +static virJSONValuePtr +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src) +{ + virJSONValuePtr server = NULL; + virStorageNetHostDefPtr host; + const char *portstr; + + if (src->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("VxHS supports only one server"));
make syntax-check failure here - need a format, e.g. virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ goto cleanup; + } + + host = src->hosts; + portstr = host->port; + + if (!portstr) + portstr = QEMU_DEFAULT_VXHS_PORT; + + if (virJSONValueObjectCreate(&server, + "s:host", host->name, + "s:port", portstr, + NULL) < 0) + server = NULL; + + cleanup: + return server; +} + + +static virJSONValuePtr +qemuBuildVxHSDriveJSON(virStorageSourcePtr src) +{ + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); + virJSONValuePtr server = NULL; + virJSONValuePtr ret = NULL; + + if (!(server = qemuBuildVxHSDriveJSONHost(src))) + return NULL; + + /* VxHS disk sepecification example:
"specification"
+ * { driver:"vxhs", + * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251", + * server.host:"1.2.3.4", + * server.port:1234} + */ + if (virJSONValueObjectCreate(&ret, + "s:driver", protocol, + "s:vdisk-id", src->path, + "a:server", server, NULL) < 0)
If you're going with the 1 host only model, then I believe this isn't necessary. For the Gluster code there's a difference based solely on whether >1 hosts are present whether the "server:" syntax is added or the legacy syntax is used. If you're not ever going to allow multiple hosts, the "server" option would seem to be unnecessary... Rather I would think you should just be able to add "s:host" and "s:port" type options.
+ virJSONValueFree(server); + + return ret; +} + + static char * qemuBuildNetworkDriveURI(virStorageSourcePtr src, qemuDomainSecretInfoPtr secinfo) @@ -1034,6 +1096,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;
@@ -1148,6 +1211,11 @@ qemuGetDriveSourceProps(virStorageSourcePtr src, if (!(fileprops = qemuBuildGlusterDriveJSON(src))) return -1; } + + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) { + if (!(fileprops = qemuBuildVxHSDriveJSON(src))) + return -1; + } break; }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b359e77..c76e9d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13788,6 +13788,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 " @@ -13851,6 +13852,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 " @@ -13996,6 +13998,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 405e655..aab287a 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk) return -1; }
+static int +qemuParseVxHSString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src->path))) + return -1; + + return qemuParseDriveURIString(def, uri, "vxhs"); +} +
/* * This method takes a string representing a QEMU command line ARGV set @@ -737,6 +748,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; } @@ -1947,6 +1964,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; } @@ -2023,6 +2044,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 ce6d213..e62f4e6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -85,7 +85,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", @@ -2633,6 +2634,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); @@ -2964,6 +2966,64 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, }
+static int +virStorageSourceParseBackingJSONVXHS(virStorageSourcePtr src,
s/VXHS/VxHS (to be consistent)
+ virJSONValuePtr json, + int opaque ATTRIBUTE_UNUSED)
These two arguments need to be aligned under the 'v' instead of under the '('
+{ + virJSONValuePtr server; + const char *hostname, *port;
Single lines for each and follow existing model - e.g. move the lines below setting these to here.
+ const char *uri = virJSONValueObjectGetString(json, "filename"); + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id"); + + src->type = VIR_STORAGE_TYPE_NETWORK; + src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; + + /* legacy URI based syntax passed via 'filename' option */ + if (uri) + return virStorageSourceParseBackingJSONUriStr(src, uri, + VIR_STORAGE_NET_PROTOCOL_VXHS);
Since we don't want to support the legacy option - this becomes a failure path with some sort of error indicating unsupported format. For the other protocols, the legacy exists because 'filename' existed prior to when the "new" syntax support was added so both need to be supported.
+ + server = virJSONValueObjectGetObject(json, "server"); + hostname = virJSONValueObjectGetString(server, "host"); + port = virJSONValueObjectGetString(server, "port");
Each of these should be set above similar to how each of the other helpers do this. Although whether "server" is necessary depends on the multiple hosts decision. The *JSONGluster uses it because the legacy syntax supports only one "host", while the non legacy syntax supports having multiple hosts (although it's really not well described in/from commit id '7b7da9e2833").
+ + if (!vdisk_id) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'vdisk-id' attribute in " + "JSON backing definition for VxHS volume")); + return -1; + } + if (!server) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'server' attribute in " + "JSON backing definition for VxHS volume")); + return -1; + } + if (!hostname) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing hostname for tcp backing server in " + "JSON backing definition for VxHS volume")); + return -1; + }
You could combine these to be like others and just have one error message.
+ + + if (VIR_STRDUP(src->path, vdisk_id) < 0) + return -1; + + if (VIR_ALLOC_N(src->hosts, 1) < 0) + return -1; + src->nhosts = 1; + + src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + + if (VIR_STRDUP(src->hosts[0].name, hostname) < 0 || + VIR_STRDUP(src->hosts[0].port, port) < 0) + return -1; + + return 0; +} + struct virStorageSourceJSONDriverParser { const char *drvname; int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque); @@ -2985,6 +3045,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = { {"sheepdog", virStorageSourceParseBackingJSONSheepdog, 0}, {"ssh", virStorageSourceParseBackingJSONSSH, 0}, {"rbd", virStorageSourceParseBackingJSONRBD, 0}, + {"vxhs", virStorageSourceParseBackingJSONVXHS, 0}, };
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1f62244..e60b6e9 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -134,6 +134,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/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 18d9fe3..9fe24d6 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -942,6 +942,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src) case VIR_STORAGE_NET_PROTOCOL_GLUSTER: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml new file mode 100644 index 0000000..660bcde --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml @@ -0,0 +1,35 @@ +<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'/> + <host name='192.168.0.2' 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/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args new file mode 100644 index 0000000..23045e1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args @@ -0,0 +1,25 @@ +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.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ +file.server.host=192.168.0.1,file.server.port=9999,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 ab3ad08..f751ec9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -892,6 +892,8 @@ 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_FAILURE("disk-drive-network-vxhs-multi-host", NONE); DO_TEST("disk-drive-no-boot", QEMU_CAPS_BOOTINDEX); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f766df1..a28fbd9 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1492,6 +1492,22 @@ mymain(void) "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\"," + "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\"" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='192.168.0.1' port='9999'/>\n" + "</source>\n"); + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\"," + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\"," + "\"server\": { \"host\":\"example.com\"," + "\"port\":\"1234\"" + "}" + "}" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='example.com' port='1234'/>\n" + "</source>\n");
Since the desire is to not support the older "driver=vxhs,..." syntax that means this second example would result in an error. If you wanted to "check" that it does error, then create a TEST_BACKING_PARSE_ERROR macro that *expects* virTestRun to fail and use the older syntax with that. John
cleanup: /* Final cleanup */
Thanks for the review! My inputs on some of the comments - On Wed, Jan 25, 2017 at 7:59 AM, John Ferlan <jferlan@redhat.com> 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.
I am testing with changed code to return error if legacy syntax is found for VxHS. Also added a test case to check for failure on legacy syntax and it seems to pass (test #41 below). Then I added a pass test case to check conversion from new native syntax to XML (test #40 below). That test fails with error 'qemuParseCommandLineDisk:901 : internal error: missing file parameter in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b...' Looks like none of the existing tests in qemuargv2xmltest test for the parsing of new syntax, and qemuParseCommandLineDisk() expects to find 'file=' for a drive or it errors out. If this is true, will it be able to parse the new syntax? Some help here please! Output from the newly added test cases (40 should pass and 41 checks for error) : 40) QEMU ARGV-2-XML disk-drive-network-vxhs ... Got unexpected warning from qemuParseCommandLineString: 2017-01-28 00:57:30.814+0000: 10391: info : libvirt version: 3.0.0 2017-01-28 00:57:30.814+0000: 10391: info : hostname: localhost.localdomain 2017-01-28 00:57:30.814+0000: 10391: error : qemuParseCommandLineDisk:901 : internal error: missing file parameter in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none' libvirt: QEMU Driver error : internal error: missing file parameter in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none' FAILED 41) QEMU ARGV-2-XML disk-drive-network-vxhs-fail ... Got expected error from qemuParseCommandLineString: libvirt: QEMU Driver error : internal error: VxHS protocol does not support URI syntax 'vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251' OK 42) QEMU ARGV-2-XML disk-usb ... OK
#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:
Present understanding is to have only one host. You are right, the "server" part is not necessary. Will have to check with the qemu community on this change.
+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,\
FWIW: I also note there is no ".type=tcp" in your output - so perhaps the "default" is tcp unless otherwise specified, but I'm sure of the qemu syntax requirements in this area. I assume that since there's only 1 server, the ".0, .1, .2" become unnecessary (something added by commit id 'f1bbc7df4' for multiple gluster hosts).
That's correct. TCP is the default.
I haven't closedly followed the qemu syntax discussion, but it would it would be possible to use:
+file.host=192.168.0.1,file.port=9999
That is correct. Above syntax would also work for us. I will pose this suggestion to the qemu community and update with their response.
Similar to how NBD (see commit id 'a1674fd9') and SSH (see commit id 'bc225b1b5') are handled.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f7bef51..2a071c9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2319,9 +2319,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> @@ -2329,6 +2329,9 @@ target's name by a slash (e.g., <code>iqn.2013-07.com.example:iscsi-pool/1</code>). If not specified, the default LUN is zero. + For "vxhs" (<span class="since">since 2.5.0</span>), the
Next release is 3.1.0
+ <code>name</code> is the UUID of the volume, assigned by the + HyperScale sever. <span class="since">Since 0.8.7</span> </dd> <dt><code>volume</code></dt> @@ -2431,6 +2434,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 4d76315..cdc39ca 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1470,6 +1470,7 @@ <value>ftp</value> <value>ftps</value> <value>tftp</value> + <value>vxhs</value> </choice> </attribute> <optional> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b569dda..7e12d32 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -604,6 +604,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_GLUSTER: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8e48d2..08bad8e 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: @@ -899,6 +902,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) }
+#define QEMU_DEFAULT_VXHS_PORT "9999" + +/* Build the VxHS host object */ +static virJSONValuePtr +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src) +{ + virJSONValuePtr server = NULL; + virStorageNetHostDefPtr host; + const char *portstr; + + if (src->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("VxHS supports only one server"));
make syntax-check failure here - need a format, e.g.
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ goto cleanup; + } + + host = src->hosts; + portstr = host->port; + + if (!portstr) + portstr = QEMU_DEFAULT_VXHS_PORT; + + if (virJSONValueObjectCreate(&server, + "s:host", host->name, + "s:port", portstr, + NULL) < 0) + server = NULL; + + cleanup: + return server; +} + + +static virJSONValuePtr +qemuBuildVxHSDriveJSON(virStorageSourcePtr src) +{ + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); + virJSONValuePtr server = NULL; + virJSONValuePtr ret = NULL; + + if (!(server = qemuBuildVxHSDriveJSONHost(src))) + return NULL; + + /* VxHS disk sepecification example:
"specification"
+ * { driver:"vxhs", + * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251", + * server.host:"1.2.3.4", + * server.port:1234} + */ + if (virJSONValueObjectCreate(&ret, + "s:driver", protocol, + "s:vdisk-id", src->path, + "a:server", server, NULL) < 0)
If you're going with the 1 host only model, then I believe this isn't necessary. For the Gluster code there's a difference based solely on whether >1 hosts are present whether the "server:" syntax is added or the legacy syntax is used.
If you're not ever going to allow multiple hosts, the "server" option would seem to be unnecessary... Rather I would think you should just be able to add "s:host" and "s:port" type options.
+ virJSONValueFree(server); + + return ret; +} + + static char * qemuBuildNetworkDriveURI(virStorageSourcePtr src, qemuDomainSecretInfoPtr secinfo) @@ -1034,6 +1096,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;
@@ -1148,6 +1211,11 @@ qemuGetDriveSourceProps(virStorageSourcePtr src, if (!(fileprops = qemuBuildGlusterDriveJSON(src))) return -1; } + + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) { + if (!(fileprops = qemuBuildVxHSDriveJSON(src))) + return -1; + } break; }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b359e77..c76e9d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13788,6 +13788,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 " @@ -13851,6 +13852,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 " @@ -13996,6 +13998,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 405e655..aab287a 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk) return -1; }
+static int +qemuParseVxHSString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src->path))) + return -1; + + return qemuParseDriveURIString(def, uri, "vxhs"); +} +
/* * This method takes a string representing a QEMU command line ARGV set @@ -737,6 +748,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; } @@ -1947,6 +1964,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; } @@ -2023,6 +2044,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 ce6d213..e62f4e6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -85,7 +85,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", @@ -2633,6 +2634,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); @@ -2964,6 +2966,64 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, }
+static int +virStorageSourceParseBackingJSONVXHS(virStorageSourcePtr src,
s/VXHS/VxHS (to be consistent)
+ virJSONValuePtr json, + int opaque ATTRIBUTE_UNUSED)
These two arguments need to be aligned under the 'v' instead of under the '('
+{ + virJSONValuePtr server; + const char *hostname, *port;
Single lines for each and follow existing model - e.g. move the lines below setting these to here.
+ const char *uri = virJSONValueObjectGetString(json, "filename"); + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id"); + + src->type = VIR_STORAGE_TYPE_NETWORK; + src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; + + /* legacy URI based syntax passed via 'filename' option */ + if (uri) + return virStorageSourceParseBackingJSONUriStr(src, uri, + VIR_STORAGE_NET_PROTOCOL_VXHS);
Since we don't want to support the legacy option - this becomes a failure path with some sort of error indicating unsupported format.
For the other protocols, the legacy exists because 'filename' existed prior to when the "new" syntax support was added so both need to be supported.
+ + server = virJSONValueObjectGetObject(json, "server"); + hostname = virJSONValueObjectGetString(server, "host"); + port = virJSONValueObjectGetString(server, "port");
Each of these should be set above similar to how each of the other helpers do this. Although whether "server" is necessary depends on the multiple hosts decision. The *JSONGluster uses it because the legacy syntax supports only one "host", while the non legacy syntax supports having multiple hosts (although it's really not well described in/from commit id '7b7da9e2833").
+ + if (!vdisk_id) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'vdisk-id' attribute in " + "JSON backing definition for VxHS volume")); + return -1; + } + if (!server) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'server' attribute in " + "JSON backing definition for VxHS volume")); + return -1; + } + if (!hostname) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing hostname for tcp backing server in " + "JSON backing definition for VxHS volume")); + return -1; + }
You could combine these to be like others and just have one error message.
+ + + if (VIR_STRDUP(src->path, vdisk_id) < 0) + return -1; + + if (VIR_ALLOC_N(src->hosts, 1) < 0) + return -1; + src->nhosts = 1; + + src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + + if (VIR_STRDUP(src->hosts[0].name, hostname) < 0 || + VIR_STRDUP(src->hosts[0].port, port) < 0) + return -1; + + return 0; +} + struct virStorageSourceJSONDriverParser { const char *drvname; int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque); @@ -2985,6 +3045,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = { {"sheepdog", virStorageSourceParseBackingJSONSheepdog, 0}, {"ssh", virStorageSourceParseBackingJSONSSH, 0}, {"rbd", virStorageSourceParseBackingJSONRBD, 0}, + {"vxhs", virStorageSourceParseBackingJSONVXHS, 0}, };
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1f62244..e60b6e9 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -134,6 +134,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/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 18d9fe3..9fe24d6 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -942,6 +942,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src) case VIR_STORAGE_NET_PROTOCOL_GLUSTER: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml new file mode 100644 index 0000000..660bcde --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml @@ -0,0 +1,35 @@ +<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'/> + <host name='192.168.0.2' 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/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args new file mode 100644 index 0000000..23045e1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args @@ -0,0 +1,25 @@ +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.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ +file.server.host=192.168.0.1,file.server.port=9999,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 ab3ad08..f751ec9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -892,6 +892,8 @@ 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_FAILURE("disk-drive-network-vxhs-multi-host", NONE); DO_TEST("disk-drive-no-boot", QEMU_CAPS_BOOTINDEX); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f766df1..a28fbd9 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1492,6 +1492,22 @@ mymain(void) "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\"," + "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\"" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='192.168.0.1' port='9999'/>\n" + "</source>\n"); + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\"," + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\"," + "\"server\": { \"host\":\"example.com\"," + "\"port\":\"1234\"" + "}" + "}" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='example.com' port='1234'/>\n" + "</source>\n");
Since the desire is to not support the older "driver=vxhs,..." syntax that means this second example would result in an error.
If you wanted to "check" that it does error, then create a TEST_BACKING_PARSE_ERROR macro that *expects* virTestRun to fail and use the older syntax with that.
John
cleanup: /* Final cleanup */
On 01/27/2017 08:43 PM, ashish mittal wrote:
Thanks for the review!
My inputs on some of the comments -
On Wed, Jan 25, 2017 at 7:59 AM, John Ferlan <jferlan@redhat.com> 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.
I am testing with changed code to return error if legacy syntax is found for VxHS. Also added a test case to check for failure on legacy syntax and it seems to pass (test #41 below).
Then I added a pass test case to check conversion from new native syntax to XML (test #40 below). That test fails with error 'qemuParseCommandLineDisk:901 : internal error: missing file parameter in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b...'
The qemu_parse_command.c changes while nice to have weren't even updated when multiple gluster servers were added (e.g. commit id '' or '7b7da9e28') Check the changes to add the new s IOW: This code knows how to parse something like: -drive 'file=gluster+unix:///Volume2/Image?socket=/path/to/sock,format=raw,if=none,id=drive-virtio-disk1' but it's clueless for: -drive file.driver=gluster,file.volume=Volume3,file.path=/Image.qcow2,\ 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,\ if=none,id=drive-virtio-disk2 \ -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk2,\ id=virtio-disk2 See
Looks like none of the existing tests in qemuargv2xmltest test for the parsing of new syntax, and qemuParseCommandLineDisk() expects to find 'file=' for a drive or it errors out. If this is true, will it be able to parse the new syntax? Some help here please!
Output from the newly added test cases (40 should pass and 41 checks for error) :
40) QEMU ARGV-2-XML disk-drive-network-vxhs ... Got unexpected warning from qemuParseCommandLineString: 2017-01-28 00:57:30.814+0000: 10391: info : libvirt version: 3.0.0 2017-01-28 00:57:30.814+0000: 10391: info : hostname: localhost.localdomain 2017-01-28 00:57:30.814+0000: 10391: error : qemuParseCommandLineDisk:901 : internal error: missing file parameter in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none' libvirt: QEMU Driver error : internal error: missing file parameter in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none' FAILED
41) QEMU ARGV-2-XML disk-drive-network-vxhs-fail ... Got expected error from qemuParseCommandLineString: libvirt: QEMU Driver error : internal error: VxHS protocol does not support URI syntax 'vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251' OK 42) QEMU ARGV-2-XML disk-usb ... OK
#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:
Present understanding is to have only one host. You are right, the "server" part is not necessary. Will have to check with the qemu community on this change.
+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,\
FWIW: I also note there is no ".type=tcp" in your output - so perhaps the "default" is tcp unless otherwise specified, but I'm sure of the qemu syntax requirements in this area. I assume that since there's only 1 server, the ".0, .1, .2" become unnecessary (something added by commit id 'f1bbc7df4' for multiple gluster hosts).
That's correct. TCP is the default.
I haven't closedly followed the qemu syntax discussion, but it would it would be possible to use:
+file.host=192.168.0.1,file.port=9999
That is correct. Above syntax would also work for us. I will pose this suggestion to the qemu community and update with their response.
Similar to how NBD (see commit id 'a1674fd9') and SSH (see commit id 'bc225b1b5') are handled.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f7bef51..2a071c9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2319,9 +2319,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> @@ -2329,6 +2329,9 @@ target's name by a slash (e.g., <code>iqn.2013-07.com.example:iscsi-pool/1</code>). If not specified, the default LUN is zero. + For "vxhs" (<span class="since">since 2.5.0</span>), the
Next release is 3.1.0
+ <code>name</code> is the UUID of the volume, assigned by the + HyperScale sever. <span class="since">Since 0.8.7</span> </dd> <dt><code>volume</code></dt> @@ -2431,6 +2434,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 4d76315..cdc39ca 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1470,6 +1470,7 @@ <value>ftp</value> <value>ftps</value> <value>tftp</value> + <value>vxhs</value> </choice> </attribute> <optional> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b569dda..7e12d32 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -604,6 +604,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_GLUSTER: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8e48d2..08bad8e 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: @@ -899,6 +902,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) }
+#define QEMU_DEFAULT_VXHS_PORT "9999" + +/* Build the VxHS host object */ +static virJSONValuePtr +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src) +{ + virJSONValuePtr server = NULL; + virStorageNetHostDefPtr host; + const char *portstr; + + if (src->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("VxHS supports only one server"));
make syntax-check failure here - need a format, e.g.
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ goto cleanup; + } + + host = src->hosts; + portstr = host->port; + + if (!portstr) + portstr = QEMU_DEFAULT_VXHS_PORT; + + if (virJSONValueObjectCreate(&server, + "s:host", host->name, + "s:port", portstr, + NULL) < 0) + server = NULL; + + cleanup: + return server; +} + + +static virJSONValuePtr +qemuBuildVxHSDriveJSON(virStorageSourcePtr src) +{ + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); + virJSONValuePtr server = NULL; + virJSONValuePtr ret = NULL; + + if (!(server = qemuBuildVxHSDriveJSONHost(src))) + return NULL; + + /* VxHS disk sepecification example:
"specification"
+ * { driver:"vxhs", + * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251", + * server.host:"1.2.3.4", + * server.port:1234} + */ + if (virJSONValueObjectCreate(&ret, + "s:driver", protocol, + "s:vdisk-id", src->path, + "a:server", server, NULL) < 0)
If you're going with the 1 host only model, then I believe this isn't necessary. For the Gluster code there's a difference based solely on whether >1 hosts are present whether the "server:" syntax is added or the legacy syntax is used.
If you're not ever going to allow multiple hosts, the "server" option would seem to be unnecessary... Rather I would think you should just be able to add "s:host" and "s:port" type options.
+ virJSONValueFree(server); + + return ret; +} + + static char * qemuBuildNetworkDriveURI(virStorageSourcePtr src, qemuDomainSecretInfoPtr secinfo) @@ -1034,6 +1096,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;
@@ -1148,6 +1211,11 @@ qemuGetDriveSourceProps(virStorageSourcePtr src, if (!(fileprops = qemuBuildGlusterDriveJSON(src))) return -1; } + + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) { + if (!(fileprops = qemuBuildVxHSDriveJSON(src))) + return -1; + } break; }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b359e77..c76e9d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13788,6 +13788,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 " @@ -13851,6 +13852,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 " @@ -13996,6 +13998,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 405e655..aab287a 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk) return -1; }
+static int +qemuParseVxHSString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src->path))) + return -1; + + return qemuParseDriveURIString(def, uri, "vxhs"); +} +
/* * This method takes a string representing a QEMU command line ARGV set @@ -737,6 +748,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; } @@ -1947,6 +1964,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; } @@ -2023,6 +2044,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 ce6d213..e62f4e6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -85,7 +85,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", @@ -2633,6 +2634,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); @@ -2964,6 +2966,64 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, }
+static int +virStorageSourceParseBackingJSONVXHS(virStorageSourcePtr src,
s/VXHS/VxHS (to be consistent)
+ virJSONValuePtr json, + int opaque ATTRIBUTE_UNUSED)
These two arguments need to be aligned under the 'v' instead of under the '('
+{ + virJSONValuePtr server; + const char *hostname, *port;
Single lines for each and follow existing model - e.g. move the lines below setting these to here.
+ const char *uri = virJSONValueObjectGetString(json, "filename"); + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id"); + + src->type = VIR_STORAGE_TYPE_NETWORK; + src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; + + /* legacy URI based syntax passed via 'filename' option */ + if (uri) + return virStorageSourceParseBackingJSONUriStr(src, uri, + VIR_STORAGE_NET_PROTOCOL_VXHS);
Since we don't want to support the legacy option - this becomes a failure path with some sort of error indicating unsupported format.
For the other protocols, the legacy exists because 'filename' existed prior to when the "new" syntax support was added so both need to be supported.
+ + server = virJSONValueObjectGetObject(json, "server"); + hostname = virJSONValueObjectGetString(server, "host"); + port = virJSONValueObjectGetString(server, "port");
Each of these should be set above similar to how each of the other helpers do this. Although whether "server" is necessary depends on the multiple hosts decision. The *JSONGluster uses it because the legacy syntax supports only one "host", while the non legacy syntax supports having multiple hosts (although it's really not well described in/from commit id '7b7da9e2833").
+ + if (!vdisk_id) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'vdisk-id' attribute in " + "JSON backing definition for VxHS volume")); + return -1; + } + if (!server) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'server' attribute in " + "JSON backing definition for VxHS volume")); + return -1; + } + if (!hostname) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing hostname for tcp backing server in " + "JSON backing definition for VxHS volume")); + return -1; + }
You could combine these to be like others and just have one error message.
+ + + if (VIR_STRDUP(src->path, vdisk_id) < 0) + return -1; + + if (VIR_ALLOC_N(src->hosts, 1) < 0) + return -1; + src->nhosts = 1; + + src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + + if (VIR_STRDUP(src->hosts[0].name, hostname) < 0 || + VIR_STRDUP(src->hosts[0].port, port) < 0) + return -1; + + return 0; +} + struct virStorageSourceJSONDriverParser { const char *drvname; int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque); @@ -2985,6 +3045,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = { {"sheepdog", virStorageSourceParseBackingJSONSheepdog, 0}, {"ssh", virStorageSourceParseBackingJSONSSH, 0}, {"rbd", virStorageSourceParseBackingJSONRBD, 0}, + {"vxhs", virStorageSourceParseBackingJSONVXHS, 0}, };
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1f62244..e60b6e9 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -134,6 +134,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/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 18d9fe3..9fe24d6 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -942,6 +942,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src) case VIR_STORAGE_NET_PROTOCOL_GLUSTER: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_NO_SUPPORT, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml new file mode 100644 index 0000000..660bcde --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml @@ -0,0 +1,35 @@ +<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'/> + <host name='192.168.0.2' 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/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args new file mode 100644 index 0000000..23045e1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args @@ -0,0 +1,25 @@ +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.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ +file.server.host=192.168.0.1,file.server.port=9999,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 ab3ad08..f751ec9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -892,6 +892,8 @@ 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_FAILURE("disk-drive-network-vxhs-multi-host", NONE); DO_TEST("disk-drive-no-boot", QEMU_CAPS_BOOTINDEX); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f766df1..a28fbd9 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1492,6 +1492,22 @@ mymain(void) "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\"," + "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\"" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='192.168.0.1' port='9999'/>\n" + "</source>\n"); + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\"," + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\"," + "\"server\": { \"host\":\"example.com\"," + "\"port\":\"1234\"" + "}" + "}" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='example.com' port='1234'/>\n" + "</source>\n");
Since the desire is to not support the older "driver=vxhs,..." syntax that means this second example would result in an error.
If you wanted to "check" that it does error, then create a TEST_BACKING_PARSE_ERROR macro that *expects* virTestRun to fail and use the older syntax with that.
John
cleanup: /* Final cleanup */
[...] Pressed send too soon, sigh.
#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.
I am testing with changed code to return error if legacy syntax is found for VxHS. Also added a test case to check for failure on legacy syntax and it seems to pass (test #41 below).
Then I added a pass test case to check conversion from new native syntax to XML (test #40 below). That test fails with error 'qemuParseCommandLineDisk:901 : internal error: missing file parameter in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b...'
The qemu_parse_command.c changes while nice to have weren't even updated when multiple gluster servers were added (e.g. commit id '' or '7b7da9e28') Check the changes to add the new s
IOW: This code knows how to parse something like:
-drive 'file=gluster+unix:///Volume2/Image?socket=/path/to/sock,format=raw,if=none,id=drive-virtio-disk1'
but it's clueless for:
-drive file.driver=gluster,file.volume=Volume3,file.path=/Image.qcow2,\ 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,\ if=none,id=drive-virtio-disk2 \ -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk2,\ id=virtio-disk2
See
Looks like none of the existing tests in qemuargv2xmltest test for the parsing of new syntax, and qemuParseCommandLineDisk() expects to find 'file=' for a drive or it errors out. If this is true, will it be able to parse the new syntax? Some help here please!
So I wouldn't expect the VxHS code to be able to do that unless you wanted to be adventurous. The good news is that this code is primarily for developers that need to take a qemu command line to generate the libvirt syntax. It has not really been kept up to date with all the most recent command line changes. I started to try over a year ago, but got very side tracked.
Output from the newly added test cases (40 should pass and 41 checks for error) :
40) QEMU ARGV-2-XML disk-drive-network-vxhs ... Got unexpected warning from qemuParseCommandLineString: 2017-01-28 00:57:30.814+0000: 10391: info : libvirt version: 3.0.0 2017-01-28 00:57:30.814+0000: 10391: info : hostname: localhost.localdomain 2017-01-28 00:57:30.814+0000: 10391: error : qemuParseCommandLineDisk:901 : internal error: missing file parameter in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none' libvirt: QEMU Driver error : internal error: missing file parameter in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none' FAILED
41) QEMU ARGV-2-XML disk-drive-network-vxhs-fail ... Got expected error from qemuParseCommandLineString: libvirt: QEMU Driver error : internal error: VxHS protocol does not support URI syntax 'vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251' OK 42) QEMU ARGV-2-XML disk-usb ... OK
#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:
Present understanding is to have only one host. You are right, the "server" part is not necessary. Will have to check with the qemu community on this change.
+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,\
FWIW: I also note there is no ".type=tcp" in your output - so perhaps the "default" is tcp unless otherwise specified, but I'm sure of the qemu syntax requirements in this area. I assume that since there's only 1 server, the ".0, .1, .2" become unnecessary (something added by commit id 'f1bbc7df4' for multiple gluster hosts).
That's correct. TCP is the default.
I haven't closedly followed the qemu syntax discussion, but it would it would be possible to use:
+file.host=192.168.0.1,file.port=9999
That is correct. Above syntax would also work for us. I will pose this suggestion to the qemu community and update with their response.
It's not that important... I was looking for a simplification and generation of only what's required. You can continue using the server syntax - perhaps just leave a note/comment in the code indicating the decision point and move on. [...] John
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.
On 02/01/2017 04:39 AM, Peter Krempa wrote:
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.
Primarily so it's clear to all... There's virStorageSourceParseBackingJSONGluster[Host] helpers which generate the file.server.# specifically for Gluster. So are you advocating making the *Host helper be made more generic to be shared with the virStorageSourceParseBackingJSONVXHS code? Might be painful depending on what the driver supports for network protocols. Or is this more you'd prefer to see the qemu_command code use the "a:servers" syntax for all protocols leaving it up to the util code to create the command line. Having 'file.server.host' syntax isn't a problem per se with/for the VxHS backend - I was noting it didn't seem like it had to be done. It does seem that there are two models - one a multi host server model and the other a single host model that could have generic code. Beyond that is the transport differentiation... I would think that the two existing checks in the *JSONGlusterHost for "!server" and "!socket" would either need to be made generic (removing the gluster in the error message) or they'd need to be made in the caller instead. That would allow each backend parser to decide what it supports (or not). John
participants (4)
-
Ashish Mittal -
ashish mittal
-
John Ferlan -
Peter Krempa