Merge lp:~justin-fathomdb/nova/libvirt-getversion-whackamole into lp:~hudson-openstack/nova/trunk

Proposed by justinsb on 2011-03-23
Status: Merged
Approved by: Josh Kearney on 2011-03-24
Approved revision: 854
Merged at revision: 867
Proposed branch: lp:~justin-fathomdb/nova/libvirt-getversion-whackamole
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 23 lines (+12/-1)
1 file modified
nova/virt/libvirt_conn.py (+12/-1)
To merge this branch: bzr merge lp:~justin-fathomdb/nova/libvirt-getversion-whackamole
Reviewer Review Type Date Requested Status
Jesse Andrews (community) Approve on 2011-03-24
Josh Kearney (community) Approve on 2011-03-24
Jay Pipes (community) 2011-03-23 Approve on 2011-03-23
Vish Ishaya 2011-03-23 Pending
Soren Hansen 2011-03-23 Pending
Review via email: mp+54475@code.launchpad.net

Commit message

Detect if user is running the default Lucid version of libvirt, and give a nicer error message

Description of the change

Check both libvirt connection object (Maverick) and libvirt namespace (Lucid) for getVersion function

Earlier versions of the python libvirt binding had getVersion in the libvirt namespace, not on the connection object. Check both.

To post a comment you must log in.
Jay Pipes (jaypipes) wrote :

lgtm.

review: Approve
justinsb (justin-fathomdb) wrote :

Though this patch let me get further, I think hit this error when launching an image " virConnect instance has no attribute 'nwfilterDefineXML'"

Paging Soren...

Soren: Is it a lost cause trying to support Lucid's libvirt? If so, there's no point merging this patch (but I could change it to give a friendly error message if an old libvirt is installed!)

Vish Ishaya (vishvananda) wrote :

I think it is a lost cause. That is why we have a backported libvirt in the ppa.

Vish

On Mar 23, 2011, at 10:01 AM, justinsb wrote:

> Though this patch let me get further, I think hit this error when launching an image " virConnect instance has no attribute 'nwfilterDefineXML'"
>
> Paging Soren...
>
> Soren: Is it a lost cause trying to support Lucid's libvirt? If so, there's no point merging this patch (but I could change it to give a friendly error message if an old libvirt is installed!)
> --
> https://code.launchpad.net/~justin-fathomdb/nova/libvirt-getversion-whackamole/+merge/54475
> You are subscribed to branch lp:nova.

justinsb (justin-fathomdb) wrote :

OK... changed this so that it gives a nicer error message if getVersion isn't available on the connection object, along with a note on where to find it in case anyone needs it in future...

Soren Hansen (soren) wrote :

> I think it is a lost cause.

+1

Josh Kearney (jk0) wrote :

Looks good to me. I don't see any harm in making an error message more friendly.

review: Approve
Jesse Andrews (anotherjesse) wrote :

Not sure if it is a lost cause - since it makes it easier to run on other platforms that we don't have a PPA for.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/virt/libvirt_conn.py'
2--- nova/virt/libvirt_conn.py 2011-03-23 05:29:32 +0000
3+++ nova/virt/libvirt_conn.py 2011-03-23 20:33:34 +0000
4@@ -981,7 +981,18 @@
5
6 """
7
8- return self._conn.getVersion()
9+ # NOTE(justinsb): getVersion moved between libvirt versions
10+ # Trying to do be compatible with older versions is a lost cause
11+ # But ... we can at least give the user a nice message
12+ method = getattr(self._conn, 'getVersion', None)
13+ if method is None:
14+ raise exception.Error(_("libvirt version is too old"
15+ " (does not support getVersion)"))
16+ # NOTE(justinsb): If we wanted to get the version, we could:
17+ # method = getattr(libvirt, 'getVersion', None)
18+ # NOTE(justinsb): This would then rely on a proper version check
19+
20+ return method()
21
22 def get_cpu_info(self):
23 """Get cpuinfo information.