From 517fe11211d49dff3772c114b533c797d020c613 Mon Sep 17 00:00:00 2001 From: armando-migliaccio Date: Tue, 12 Aug 2014 09:11:50 -0700 Subject: [PATCH] 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 --- neutron/db/l3_db.py | 8 +++++++- neutron/db/l3_dvr_db.py | 3 ++- neutron/db/l3_gwmode_db.py | 3 ++- neutron/tests/unit/db/test_l3_dvr_db.py | 6 ++++++ neutron/tests/unit/test_extension_ext_gw_mode.py | 9 +++++++++ neutron/tests/unit/test_l3_plugin.py | 13 +++++++++++++ 6 files changed, 39 insertions(+), 3 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 58454ef62c..60aad28d25 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 c272dd2a77..b73ad85182 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 d0ec612fbd..dce6cafe90 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 c0329ee6fa..5cec5587b2 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 48c5aec635..a9e97066ab 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 50f7ba7dc8..f1bc02b7e7 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,