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 <[email protected]> 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
_______________________________________________
OpenStack-operators mailing list
[email protected]
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-operators
