Fix KeyError during sync_routers

Method sync_routers is used by the L3 agent to query
routers it knows about. Routers and GW ports lists
are populated in two different times, which means that
they can be interleaved by a delete request which
results in gateway ports being missing in one of the
two data structures.

This patch takes care of the race condition.

Closes-bug: #1355409

Change-Id: Id3a6fe145058f690e107bfe7023980ede61cff90

diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py
index 58454ef62cd08ac94a5f2d7fbf58b861851be3b1..60aad28d25262a5a12f9ec6adea79e9dff84ea29 100644 (file)
--- a/neutron/db/l3_db.py
+++ b/neutron/db/l3_db.py
@@ -883,7 +883,8 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase):
     def _build_routers_list(self, context, routers, gw_ports):
         for router in routers:
             gw_port_id = router['gw_port_id']
-            if gw_port_id:
+            # Collect gw ports only if available
+            if gw_port_id and gw_ports.get(gw_port_id):
                 router['gw_port'] = gw_ports[gw_port_id]
         return routers
 
@@ -916,6 +917,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase):
             gw_ports = dict((gw_port['id'], gw_port)
                             for gw_port in
                             self.get_sync_gw_ports(context, gw_port_ids))
+        # NOTE(armando-migliaccio): between get_routers and get_sync_gw_ports
+        # gw ports may get deleted, which means that router_dicts may contain
+        # ports that gw_ports does not; we should rebuild router_dicts, but
+        # letting the callee check for missing gw_ports sounds like a good
+        # defensive approach regardless
         return self._build_routers_list(context, router_dicts, gw_ports)
 
     def _get_sync_floating_ips(self, context, router_ids):

diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py
index c272dd2a77f78a67fb97fdb4194daed25c3e0e1c..b73ad851827fdd7b2785745acfd63643d001035c 100644 (file)
--- a/neutron/db/l3_dvr_db.py
+++ b/neutron/db/l3_dvr_db.py
@@ -237,7 +237,8 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
 
         for rtr in routers:
             gw_port_id = rtr['gw_port_id']
-            if gw_port_id:
+            # Collect gw ports only if available
+            if gw_port_id and gw_ports.get(gw_port_id):
                 rtr['gw_port'] = gw_ports[gw_port_id]
                 if 'enable_snat' in rtr[l3.EXTERNAL_GW_INFO]:
                     rtr['enable_snat'] = (

diff --git a/neutron/db/l3_gwmode_db.py b/neutron/db/l3_gwmode_db.py
index d0ec612fbdbe4f06aa581be181ad368dc07bc3ed..dce6cafe90f3a5fbf0d84be922eb4d1a72c3aa23 100644 (file)
--- a/neutron/db/l3_gwmode_db.py
+++ b/neutron/db/l3_gwmode_db.py
@@ -65,7 +65,8 @@ class L3_NAT_dbonly_mixin(l3_db.L3_NAT_dbonly_mixin):
     def _build_routers_list(self, context, routers, gw_ports):
         for rtr in routers:
             gw_port_id = rtr['gw_port_id']
-            if gw_port_id:
+            # Collect gw ports only if available
+            if gw_port_id and gw_ports.get(gw_port_id):
                 rtr['gw_port'] = gw_ports[gw_port_id]
                 # Add enable_snat key
                 rtr['enable_snat'] = rtr[EXTERNAL_GW_INFO]['enable_snat']

diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py
index c0329ee6fa5f06119076c89bc79017c53a7f184b..5cec5587b278a6bbbe7c12e015505e90109b7849 100644 (file)
--- a/neutron/tests/unit/db/test_l3_dvr_db.py
+++ b/neutron/tests/unit/db/test_l3_dvr_db.py
@@ -167,3 +167,9 @@ class L3DvrTestCase(base.BaseTestCase):
             self.mixin._create_gw_port(
                 self.ctx, router_id, router_db, mock.ANY)
             self.assertFalse(cs.call_count)
+
+    def test_build_routers_list_with_gw_port_mismatch(self):
+        routers = [{'gw_port_id': 'foo_gw_port_id', 'id': 'foo_router_id'}]
+        gw_ports = {}
+        routers = self.mixin._build_routers_list(self.ctx, routers, gw_ports)
+        self.assertIsNone(routers[0].get('gw_port'))

diff --git a/neutron/tests/unit/test_extension_ext_gw_mode.py b/neutron/tests/unit/test_extension_ext_gw_mode.py
index 48c5aec635acb31879e2ac93ee5ff8f891f5df02..a9e97066abac20740cfba49ebb89456f9a97adde 100644 (file)
--- a/neutron/tests/unit/test_extension_ext_gw_mode.py
+++ b/neutron/tests/unit/test_extension_ext_gw_mode.py
@@ -288,6 +288,15 @@ class TestL3GwModeMixin(base.BaseTestCase):
         self.assertEqual(FAKE_GW_PORT_ID, router['gw_port']['id'])
         self.assertFalse(router.get('enable_snat'))
 
+    def test_build_routers_list_with_gw_port_mismatch(self):
+        router_dict = self.target_object._make_router_dict(self.router)
+        routers = self.target_object._build_routers_list(
+            self.context, [router_dict], {})
+        self.assertEqual(1, len(routers))
+        router = routers[0]
+        self.assertIsNone(router.get('gw_port'))
+        self.assertIsNone(router.get('enable_snat'))
+
 
 class ExtGwModeIntTestCase(test_db_plugin.NeutronDbPluginV2TestCase,
                            test_l3_plugin.L3NatTestCaseMixin):

diff --git a/neutron/tests/unit/test_l3_plugin.py b/neutron/tests/unit/test_l3_plugin.py
index 50f7ba7dc81d8b4d4df8f8591a8fa786fcbfb6fe..f1bc02b7e71eaa9151750bb47ada6d55d6b9358b 100644 (file)
--- a/neutron/tests/unit/test_l3_plugin.py
+++ b/neutron/tests/unit/test_l3_plugin.py
@@ -313,6 +313,19 @@ class TestL3NatAgentSchedulingServicePlugin(TestL3NatServicePlugin,
     supported_extension_aliases = ["router", "l3_agent_scheduler"]
 
 
+class L3NATdbonlyMixinTestCase(base.BaseTestCase):
+
+    def setUp(self):
+        super(L3NATdbonlyMixinTestCase, self).setUp()
+        self.mixin = l3_db.L3_NAT_dbonly_mixin()
+
+    def test_build_routers_list_with_gw_port_mismatch(self):
+        routers = [{'gw_port_id': 'foo_gw_port_id', 'id': 'foo_router_id'}]
+        gw_ports = {}
+        routers = self.mixin._build_routers_list(mock.ANY, routers, gw_ports)
+        self.assertIsNone(routers[0].get('gw_port'))
+
+
 class L3NatTestCaseMixin(object):
 
     def _create_router(self, fmt, tenant_id, name=None,