Merge lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu into lp:~hudson-openstack/nova/trunk
- libvirt-is-more-than-kvm-and-qemu
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Paul Voccio |
Approved revision: | 815 |
Merged at revision: | 824 |
Proposed branch: | lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu |
Merge into: | lp:~hudson-openstack/nova/trunk |
Diff against target: |
72 lines (+31/-18) 2 files modified
bin/nova-manage (+4/-2) nova/virt/libvirt_conn.py (+27/-16) |
To merge this branch: | bzr merge lp:~soren/nova/libvirt-is-more-than-kvm-and-qemu |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Paul Voccio (community) | Approve | ||
Christian Berendt (community) | Approve | ||
Rick Harris (community) | Approve | ||
Brian Lamar (community) | Approve | ||
Review via email: mp+53610@code.launchpad.net |
Commit message
Fix a couple of things that assume that libvirt == kvm/qemu.
Description of the change
Rick Harris (rconradharris) wrote : | # |
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.connectio
> 9 + if (FLAGS.
> 10 + (FLAGS.
> 11 + FLAGS.libvirt_type not in ['kvm', 'qemu'])):
> 12 msg = _('Only KVM is supported for now. Sorry!')
> 13 raise exception.
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.."
Christian Berendt (berendt) wrote : | # |
I fixed the XML paths from //cpu/... to //host/cpu/... with the already merged branch https:/
Soren Hansen (soren) wrote : | # |
> 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.connectio
> > 9 + if (FLAGS.
> > 10 + (FLAGS.
> > 11 + FLAGS.libvirt_type not in ['kvm', 'qemu'])):
> > 12 msg = _('Only KVM is supported for now. Sorry!')
> > 13 raise exception.
> 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.
- 809. By Soren Hansen
-
Make error message match the check.
Soren Hansen (soren) wrote : | # |
> 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) wrote : | # |
Filed https:/
Christian Berendt (berendt) wrote : | # |
Do you want to remove the "+<<<<<<< TREE" ... "+>>>>>>> MERGE-SOURCE" stuff? :)
Rick Harris (rconradharris) wrote : | # |
> 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) wrote : | # |
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://
Ubuntu Developer | http://
OpenStack Developer | http://
Rick Harris (rconradharris) wrote : | # |
> 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.
- 810. By Soren Hansen
-
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) wrote : | # |
> Do you want to remove the "+<<<<<<< TREE" ... "+>>>>>>> MERGE-SOURCE" stuff?
> :)
Sorry, merged with trunk now. I didn't notice your branch get merged :)
- 811. By Soren Hansen
-
Merge trunk
Christian Berendt (berendt) wrote : | # |
You merged line 44 the wrong way...
"+ topology_nodes = xml.xpathEval(
Should be //host/cpu/...
Soren Hansen (soren) wrote : | # |
> You merged line 44 the wrong way...
>
> "+ topology_nodes = xml.xpathEval(
>
> Should be //host/cpu/...
Good catch, thanks. Fixed.
- 812. By Soren Hansen
-
I suck at merging.
- 813. By Soren Hansen
-
pep8
Rick Harris (rconradharris) wrote : | # |
Needs one more round of merge+conflict resolution.
Soren Hansen (soren) wrote : | # |
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://
Ubuntu Developer | http://
OpenStack Developer | http://
Soren Hansen (soren) wrote : | # |
> 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.
- 814. By Soren Hansen
-
Merge with trunk.
Rick Harris (rconradharris) wrote : | # |
> 59 - if set(tkeys) != set(keys):
> 65 + if list(set(tkeys)) != list(set(keys)):
Hate to say it, but bad-merge. *Takes cover*
- 815. By Soren Hansen
-
Fix mis-merge
Soren Hansen (soren) wrote : | # |
Awesome. Leave it to me to mismerge two branches, both of which I wrote myself.
Soren Hansen (soren) wrote : | # |
Anyways, it's fixed now. I'm reasonably sure I didn't break it this time.
Preview Diff
1 | === modified file 'bin/nova-manage' | |||
2 | --- bin/nova-manage 2011-03-16 17:06:30 +0000 | |||
3 | +++ bin/nova-manage 2011-03-17 21:24:52 +0000 | |||
4 | @@ -579,8 +579,10 @@ | |||
5 | 579 | ctxt = context.get_admin_context() | 579 | ctxt = context.get_admin_context() |
6 | 580 | instance_id = ec2utils.ec2_id_to_id(ec2_id) | 580 | instance_id = ec2utils.ec2_id_to_id(ec2_id) |
7 | 581 | 581 | ||
10 | 582 | if FLAGS.connection_type != 'libvirt': | 582 | if (FLAGS.connection_type != 'libvirt' or |
11 | 583 | msg = _('Only KVM is supported for now. Sorry!') | 583 | (FLAGS.connection_type == 'libvirt' and |
12 | 584 | FLAGS.libvirt_type not in ['kvm', 'qemu'])): | ||
13 | 585 | msg = _('Only KVM and QEmu are supported for now. Sorry!') | ||
14 | 584 | raise exception.Error(msg) | 586 | raise exception.Error(msg) |
15 | 585 | 587 | ||
16 | 586 | if (FLAGS.volume_driver != 'nova.volume.driver.AOEDriver' and \ | 588 | if (FLAGS.volume_driver != 'nova.volume.driver.AOEDriver' and \ |
17 | 587 | 589 | ||
18 | === modified file 'nova/virt/libvirt_conn.py' | |||
19 | --- nova/virt/libvirt_conn.py 2011-03-17 10:44:27 +0000 | |||
20 | +++ nova/virt/libvirt_conn.py 2011-03-17 21:24:52 +0000 | |||
21 | @@ -991,24 +991,35 @@ | |||
22 | 991 | + xml.serialize()) | 991 | + xml.serialize()) |
23 | 992 | 992 | ||
24 | 993 | cpu_info = dict() | 993 | cpu_info = dict() |
31 | 994 | cpu_info['arch'] = xml.xpathEval('//host/cpu/arch')[0].getContent() | 994 | |
32 | 995 | cpu_info['model'] = xml.xpathEval('//host/cpu/model')[0].getContent() | 995 | arch_nodes = xml.xpathEval('//host/cpu/arch') |
33 | 996 | cpu_info['vendor'] = xml.xpathEval('//host/cpu/vendor')[0].getContent() | 996 | if arch_nodes: |
34 | 997 | 997 | cpu_info['arch'] = arch_nodes[0].getContent() | |
35 | 998 | topology_node = xml.xpathEval('//host/cpu/topology')[0]\ | 998 | |
36 | 999 | .get_properties() | 999 | model_nodes = xml.xpathEval('//host/cpu/model') |
37 | 1000 | if model_nodes: | ||
38 | 1001 | cpu_info['model'] = model_nodes[0].getContent() | ||
39 | 1002 | |||
40 | 1003 | vendor_nodes = xml.xpathEval('//host/cpu/vendor') | ||
41 | 1004 | if vendor_nodes: | ||
42 | 1005 | cpu_info['vendor'] = vendor_nodes[0].getContent() | ||
43 | 1006 | |||
44 | 1007 | topology_nodes = xml.xpathEval('//host/cpu/topology') | ||
45 | 1000 | topology = dict() | 1008 | topology = dict() |
50 | 1001 | while topology_node: | 1009 | if topology_nodes: |
51 | 1002 | name = topology_node.get_name() | 1010 | topology_node = topology_nodes[0].get_properties() |
52 | 1003 | topology[name] = topology_node.getContent() | 1011 | while topology_node: |
53 | 1004 | topology_node = topology_node.get_next() | 1012 | name = topology_node.get_name() |
54 | 1013 | topology[name] = topology_node.getContent() | ||
55 | 1014 | topology_node = topology_node.get_next() | ||
56 | 1005 | 1015 | ||
63 | 1006 | keys = ['cores', 'sockets', 'threads'] | 1016 | keys = ['cores', 'sockets', 'threads'] |
64 | 1007 | tkeys = topology.keys() | 1017 | tkeys = topology.keys() |
65 | 1008 | if set(tkeys) != set(keys): | 1018 | if set(tkeys) != set(keys): |
66 | 1009 | ks = ', '.join(keys) | 1019 | ks = ', '.join(keys) |
67 | 1010 | raise exception.Invalid(_("Invalid xml: topology(%(topology)s) " | 1020 | raise exception.Invalid(_("Invalid xml: topology" |
68 | 1011 | "must have %(ks)s") % locals()) | 1021 | "(%(topology)s) must have " |
69 | 1022 | "%(ks)s") % locals()) | ||
70 | 1012 | 1023 | ||
71 | 1013 | feature_nodes = xml.xpathEval('//host/cpu/feature') | 1024 | feature_nodes = xml.xpathEval('//host/cpu/feature') |
72 | 1014 | features = list() | 1025 | features = list() |
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.