Merge lp://qastaging/~midokura/nova/network-refactoring-l2 into lp://qastaging/~hudson-openstack/nova/trunk

Hi!

Looks like some excellent cleanup.

Tiny consistency suggestion

For the unfilter_instance() method on the driver, you use a kwarg "network_info":

184 + self.driver.unfilter_instance(instance_ref, network_info=network_info)

But for all the other driver methods, you are passing network_info as a positional param:

132 + self.driver.destroy(instance_ref, network_info)
166 + self.driver.plug_vifs(instance_ref, network_info)

It would be good to keep this consistent.

Also, here:

262 + @property
263 + def _create_bridge(self):
264 + """Indicate whether this manager requires VIF to create a bridge."""
265 + return False

and

271 + @property
272 + def _create_vlan(self):
273 + """Indicate whether this manager requires VIF to create a VLAN tag."""
274 + return False

I would recommend naming those should_create_bridge and should_create_vlan to better indicate that the property is a boolean value and not an action. You could also consider making these simply class-level constants.

948 + LOG.warning("Failed while unplugging vif of instance '%s'" % \
1790 + LOG.warning("Failed while unplugging vif of instance '%s'" % \

Need i18n in added strings above...

Cheers!
jay

There is a commit somewhere that someone used <danwent@dan-xs3-cs> their email. Assuming it is <email address hidden> the mailmap should be updated so the tests pass:

--- ../nova/.mailmap 2011-07-11 13:16:24.801028000 -0500
+++ .mailmap 2011-07-22 13:52:53.311012292 -0500
@@ -52,3 +52,4 @@
 <email address hidden> <email address hidden>
 <email address hidden> <email address hidden>
 <email address hidden> <email address hidden>
+<email address hidden> <danwent@dan-xs3-cs>

Thanks Jason, that is my fault. Any idea how I fix this with bzr? I am a
git person and my googling has turned anything up.

Dan

On Fri, Jul 22, 2011 at 12:04 PM, Jason Kölker <email address hidden>wrote:

> Review: Needs Fixing
> There is a commit somewhere that someone used <danwent@dan-xs3-cs> their
> email. Assuming it is <email address hidden> the mailmap should be updated so the
> tests pass:
>
>
> --- ../nova/.mailmap 2011-07-11 13:16:24.801028000 -0500
> +++ .mailmap 2011-07-22 13:52:53.311012292 -0500
> @@ -52,3 +52,4 @@
> <email address hidden> <email address hidden>
> <email address hidden> <email address hidden>
> <email address hidden> <email address hidden>
> +<email address hidden> <danwent@dan-xs3-cs>
>
> --
>
> https://code.launchpad.net/~midokura/nova/network-refactoring-l2/+merge/68840
> You are subscribed to branch lp:~midokura/nova/network-refactoring-l2.
>

--
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dan Wendlandt
Nicira Networks, Inc.
www.nicira.com | www.openvswitch.org
Sr. Product Manager
cell: 650-906-2650
~~~~~~~~~~~~~~~~~~~~~~~~~~~

I should state, I have fixed the issue for the more recent commits by
correctly running bzr whoami, but I am not sure how to apply this
retroactively. Thanks,

Dan

On Fri, Jul 22, 2011 at 12:56 PM, Dan Wendlandt <email address hidden> wrote:

> Thanks Jason, that is my fault. Any idea how I fix this with bzr? I am a
> git person and my googling has turned anything up.
>
> Dan
>
>
> On Fri, Jul 22, 2011 at 12:04 PM, Jason Kölker <email address hidden>wrote:
>
>> Review: Needs Fixing
>> There is a commit somewhere that someone used <danwent@dan-xs3-cs> their
>> email. Assuming it is <email address hidden> the mailmap should be updated so
>> the tests pass:
>>
>>
>> --- ../nova/.mailmap 2011-07-11 13:16:24.801028000 -0500
>> +++ .mailmap 2011-07-22 13:52:53.311012292 -0500
>> @@ -52,3 +52,4 @@
>> <email address hidden> <email address hidden>
>> <email address hidden> <email address hidden>
>> <email address hidden> <email address hidden>
>> +<email address hidden> <danwent@dan-xs3-cs>
>>
>> --
>>
>> https://code.launchpad.net/~midokura/nova/network-refactoring-l2/+merge/68840
>> You are subscribed to branch lp:~midokura/nova/network-refactoring-l2.
>>
>
>
>
> --
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Dan Wendlandt
> Nicira Networks, Inc.
> www.nicira.com | www.openvswitch.org
> Sr. Product Manager
> cell: 650-906-2650
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>

--
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dan Wendlandt
Nicira Networks, Inc.
www.nicira.com | www.openvswitch.org
Sr. Product Manager
cell: 650-906-2650
~~~~~~~~~~~~~~~~~~~~~~~~~~~

On Fri, Jul 22, 2011 at 4:01 PM, dan wendlandt <email address hidden> wrote:
> I should state, I have fixed the issue for the more recent commits by
> correctly running bzr whoami, but I am not sure how to apply this
> retroactively.  Thanks,

I don't think you need to do anything other than what Jason mentioned
about the mailmap edit.

Cheers,
jay

Ah, that's easy. I had misunderstood what was being asked. Thanks Jay!

Ryu's the owner of the branch, so I'll let him make the change.

Dan

On Fri, Jul 22, 2011 at 1:07 PM, Jay Pipes <email address hidden> wrote:

> On Fri, Jul 22, 2011 at 4:01 PM, dan wendlandt <email address hidden> wrote:
> > I should state, I have fixed the issue for the more recent commits by
> > correctly running bzr whoami, but I am not sure how to apply this
> > retroactively. Thanks,
>
> I don't think you need to do anything other than what Jason mentioned
> about the mailmap edit.
>
> Cheers,
> jay
>
> --
>
> https://code.launchpad.net/~midokura/nova/network-refactoring-l2/+merge/68840
> You are subscribed to branch lp:~midokura/nova/network-refactoring-l2.
>

--
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dan Wendlandt
Nicira Networks, Inc.
www.nicira.com | www.openvswitch.org
Sr. Product Manager
cell: 650-906-2650
~~~~~~~~~~~~~~~~~~~~~~~~~~~

Thanks Jay for the review! I have made the changes you suggested.

Dan, I also edited .mailmap file as Jason suggested and the tests passed. Good to know that's how we can fix that issue.

Rock on. Looks great, guys!

Is bueno

> I should state, I have fixed the issue for the more recent commits by
> correctly running bzr whoami, but I am not sure how to apply this
> retroactively. Thanks,

Unfortunatly bzr doesn't allow history rewriting directly, it *can* be done if you pull it into git with git-bzr do the rewrite, export it back to bzr and then push it up to lp with --overwrite. But the .mailmap is way easier ;).

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.

> 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?

I had a similar thought but then I remember that pattern was in use in a number of places in the internal Nova code. I think a separate refactoring patch that removed FLAGS.stub_blah flags would be a better approach.

> 1158 +# Copyright (C) 2011 Midokura KK
> 1159 +# Copyright (C) 2011 Nicira, Inc
>
> I'm not certain, but I believe code belongs to OpenStack, LLC?

Leaving company/individual contributor copyright headers in the code for large portions of original code is OK, I believe. The CLA does not require copyright assignment; it grants the OpenStack LLC copyright rights to distribute/redistribute the code... The CLA ensures that the contributed code is not patent-encumbered and that the contributor is legally able to contribute the code.

Cheers!
jay

Brian, thanks for your thorough review.

> 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?

I completely agree that all the stubbing logic should be moved out of the compute manager. Unfortunately, as Jay pointed out, it's used in many places in Nova and some unit tests rely on this flag to be set for them to run successfully, which is why I added this flag here. Since fixing this will require non-trivial changes in Nova somewhat unrelated to this branch, Jay's suggestion to have another re-factoring branch focused on fixing this particular issue sounds like a good idea to me. What do you think?

I made all the other changes you suggested.

Thanks,
Ryu

>
> I completely agree that all the stubbing logic should be moved out of the
> compute manager. Unfortunately, as Jay pointed out, it's used in many places
> in Nova and some unit tests rely on this flag to be set for them to run
> successfully, which is why I added this flag here. Since fixing this will
> require non-trivial changes in Nova somewhat unrelated to this branch, Jay's
> suggestion to have another re-factoring branch focused on fixing this
> particular issue sounds like a good idea to me. What do you think?

I made a bug for this here:
https://bugs.launchpad.net/nova/+bug/816025

looks like a merge conflict created by the multiple dns fix:

274 +<<<<<<< TREE
275 'dns': [],
276 'ips': [ip_dict(ip) for ip in network_IPs]}
277 +=======
278 + 'dns': [network['dns']],
279 + 'ips': [ip_dict(ip) for ip in network_IPs],
280 + 'should_create_bridge': self.SHOULD_CREATE_BRIDGE,
281 + 'should_create_vlan': self.SHOULD_CREATE_VLAN}
282 +>>>>>>> MERGE-SOURCE

Code looks good. I'm doing a quick test to make sure that I can still run multi_host networks. I will post the results shortly.

Test looks good. I'll approve as soon as the conflict is resolved.

This looks good! (except for the merge conflict). I'm going to be optimistic and pre-approve this since I won't have a chance to look later today.

looks like everything has been addressed, approving