Merge lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu into lp:~hudson-openstack/nova/trunk

[Soren Hansen]

Fix a couple of things that assume that libvirt == kvm/qemu.

[Brian Lamar (blamar)]

Looks good. The one comment I have is probably not even your code :)

Line 63: if list(set(tkeys)) != list(set(keys)):

Is the list() call needed? "if set(tkeys) != set(keys):" still works I think, and might be more clear.

review: Approve

[Rick Harris (rconradharris)]

Nice work.

A couple of tiny nits:

> 30 + if len(arch_nodes):

Usually better to just write:

  if arch_nodes:

> 56 + while topology_node != None:

Comparisons to None should use identity not equivalence:

  while topology_node is not None:

or if its None-ness doesn't matter:

  while topology_node:

> 8 - if FLAGS.connection_type != 'libvirt':
> 9 + if (FLAGS.connection_type != 'libvirt' or
> 10 + (FLAGS.connection_type == 'libvirt' and
> 11 + FLAGS.libvirt_type not in ['kvm', 'qemu'])):
> 12 msg = _('Only KVM is supported for now. Sorry!')
> 13 raise exception.Error(msg)

The logic differs from the logging. Is live-migration supported for both KVM and QEMU now? If so, we should probably update the logging to reflect that: "Only KVM or QEMU is supported.."

review: Needs Fixing

[Christian Berendt (berendt)]

I fixed the XML paths from //cpu/... to //host/cpu/... with the already merged branch https://code.launchpad.net/~berendt/nova/lp735298. Please fix...

review: Needs Fixing

[Soren Hansen (soren)]

> A couple of tiny nits:
>
> > 30 + if len(arch_nodes):
>
> Usually better to just write:
>
> if arch_nodes:

The reasoning was that I'm not just referencing it afterwards, I'm specifically indexing into it. To ensure that that is a valid operation, it needs to have a non-zero length, not just be any value that evaluates to True.

> > 56 + while topology_node != None:
> Comparisons to None should use identity not equivalence:

You are quite right. I just didn't go into this to fix existing style issues (I did not write that comparison), but rather just to fix a specific bug.

> while topology_node is not None:
>
> or if its None-ness doesn't matter:
>
> while topology_node:

Right.

I'll fix this as well as the thing Brian points out in a separate patch.

> > 8 - if FLAGS.connection_type != 'libvirt':
> > 9 + if (FLAGS.connection_type != 'libvirt' or
> > 10 + (FLAGS.connection_type == 'libvirt' and
> > 11 + FLAGS.libvirt_type not in ['kvm', 'qemu'])):
> > 12 msg = _('Only KVM is supported for now. Sorry!')
> > 13 raise exception.Error(msg)

> The logic differs from the logging. Is live-migration supported for both KVM
> and QEMU now? If so, we should probably update the logging to reflect that:
> "Only KVM or QEMU is supported.."

Very good point. I do believe QEMu is migratable as well. I'll update the error message.

[lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu]

809. By Soren Hansen on 2011-03-17

Make error message match the check.

[Soren Hansen (soren)]

> Looks good. The one comment I have is probably not even your code :)

No, it's not :)

> Line 63: if list(set(tkeys)) != list(set(keys)):
>
> Is the list() call needed? "if set(tkeys) != set(keys):" still works I think,
> and might be more clear.

..and more correct. When list()ifying the set()s, they might end up in different orders and hence fail the comparison. I'll fix this in a different patch.

[Soren Hansen (soren)]

Filed https://code.launchpad.net/~soren/nova/stuff/+merge/53781 to address the concerns raised here that were unrelated to the problem I was aiming to fix with this patch.

[Christian Berendt (berendt)]

Do you want to remove the "+<<<<<<< TREE" ... "+>>>>>>> MERGE-SOURCE" stuff? :)

[Rick Harris (rconradharris)]

> The reasoning was that I'm not just referencing it afterwards, I'm specifically indexing into it.
> To ensure that that is a valid operation, it needs to have a non-zero length, not just be any value
> that evaluates to True.

Sure. Since you're indexing into the object, it's guaranteed to be a sequence. Sequences evaluate to truthy iff it has at least one element, so `if seq:` is just as safe as `if len(seq):`.

The only difference is, the former is Pep-8 and a constant-time operation.

`len(seq)` really only makes sense if you're comparing to a value > 0, or if you're re-using it in a later operation.

That said, it's obviously not a big deal, so, I'll leave to your discretion whether to 'fix'. :)

[Soren Hansen (soren)]

2011/3/17 Rick Harris <email address hidden>:
>> The reasoning was that I'm not just referencing it afterwards, I'm specifically indexing into it.
>> To ensure that that is a valid operation, it needs to have a non-zero length, not just be any value
>> that evaluates to True.
> Sure. Since you're indexing into the object, it's guaranteed to be a sequence. Sequences evaluate to truthy iff it has at least one element, so `if seq:` is just as safe as `if len(seq):`.

I think you're missing my point. I'm using len() to ensure it's
actually a sequence and not just something else that happens to
evaluate to True. Hmmm.. Come to think of it, I guess I'm screwed
either way. If it's not a sequence, len() will just blow up instead of
the attempt to index into it. /me fixes :)

--
Soren Hansen        | http://linux2go.dk/
Ubuntu Developer    | http://www.ubuntu.com/
OpenStack Developer | http://www.openstack.org/

[Rick Harris (rconradharris)]

> I think you're missing my point... If it's not a sequence, len() will just blow up instead of
the attempt to index into it

No, that's exactly my point :-) Hence, why I said it was guaranteed to be a sequence. Your code assumed it.

[lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu]

810. By Soren Hansen on 2011-03-17

Just use 'if foo' instead of 'if len(foo)'. It will fail as spectacularly if its not acting on a sequence anyways.

[Soren Hansen (soren)]

> Do you want to remove the "+<<<<<<< TREE" ... "+>>>>>>> MERGE-SOURCE" stuff?
> :)

Sorry, merged with trunk now. I didn't notice your branch get merged :)

[lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu]

811. By Soren Hansen on 2011-03-17

Merge trunk

[Christian Berendt (berendt)]

You merged line 44 the wrong way...

"+ topology_nodes = xml.xpathEval('//cpu/topology')"

Should be //host/cpu/...

review: Needs Fixing

[Soren Hansen (soren)]

> You merged line 44 the wrong way...
>
> "+ topology_nodes = xml.xpathEval('//cpu/topology')"
>
> Should be //host/cpu/...

Good catch, thanks. Fixed.

[lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu]

812. By Soren Hansen on 2011-03-17

I suck at merging.

813. By Soren Hansen on 2011-03-17

pep8

[Rick Harris (rconradharris)]

Needs one more round of merge+conflict resolution.

review: Needs Fixing

[Soren Hansen (soren)]

2011/3/17 Rick Harris <email address hidden>:
> Review: Needs Fixing
> Needs one more round of merge+conflict resolution.

Ok, everybody hold still. It's my turn to get stuff merged. Really :)

--
Soren Hansen        | http://linux2go.dk/
Ubuntu Developer    | http://www.ubuntu.com/
OpenStack Developer | http://www.openstack.org/

[Soren Hansen (soren)]

> Do you want to remove the "+<<<<<<< TREE" ... "+>>>>>>> MERGE-SOURCE" stuff?
> :)

Just FYI, those were never in bzr, by the way. They just show up here because Launchpad can tell that if you attempted to merge this branch into trunk, these conflicts would be there.

[lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu]

814. By Soren Hansen on 2011-03-17

Merge with trunk.

[Rick Harris (rconradharris)]

> 59 - if set(tkeys) != set(keys):
> 65 + if list(set(tkeys)) != list(set(keys)):

Hate to say it, but bad-merge. *Takes cover*

review: Needs Fixing

[lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu]

815. By Soren Hansen on 2011-03-17

Fix mis-merge

[Soren Hansen (soren)]

Awesome. Leave it to me to mismerge two branches, both of which I wrote myself.

[Soren Hansen (soren)]

Anyways, it's fixed now. I'm reasonably sure I didn't break it this time.

[Rick Harris (rconradharris)]

lgtm

review: Approve

[Christian Berendt (berendt)]

lgtm

review: Approve

[Paul Voccio (pvo)]

gogogo

review: Approve

=== modified file 'bin/nova-manage'
--- bin/nova-manage	2011-03-16 17:06:30 +0000
+++ bin/nova-manage	2011-03-17 21:24:52 +0000
@@ -579,8 +579,10 @@
         ctxt = context.get_admin_context()
         instance_id = ec2utils.ec2_id_to_id(ec2_id)
 
-        if FLAGS.connection_type != 'libvirt':
-            msg = _('Only KVM is supported for now. Sorry!')
+        if (FLAGS.connection_type != 'libvirt' or
+           (FLAGS.connection_type == 'libvirt' and
+            FLAGS.libvirt_type not in ['kvm', 'qemu'])):
+            msg = _('Only KVM and QEmu are supported for now. Sorry!')
             raise exception.Error(msg)
 
         if (FLAGS.volume_driver != 'nova.volume.driver.AOEDriver' and \

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-03-17 10:44:27 +0000
+++ nova/virt/libvirt_conn.py	2011-03-17 21:24:52 +0000
@@ -991,24 +991,35 @@
                                       + xml.serialize())
 
         cpu_info = dict()
-        cpu_info['arch'] = xml.xpathEval('//host/cpu/arch')[0].getContent()
-        cpu_info['model'] = xml.xpathEval('//host/cpu/model')[0].getContent()
-        cpu_info['vendor'] = xml.xpathEval('//host/cpu/vendor')[0].getContent()
-
-        topology_node = xml.xpathEval('//host/cpu/topology')[0]\
-                        .get_properties()
+
+        arch_nodes = xml.xpathEval('//host/cpu/arch')
+        if arch_nodes:
+            cpu_info['arch'] = arch_nodes[0].getContent()
+
+        model_nodes = xml.xpathEval('//host/cpu/model')
+        if model_nodes:
+            cpu_info['model'] = model_nodes[0].getContent()
+
+        vendor_nodes = xml.xpathEval('//host/cpu/vendor')
+        if vendor_nodes:
+            cpu_info['vendor'] = vendor_nodes[0].getContent()
+
+        topology_nodes = xml.xpathEval('//host/cpu/topology')
         topology = dict()
-        while topology_node:
-            name = topology_node.get_name()
-            topology[name] = topology_node.getContent()
-            topology_node = topology_node.get_next()
+        if topology_nodes:
+            topology_node = topology_nodes[0].get_properties()
+            while topology_node:
+                name = topology_node.get_name()
+                topology[name] = topology_node.getContent()
+                topology_node = topology_node.get_next()
 
-        keys = ['cores', 'sockets', 'threads']
-        tkeys = topology.keys()
-        if set(tkeys) != set(keys):
-            ks = ', '.join(keys)
-            raise exception.Invalid(_("Invalid xml: topology(%(topology)s) "
-                                      "must have %(ks)s") % locals())
+            keys = ['cores', 'sockets', 'threads']
+            tkeys = topology.keys()
+            if set(tkeys) != set(keys):
+                ks = ', '.join(keys)
+                raise exception.Invalid(_("Invalid xml: topology"
+                                          "(%(topology)s) must have "
+                                          "%(ks)s") % locals())
 
         feature_nodes = xml.xpathEval('//host/cpu/feature')
         features = list()