From da141f0859fe5d7e77cd8fbdd84b59e19ffc7d17 Mon Sep 17 00:00:00 2001 From: Daniel Gonzalez Date: Mon, 12 Mar 2018 17:48:54 +0100 Subject: [PATCH] Fix l3-agent crash on routers without ha_state l3-agent checks the HA state of routers when a router is updated. To ensure that the HA state is only checked on HA routers the following check is performed: `if router.get('ha') and not is_dvr_only_agent`. This check should ensure that the check is only performed on DvrEdgeHaRouter and HaRouter objects. Unfortunately, there are cases where we have DvrEdgeRouter objects running on 'dvr_snat' agents. E.g. when deploying a loadbalancer with neutron-lbaas in a landscape with 6 network nodes and max_l3_agents_per_router set to 3, it may happen that the loadbalancer is placed on a network node that does not have a DvrEdgeHaRouter running on it. In such a case, neutron will deploy a DvrEdgeRouter object on the network node to serve the loadbalancer, just like it would deploy a DvrEdgeRouter on a compute node when deploying a VM. Under such circumstances each update to the router will lead to an AttributeError, because the DvrEdgeRouter object does not have the ha_state attribute. This patch circumvents the issue by doing an additional check on the router object to ensure that it actually has the ha_state attribute. Closes-Bug: #1755243 Change-Id: I755990324db445efd0ee0b8a9db1f4d7bfb58e26 (cherry picked from commit 8c2dae659a806fdc20331de4b8a917ec3ae0e6f6) --- neutron/agent/l3/agent.py | 5 +-- .../functional/agent/l3/test_dvr_router.py | 32 ++++++++++++++++--- neutron/tests/unit/agent/l3/test_agent.py | 7 ++-- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py index bc1f7da5d37..98c264a1917 100644 --- a/neutron/agent/l3/agent.py +++ b/neutron/agent/l3/agent.py @@ -489,14 +489,15 @@ class L3NATAgent(ha.AgentMixin, self.l3_ext_manager.add_router(self.context, router) def _process_updated_router(self, router): + ri = self.router_info[router['id']] is_dvr_only_agent = (self.conf.agent_mode in [lib_const.L3_AGENT_MODE_DVR, l3_constants.L3_AGENT_MODE_DVR_NO_EXTERNAL]) + is_ha_router = getattr(ri, 'ha_state', None) is not None # For HA routers check that DB state matches actual state - if router.get('ha') and not is_dvr_only_agent: + if router.get('ha') and not is_dvr_only_agent and is_ha_router: self.check_ha_state_for_router( router['id'], router.get(l3_constants.HA_ROUTER_STATE_KEY)) - ri = self.router_info[router['id']] ri.router = router registry.notify(resources.ROUTER, events.BEFORE_UPDATE, self, router=ri) diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py index 9735ae41ce5..446a9fad623 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -1124,11 +1124,11 @@ class TestDvrRouter(framework.L3AgentTestFramework): self.assertFalse(sg_device) self.assertTrue(qg_device) - def _mocked_dvr_ha_router(self, agent, enable_gw=True, + def _mocked_dvr_ha_router(self, agent, enable_ha=True, enable_gw=True, enable_centralized_fip=False, snat_bound_fip=False): r_info = self.generate_dvr_router_info( - enable_ha=True, + enable_ha=enable_ha, enable_snat=True, agent=agent, enable_gw=enable_gw, @@ -1164,7 +1164,7 @@ class TestDvrRouter(framework.L3AgentTestFramework): def _create_dvr_ha_router(self, agent, enable_gw=True, enable_centralized_fip=False, - snat_bound_fip=False): + snat_bound_fip=False, ha_interface=True): get_ns_name = mock.patch.object(namespaces.RouterNamespace, '_get_ns_name').start() get_snat_ns_name = mock.patch.object(dvr_snat_ns.SnatNamespace, @@ -1172,7 +1172,11 @@ class TestDvrRouter(framework.L3AgentTestFramework): (r_info, mocked_r_ns_name, mocked_r_snat_ns_name) = self._mocked_dvr_ha_router( - agent, enable_gw, enable_centralized_fip, snat_bound_fip) + agent, ha_interface, enable_gw, enable_centralized_fip, + snat_bound_fip) + + if not ha_interface: + r_info['ha'] = True get_ns_name.return_value = mocked_r_ns_name get_snat_ns_name.return_value = mocked_r_snat_ns_name @@ -1330,6 +1334,26 @@ class TestDvrRouter(framework.L3AgentTestFramework): def test_dvr_ha_router_failover_without_gw(self): self._test_dvr_ha_router_failover(enable_gw=False) + def test_dvr_non_ha_router_update(self): + self._setup_dvr_ha_agents() + self._setup_dvr_ha_bridges() + + router1 = self._create_dvr_ha_router(self.agent) + router2 = self._create_dvr_ha_router(self.failover_agent, + ha_interface=False) + + r1_chsfr = mock.patch.object(self.agent, + 'check_ha_state_for_router').start() + r2_chsfr = mock.patch.object(self.failover_agent, + 'check_ha_state_for_router').start() + + utils.wait_until_true(lambda: router1.ha_state == 'master') + + self.agent._process_updated_router(router1.router) + self.assertTrue(r1_chsfr.called) + self.failover_agent._process_updated_router(router2.router) + self.assertFalse(r2_chsfr.called) + def _setup_dvr_router_static_routes( self, router_namespace=True, check_fpr_int_rule_delete=False): """Test to validate the extra routes on dvr routers.""" diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index 540395f546d..d19bfece0b9 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -2532,9 +2532,10 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): 'distributed': True, 'ha': True, 'external_gateway_info': {}, 'routes': [], 'admin_state_up': True} - - agent._process_router_if_compatible(router) - self.assertIn(router['id'], agent.router_info) + with mock.patch.object(agent, 'check_ha_state_for_router') as chsfr: + agent._process_router_if_compatible(router) + self.assertIn(router['id'], agent.router_info) + self.assertFalse(chsfr.called) def test_process_router_if_compatible_with_no_ext_net_in_conf(self): self.conf.set_override('external_network_bridge', 'br-ex')