From: armando-migliaccio <armamig@gmail.com>
Date: Tue, 12 Aug 2014 16:11:50 +0000 (-0700)
Subject: Fix KeyError during sync_routers
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=aee5344db7972e2e12ed056c2e6467f7952aec3f;p=openstack-build%2Fneutron-build.git

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 58454ef62..60aad28d2 100644
--- 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 c272dd2a7..b73ad8518 100644
--- 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 d0ec612fb..dce6cafe9 100644
--- 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 c0329ee6f..5cec5587b 100644
--- 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 48c5aec63..a9e97066a 100644
--- 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 50f7ba7dc..f1bc02b7e 100644
--- 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,