[libvirt] [PATCH v5 1/9] Add support for Veritas HyperScale (VxHS) block device protocol
ashish mittal
ashmit602 at gmail.com
Wed Aug 30 00:50:07 UTC 2017
Thanks for all the reviews! I will work on each item and get back.
On Tue, Aug 29, 2017 at 4:00 PM, John Ferlan <jferlan at redhat.com> wrote:
>
> Probably need to beef this up a little bit... I can figure something out.
>
> On 08/29/2017 02:39 AM, Ashish Mittal wrote:
>> Sample XML for a VxHS disk:
>>
>> <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 at veritas.com>
>> ---
>> v5 changelog:
>> (1) Rebased to latest master.
>> (2) Review comments from Peter Krempa on patch v4 1/3 have been addressed
>>
>
> Not all were, but it's also not something simple to figure out.
>
>> v4 changelog:
>> (1) Fixes per review comments from v3.
>> (2) Had to remove a test from the previous version that checked for
>> error when multiple hosts are specified for VxHS device.
>> This started failing virschematest with the error
>> "XML document failed to validate against schema" as the
>> docs/schemas/domain.rng specifies only a single host.
>>
>> 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.
>>
>> 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.
>>
>> docs/formatdomain.html.in | 15 ++++++++---
>> docs/schemas/domaincommon.rng | 13 ++++++++++
>> src/libxl/libxl_conf.c | 1 +
>> src/qemu/qemu_block.c | 60 +++++++++++++++++++++++++++++++++++++++++++
>> src/qemu/qemu_command.c | 9 +++++++
>> src/qemu/qemu_driver.c | 3 +++
>> src/qemu/qemu_parse_command.c | 15 +++++++++++
>> src/util/virstoragefile.c | 40 ++++++++++++++++++++++++++++-
>> src/util/virstoragefile.h | 1 +
>> src/xenconfig/xen_xl.c | 1 +
>> 10 files changed, 154 insertions(+), 4 deletions(-)
>>
>
> Which email address would be "preferred" once this gets finalized - work
> or gmail? I don't care either way - just need to know as that becomes
> part of the git history.
>
Work email please. Thanks!
> The tree is currently "frozen" for the 3.7.0 release, but I think we can
> at least start adding some adjustments for at least the first few
> patches once it thaws for 3.8.0. Once the TLS patches start - some more
> adjustment will be necessary I think - adjustments that you may need to
> make... I can make changes for the first patches as it's mostly simple
> things except for the process of capabilities creation...
>
> FWIW: To reduce the change in this one patch - I took the liberty of
> creating a patch that will be inserted before this one that only creates
> the new symbol and adjusts all the switches appropriately and making you
> the author.
>
> You are missing a patch to set up the capabilities - from Peter's review
> of v4 patch 1/3
> (https://www.redhat.com/archives/libvir-list/2017-June/msg01389.html)
>
> "Additionally since libvirt supports QAPI introspection, this means we
> are now able to detect whether qemu supports vxhs and should report an
> error if it doesn't.
>
> You'll need to add a capability bit in qemu_capabilities.h and the
> detection string in qemu_capabilities.c to virQEMUCapsQMPSchemaQueries[]."
>
> Because libvirt could be run on some host that's not running a QEMU with
> the VxHS code we have to have a mechanism that does the appropriate
> checks to ensure the underlying QEMU supports what we're trying to a
> domain about to be run and generate an appropriate message if it's not
> present.
>
> In any case, all that noted, it seems that this can be accomplished by
> adding the following line to virQEMUCapsQMPSchemaQueries[]:
>
> "blockdev-add/arg-type/+vxhs"
>
> But there will also need to be a patch to add 2.10 replies and xml data.
> Both of these I can do as it's not necessarily intuitively obvious...
>
> So... What I'll do is make some adjustments to this series, then repost
> as a v6 so that you (and others) can look.
>
Thanks! Appreciate the help!
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index fba8cfc..64397d4 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -2520,9 +2520,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>
>> @@ -2530,6 +2530,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 3.8.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>
>> @@ -2632,6 +2635,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 3f56d8f..458b8d8 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -1642,6 +1642,18 @@
>> </element>
>> </define>
>>
>> + <define name="diskSourceNetworkProtocolVxHS">
>> + <element name="source">
>> + <attribute name="protocol">
>> + <choice>
>> + <value>vxhs</value>
>> + </choice>
>> + </attribute>
>> + <attribute name="name"/>
>> + <ref name="diskSourceNetworkHost"/>
>> + </element>
>> + </define>
>> +
>> <define name="diskSourceNetwork">
>> <attribute name="type">
>> <value>network</value>
>> @@ -1652,6 +1664,7 @@
>> <ref name="diskSourceNetworkProtocolRBD"/>
>> <ref name="diskSourceNetworkProtocolHTTP"/>
>> <ref name="diskSourceNetworkProtocolSimple"/>
>> + <ref name="diskSourceNetworkProtocolVxHS"/>
>> </choice>
>> </define>
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index 4416a09..34233a9 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -666,6 +666,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_block.c b/src/qemu/qemu_block.c
>> index 7fb12ea..a4d0160 100644
>> --- a/src/qemu/qemu_block.c
>> +++ b/src/qemu/qemu_block.c
>> @@ -23,6 +23,7 @@
>>
>> #include "viralloc.h"
>> #include "virstring.h"
>> +#include "qemu_alias.h"
>
> This isn't needed yet, so I'll remove it now.
>
>>
>> #define VIR_FROM_THIS VIR_FROM_QEMU
>>
>> @@ -482,6 +483,60 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src)
>> }
>>
>>
>> +static virJSONValuePtr
>> +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src)
>> +{
>> + virJSONValuePtr server = NULL;
>> + virStorageNetHostDefPtr host;
>> + unsigned int port;
>> +
>> + if (src->nhosts != 1) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("protocol VxHS accepts only one host"));
>
> Since other places use the string "VxHS protocol" - I'll swap these.
>
>> + return NULL;
>> + }
>> +
>> + host = src->hosts;
>> + port = host->port;
>> +
>> + if (virJSONValueObjectCreate(&server,
>> + "s:host", host->name,
>> + "u:port", port,
>> + NULL) < 0)
>> + server = NULL;
>> +
>> + return server;
>> +}
>> +
>> +
>> +static virJSONValuePtr
>> +qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr src)
>> +{
>> + const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
>> + virJSONValuePtr server = NULL;
>> + virJSONValuePtr ret = NULL;
>> +
>> + if (!(server = qemuBuildVxHSDriveJSONHost(src)))
>> + return NULL;
>
> I see you took the advice from a previous review and created a helper;
> however, qemuBlockStorageSourceBuildHostsJSONSocketAddress should
> suffice your needs as long as you move the "if (src->nhosts != 1) {"
> check moves into here.
>
> It was created from qemuBuildGlusterDriveJSONHosts as part of commit id
> '7ee3df577'.
>
> Of course converting to it means adding the ".0" to the .args output.
> IIRC, this is something talked about in previous reviews, but now that
> we have a general purpose function - we may as well use it.
>
> It will also create a "server.0.type = tcp" entry which probably
> wouldn't be a bad thing in this case.
>
>> +
>> + /* VxHS disk specification example:
>> + * { driver:"vxhs",
>> + * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251",
>> + * server.host:"1.2.3.4",
>> + * server.port:1234}
>
> I'll modify the above to follow the gluster example and be:
>
> server:[{type:"tcp", host:"1.2.3.4", port:9999}]}
>
> NB: 9999 is just a type-a consistency thing
>
>> + */
>> + if (virJSONValueObjectCreate(&ret,
>> + "s:driver", protocol,
>> + "s:vdisk-id", src->path,
>> + "a:server", server, NULL) < 0) {
>> + virJSONValueFree(server);
>> + ret = NULL;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +
>> /**
>> * qemuBlockStorageSourceGetBackendProps:
>> * @src: disk source
>> @@ -512,6 +567,11 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src)
>> goto cleanup;
>> break;
>>
>> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
>> + if (!(fileprops = qemuBlockStorageSourceGetVxHSProps(src)))
>> + goto cleanup;
>> + break;
>> +
>> case VIR_STORAGE_NET_PROTOCOL_NBD:
>> case VIR_STORAGE_NET_PROTOCOL_RBD:
>> case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index a68ff71..0fd2674 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -991,6 +991,11 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
>> ret = virBufferContentAndReset(&buf);
>> break;
>>
>> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("'VxHS' protocol does not support URI syntax"));
>
> I'll drop the single quotes... the 'ssh' is made to look as if someone
> called virStorageNetProtocolTypeToString to convert src->protocol to a
> string as was done for VIR_STORAGE_NET_PROTOCOL_NBD at the top of this
> switch statement.
>
>> + goto cleanup;
>> +
>> case VIR_STORAGE_NET_PROTOCOL_SSH:
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("'ssh' protocol is not yet supported"));
>> @@ -1325,6 +1330,10 @@ qemuDiskSourceNeedsProps(virStorageSourcePtr src)
>> src->nhosts > 1)
>> return true;
>>
>> + if (actualType == VIR_STORAGE_TYPE_NETWORK &&
>> + src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS)
>> + return true;
>> +
>> return false;
>> }
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 2ba6c80..da28c4f 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -13732,6 +13732,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 "
>> @@ -13795,6 +13796,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 "
>> @@ -13940,6 +13942,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 8cb96a2..6286c2e 100644
>> --- a/src/qemu/qemu_parse_command.c
>> +++ b/src/qemu/qemu_parse_command.c
>> @@ -736,6 +736,11 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
>> if (VIR_STRDUP(def->src->path, vdi) < 0)
>> goto error;
>> }
>> + } else if (STRPREFIX(def->src->path, "vxhs:")) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("VxHS protocol does not support URI syntax '%s'"),
>> + def->src->path);
>> + goto error;
>> } else {
>> def->src->type = VIR_STORAGE_TYPE_FILE;
>> }
>> @@ -1944,6 +1949,10 @@ qemuParseCommandLine(virCapsPtr caps,
>> disk->src->type = VIR_STORAGE_TYPE_NETWORK;
>> disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG;
>> val += strlen("sheepdog:");
>> + } else if (STRPREFIX(val, "vxhs:")) {
>> + disk->src->type = VIR_STORAGE_TYPE_NETWORK;
>> + disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
>> + val += strlen("vxhs:");
>> } else {
>> disk->src->type = VIR_STORAGE_TYPE_FILE;
>> }
>> @@ -2020,6 +2029,12 @@ qemuParseCommandLine(virCapsPtr caps,
>> goto error;
>>
>> break;
>> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("VxHS protocol does not support URI "
>> + "syntax '%s'"), disk->src->path);
>> + goto error;
>> + break;
>
> The break; would never be reached and "theoretically speaking" we should
> never get here anyway since it's not possible to use the "older" format.
> Still at least you modified qemu_parse_command - not many do...
>
> The rest was fine...
>
> John
>
Regards,
Ashish
>
>> 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 fbc8245..e9a59e0 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",
>> @@ -2712,6 +2713,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);
>> @@ -3210,6 +3212,38 @@ virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src,
>> return virStorageSourceParseBackingJSONInternal(src, json);
>> }
>>
>> +static int
>> +virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src,
>> + virJSONValuePtr json,
>> + int opaque ATTRIBUTE_UNUSED)
>> +{
>> + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id");
>> + virJSONValuePtr server = virJSONValueObjectGetObject(json, "server");
>> +
>> + if (!vdisk_id || !server) {
>> + virReportError(VIR_ERR_INVALID_ARG, "%s",
>> + _("missing 'vdisk-id' or 'server' attribute in "
>> + "JSON backing definition for VxHS volume"));
>> + return -1;
>> + }
>> +
>> + src->type = VIR_STORAGE_TYPE_NETWORK;
>> + src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
>> +
>> + if (VIR_STRDUP(src->path, vdisk_id) < 0)
>> + return -1;
>> +
>> + if (VIR_ALLOC_N(src->hosts, 1) < 0)
>> + return -1;
>> + src->nhosts = 1;
>> +
>> + if (virStorageSourceParseBackingJSONInetSocketAddress(src->hosts,
>> + server) < 0)
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> struct virStorageSourceJSONDriverParser {
>> const char *drvname;
>> int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque);
>> @@ -3232,6 +3266,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = {
>> {"ssh", virStorageSourceParseBackingJSONSSH, 0},
>> {"rbd", virStorageSourceParseBackingJSONRBD, 0},
>> {"raw", virStorageSourceParseBackingJSONRaw, 0},
>> + {"vxhs", virStorageSourceParseBackingJSONVxHS, 0},
>> };
>>
>>
>> @@ -3992,6 +4027,9 @@ virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol)
>> /* we don't provide a default for RBD */
>> return 0;
>>
>> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
>> + return 9999;
>> +
>> case VIR_STORAGE_NET_PROTOCOL_LAST:
>> case VIR_STORAGE_NET_PROTOCOL_NONE:
>> return 0;
>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
>> index 6c388b1..f7e897f 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 d168d3f..8acbfe3 100644
>> --- a/src/xenconfig/xen_xl.c
>> +++ b/src/xenconfig/xen_xl.c
>> @@ -1024,6 +1024,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,
>>
More information about the libvir-list
mailing list