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

[Vish Ishaya]

Makes sure ips are moved on the bridge for nodes running dnsmasq so that the gateway ip is always first.

[lp:~vishvananda/nova/fix-dhcp]

1544. By Vish Ishaya on 2011-09-21

add tests and fix bug when no ip was set

[Soren Hansen (soren)]

lgtm

I do wonder, though, if this is a bug in dnsmasq of if it's by design? Have we talked to dnsmasq upstream at all? According to man ip(8), the addresses are not discriminated, so the order shouldn't matter, AFAICS.

review: Approve

[Soren Hansen (soren)]

> I do wonder, though, if this is a bug in dnsmasq of if it's by design?

That should have said:

I do wonder, though, if this is a bug in dnsmasq or if it's by design?

[lp:~vishvananda/nova/fix-dhcp]

1545. By Vish Ishaya on 2011-09-21

pep8

[Dan Prince (dan-prince)]

This looks good.

review: Approve

=== modified file 'nova/network/linux_net.py'
--- nova/network/linux_net.py	2011-09-20 09:37:07 +0000
+++ nova/network/linux_net.py	2011-09-21 13:00:32 +0000
@@ -472,22 +472,30 @@
 
     # NOTE(vish): The ip for dnsmasq has to be the first address on the
     #             bridge for it to respond to reqests properly
-    suffix = network_ref['cidr'].rpartition('/')[2]
-    out, err = _execute('ip', 'addr', 'add',
-                            '%s/%s' %
-                            (network_ref['dhcp_server'], suffix),
-                            'brd',
-                            network_ref['broadcast'],
-                            'dev',
-                            dev,
-                            run_as_root=True,
-                            check_exit_code=False)
-    if err and err != 'RTNETLINK answers: File exists\n':
-        raise exception.Error('Failed to add ip: %s' % err)
-    if FLAGS.send_arp_for_ha:
-        _execute('arping', '-U', network_ref['gateway'],
-                  '-A', '-I', dev,
-                  '-c', 1, run_as_root=True, check_exit_code=False)
+    full_ip = '%s/%s' % (network_ref['dhcp_server'],
+                         network_ref['cidr'].rpartition('/')[2])
+    new_ip_params = [[full_ip, 'brd', network_ref['broadcast']]]
+    old_ip_params = []
+    out, err = _execute('ip', 'addr', 'show', 'dev', dev,
+                        'scope', 'global', run_as_root=True)
+    for line in out.split('\n'):
+        fields = line.split()
+        if fields and fields[0] == 'inet':
+            ip_params = fields[1:-1]
+            old_ip_params.append(ip_params)
+            if ip_params[0] != full_ip:
+                new_ip_params.append(ip_params)
+    if not old_ip_params or old_ip_params[0][0] != full_ip:
+        for ip_params in old_ip_params:
+            _execute(*_ip_bridge_cmd('del', ip_params, dev),
+                        run_as_root=True)
+        for ip_params in new_ip_params:
+            _execute(*_ip_bridge_cmd('add', ip_params, dev),
+                        run_as_root=True)
+        if FLAGS.send_arp_for_ha:
+            _execute('arping', '-U', network_ref['dhcp_server'],
+                      '-A', '-I', dev,
+                      '-c', 1, run_as_root=True, check_exit_code=False)
     if(FLAGS.use_ipv6):
         _execute('ip', '-f', 'inet6', 'addr',
                      'change', network_ref['cidr_v6'],

=== modified file 'nova/tests/test_linux_net.py'
--- nova/tests/test_linux_net.py	2011-09-07 20:37:58 +0000
+++ nova/tests/test_linux_net.py	2011-09-21 13:00:32 +0000
@@ -345,3 +345,72 @@
         expected = ("10.0.0.1,fake_instance00.novalocal,192.168.0.100")
         actual = self.driver._host_dhcp(fixed_ips[0])
         self.assertEquals(actual, expected)
+
+    def _test_initialize_gateway(self, existing, expected):
+        self.flags(fake_network=False)
+        executes = []
+
+        def fake_execute(*args, **kwargs):
+            executes.append(args)
+            if args[0] == 'ip' and args[1] == 'addr' and args[2] == 'show':
+                return existing, ""
+        self.stubs.Set(utils, 'execute', fake_execute)
+        network = {'dhcp_server': '192.168.1.1',
+                   'cidr': '192.168.1.0/24',
+                   'broadcast': '192.168.1.255',
+                   'cidr_v6': '2001:db8::/64'}
+        self.driver.initialize_gateway_device('eth0', network)
+        self.assertEqual(executes, expected)
+
+    def test_initialize_gateway_moves_wrong_ip(self):
+        existing = ("2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> "
+            "    mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000\n"
+            "    link/ether de:ad:be:ef:be:ef brd ff:ff:ff:ff:ff:ff\n"
+            "    inet 192.168.0.1/24 brd 192.168.0.255 scope global eth0\n"
+            "    inet6 dead::beef:dead:beef:dead/64 scope link\n"
+            "    valid_lft forever preferred_lft forever\n")
+        expected = [
+            ('ip', 'addr', 'show', 'dev', 'eth0', 'scope', 'global'),
+            ('ip', 'addr', 'del', '192.168.0.1/24',
+             'brd', '192.168.0.255', 'scope', 'global', 'dev', 'eth0'),
+            ('ip', 'addr', 'add', '192.168.1.1/24',
+             'brd', '192.168.1.255', 'dev', 'eth0'),
+            ('ip', 'addr', 'add', '192.168.0.1/24',
+             'brd', '192.168.0.255', 'scope', 'global', 'dev', 'eth0'),
+            ('ip', '-f', 'inet6', 'addr', 'change',
+             '2001:db8::/64', 'dev', 'eth0'),
+            ('ip', 'link', 'set', 'dev', 'eth0', 'promisc', 'on'),
+        ]
+        self._test_initialize_gateway(existing, expected)
+
+    def test_initialize_gateway_no_move_right_ip(self):
+        existing = ("2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> "
+            "    mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000\n"
+            "    link/ether de:ad:be:ef:be:ef brd ff:ff:ff:ff:ff:ff\n"
+            "    inet 192.168.1.1/24 brd 192.168.1.255 scope global eth0\n"
+            "    inet 192.168.0.1/24 brd 192.168.0.255 scope global eth0\n"
+            "    inet6 dead::beef:dead:beef:dead/64 scope link\n"
+            "    valid_lft forever preferred_lft forever\n")
+        expected = [
+            ('ip', 'addr', 'show', 'dev', 'eth0', 'scope', 'global'),
+            ('ip', '-f', 'inet6', 'addr', 'change',
+             '2001:db8::/64', 'dev', 'eth0'),
+            ('ip', 'link', 'set', 'dev', 'eth0', 'promisc', 'on'),
+        ]
+        self._test_initialize_gateway(existing, expected)
+
+    def test_initialize_gateway_add_if_blank(self):
+        existing = ("2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> "
+            "    mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000\n"
+            "    link/ether de:ad:be:ef:be:ef brd ff:ff:ff:ff:ff:ff\n"
+            "    inet6 dead::beef:dead:beef:dead/64 scope link\n"
+            "    valid_lft forever preferred_lft forever\n")
+        expected = [
+            ('ip', 'addr', 'show', 'dev', 'eth0', 'scope', 'global'),
+            ('ip', 'addr', 'add', '192.168.1.1/24',
+             'brd', '192.168.1.255', 'dev', 'eth0'),
+            ('ip', '-f', 'inet6', 'addr', 'change',
+             '2001:db8::/64', 'dev', 'eth0'),
+            ('ip', 'link', 'set', 'dev', 'eth0', 'promisc', 'on'),
+        ]
+        self._test_initialize_gateway(existing, expected)