Code review comment for lp://qastaging/~midokura/nova/network-refactoring-l2

Revision history for this message
Brian Lamar (blamar) wrote :

44 + if not FLAGS.stub_network:

Is this truly necessary? Test/stubbing code should only be in tests, not in module code. Is this something we might look at quickly while doing this re-factoring?

239 + """Constant to indicate whether this manager requires VIF to create a
240 + VLAN tag."""

I think the convention is to have this be a comment with # not """, so maybe something like:

# If True, this manager requires VIF to create VLAN tag
SHOULD_CREATE_VLAN = False

Regardless, the "Constant to indicate whether" is superfluous and its removal would shorten these and help readability either way.

957 + try:
958 + for (network, mapping) in network_info:
959 + self.vif_driver.unplug(instance, network, mapping)
960 + except:
961 + LOG.warning(_("Failed while unplugging vif of instance '%s'"),
962 + instance['name'])
963 + raise
964 +

I'm dubious as to the necessity of this try/except. If you're only logging, it would be better IMO to rely on good error handling in the self.vif_driver.unplug method and not in this one. The same thing happens on lines 1799-1807.

1158 +# Copyright (C) 2011 Midokura KK
1159 +# Copyright (C) 2011 Nicira, Inc

I'm not certain, but I believe code belongs to OpenStack, LLC?

1187 +flags.DEFINE_string('libvirt_ovs_integration_bridge', 'br-int',

Can we maybe shorten 'libvirt_ovs_integration_bridge' to just 'libvirt_ovs_bridge'? I'm not in love with having such long flag names, and we can always clarify in the description string.

review: Needs Fixing

« Back to merge proposal