Open Stack

On 8/21/2014 12:26 PM, Daniel P. Berrange wrote:
> On Thu, Aug 21, 2014 at 12:23:12PM -0500, Matt Riedemann wrote:
>>
>>
>> On 8/21/2014 11:37 AM, Clark Boylan wrote:
>>>
>>>
>>> On Thu, Aug 21, 2014, at 09:25 AM, Matt Riedemann wrote:
>>>>
>>>>
>>>> On 8/21/2014 10:23 AM, Daniel P. Berrange wrote:
>>>>> On Thu, Aug 21, 2014 at 11:14:33AM -0400, Solly Ross wrote:
>>>>>> (reply inline)
>>>>>>
>>>>>> ----- Original Message -----
>>>>>>> From: "Daniel P. Berrange" <berrange at redhat.com>
>>>>>>> To: "OpenStack Development Mailing List (not for usage questions)" <openstack-dev at lists.openstack.org>
>>>>>>> Sent: Thursday, August 21, 2014 11:05:18 AM
>>>>>>> Subject: Re: [openstack-dev] [nova][libvirt] Non-readonly connection to libvirt in unit tests
>>>>>>>
>>>>>>> On Thu, Aug 21, 2014 at 10:52:42AM -0400, Solly Ross wrote:
>>>>>>>> FYI, the context of this is that I would like to be able to test some
>>>>>>>> of the libvirt storage pool code against a live file system, as we
>>>>>>>> currently test the storage pool code.  To do this, we need at least to
>>>>>>>> be able to get a proper connection to a session daemon.  IMHO, since
>>>>>>>> these calls aren't "expensive", so to speak, it should be fine to have
>>>>>>>> them run against a real libvirt.
>>>>>>>
>>>>>>> No it really isn't OK to run against the real libvirt host system when
>>>>>>> in the unit tests. Unit tests must *not* rely on external system state
>>>>>>> in this way because it will lead to greater instability and unreliability
>>>>>>> of our unit tests. If you want to test stuff against the real libvirt
>>>>>>> storage pools then that becomes a functional / integration test suite
>>>>>>> which is pretty much what tempest is targetting.
>>>>>>
>>>>>> That's all well and good, but we *currently* manipulates the actual file
>>>>>> system manually in tests.  Should we then say that we should never manipulate
>>>>>> the actual file system either?  In that case, there are some tests which
>>>>>> need to be refactored.
>>>>>
>>>>> Places where the tests manipulate the filesystem though should be doing
>>>>> so in an isolated playpen directory, not in the "live" location where
>>>>> a deployed nova runs, so that's not the same thing.
>>>>>
>>>>>>>>> So If we require libvirt-python for tests and that requires
>>>>>>>>> libvirt-bin, what's stopping us from just removing fakelibvirt since
>>>>>>>>> it's kind of useless now anyway, right?
>>>>>>>>
>>>>>>>> The thing about fakelibvirt is that it allows us to operate against
>>>>>>>> against a libvirt API without actually doing libvirt-y things like
>>>>>>>> launching VMs.  Now, libvirt does have a "test:///default" URI that
>>>>>>>> IIRC has similar functionality, so we could start to phase out fake
>>>>>>>> libvirt in favor of that.  However, there are probably still some
>>>>>>>> spots where we'll want to use fakelibvirt.
>>>>>>>
>>>>>>> I'm actually increasingly of the opinion that we should not in fact
>>>>>>> be trying to use the real libvirt library in the unit tests at all
>>>>>>> as it is not really adding any value. We typically nmock out all the
>>>>>>> actual API calls we exercise so despite "using" libvirt-python we
>>>>>>> are not in fact exercising its code or even validating that we're
>>>>>>> passing the correct numbers of parameters to API calls. Pretty much
>>>>>>> all we really relying on is the existance of the various global
>>>>>>> constants that are defined, and that has been nothing but trouble
>>>>>>> because the constants may or may not be defined depending on the
>>>>>>> version.
>>>>>>
>>>>>> Isn't that what 'test:///default' is supposed to be?  A version of libvirt
>>>>>> with libvirt not actually touching the rest of the system?
>>>>>
>>>>> Yes, that is what it allows for, however, even if we used that URI we
>>>>> still wouldn't be actually exercising any of the libvirt code in any
>>>>> meaningful way because our unit tests mock out all the API calls that
>>>>> get touched. So using libvirt-python + test:///default URI doesn't
>>>>> really seem to buy us anything, but it does still mean that developers
>>>>> need to have libvirt installed in order to run  the unit tests. I'm
>>>>> not convinced that is a beneficial tradeoff.
>>>>>
>>>>>>> The downside of fakelibvirt is that it is a half-assed implementation
>>>>>>> of libvirt that we evolve in an adhoc fashion. I'm exploring the idea
>>>>>>> of using pythons introspection abilities to query the libvirt-python
>>>>>>> API and automatically generate a better 'fakelibvirt' that we can
>>>>>>> guarantee to match the signatures of the real libvirt library. If we
>>>>>>> had something like that which we had more confidence in, then we could
>>>>>>> make the unit tests use that unconditionally. This would make our unit
>>>>>>> tests more reliable since we would not be suspectible to different API
>>>>>>> coverage in different libvirt module versions which have tripped us up
>>>>>>> so many times
>>>>>
>>>>> Regards,
>>>>> Daniel
>>>>>
>>>>
>>>> +1000 to removing the need to have libvirt installed to run unit tests,
>>>> but that's what I'm seeing today unless I'm mistaken since we require
>>>> libvirt-python which requires libvirt as already pointed out.
>>>>
>>>> If you revert the change to require libvirt-python and try to run the
>>>> unit tests, it fails, see bug 1357437 [1].
>>>>
>>>> [1] https://bugs.launchpad.net/nova/+bug/1357437
>>>>
>>> Reverting the change to require libvirt-python is insufficient. That
>>> revert will flip back to using system packages and include libvirt
>>> python lib from your operating system. Libvirt will still be required
>>> just via a different avenue (nova does try to fall back on its fake
>>> libvirt but iirc that doesn't always work so well).
>>>
>>> If you want to stop depending on libvirt in unittests you should remove
>>> the libvirt-python requirement and not try to use the system package
>>> installed version of libvirt. Then the fake libvirt will have to keep
>>> working and will be tested in the gate.
>>
>> Yeah I'm not suggesting we revert that change to test-requirements, I'm more
>> agreeing with the sentiment to use fakelibvirt only and make that thing work
>> (and keep it current).
>
> Given the point in the release cycle it probably isn't wise to start messing
> around with this even more, since we don't want to put more instability on
> the gate. Resolving the future of fakelibvirt in tests one way or the other
> is definitely something I'd like to address in Kilo though. We've suffered
> under the confusion & inconsistency of our current approach for too long.
>
> Regards,
> Daniel
>

+1

-- 

Thanks,

Matt Riedemann