]> review.fuel-infra Code Review - openstack-build/neutron-build.git/commitdiff
Refactor configuring of floating ips on a router
authorCarl Baldwin <carl.baldwin@hp.com>
Fri, 27 Sep 2013 04:04:31 +0000 (04:04 +0000)
committerCarl Baldwin <carl.baldwin@hp.com>
Wed, 6 Nov 2013 08:24:30 +0000 (08:24 +0000)
This approach to configuring floating ips is stateless and idempotent.
This allows it to handle corner cases, such as reusing a floating ip
address with a different floating ip id in a way that is easier to
understand.

The concept is to wipe the floating ips clean and rebuild them each
time with the following optimizations.  To avoid bad performance in
manipulating iptables, it is called in the context of a call to
defer_apply_on.  To avoid a disruption in network flow a set
difference is use to determine the set of addresses that no longer
belong on the inteface rather than removing them all blindly.

Change-Id: I98aacbbb52b35688036990961d02e0b273504a77
Fixes: Bug #1209011
neutron/agent/l3_agent.py
neutron/agent/linux/iptables_manager.py
neutron/tests/unit/test_l3_agent.py

index 16c73fa81d7daa950e47a71955fb28f7cfee0616..28b691d215a2b03e57b27be9f1ba40bcba4305a9 100644 (file)
@@ -50,6 +50,7 @@ NS_PREFIX = 'qrouter-'
 INTERNAL_DEV_PREFIX = 'qr-'
 EXTERNAL_DEV_PREFIX = 'qg-'
 RPC_LOOP_INTERVAL = 1
+FLOATING_IP_CIDR_SUFFIX = '/32'
 
 
 class L3PluginApi(proxy.RpcProxy):
@@ -95,7 +96,6 @@ class RouterInfo(object):
         self._snat_enabled = None
         self._snat_action = None
         self.internal_ports = []
-        self.floating_ips = []
         self.root_helper = root_helper
         self.use_namespaces = use_namespaces
         # Invoke the setter for establishing initial SNAT action
@@ -409,7 +409,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager):
                                internal_cidrs, interface_name)
 
         # Process DNAT rules for floating IPs
-        if ex_gw_port or ri.ex_gw_port:
+        if ex_gw_port:
             self.process_router_floating_ips(ri, ex_gw_port)
 
         ri.ex_gw_port = ex_gw_port
@@ -440,45 +440,46 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager):
         ri.iptables_manager.apply()
 
     def process_router_floating_ips(self, ri, ex_gw_port):
-        floating_ips = ri.router.get(l3_constants.FLOATINGIP_KEY, [])
-        existing_floating_ip_ids = set([fip['id'] for fip in ri.floating_ips])
-        cur_floating_ip_ids = set([fip['id'] for fip in floating_ips])
-
-        id_to_fip_map = {}
-
-        for fip in floating_ips:
-            if fip['port_id']:
-                if fip['id'] not in existing_floating_ip_ids:
-                    ri.floating_ips.append(fip)
-                    self.floating_ip_added(ri, ex_gw_port,
-                                           fip['floating_ip_address'],
-                                           fip['fixed_ip_address'])
-
-                # store to see if floatingip was remapped
-                id_to_fip_map[fip['id']] = fip
-
-        floating_ip_ids_to_remove = (existing_floating_ip_ids -
-                                     cur_floating_ip_ids)
-        for fip in ri.floating_ips:
-            if fip['id'] in floating_ip_ids_to_remove:
-                ri.floating_ips.remove(fip)
-                self.floating_ip_removed(ri, ri.ex_gw_port,
-                                         fip['floating_ip_address'],
-                                         fip['fixed_ip_address'])
-            else:
-                # handle remapping of a floating IP
-                new_fip = id_to_fip_map[fip['id']]
-                new_fixed_ip = new_fip['fixed_ip_address']
-                existing_fixed_ip = fip['fixed_ip_address']
-                if (new_fixed_ip and existing_fixed_ip and
-                        new_fixed_ip != existing_fixed_ip):
-                    floating_ip = fip['floating_ip_address']
-                    self.floating_ip_removed(ri, ri.ex_gw_port,
-                                             floating_ip, existing_fixed_ip)
-                    self.floating_ip_added(ri, ri.ex_gw_port,
-                                           floating_ip, new_fixed_ip)
-                    ri.floating_ips.remove(fip)
-                    ri.floating_ips.append(new_fip)
+        """Configure the router's floating IPs
+        Configures floating ips in iptables and on the router's gateway device.
+
+        Cleans up floating ips that should not longer be configured.
+        """
+        interface_name = self.get_external_device_name(ex_gw_port['id'])
+        device = ip_lib.IPDevice(interface_name, self.root_helper,
+                                 namespace=ri.ns_name())
+
+        # Clear out all iptables rules for floating ips
+        ri.iptables_manager.ipv4['nat'].clear_rules_by_tag('floating_ip')
+
+        existing_cidrs = set([addr['cidr'] for addr in device.addr.list()])
+        new_cidrs = set()
+
+        # Loop once to ensure that floating ips are configured.
+        for fip in ri.router.get(l3_constants.FLOATINGIP_KEY, []):
+            fip_ip = fip['floating_ip_address']
+            ip_cidr = str(fip_ip) + FLOATING_IP_CIDR_SUFFIX
+
+            new_cidrs.add(ip_cidr)
+
+            if ip_cidr not in existing_cidrs:
+                net = netaddr.IPNetwork(ip_cidr)
+                device.addr.add(net.version, ip_cidr, str(net.broadcast))
+                self._send_gratuitous_arp_packet(ri, interface_name, fip_ip)
+
+            # Rebuild iptables rules for the floating ip.
+            fixed = fip['fixed_ip_address']
+            for chain, rule in self.floating_forward_rules(fip_ip, fixed):
+                ri.iptables_manager.ipv4['nat'].add_rule(chain, rule,
+                                                         tag='floating_ip')
+
+        ri.iptables_manager.apply()
+
+        # Clean up addresses that no longer belong on the gateway interface.
+        for ip_cidr in existing_cidrs - new_cidrs:
+            if ip_cidr.endswith(FLOATING_IP_CIDR_SUFFIX):
+                net = netaddr.IPNetwork(ip_cidr)
+                device.addr.delete(net.version, ip_cidr)
 
     def _get_ex_gw_port(self, ri):
         return ri.router.get('gw_port')
@@ -602,34 +603,6 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager):
                  (internal_cidr, ex_gw_ip))]
         return rules
 
-    def floating_ip_added(self, ri, ex_gw_port, floating_ip, fixed_ip):
-        ip_cidr = str(floating_ip) + '/32'
-        interface_name = self.get_external_device_name(ex_gw_port['id'])
-        device = ip_lib.IPDevice(interface_name, self.root_helper,
-                                 namespace=ri.ns_name())
-
-        if ip_cidr not in [addr['cidr'] for addr in device.addr.list()]:
-            net = netaddr.IPNetwork(ip_cidr)
-            device.addr.add(net.version, ip_cidr, str(net.broadcast))
-            self._send_gratuitous_arp_packet(ri, interface_name, floating_ip)
-
-        for chain, rule in self.floating_forward_rules(floating_ip, fixed_ip):
-            ri.iptables_manager.ipv4['nat'].add_rule(chain, rule)
-        ri.iptables_manager.apply()
-
-    def floating_ip_removed(self, ri, ex_gw_port, floating_ip, fixed_ip):
-        ip_cidr = str(floating_ip) + '/32'
-        net = netaddr.IPNetwork(ip_cidr)
-        interface_name = self.get_external_device_name(ex_gw_port['id'])
-
-        device = ip_lib.IPDevice(interface_name, self.root_helper,
-                                 namespace=ri.ns_name())
-        device.addr.delete(net.version, ip_cidr)
-
-        for chain, rule in self.floating_forward_rules(floating_ip, fixed_ip):
-            ri.iptables_manager.ipv4['nat'].remove_rule(chain, rule)
-        ri.iptables_manager.apply()
-
     def floating_forward_rules(self, floating_ip, fixed_ip):
         return [('PREROUTING', '-d %s -j DNAT --to %s' %
                  (floating_ip, fixed_ip)),
index acb32b821914cbaad6daef8aca3cd59ff49cffad..87c7c808482329e5a4238803dc7495b89df795b8 100644 (file)
@@ -63,12 +63,13 @@ class IptablesRule(object):
     """
 
     def __init__(self, chain, rule, wrap=True, top=False,
-                 binary_name=binary_name):
+                 binary_name=binary_name, tag=None):
         self.chain = get_chain_name(chain, wrap)
         self.rule = rule
         self.wrap = wrap
         self.top = top
         self.wrap_name = binary_name[:16]
+        self.tag = tag
 
     def __eq__(self, other):
         return ((self.chain == other.chain) and
@@ -177,7 +178,7 @@ class IptablesTable(object):
         self.rules = [r for r in self.rules
                       if jump_snippet not in r.rule]
 
-    def add_rule(self, chain, rule, wrap=True, top=False):
+    def add_rule(self, chain, rule, wrap=True, top=False, tag=None):
         """Add a rule to the table.
 
         This is just like what you'd feed to iptables, just without
@@ -195,7 +196,8 @@ class IptablesTable(object):
         if '$' in rule:
             rule = ' '.join(map(self._wrap_target_chain, rule.split(' ')))
 
-        self.rules.append(IptablesRule(chain, rule, wrap, top, self.wrap_name))
+        self.rules.append(IptablesRule(chain, rule, wrap, top, self.wrap_name,
+                                       tag))
 
     def _wrap_target_chain(self, s):
         if s.startswith('$'):
@@ -231,6 +233,13 @@ class IptablesTable(object):
         for rule in chained_rules:
             self.rules.remove(rule)
 
+    def clear_rules_by_tag(self, tag):
+        if not tag:
+            return
+        rules = [rule for rule in self.rules if rule.tag == tag]
+        for rule in rules:
+            self.rules.remove(rule)
+
 
 class IptablesManager(object):
     """Wrapper for iptables.
index f7cfdcff3faa57e65fab50ee2ca21d781cde6d94..f0f5068a8133efc75253e87d3eb567ff7a7ffad5 100644 (file)
@@ -217,39 +217,6 @@ class TestBasicRouterOperations(base.BaseTestCase):
     def test_agent_remove_external_gateway(self):
         self._test_external_gateway_action('remove')
 
-    def _test_floating_ip_action(self, action):
-        router_id = _uuid()
-        ri = l3_agent.RouterInfo(router_id, self.conf.root_helper,
-                                 self.conf.use_namespaces, None)
-        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
-        floating_ip = '20.0.0.100'
-        fixed_ip = '10.0.0.23'
-        ex_gw_port = {'fixed_ips': [{'ip_address': '20.0.0.30',
-                                     'subnet_id': _uuid()}],
-                      'subnet': {'gateway_ip': '20.0.0.1'},
-                      'id': _uuid(),
-                      'mac_address': 'ca:fe:de:ad:be:ef',
-                      'ip_cidr': '20.0.0.30/24'}
-        interface_name = agent.get_external_device_name(ex_gw_port['id'])
-
-        if action == 'add':
-            self.device_exists.return_value = False
-            agent.floating_ip_added(ri, ex_gw_port, floating_ip, fixed_ip)
-            self.send_arp.assert_called_once_with(ri, interface_name,
-                                                  floating_ip)
-
-        elif action == 'remove':
-            self.device_exists.return_value = True
-            agent.floating_ip_removed(ri, ex_gw_port, floating_ip, fixed_ip)
-        else:
-            raise Exception("Invalid action %s" % action)
-
-    def test_agent_add_floating_ip(self):
-        self._test_floating_ip_action('add')
-
-    def test_agent_remove_floating_ip(self):
-        self._test_floating_ip_action('remove')
-
     def _check_agent_method_called(self, agent, calls, namespace):
         if namespace:
             self.mock_ip.netns.execute.assert_has_calls(
@@ -405,6 +372,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
 
     def test_process_router(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+        agent.process_router_floating_ips = mock.Mock()
         router = self._prepare_router_data()
         fake_floatingips1 = {'floatingips': [
             {'id': _uuid(),
@@ -414,6 +382,9 @@ class TestBasicRouterOperations(base.BaseTestCase):
         ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
                                  self.conf.use_namespaces, router=router)
         agent.process_router(ri)
+        ex_gw_port = agent._get_ex_gw_port(ri)
+        agent.process_router_floating_ips.assert_called_with(ri, ex_gw_port)
+        agent.process_router_floating_ips.reset_mock()
 
         # remap floating IP to a new fixed ip
         fake_floatingips2 = copy.deepcopy(fake_floatingips1)
@@ -421,16 +392,94 @@ class TestBasicRouterOperations(base.BaseTestCase):
 
         router[l3_constants.FLOATINGIP_KEY] = fake_floatingips2['floatingips']
         agent.process_router(ri)
+        ex_gw_port = agent._get_ex_gw_port(ri)
+        agent.process_router_floating_ips.assert_called_with(ri, ex_gw_port)
+        agent.process_router_floating_ips.reset_mock()
 
         # remove just the floating ips
         del router[l3_constants.FLOATINGIP_KEY]
         agent.process_router(ri)
+        ex_gw_port = agent._get_ex_gw_port(ri)
+        agent.process_router_floating_ips.assert_called_with(ri, ex_gw_port)
+        agent.process_router_floating_ips.reset_mock()
 
         # now no ports so state is torn down
         del router[l3_constants.INTERFACE_KEY]
         del router['gw_port']
         agent.process_router(ri)
         self.send_arp.assert_called_once()
+        self.assertFalse(agent.process_router_floating_ips.called)
+
+    @mock.patch('neutron.agent.linux.ip_lib.IPDevice')
+    def test_process_router_floating_ip_add(self, IPDevice):
+        fip = {
+            'id': _uuid(), 'port_id': _uuid(),
+            'floating_ip_address': '15.1.2.3',
+            'fixed_ip_address': '192.168.0.1'
+        }
+
+        IPDevice.return_value = device = mock.Mock()
+        device.addr.list.return_value = []
+
+        ri = mock.MagicMock()
+        ri.router.get.return_value = [fip]
+
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+
+        agent.process_router_floating_ips(ri, {'id': _uuid()})
+
+        device.addr.add.assert_called_once_with(4, '15.1.2.3/32', '15.1.2.3')
+
+        nat = ri.iptables_manager.ipv4['nat']
+        nat.clear_rules_by_tag.assert_called_once_with('floating_ip')
+        rules = agent.floating_forward_rules('15.1.2.3', '192.168.0.1')
+        for chain, rule in rules:
+            nat.add_rule.assert_any_call(chain, rule, tag='floating_ip')
+
+    @mock.patch('neutron.agent.linux.ip_lib.IPDevice')
+    def test_process_router_floating_ip_remove(self, IPDevice):
+        IPDevice.return_value = device = mock.Mock()
+        device.addr.list.return_value = [{'cidr': '15.1.2.3/32'}]
+
+        ri = mock.MagicMock()
+        ri.router.get.return_value = []
+
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+
+        agent.process_router_floating_ips(ri, {'id': _uuid()})
+
+        device.addr.delete.assert_called_once_with(4, '15.1.2.3/32')
+
+        nat = ri.iptables_manager.ipv4['nat']
+        nat = ri.iptables_manager.ipv4['nat']
+        nat.clear_rules_by_tag.assert_called_once_with('floating_ip')
+
+    @mock.patch('neutron.agent.linux.ip_lib.IPDevice')
+    def test_process_router_floating_ip_remap(self, IPDevice):
+        fip = {
+            'id': _uuid(), 'port_id': _uuid(),
+            'floating_ip_address': '15.1.2.3',
+            'fixed_ip_address': '192.168.0.2'
+        }
+
+        IPDevice.return_value = device = mock.Mock()
+        device.addr.list.return_value = [{'cidr': '15.1.2.3/32'}]
+        ri = mock.MagicMock()
+
+        ri.router.get.return_value = [fip]
+
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+
+        agent.process_router_floating_ips(ri, {'id': _uuid()})
+
+        self.assertFalse(device.addr.add.called)
+        self.assertFalse(device.addr.delete.called)
+
+        nat = ri.iptables_manager.ipv4['nat']
+        nat.clear_rules_by_tag.assert_called_once_with('floating_ip')
+        rules = agent.floating_forward_rules('15.1.2.3', '192.168.0.2')
+        for chain, rule in rules:
+            nat.add_rule.assert_any_call(chain, rule, tag='floating_ip')
 
     def test_process_router_snat_disabled(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)