Re: [libvirt] [PATCH v3] Add support for Veritas HyperScale (VxHS) block device protocol

On Tue, Apr 11, 2017 at 3:47 PM, John Ferlan <jferlan(a)redhat.com&gt; wrote: > > > On 04/10/2017 07:32 PM, ashish mittal wrote: >> Hi, >> >> I'm trying to figure out what changes are needed in the libvirt vxhs >> patch to support passing TLS X509 arguments to qemu, similar to the >> following - >> >> Sample QEMU command line passing TLS credentials to the VxHS block >> device (run in secure mode): >> ./qemu-io --object >> tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read >> -v 66000 2.5k' 'json:{"server.host": "127.0.0.1", "server.port": "9999", >> "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}' >> >> I was hoping to find some NBD code related to this, but not able to >> locate it. Any pointers will be appreciated. > > Well you have a couple of things to deal with... There's the creation of > the TLS object and there's altering the parameters used for the qemu > command based on your needs/model. > > First off you'll need to figure out where/how you're going to define > where the TLS creds exist. For that, I suspect you'll have code similar > to how chardevTLS support was added. Essentially some way to either use > an existing TLS environment or a way to allow someone to define a vxhs > specific environment (hint, see src/qemu/qemu_conf.c, src/qemu/qemu.conf > - I've made changes recently there too). > > For the TLS object creation on the command line, see > qemuBuildTLSx509CommandLine to see how the code builds the > "tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client" portion > of your command line. > > I forget if hot plug was in your plan, but see qemuDomainGetTLSObjects, > qemuDomainAddTLSObjects, and qemuDomainDelTLSObjects for that. > > The rest of the command line is going to be a bit tricky since using the > "newer" driver syntax for libvirt is "sparse". Traditionally libvirt has > used "-drive file=[$uri:]$path,format=$driver,..." (use grep "\-drive > file" tests/*/*.args from a libvirt git directory - you can grep that > output for gluster or rbd to see the uri format). > > IIUC the qemu changes correctly though, you cannot use that "file=" > syntax, instead you'll need to format the command line similar to how > things were done for gluster to add multiple host support where the > syntax is "-drive 'file.driver=gluster,file.volume=..." (use grep > "\-drive file.driver" tests/*/*.args to see how this is done for gluster). > Hi! Thanks for explaining how TLS could work for block devices in general, and specifically for VxHS. I agree that the plan should be to first have the base VxHS patch ready (sans TLS support), and then build the TLS support in a series of patches on top of the base patch. To that end, I have made changes to v3 to fix the problems pointed out in this series and plan to submit the new patch soon. I do have some confusion regarding your comment above - I thought I have already implemented the new syntax similar to gluster's "-drive 'file.driver=gluster,file.volume=..." in this v3 patch series. >>> v3 changelog: >>> (1) Implemented the modern syntax for VxHS disk specification. >>> (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax. Please see the newly added xml2argv test +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args +-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,\ Let me know if I'm still missing something on the new syntax. Thanks!

> That code/support was added in a series starting at commit id '22ad4a7c' > and working your way forward through about 18 patches. Using a visual > tool like gitk helps a lot... > > I think what will be easiest is to start at that commit and look "up" > for gluster specific changes. Be careful not to fully cut-n-paste > because there have been patches since that time to fix some issues with > the initial implementation. I point it out only as a way for you to see > which modules and where "similar" code exists. > > You'll also note there is an nbd patch in that series of patches - not > sure how much that helps, but it perhaps gives you some amount of > guidelines. Although I don't believe nbd was added to the command line - > it was just a way of syntax generation/testing. > > > John > >> >> Thanks, >> Ashish >> >> On Wed, Feb 1, 2017 at 8:36 AM, John Ferlan <jferlan(a)redhat.com&gt; wrote: >>> [...] >>> 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