[libvirt] [PATCH v4 0/9] qemu: Add quorum support to libvirt

[Matthias Gatto]

On Thu, Apr 16, 2015 at 3:43 PM, John Ferlan <jferlan at redhat.com> wrote:
>
>
> On 03/17/2015 03:25 PM, Matthias Gatto wrote:
>> The purpose of these patches is to introduce quorum for libvirt
>> I've try to follow this proposal:
>> http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html
>>
>
>
> Before we reach a year waiting on this and so we make some progress...
>
> I started looking at the series and naturally found conflicts with the
> patches. Not to be deterred I started with the first 5 patches since I
> figured they were mostly setup and not functional change.
>
> Of those patch 2 had the most conflict. Once resolved, patches 3-5
> applied without issue... Patch 6, more conflicts, so I just focused on
> 1-5...
>
> Patch 2 had numerous conflicts in virStorageBackendCreateQemuImgCmd due
> to commit id fbcf7da95b872ac45cabc4356fc9c06e809d0237.
>
> After applying patch 5, I cscope searched on "->backingStore" in order
> to find any 'new' references that might have crept in (ignoring any
> backingStoreRaw references) and found:
>
> f9ea3d60119e82c02c00fbf3678c3ed20634dea1 (qemu_monitor_json.c)
>
> which I adjusted patch 2 in order to keep it in line.
>
> Beyond that in patch 2:
>
>   * In storage_conf.c/*VolDefFormat() - I changed the "&(def->target)"
> to just "&def->target" which was mostly a consistency thing. I found a
> couple of others as well and adjusted them. I don't think it matters,
> since none of the references were "&(x->y)->z"
>
>   * In storage_backend_fs.c/virStorageBackendProbeTarget() - you created
> a *local* backingStore reference and then used that for an assignment,
> but didn't update target->backingStore in at least the first
> case/instance, so since we're not updating the setting in this pass, I
> adjusted the code as follows:
>
> -        if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
> +        backingStore = virStorageSourceGetBackingStore(target, 0);
> +        if (!(backingStore = virStorageSourceNewFromBacking(meta)))
>
> back to setting target->backingStore in the if statement, then fetching
> after the if statement into the local variable:
>
> -        backingStore = virStorageSourceGetBackingStore(target, 0);
> -        if (!(backingStore = virStorageSourceNewFromBacking(meta)))
> +        if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
>              goto cleanup;
>
> +        backingStore = virStorageSourceGetBackingStore(target, 0);
>
>
> I believe the virStorageSourceFree(backingStore) that follows will be OK
> since you'd be replacing it anyway in the if statement.
>
> I'll post my changes as a reply to patch 2 shortly.
>
> Of course changing the storage_backend_fs.c in patch 2 caused a ripple
> effect (like I expected) in patch 4, but no changes were necessary since
> patch 4 did the set and I had added the fetch after the set as part of
> my patch 2 adjustment.
>
> In patch 3, you have a 'bool' function returning a pointer value (which
> can either be "something" or NULL.  I'm changing that to "return
> !!src->backingStore;", which is what I believe you're really trying to
> return here.  There's a couple places where it's checked, but luckily so
> far nothing happens with those checks and from what I read...
>
>  * virDomainDiskBackingStoreParse -> Code has already dereferenced
> backingStore so we know it won't return false
>
>  * virStorageBackendProbeTarget -> virStorageSourceNewFromBacking
> fetch/failure would be the same
>
>  * virStorageBackendLogicalMakeVol -> If backingStore was NULL we would
> have failed earlier
>
>  * virStorageSourceCopy -> virStorageSourceCopy fetch/failure would be
> the same
>
> As long as you think that looks good - I can push patches 1-4.
>
> While Patch 5 did apply cleanly, there are some issues with it. In patch
> 3 it's returning true/false and failures in virStorageBackendProbeTarget
> and virStorageSourceCopy would return some message as to why it failed
> (most likely memory allocation failures) which then propagate back to
> the caller to get a "reason" for the failure. With your change to patch
> 5, you're returning true/false only without always setting the "reason"
> (eg virReportError). That reason needs to be set so failure reason can
> be propagated back to the caller instead of the infamous "failed for
> some reason".
>
> Additionally in all the places that don't check the return, you should
> add a ignore_value() around them to signify the code doesn't care (or
> maybe check those places to see if that "don't care" assumption is true).
>
> Once 5 is reworked, you'll have to rework patches 6-9 and repost since
> that's where the real/new functionality starts.
>
> Tks,
>
> John
>
>> This feature ask for 6 task:
>>
>> 1) Allow a _virStorageSource to contain more than one backing store.
>>
>> Because all the actual libvirt code use the backingStore field
>> as a pointer and we needs want to change that, I've decide to encapsulate
>> the backingStore field to simplifie the array manipulation.
>>
>> 2) Add the missing field a quorum need in _virStorageSource and
>> the VIR_STORAGE_TYPE_QUORUM and VIR_STORAGE_FILE_QUORUM in
>> their respectives enums.
>>
>> 3) Parse and format the xml
>> Because a quorum allows to have more than one backing store at the same level
>> we need to change virDomainDiskDefFormat and virDomainDiskDefParseXML
>> to call virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse
>> in a loop.
>> virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse can
>> call themself recursively in a loop because a quorum can contain another
>> quorum
>>
>> 4) Add nodename
>> We need to add nodename support in _virStorageSource because qemu
>> use them for their child.
>>
>> 5) Build qemu string
>> As for the xml, we have to call the function which create quorum recursively.
>> But this task have the problem explained here:
>> http://www.redhat.com/archives/libvir-list/2014-October/msg00529.html
>> The _virStorageSource missing some informations that can be passed to
>> a child, and therefore this version of quorum is incomplet.
>>
>> 6) Allow to hotplug/change a disk in a quorum
>> This part is not present in these patches because for this task
>> we have to use blockdev-add, and currently libvirt use
>> device_add for hotpluging that doesn't allow to hotplug quorum childs.
>>
>> There is 3 way to handle this problem:
>>       1) create a virDomainBlockDevAdd function in libvirt witch call
>>       blockdev-add.
>>
>>       2) use blockdev-add instead of device_add in qemuMonitorJSONAddDevice
>>
>>       3) write a hack which uses blockdev-add when only attaching quorum
>>       (but i'm pretty sure this solution is not the good one)
>>
>> V2:
>>       -Rebase on master
>>       -Add Documentation
>>
>> V3:
>>       -Transforme the backingStore field in virStorageSource into
>>        an array of pointer instead of a pointer
>>       -Modify virStorageSourceSetBackingStore to allow it to expand
>>        the backingStore size.
>>
>> V4:
>>       -Rebase on master
>>
>> Matthias Gatto (9):
>>   virstoragefile: Add virStorageSourceGetBackingStore
>>   virstoragefile: Always use virStorageSourceGetBackingStore to get
>>     backing store
>>   virstoragefile: Add virStorageSourceSetBackingStore
>>   virstoragefile: Always use virStorageSourceSetBackingStore to set
>>     backing store
>>   virstoragefile: change backingStore to backingStores.
>>   virstoragefile: Add quorum in virstoragefile
>>   domain_conf: Read and Write quorum config
>>   qemu: Add quorum support in qemuBuildDriveDevStr
>>   virstoragefile: Add node-name
>>
>>  docs/formatdomain.html.in             |  27 ++++-
>>  docs/schemas/domaincommon.rng         |  95 +++++++++++------
>>  docs/schemas/storagecommon.rng        |   1 +
>>  docs/schemas/storagevol.rng           |   1 +
>>  src/conf/domain_conf.c                | 195 ++++++++++++++++++++++++++--------
>>  src/conf/storage_conf.c               |  22 ++--
>>  src/libvirt_private.syms              |   2 +
>>  src/qemu/qemu_cgroup.c                |   4 +-
>>  src/qemu/qemu_command.c               | 114 ++++++++++++++++++++
>>  src/qemu/qemu_domain.c                |   2 +-
>>  src/qemu/qemu_driver.c                |  30 +++---
>>  src/qemu/qemu_migration.c             |   1 +
>>  src/security/security_dac.c           |   2 +-
>>  src/security/security_selinux.c       |   4 +-
>>  src/security/virt-aa-helper.c         |   2 +-
>>  src/storage/storage_backend.c         |  35 +++---
>>  src/storage/storage_backend_fs.c      |  37 ++++---
>>  src/storage/storage_backend_gluster.c |  10 +-
>>  src/storage/storage_backend_logical.c |  15 ++-
>>  src/storage/storage_driver.c          |   2 +-
>>  src/util/virstoragefile.c             | 116 +++++++++++++++++---
>>  src/util/virstoragefile.h             |  13 ++-
>>  tests/virstoragetest.c                |  18 ++--
>>  23 files changed, 573 insertions(+), 175 deletions(-)
>>

Thank you for your review,
And sorry for the time I take to answer, I was in holiday last week.
You modification look good for me, so if you can apply it it would be nice.

I rework the patch 5-9 now.