<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <tt> </tt><tt> </tt> <div class="moz-cite-prefix"><tt>On 10/05/2015 04:31 PM, Peter Krempa wrote:</tt><tt> </tt></div> <blockquote cite="mid:20151005110127.GC4876@andariel.pipo.sk" type="cite"> <pre wrap="">On Mon, Oct 05, 2015 at 15:30:42 +0530, Prasanna Kumar Kalever wrote: </pre> <blockquote type="cite"> <pre wrap="">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:{ </pre> </blockquote> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> "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,, </pre> </blockquote> <pre wrap=""> 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. </pre> <blockquote type="cite"> <pre wrap=""> "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 <a class="moz-txt-link-freetext" href="https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg07062.html">https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg07062.html</a> </pre> </blockquote> <pre wrap=""> So this patch, if it will be acked needs to wait until qemu accepts the patch as final. </pre> <blockquote type="cite"> <pre wrap=""> Credits: Sincere thanks to Kevin Wolf <a class="moz-txt-link-rfc2396E" href="mailto:kwolf@redhat.com"><kwolf@redhat.com></a> and "Deepak C Shetty" <a class="moz-txt-link-rfc2396E" href="mailto:deepakcs@redhat.com"><deepakcs@redhat.com></a> for their support Signed-off-by: Prasanna Kumar Kalever <a class="moz-txt-link-rfc2396E" href="mailto:prasanna.kalever@redhat.com"><prasanna.kalever@redhat.com></a> --- 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 </pre> </blockquote> <pre wrap=""> qemuNetworkDriveGetPort can be possibly improved to include this too, instead of the define ... [1] </pre> <blockquote type="cite"> <pre wrap="">+ +static virJSONValuePtr +qemuBuildGlusterDriveJSON(virStorageSourcePtr src) +{ + virJSONValuePtr parent = NULL; + virJSONValuePtr object = NULL; + virJSONValuePtr dict_array = NULL; + int port; + int i; </pre> </blockquote> <pre wrap=""> This won't pass syntax-check. Please make sure that you'll run it before posting patches. </pre> <blockquote type="cite"> <pre wrap="">+ + 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 , </pre> </blockquote> <pre wrap=""> [1] and a ugly ternary operator. </pre> <blockquote type="cite"> <pre wrap="">+ "s:transport", + virStorageNetHostTransportTypeToString(src->hosts[i].transport), + NULL) < 0) + goto cleanup; + if (virJSONValueArrayAppend(dict_array, object) < 0) { + virJSONValueFree(object); </pre> </blockquote> <pre wrap=""> The above statement is already in the cleanup section. </pre> <blockquote type="cite"> <pre wrap="">+ 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); </pre> </blockquote> <pre wrap=""> The above statement is already in the cleanup section. </pre> <blockquote type="cite"> <pre wrap="">+ 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); </pre> </blockquote> <pre wrap=""> Same thing. </pre> <blockquote type="cite"> <pre wrap="">+ 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 </pre> </blockquote> <pre wrap=""> 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. </pre> <blockquote type="cite"> <pre wrap="">+ */ + 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: </pre> </blockquote> <pre wrap=""> So this is done for all protocols that originally use an URI ... </pre> <blockquote type="cite"> <pre wrap="">- 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 */ </pre> </blockquote> <pre wrap=""> ... 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. </pre> <blockquote type="cite"> <pre wrap="">+ if(!(json = qemuBuildGlusterDriveJSON(src))) + goto cleanup; + + if (!(str = virJSONValueToString(json, false))) + goto cleanup; + + if (virAsprintf (&ret, "json:%s", str) < 0) + goto cleanup; + } break; </pre> </blockquote> <pre wrap=""> 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</pre> </blockquote> <tt> </tt><tt>IIUC this is part of the storage pool feature of libvirt and that can be handled</tt><tt> </tt><tt>as part of a separate patch. Using gluster as a block backend for qemu</tt><tt> doesn't</tt><tt> </tt><tt>enforce using gluster as a file backend for libvirt storage pool, they both are</tt><tt> </tt><tt>separate, IMHO</tt><tt> </tt><tt> </tt> <blockquote cite="mid:20151005110127.GC4876@andariel.pipo.sk" type="cite"> <pre wrap=""> 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.</pre> </blockquote> <tt> </tt><tt>We tried to take a disk-only external snapshot with libvirtd running with</tt><tt> </tt><tt>this patch and I was unable to see the "json:{..." format for backing file.</tt><tt> </tt><tt> </tt><tt>I think we understand what you are saying, but we are unable to reproduce</tt><tt> </tt><tt>the problem ( of having the json way in backing file). This is what we did ....</tt><tt> </tt> <tt>* Domain HFM running with CentOs7.qcow2 as a network disk type w/ protocol=gluster</tt><tt> </tt><tt>0 :) dhcp1-86 ~/deepakcs $ virsh list --all</tt><tt> </tt><tt>�Id�� Name�� State</tt><tt> </tt><tt>----------------------------------------------------</tt><tt> </tt><tt>�4�� HFM�� running</tt><tt> </tt><tt> </tt><tt> </tt><tt>* Create a qcow2 file for use with external snapshot</tt><tt> </tt><tt>qemu-img create -f qcow2 -b gluster://10.70.1.86/sample/CentOs7.qcow2 gluster://10.70.1.86/sample/newsnap.qcow2</tt><tt> </tt><tt> </tt><tt>* Create a domainsnapshot XML (snap.xml)</tt><tt> </tt><tt> </tt><tt><domainsnapshot></tt><tt> </tt><tt>�� <disks></tt><tt> </tt><tt>�� <disk name="vda" snapshot="external" type="network"></tt><tt> </tt><tt>�� <source protocol="gluster" name="sample/newsnap.qcow2"></tt><tt> </tt><tt>�� <host name="10.70.1.86" port="24007"/></tt><tt> </tt><tt>�� <host name="10.70.1.88" port="24007"/></tt><tt> </tt><tt>�� <host name="10.70.1.87" port="24007"/></tt><tt> </tt><tt>�� </source></tt><tt> </tt><tt>�� </disk></tt><tt> </tt><tt>�� </disks></tt><tt> </tt><tt></domainsnapshot></tt><tt> </tt><tt> </tt><tt>* Run virsh snap cmd (error in bold)</tt><tt> </tt><tt> 0 :) dhcp1-86 ~/deepakcs $ virsh snapshot-create HFM --xmlfile ./snap.xml --disk-only --reuse-external</tt><tt> </tt><tt>error: internal error: missing storage backend for network files using gluster protocol</tt><tt> </tt><tt> </tt><tt>* Run virsh snap cmd w/ domain is off (error in bold)</tt><tt> </tt><tt>0 :) dhcp1-86 ~/deepakcs $� virsh list --all</tt><tt> </tt><tt>�Id�� Name�� State</tt><tt> </tt><tt>----------------------------------------------------</tt><tt> </tt><tt>�-�� HFM�� shut off</tt><tt> </tt><tt> </tt><tt>0 :) dhcp1-86 ~/deepakcs $ virsh snapshot-create HFM --xmlfile ./snap.xml --disk-only --reuse-external</tt><tt> </tt><tt>error: internal error: external inactive snapshots are not supported on 'network' disks using 'gluster' protocol</tt><tt> </tt><tt> </tt><tt> </tt> <blockquote cite="mid:20151005110127.GC4876@andariel.pipo.sk" type="cite"> <pre wrap=""> 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.)</pre> </blockquote> <tt>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 :) </tt><tt> thanx, deepak </tt><tt></tt> </body> </html>