Open Stack

On 5/15/2015 8:54 AM, Daniel P. Berrange wrote:
> On Fri, May 15, 2015 at 02:45:06PM +0100, John Garbutt wrote:
>> On 15 May 2015 at 13:28, Daniel P. Berrange <berrange at redhat.com> wrote:
>>> On Fri, May 15, 2015 at 11:51:22AM +0100, Daniel P. Berrange wrote:
>>>> On Thu, May 14, 2015 at 02:23:25PM -0500, Matt Riedemann wrote:
>>>>> There are some workarounds in the code [3] I'd like to see removed by
>>>>> bumping the minimum required version.
>>>>
>>>> Sure, its nice to remove workarounds from a cleanliness POV, but I'm generally
>>>> pretty conservative about doing so, because in the majority of case (while it
>>>> looks ugly) it is not really a significant burden on maintainers to keep it
>>>> around.
>>>>
>>>> This example is really just that. It certainly looks ugly, but we have the
>>>> code there now, it is doing the job for people who have that problem and it
>>>> isn't really having any measurable impact on our ability to maintain the
>>>> libvirt code. Removing this code won't lessen our maintainance burden in
>>>> any way, but it will unquestionably impact our users by removing support for
>>>> the platform they may be currently deployed on.
>>>
>>> BTW, the code you quote here:
>>>
>>>     http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/host.py?id=2015.1.0#n754
>>>
>>> is not actually working around a bug the libvirt hypervisor. It is in fact
>>> a bug in the libvirt-python API binding. As such we don't actually need to
>>> increase the min required libvirt to be able to remove that check. In fact
>>> increasing the min required libvirt is the wrong thing todo, because it is
>>> possible for someone to have the min required libvirt, but by accessing it
>>> via an older libvirt-python which still has the bug.
>>>
>>> So what's really needed is a dep on libvirt-python >= 1.2.0, not libvirt.
>>>
>>> We don't express min required versions for libvirt-python in the
>>> requirements.txt file though, since it is an optional package and we
>>> don't have any mechanism for recording min versions for those AFAIK.
>>
>> Does this mean we can drop the above [3] code?
>> https://github.com/openstack/requirements/blob/master/global-requirements.txt#L56
>
> Hmm, I didn't know it was listed in global-requirements.txt - I only
> checked the requirements.txt and test-requirements.txt in Nova itself
> which does not list libvirt-python.
>
> Previously test-requirements.txt did have it, but we dropped it, since
> the unit tests now exclusively use fakelibvirt.
>
> To answer your question though, if global-requirements.txt is enforcing
> that we have libvirt-python 1.2.5, then we can drop that particular
> workaround.
>
> Regards,
> Daniel
>

Right, I plan to add libvirt-python back to nova's test-requirements.txt 
to remove the workaround in host.py.

We originally removed libvirt-python from nova's test-requirements.txt 
because it was mucking up nova unit tests which were at the time doing a 
conditional import of libvirt-python so if you had that, the unit tests 
ran against whatever version you got and if it didn't import, you ran 
against fakelibvirt.  We should be using fakelibvirt everywhere in the 
unit tests now so we can add libvirt-python back to 
test-requirements.txt as an indication of the minimum required version 
of an optional dependency (required if you're using libvirt).

-- 

Thanks,

Matt Riedemann