Merge lp:~vishvananda/nova/lp710959 into lp:~hudson-openstack/nova/trunk

[Vish Ishaya]

Automates the setup for FlatDHCP regardless of whether the interface has an ip address.

FlatDHCP only has worked until now if the underlying interface that you bridge into has no ip address. This branch will do the necessary setup to allow it to work by:

* Creating a bridge with the private address for dhcp
* Moving any existing ips from the interface onto the bridge
* Adding the interface to the bridge
* Recreating the default route if it was deleted by moving the interface

It will additionally add a route to compute hosts for bridges that it adds to the compute hosts. This seems to be necessary in some cases where the default route uses a different interface.

Automates the setup for FlatDHCP regardless of whether the interface has an ip address.

FlatDHCP only has worked until now if the underlying interface that you bridge into has no ip address. This branch will do the necessary setup to allow it to work by:

* Creating a bridge with the private address for dhcp
* Moving any existing ips from the interface onto the bridge
* Adding the interface to the bridge
* Recreating the default route if it was deleted by moving the interface

It will additionally add a route to compute hosts for bridges that it adds to the compute hosts. This seems to be necessary in some cases where the default route uses a different interface.

This is some pretty heavy-handed networking setup, so it only does this if --flat_interface is set in flags.

Important notes:

* route needs to be added to sudoers in the packaging when this branch is merged
* it is using text output from route and ip, so it should be verified that this works in internationalized environments.
* I couldn't verify whether the changes impacted IPv6. Specifically the following line:
206 if(FLAGS.use_ipv6):
207 _execute("sudo ip -f inet6 addr change %s dev %s" %
208 (net_attrs['cidr_v6'], bridge))
I'm not sure if this can be run more than once without throwing an error.
* It should be verified that adding the route on the compute host doesn't create any weird issues with vlan mode on multiple machines. I don't see any reason why it would, but I haven't been able to test every configuration.

[Jay Pipes (jaypipes)]

Seems like a fairly risky patch to include in Bexar, no? Vishy, can we get a test case that verifies the bug fix, or is that not possible because of the faked out networking components?

review: Needs Information

[Vish Ishaya (vishvananda)]

I don't think it is reasonable to try and get this into Bexar. There is already a note in release notes saying the current limitations of flatdhcp. I might be able to mock out and write tests for parts of this, but the only way i actually feel comfortable that it is working is to run smoketests against a multinode deploy of it. There is too much that just happens to work on one machine that breaks against more than one. On the upside, I've successfully deployed this onto multiple vms with vagrant and it actually works! Patches for smoketests are coming soon, as well as instructions for doing a full multinode deploy and test.

[Thierry Carrez (ttx)]

I agree this is not a Bexar target, even if it's very annoying. TBH I suspect Bexar testing will uncover more of these types of bugs after release, and we'll be lucky if they are all that easily workaroundable :) Fix in cactus please.

review: Needs Resubmitting (rcfe)

[Adrien Cunin (adri2000)]

About using text output from commands, I'd suggest at least using route -n instead of route. I've encountered a problem with my gateway hostname being long enough for route to cut out the extra characters, and therefore nova extracting a wrong hostname.

[Hisaharu Ishii (ishii-hisaharu)]

> * I couldn't verify whether the changes impacted IPv6. Specifically the following line:
> 206 if(FLAGS.use_ipv6):
> 207 _execute("sudo ip -f inet6 addr change %s dev %s" %
> 208 (net_attrs['cidr_v6'], bridge))
> I'm not sure if this can be run more than once without throwing an error.

I wrote above codes to fix bug #707908.
As you know, each network interfaces can have more than one ipv6 addresses.
When some interface A already has ipv6 address B
 "sudo ifconfig A add B"　throws the "SIOCSIFADDR: File exists" error.
 "sudo ip -f inet6 addr change B dev A"　does nothing without throwing the error.
And when interface A does not have address B
 "sudo ip -f inet6 addr change B dev A" adds address B to A.

[lp:~vishvananda/nova/lp710959]

647. By Vish Ishaya on 2011-02-03

Don't need a route for guests. Turns out the issue with routing from the guests was due to duplicate macs

648. By Vish Ishaya on 2011-02-05

allow for bridge to be the public interface

[Thierry Carrez (ttx)]

review: Abstain

[Vish Ishaya (vishvananda)]

merged trunk and got rid of calls to ifconfig. This should be ready for merge.

[lp:~vishvananda/nova/lp710959]

649. By Vish Ishaya on 2011-02-08

use route -n instead of route to avoid chopped names

650. By Vish Ishaya on 2011-02-08

merge source and remove ifconfig

[Jay Pipes (jaypipes)]

Hi! Code looks good!

One question though (that can be fixed after merge, if necessary):

57 + if err and err != "RTNETLINK answers: File exists\n":
58 + raise exception.Error("Failed to add ip: %s" % err)

I don't think this i18n-proof. Is there a way to instead compare against an error code, instead of an English string?

Approving, though, since the above can be address later.

-jay

review: Approve

[Vish Ishaya (vishvananda)]

Yes that is one of the questions I raised earlier. Can we just set LC=ALL in the environment or something? I suppose I could check for process execution error and find the exit code instead, but I'll have to verify that the exit codes are actually meaningful.

Vish

On Feb 8, 2011, at 8:16 AM, Jay Pipes wrote:

> Review: Approve
> Hi! Code looks good!
>
> One question though (that can be fixed after merge, if necessary):
>
> 57 + if err and err != "RTNETLINK answers: File exists\n":
> 58 + raise exception.Error("Failed to add ip: %s" % err)
>
> I don't think this i18n-proof. Is there a way to instead compare against an error code, instead of an English string?
>
> Approving, though, since the above can be address later.
>
> -jay
> --
> https://code.launchpad.net/~vishvananda/nova/lp710959/+merge/48341
> You are the owner of lp:~vishvananda/nova/lp710959.

[Jay Pipes (jaypipes)]

On Tue, Feb 8, 2011 at 12:58 PM, Vish Ishaya <email address hidden> wrote:
> Yes that is one of the questions I raised earlier.  Can we just set LC=ALL in the environment or something?  I suppose I could check for process execution error and find the exit code instead, but I'll have to verify that the exit codes are actually meaningful.

Ya, I wasn't sure about it either, which is why I noted it instead of
marking anything as Needs Fixing :)

-jay

[Devin Carlen (devcamcar)]

This looks good but we could use some more detailed doc strings detailing what the higher level interactions here are. There are lots of notes but nothing that lets the reader infer a bigger picture.

review: Needs Fixing

[lp:~vishvananda/nova/lp710959]

651. By Vish Ishaya on 2011-02-08

add docstring and revert set_ip changes as they are unnecessary

[Vish Ishaya (vishvananda)]

Devin:

added a docstring for you.

[Devin Carlen (devcamcar)]

approve

review: Approve

=== modified file 'nova/network/linux_net.py'
--- nova/network/linux_net.py	2011-02-08 06:07:05 +0000
+++ nova/network/linux_net.py	2011-02-08 18:57:21 +0000
@@ -20,6 +20,7 @@
 import os
 
 from nova import db
+from nova import exception
 from nova import flags
 from nova import log as logging
 from nova import utils
@@ -185,27 +186,72 @@
 
 
 def ensure_bridge(bridge, interface, net_attrs=None):
-    """Create a bridge unless it already exists"""
+    """Create a bridge unless it already exists.
+
+    :param interface: the interface to create the bridge on.
+    :param net_attrs: dictionary with  attributes used to create the bridge.
+
+    If net_attrs is set, it will add the net_attrs['gateway'] to the bridge
+    using net_attrs['broadcast'] and net_attrs['cidr'].  It will also add
+    the ip_v6 address specified in net_attrs['cidr_v6'] if use_ipv6 is set.
+
+    The code will attempt to move any ips that already exist on the interface
+    onto the bridge and reset the default gateway if necessary.
+    """
     if not _device_exists(bridge):
         LOG.debug(_("Starting Bridge interface for %s"), interface)
         _execute("sudo brctl addbr %s" % bridge)
         _execute("sudo brctl setfd %s 0" % bridge)
         # _execute("sudo brctl setageing %s 10" % bridge)
         _execute("sudo brctl stp %s off" % bridge)
-        if interface:
-            _execute("sudo brctl addif %s %s" % (bridge, interface))
+        _execute("sudo ip link set %s up" % bridge)
     if net_attrs:
-        _execute("sudo ip addr add %s/%s dev %s broadcast %s" % \
-                 (net_attrs['gateway'],
-                 net_attrs['netmask'],
-                 bridge,
-                 net_attrs['broadcast']))
+        # NOTE(vish): The ip for dnsmasq has to be the first address on the
+        #             bridge for it to respond to reqests properly
+        suffix = net_attrs['cidr'].rpartition('/')[2]
+        out, err = _execute("sudo ip addr add %s/%s brd %s dev %s" %
+                            (net_attrs['gateway'],
+                             suffix,
+                             net_attrs['broadcast'],
+                             bridge),
+                            check_exit_code=False)
+        if err and err != "RTNETLINK answers: File exists\n":
+            raise exception.Error("Failed to add ip: %s" % err)
         if(FLAGS.use_ipv6):
             _execute("sudo ip -f inet6 addr change %s dev %s" %
                      (net_attrs['cidr_v6'], bridge))
-            _execute("sudo ip link set %s up" % bridge)
-    else:
-        _execute("sudo ip link set %s up" % bridge)
+        # NOTE(vish): If the public interface is the same as the
+        #             bridge, then the bridge has to be in promiscuous
+        #             to forward packets properly.
+        if(FLAGS.public_interface == bridge):
+            _execute("sudo ip link set dev %s promisc on" % bridge)
+    if interface:
+        # NOTE(vish): This will break if there is already an ip on the
+        #             interface, so we move any ips to the bridge
+        gateway = None
+        out, err = _execute("sudo route -n")
+        for line in out.split("\n"):
+            fields = line.split()
+            if fields and fields[0] == "0.0.0.0" and fields[-1] == interface:
+                gateway = fields[1]
+        out, err = _execute("sudo ip addr show dev %s scope global" %
+                            interface)
+        for line in out.split("\n"):
+            fields = line.split()
+            if fields and fields[0] == "inet":
+                params = ' '.join(fields[1:-1])
+                _execute("sudo ip addr del %s dev %s" % (params, fields[-1]))
+                _execute("sudo ip addr add %s dev %s" % (params, bridge))
+        if gateway:
+            _execute("sudo route add 0.0.0.0 gw %s" % gateway)
+        out, err = _execute("sudo brctl addif %s %s" %
+                            (bridge, interface),
+                            check_exit_code=False)
+
+        if (err and err != "device %s is already a member of a bridge; can't "
+                           "enslave it to bridge %s.\n" % (interface, bridge)):
+            raise exception.Error("Failed to add interface: %s" % err)
+
     if FLAGS.use_nova_chains:
         (out, err) = _execute("sudo iptables -N nova_forward",
                               check_exit_code=False)