[libvirt] [PATCH 1/1] qemu: add support for multiple gluster hosts
Deepak C Shetty
deepakcs at redhat.com
Mon Oct 5 13:29:54 UTC 2015
On 10/05/2015 04:31 PM, Peter Krempa wrote:
> On Mon, Oct 05, 2015 at 15:30:42 +0530, Prasanna Kumar Kalever wrote:
>> currently libvirt has the capability to parse only one host and convert that
>> into URI formatted string, with the help of this patch libvirt will be able
>> to parse multiple hosts from the domain xml and can convert that into JSON
>> formatted string
>>
>> before:
>> ------
>> example.xml:
>> ...
>> <disk type='network' device='disk'>
>> <driver name='qemu' type='qcow2' cache='none'/>
>> <source protocol='gluster' name='testvol/a.qcow2'>
>> <host name='1.2.3.4' port='24007' transport="tcp"/>
>> </source>
>> <target dev='vda' bus='virtio'/>
>> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
>> </disk>
>> ...
>>
>> resultant string:
>> file=gluster://1.2.3.4:24007/testvol/a.qcow2, \
>> if=none,id=drive-virtio-disk0,format=qcow2,cache=none
>>
>> after:
>> -----
>> example.xml:
>> ...
>> <disk type='network' device='disk'>
>> <driver name='qemu' type='qcow2' cache='none'/>
>> <source protocol='gluster' name='testvol/a.qcow2'>
>> <host name='1.2.3.4' port='24009' transport="tcp"/>
>> <host name='3.4.5.6' port="24008"/>
>> <host name='5.6.7.8' />
>> </source>
>> <target dev='vda' bus='virtio'/>
>> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
>> </disk>
>> ...
>>
>> resultant string:
>> -drive file=json:{
>> "file": {
>> "driver": "gluster",,
>> "volname": "testvol",,
>> "image-path": "/a.qcow2",,
>> "volfile-servers": [
>> {
>> "server": "1.2.3.4",,
>> "port": 24009,,
>> "transport": "tcp"
>> },,
>> {
>> "server": "3.4.5.6",,
>> "port": 24008,,
>> "transport": "tcp"
>> },,
>> {
>> "server": "5.6.7.8",,
>> "port": 24007,,
> The double commas look like a result of our command line escaping
> function. Are they actually required with 'json:' sources? If no, we
> will need probably a way to avoid them.
>
>> "transport": "tcp"
>> }
>> ]
>> },,
>> "driver": "qcow2"
>> }
>> ,if=none,id=drive-virtio-disk0,cache=none
>>
>> if the number of hosts supplied is only one, then we use existing URI format
>> for backward compatibility and if number of hosts is greater than one we switch
>> to the new JSON format
>>
>> this patch requires qemu latest patch
>> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg07062.html
> So this patch, if it will be acked needs to wait until qemu accepts the
> patch as final.
>
>> Credits: Sincere thanks to Kevin Wolf <kwolf at redhat.com> and
>> "Deepak C Shetty" <deepakcs at redhat.com> for their support
>>
>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever at redhat.com>
>> ---
>> src/qemu/qemu_command.c | 192 ++++++++++++++++++++++++++++++++++++------------
>> 1 file changed, 145 insertions(+), 47 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index bb1835c..8c1bf1a 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -3256,16 +3256,106 @@ qemuNetworkDriveGetPort(int protocol,
>> return -1;
>> }
>>
>> +#define QEMU_DEFAULT_GLUSTER_PORT 24007
> qemuNetworkDriveGetPort can be possibly improved to include this too,
> instead of the define ... [1]
>
>> +
>> +static virJSONValuePtr
>> +qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
>> +{
>> + virJSONValuePtr parent = NULL;
>> + virJSONValuePtr object = NULL;
>> + virJSONValuePtr dict_array = NULL;
>> + int port;
>> + int i;
> This won't pass syntax-check. Please make sure that you'll run it before
> posting patches.
>
>> +
>> + if (!(parent = virJSONValueNewObject()))
>> + return NULL;
>> + /* 1. prepare { driver:"gluster", volname:"testvol", image-path:"/a.img"} */
>> + if (virJSONValueObjectAdd(parent,
>> + "s:driver", virStorageNetProtocolTypeToString(src->protocol),
>> + "s:volname", src->volume,
>> + "s:image-path", src->path,
>> + NULL) < 0)
>> + goto cleanup;
>> +
>> + if (!(dict_array = virJSONValueNewArray()))
>> + goto cleanup;
>> + /* 2. prepare array [ {server:"1.2.3.4", port:24007, transport:"tcp"} ,
>> + * {server:"5.6.7.8", port:24008, transport:"rdma"},
>> + * {}, ... ] */
>> + for (i = 0; i < src->nhosts; i++) {
>> + if (!(object = virJSONValueNewObject()))
>> + goto cleanup;
>> + port = qemuNetworkDriveGetPort(src->protocol, src->hosts[i].port);
>> + if (virJSONValueObjectAdd(object, "s:server", src->hosts[i].name,
>> + "i:port", port ? port : QEMU_DEFAULT_GLUSTER_PORT ,
> [1] and a ugly ternary operator.
>
>> + "s:transport",
>> + virStorageNetHostTransportTypeToString(src->hosts[i].transport),
>> + NULL) < 0)
>> + goto cleanup;
>> + if (virJSONValueArrayAppend(dict_array, object) < 0) {
>> + virJSONValueFree(object);
> The above statement is already in the cleanup section.
>
>> + goto cleanup;
>> + }
>> + }
>> + /* 3. append 1 and 2
>> + * { driver:"gluster", volname:"testvol", image-path:"/a.img",
>> + * volfile-servers :[ {server:"1.2.3.4", port:24007, transport:"tcp"} ,
>> + * {server:"5.6.7.8", port:24008, transport:"rdma"},
>> + * {}, ... ] }
>> + */
>> + if (virJSONValueObjectAppend(parent, "volfile-servers", dict_array) < 0) {
>> + virJSONValueFree(dict_array);
> The above statement is already in the cleanup section.
>
>> + goto cleanup;
>> + }
>> + /* 4. assign key to 3
>> + * { file: { driver:"gluster", volname:"testvol", image-path:"/a.img",
>> + * volfile-servers :[ {server:"1.2.3.4", port:24007, transport:"tcp"} ,
>> + * {server:"5.6.7.8", port:24008, transport:"rdma"},
>> + * {}, ... ] } }
>> + */
>> + if (!(object = virJSONValueNewObject()))
>> + goto cleanup;
>> + if (virJSONValueObjectAppend(object, "file", parent) < 0) {
>> + virJSONValueFree(parent);
> Same thing.
>
>> + goto cleanup;
>> + }
>> + /* 5. append format to 4
>> + * { file: { driver:"gluster", volname:"testvol", image-path:"/a.img",
>> + * volfile-servers :[ {server:"1.2.3.4", port:24007, transport:"tcp"} ,
>> + * {server:"5.6.7.8", port:24008, transport:"rdma"},
>> + * {}, ... ] }, driver:"qcow2" }
>> + */
>> + if (virJSONValueObjectAdd(object,
>> + "s:driver", virStorageFileFormatTypeToString(src->format),
>> + NULL) < 0)
>> + goto cleanup;
>> + else
>> + /* since we have already captured the format type, let's make it '0' to
>> + * avoid further checks for format information
> NACK to this, this would remove it from the live definition and it would
> disappear from the live XML. That can't happen. Various block job
> operations and a lot of other code depend on knowing the format.
>
> Also this violates the code style since the "body" of the else statement
> is multi-line due to the comment so both paths need a block. Also,
> since the true part jumps to 'cleanup' you can remove else completely to
> make the code more unambiguous.
>
>> + */
>> + src->format = 0;
>> +
>> + return object;
>> +
>> +cleanup:
>> + virJSONValueFree(dict_array);
>> + virJSONValueFree(parent);
>> + virJSONValueFree(object);
>> + return NULL;
>> +}
>> +
>> #define QEMU_DEFAULT_NBD_PORT "10809"
>>
>> static char *
>> -qemuBuildNetworkDriveURI(virStorageSourcePtr src,
>> +qemuBuildNetworkDriveStr(virStorageSourcePtr src,
>> const char *username,
>> const char *secret)
>> {
>> char *ret = NULL;
>> + char *str = NULL;
>> virBuffer buf = VIR_BUFFER_INITIALIZER;
>> virURIPtr uri = NULL;
>> + virJSONValuePtr json = NULL;
>> size_t i;
>>
>> switch ((virStorageNetProtocol) src->protocol) {
>> @@ -3331,61 +3421,67 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
>> case VIR_STORAGE_NET_PROTOCOL_TFTP:
>> case VIR_STORAGE_NET_PROTOCOL_ISCSI:
>> case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
> So this is done for all protocols that originally use an URI ...
>
>> - if (src->nhosts != 1) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("protocol '%s' accepts only one host"),
>> - virStorageNetProtocolTypeToString(src->protocol));
>> - goto cleanup;
>> - }
>> -
>> - if (VIR_ALLOC(uri) < 0)
>> - goto cleanup;
>> -
>> - if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
>> - if (VIR_STRDUP(uri->scheme,
>> - virStorageNetProtocolTypeToString(src->protocol)) < 0)
>> + if (src->nhosts == 1) {
>> + /* URI syntax generation */
>> + if (VIR_ALLOC(uri) < 0)
>> goto cleanup;
>> - } else {
>> - if (virAsprintf(&uri->scheme, "%s+%s",
>> - virStorageNetProtocolTypeToString(src->protocol),
>> - virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0)
>> - goto cleanup;
>> - }
>> -
>> - if ((uri->port = qemuNetworkDriveGetPort(src->protocol, src->hosts->port)) < 0)
>> - goto cleanup;
>>
>> - if (src->path) {
>> - if (src->volume) {
>> - if (virAsprintf(&uri->path, "/%s%s",
>> - src->volume, src->path) < 0)
>> + if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
>> + if (VIR_STRDUP(uri->scheme,
>> + virStorageNetProtocolTypeToString(src->protocol)) < 0)
>> goto cleanup;
>> } else {
>> - if (virAsprintf(&uri->path, "%s%s",
>> - src->path[0] == '/' ? "" : "/",
>> - src->path) < 0)
>> + if (virAsprintf(&uri->scheme, "%s+%s",
>> + virStorageNetProtocolTypeToString(src->protocol),
>> + virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0)
>> goto cleanup;
>> }
>> - }
>>
>> - if (src->hosts->socket &&
>> - virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0)
>> - goto cleanup;
>> + if ((uri->port = qemuNetworkDriveGetPort(src->protocol, src->hosts->port)) < 0)
>> + goto cleanup;
>>
>> - if (username) {
>> - if (secret) {
>> - if (virAsprintf(&uri->user, "%s:%s", username, secret) < 0)
>> - goto cleanup;
>> - } else {
>> - if (VIR_STRDUP(uri->user, username) < 0)
>> - goto cleanup;
>> + if (src->path) {
>> + if (src->volume) {
>> + if (virAsprintf(&uri->path, "/%s%s",
>> + src->volume, src->path) < 0)
>> + goto cleanup;
>> + } else {
>> + if (virAsprintf(&uri->path, "%s%s",
>> + src->path[0] == '/' ? "" : "/",
>> + src->path) < 0)
>> + goto cleanup;
>> + }
>> }
>> - }
>>
>> - if (VIR_STRDUP(uri->server, src->hosts->name) < 0)
>> - goto cleanup;
>> + if (src->hosts->socket &&
>> + virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0)
>> + goto cleanup;
>>
>> - ret = virURIFormat(uri);
>> + if (username) {
>> + if (secret) {
>> + if (virAsprintf(&uri->user, "%s:%s", username, secret) < 0)
>> + goto cleanup;
>> + } else {
>> + if (VIR_STRDUP(uri->user, username) < 0)
>> + goto cleanup;
>> + }
>> + }
>> +
>> + if (VIR_STRDUP(uri->server, src->hosts->name) < 0)
>> + goto cleanup;
>> +
>> + ret = virURIFormat(uri);
>> + } else {
>> + /* switch to new json formated syntax */
> ... but you do Gluster here unconditionally. This would actually turn a
> HTTP (or other) disk definition into a gluster source. That can't
> happen, this either needs to go into a separate section under the
> VIR_STORAGE_NET_PROTOCOL_GLUSTER or the JSON generator needs to be able
> to generate json for all the protocols.
>
>> + if(!(json = qemuBuildGlusterDriveJSON(src)))
>> + goto cleanup;
>> +
>> + if (!(str = virJSONValueToString(json, false)))
>> + goto cleanup;
>> +
>> + if (virAsprintf (&ret, "json:%s", str) < 0)
>> + goto cleanup;
>> + }
>>
>> break;
>>
> So while all the above looks pretty okay at this point (except for
> clearing the image type) this patch is incomplete.
>
> Since libvirt is loading the backing chain to be able to traverse it and
> set correct permissions on individual images. This equals to two
> additional parts of the code that need modification:
>
> 1) virStorageFileBackendGlusterInit has the same "src->nhosts != 1"
> condition and is not prepared to work with multiple protocols
IIUC this is part of the storage pool feature of libvirt and that can be
handled
as part of a separate patch. Using gluster as a block backend for
qemudoesn't
enforce using gluster as a file backend for libvirt storage pool, they
both are
separate, IMHO
>
> 2) I presume (I didn't test it yet) that the qemu patch that adds this
> stuff will result into using the "json:{" protocol string in the
> 'backing_store' field once you do a snapshot of a gluster disk that uses
> this new syntax. If that happens the VM will not be able to start in
> libvirt any more, as libvirt does not have a 'json:{' protocol parser to
> parse the backing store.
We tried to take a disk-only external snapshot with libvirtd running with
this patch and I was unable to see the "json:{..." format for backing file.
I think we understand what you are saying, but we are unable to reproduce
the problem ( of having the json way in backing file). This is what we
did ....
* Domain HFM running with CentOs7.qcow2 as a network disk type w/
protocol=gluster
0 :) dhcp1-86 ~/deepakcs $ virsh list --all
Id Name State
----------------------------------------------------
4 HFM running
* Create a qcow2 file for use with external snapshot
qemu-img create -f qcow2 -b gluster://10.70.1.86/sample/CentOs7.qcow2
gluster://10.70.1.86/sample/newsnap.qcow2
* Create a domainsnapshot XML (snap.xml)
<domainsnapshot>
<disks>
<disk name="vda" snapshot="external" type="network">
<source protocol="gluster" name="sample/newsnap.qcow2">
<host name="10.70.1.86" port="24007"/>
<host name="10.70.1.88" port="24007"/>
<host name="10.70.1.87" port="24007"/>
</source>
</disk>
</disks>
</domainsnapshot>
* Run virsh snap cmd (error in bold)
0 :) dhcp1-86 ~/deepakcs $ virsh snapshot-create HFM --xmlfile
./snap.xml --disk-only --reuse-external
*error: internal error: missing storage backend for network files using
gluster protocol*
* Run virsh snap cmd w/ domain is off (error in bold)
0 :) dhcp1-86 ~/deepakcs $ virsh list --all
Id Name State
----------------------------------------------------
- HFM shut off
0 :) dhcp1-86 ~/deepakcs $ virsh snapshot-create HFM --xmlfile
./snap.xml --disk-only --reuse-external
*error: internal error: external inactive snapshots are not supported on
'network' disks using 'gluster' protocol*
>
> So in order to accept this patch, the helpers in
> src/util/virstoragefile.c (virStorageSourceParseBackingURI) will need to
> be made modular so that they can accept a driver specific callback, that
> will then parse the 'json:{' source definitions. (We can't just hardcode
> it there, since src/util "should" not contain any qemu specific code.)
Questions:
1) Why does snapshot doesn't work, is it bcos the support for json
is not present in the backup parse code ?
The error msg seen above, isn't clear
2) IIUC, you mean that the backing file will be in the json format
something like ...
backing file: json"{...}" in `qemu-img info --backing-chain`
output ?
How to create such a scenario, so that we can "see" the problem
happening and hence debug/fix it :)
thanx,
deepak
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151005/0b8efc03/attachment-0001.htm>
More information about the libvir-list
mailing list