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@xxxxxxxxxx> and
"Deepak C Shetty" <deepakcs@xxxxxxxxxx> for their support
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@xxxxxxxxxx>
---
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 qemu doesn'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
|