Create dvr base class and stop passing around snat_ports

The one thing that I see that the two dvr classes have in common is
the ability to map internal ports to snat ports.  The dvr local router
needs it to set up a redirect to the central part.  The central part
needs it to create the port for the internal network.

This change renames the mapping method to something more logical and
removes snat_ports as an argument to two methods because it is a quick
O(1) operation to get it from the router dict and passing it around
just tangles things up.

Change-Id: Icc099c1a97e3e68eeaf4690bc83167ba30d8099a

diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py
index d54015d050f1556f5e80c62a8165b24cd2a52fbf..72e6412fb1cdfd0cb9f918c3b55407de2d1831f8 100644 (file)
--- a/neutron/agent/l3/dvr_edge_router.py
+++ b/neutron/agent/l3/dvr_edge_router.py
@@ -34,8 +34,7 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
         super(DvrEdgeRouter, self).external_gateway_added(
             ex_gw_port, interface_name)
         if self._is_this_snat_host():
-            snat_ports = self.get_snat_interfaces()
-            self._create_dvr_gateway(ex_gw_port, interface_name, snat_ports)
+            self._create_dvr_gateway(ex_gw_port, interface_name)
 
     def external_gateway_updated(self, ex_gw_port, interface_name):
         if not self._is_this_snat_host():
@@ -71,8 +70,7 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
         if not self._is_this_snat_host():
             return
 
-        snat_ports = self.get_snat_interfaces()
-        sn_port = self._map_internal_interfaces(port, snat_ports)
+        sn_port = self.get_snat_port_for_internal_port(port)
         if not sn_port:
             return
 
@@ -93,8 +91,7 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
         if not self.ex_gw_port:
             return
 
-        snat_ports = self.get_snat_interfaces()
-        sn_port = self._map_internal_interfaces(port, snat_ports)
+        sn_port = self.get_snat_port_for_internal_port(port)
         if not sn_port:
             return
 
@@ -110,12 +107,11 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
             self.driver.unplug(snat_interface, namespace=ns_name,
                                prefix=prefix)
 
-    def _create_dvr_gateway(self, ex_gw_port, gw_interface_name,
-                            snat_ports):
+    def _create_dvr_gateway(self, ex_gw_port, gw_interface_name):
         """Create SNAT namespace."""
         snat_ns = self.create_snat_namespace()
         # connect snat_ports to br_int from SNAT namespace
-        for port in snat_ports:
+        for port in self.get_snat_interfaces():
             # create interface_name
             interface_name = self.get_snat_int_device_name(port['id'])
             self._internal_network_added(

diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py
index 21cc3877bdef8d123808d8fe5a82ed1dec41f923..1336fd2b9a3f3467409cdb2f7c1110b6fdac162d 100755 (executable)
--- a/neutron/agent/l3/dvr_local_router.py
+++ b/neutron/agent/l3/dvr_local_router.py
@@ -19,7 +19,7 @@ from oslo_log import log as logging
 from oslo_utils import excutils
 
 from neutron.agent.l3 import dvr_fip_ns
-from neutron.agent.l3 import router_info as router
+from neutron.agent.l3 import dvr_router_base
 from neutron.agent.linux import ip_lib
 from neutron.common import constants as l3_constants
 from neutron.common import exceptions
@@ -31,12 +31,9 @@ LOG = logging.getLogger(__name__)
 MASK_30 = 0x3fffffff
 
 
-class DvrLocalRouter(router.RouterInfo):
+class DvrLocalRouter(dvr_router_base.DvrRouterBase):
     def __init__(self, agent, host, *args, **kwargs):
-        super(DvrLocalRouter, self).__init__(*args, **kwargs)
-
-        self.agent = agent
-        self.host = host
+        super(DvrLocalRouter, self).__init__(agent, host, *args, **kwargs)
 
         self.floating_ips_dict = {}
         # Linklocal subnet for router and floating IP namespace link
@@ -49,9 +46,6 @@ class DvrLocalRouter(router.RouterInfo):
         floating_ips = super(DvrLocalRouter, self).get_floating_ips()
         return [i for i in floating_ips if i['host'] == self.host]
 
-    def get_snat_interfaces(self):
-        return self.router.get(l3_constants.SNAT_ROUTER_INTF_KEY, [])
-
     def _handle_fip_nat_rules(self, interface_name, action):
         """Configures NAT rules for Floating IPs for DVR.
 
@@ -200,17 +194,6 @@ class DvrLocalRouter(router.RouterInfo):
                                            subnet_id,
                                            'add')
 
-    def _map_internal_interfaces(self, int_port, snat_ports):
-        """Return the SNAT port for the given internal interface port."""
-        fixed_ip = int_port['fixed_ips'][0]
-        subnet_id = fixed_ip['subnet_id']
-        match_port = [p for p in snat_ports if
-                      p['fixed_ips'][0]['subnet_id'] == subnet_id]
-        if match_port:
-            return match_port[0]
-        else:
-            LOG.error(_LE('DVR: no map match_port found!'))
-
     @staticmethod
     def _get_snat_idx(ip_cidr):
         """Generate index for DVR snat rules and route tables.
@@ -305,8 +288,7 @@ class DvrLocalRouter(router.RouterInfo):
         if not ex_gw_port:
             return
 
-        snat_ports = self.get_snat_interfaces()
-        sn_port = self._map_internal_interfaces(port, snat_ports)
+        sn_port = self.get_snat_port_for_internal_port(port)
         if not sn_port:
             return
 
@@ -317,8 +299,7 @@ class DvrLocalRouter(router.RouterInfo):
         if not self.ex_gw_port:
             return
 
-        snat_ports = self.get_snat_interfaces()
-        sn_port = self._map_internal_interfaces(port, snat_ports)
+        sn_port = self.get_snat_port_for_internal_port(port)
         if not sn_port:
             return
 
@@ -348,14 +329,13 @@ class DvrLocalRouter(router.RouterInfo):
         ip_wrapr = ip_lib.IPWrapper(namespace=self.ns_name)
         ip_wrapr.netns.execute(['sysctl', '-w',
                                'net.ipv4.conf.all.send_redirects=0'])
-        snat_ports = self.get_snat_interfaces()
         for p in self.internal_ports:
-            gateway = self._map_internal_interfaces(p, snat_ports)
+            gateway = self.get_snat_port_for_internal_port(p)
             id_name = self.get_internal_device_name(p['id'])
             if gateway:
                 self._snat_redirect_add(gateway, p, id_name)
 
-        for port in snat_ports:
+        for port in self.get_snat_interfaces():
             for ip in port['fixed_ips']:
                 self._update_arp_entry(ip['ip_address'],
                                        port['mac_address'],
@@ -372,9 +352,8 @@ class DvrLocalRouter(router.RouterInfo):
             to_fip_interface_name = (
                 self.get_external_device_interface_name(ex_gw_port))
             self.process_floating_ip_addresses(to_fip_interface_name)
-        snat_ports = self.get_snat_interfaces()
         for p in self.internal_ports:
-            gateway = self._map_internal_interfaces(p, snat_ports)
+            gateway = self.get_snat_port_for_internal_port(p)
             internal_interface = self.get_internal_device_name(p['id'])
             self._snat_redirect_remove(gateway, p, internal_interface)
 

diff --git a/neutron/agent/l3/dvr_router_base.py b/neutron/agent/l3/dvr_router_base.py
new file mode 100644 (file)
index 0000000..0c872c4
--- /dev/null
+++ b/neutron/agent/l3/dvr_router_base.py
@@ -0,0 +1,42 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from oslo_log import log as logging
+
+from neutron.agent.l3 import router_info as router
+from neutron.common import constants as l3_constants
+from neutron.i18n import _LE
+
+LOG = logging.getLogger(__name__)
+
+
+class DvrRouterBase(router.RouterInfo):
+    def __init__(self, agent, host, *args, **kwargs):
+        super(DvrRouterBase, self).__init__(*args, **kwargs)
+
+        self.agent = agent
+        self.host = host
+
+    def get_snat_interfaces(self):
+        return self.router.get(l3_constants.SNAT_ROUTER_INTF_KEY, [])
+
+    def get_snat_port_for_internal_port(self, int_port):
+        """Return the SNAT port for the given internal interface port."""
+        snat_ports = self.get_snat_interfaces()
+        fixed_ip = int_port['fixed_ips'][0]
+        subnet_id = fixed_ip['subnet_id']
+        match_port = [p for p in snat_ports
+                      if p['fixed_ips'][0]['subnet_id'] == subnet_id]
+        if match_port:
+            return match_port[0]
+        else:
+            LOG.error(_LE('DVR: no map match_port found!'))

diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py
index 5ad2e75dbf9afa4b874a73aad154b71f2ed24d6b..09416ba0a176d7dacf0755a683a7f571f69e8244 100644 (file)
--- a/neutron/tests/unit/agent/l3/test_agent.py
+++ b/neutron/tests/unit/agent/l3/test_agent.py
@@ -337,7 +337,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         if action == 'add':
             self.device_exists.return_value = False
 
-            ri._map_internal_interfaces = mock.Mock(return_value=sn_port)
+            ri.get_snat_port_for_internal_port = mock.Mock(
+                return_value=sn_port)
             ri._snat_redirect_add = mock.Mock()
             ri._set_subnet_arp_info = mock.Mock()
             ri._internal_network_added = mock.Mock()
@@ -356,7 +357,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
                 dvr_snat_ns.SNAT_INT_DEV_PREFIX)
         elif action == 'remove':
             self.device_exists.return_value = False
-            ri._map_internal_interfaces = mock.Mock(return_value=sn_port)
+            ri.get_snat_port_for_internal_port = mock.Mock(
+                return_value=sn_port)
             ri._snat_redirect_modify = mock.Mock()
             ri.internal_network_removed(port)
             ri._snat_redirect_modify.assert_called_with(
@@ -432,8 +434,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
                 interface_name, ip_cidrs, **kwargs)
         else:
             ri._create_dvr_gateway.assert_called_once_with(
-                ex_gw_port, interface_name,
-                self.snat_ports)
+                ex_gw_port, interface_name)
 
     def _test_external_gateway_action(self, action, router, dual_stack=False):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
@@ -518,7 +519,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
 
         elif action == 'remove':
             self.device_exists.return_value = True
-            ri._map_internal_interfaces = mock.Mock(return_value=sn_port)
+            ri.get_snat_port_for_internal_port = mock.Mock(
+                return_value=sn_port)
             ri._snat_redirect_remove = mock.Mock()
             ri.external_gateway_removed(ex_gw_port, interface_name)
             if not router.get('distributed'):
@@ -700,7 +702,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             else:
                 self.assertIn(r.rule, expected_rules)
 
-    def test__map_internal_interfaces(self):
+    def test_get_snat_port_for_internal_port(self):
         router = l3_test_common.prepare_router_data(num_internal_ports=4)
         ri = dvr_router.DvrEdgeRouter(mock.sentinel.agent,
                                       HOSTNAME,
@@ -714,13 +716,15 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
                 'ip_address': '101.12.13.14'}]}
         internal_ports = ri.router.get(l3_constants.INTERFACE_KEY, [])
         # test valid case
-        res_port = ri._map_internal_interfaces(internal_ports[0], [test_port])
-        self.assertEqual(test_port, res_port)
-        # test invalid case
-        test_port['fixed_ips'][0]['subnet_id'] = 1234
-        res_ip = ri._map_internal_interfaces(internal_ports[0], [test_port])
-        self.assertNotEqual(test_port, res_ip)
-        self.assertIsNone(res_ip)
+        with mock.patch.object(ri, 'get_snat_interfaces') as get_interfaces:
+            get_interfaces.return_value = [test_port]
+            res_port = ri.get_snat_port_for_internal_port(internal_ports[0])
+            self.assertEqual(test_port, res_port)
+            # test invalid case
+            test_port['fixed_ips'][0]['subnet_id'] = 1234
+            res_ip = ri.get_snat_port_for_internal_port(internal_ports[0])
+            self.assertNotEqual(test_port, res_ip)
+            self.assertIsNone(res_ip)
 
     def test_process_cent_router(self):
         router = l3_test_common.prepare_router_data()
@@ -1953,7 +1957,9 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         interface_name = ri.get_snat_int_device_name(port_id)
         self.device_exists.return_value = False
 
-        ri._create_dvr_gateway(dvr_gw_port, interface_name, self.snat_ports)
+        with mock.patch.object(ri, 'get_snat_interfaces') as get_interfaces:
+            get_interfaces.return_value = self.snat_ports
+            ri._create_dvr_gateway(dvr_gw_port, interface_name)
 
         # check 2 internal ports are plugged
         # check 1 ext-gw-port is plugged