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

[ashish mittal]

Looking at the code, I suspect significant changes will be needed to
support passing TLS arguments for disk devices. Here's what I have
right now -

int
qemuProcessPrepareDomain(virConnectPtr conn,
                         virQEMUDriverPtr driver,
                         virDomainObjPtr vm,
                         unsigned int flags)
{
  ...
    VIR_DEBUG("Prepare chardev source backends for TLS");
    qemuDomainPrepareChardevSource(vm->def, driver);   <== This is
where char devices prepares their TLS...

 <=== This is where we would expect a generic "Prepare disk source
backends for TLS" for all the disk devices
 <=== -- OR -- I can do something specific for VxHS here

    VIR_DEBUG("Add secrets to disks, hostdevs, and chardevs");
    if (qemuDomainSecretPrepare(conn, driver, vm) < 0)     <== perhaps
changes will be needed here also, but we plan to pass the cert
directory, endpoint and ID as plain-text..still would need a place to
save these new values..

        goto cleanup;
  ...
}

I'm thinking, I would also need to extend one of the following
structures to save the TLS related info.

struct _virDomainDiskDef {
}
    --- OR ---
struct _qemuDomainDiskPrivate {
}

Given that adding TLS support for VxHS (and disk devices in general)
will not be trivial, I want to check if this can be taken up at a
later time?

Thanks,
Ashish


On Tue, Apr 11, 2017 at 4:26 PM, ashish mittal <ashmit602 at gmail.com> wrote:
> Thanks for the information. I'll work with this and get back if I get
> stuck somewhere.
>
> My immediate objective is to figure out how to pass the TLS x509
> certificate information to the vxhs block device on the qemu command
> line. I guess I expected some other block device (i.e. NBD) to call
> the qemuBuildTLSx509CommandLine(), but got confused when I did not
> find that...
>
> On Tue, Apr 11, 2017 at 3:47 PM, John Ferlan <jferlan at redhat.com> 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).
>>
>> 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 at redhat.com> 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