From dd71021347f782608575d551f2329715c00546ce Mon Sep 17 00:00:00 2001 From: Jian Wen Date: Wed, 27 Nov 2013 22:23:26 +0800 Subject: [PATCH] l3_agent: make process_router more robust If internal_network_added/removed fails, _sync_routers_task will call process_router to do fault recovery. Because the port is already added/removed to/from ri.internal_ports, internal_network_added or internal_network_removed will not be called again. The patch fix this issue by calling ri.internal_ports.append/removed only if internal_network_added/removed succeed. Without the patch, the added testcases would fail. Change-Id: I2d2e004caa670c1624257c1d7ccc900438b42d08 Co-Authored-By: Hirofumi Ichihara Closes-Bug: #1255871 --- neutron/agent/l3_agent.py | 4 +-- neutron/tests/unit/test_l3_agent.py | 55 +++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/neutron/agent/l3_agent.py b/neutron/agent/l3_agent.py index c9bbe135255..2a5e89485c4 100644 --- a/neutron/agent/l3_agent.py +++ b/neutron/agent/l3_agent.py @@ -391,13 +391,13 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager): p['id'] not in current_port_ids] for p in new_ports: self._set_subnet_info(p) - ri.internal_ports.append(p) self.internal_network_added(ri, p['network_id'], p['id'], p['ip_cidr'], p['mac_address']) + ri.internal_ports.append(p) for p in old_ports: - ri.internal_ports.remove(p) self.internal_network_removed(ri, p['id'], p['ip_cidr']) + ri.internal_ports.remove(p) internal_cidrs = [p['ip_cidr'] for p in ri.internal_ports] # TODO(salv-orlando): RouterInfo would be a better place for diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py index a23ece46600..cf78c328b12 100644 --- a/neutron/tests/unit/test_l3_agent.py +++ b/neutron/tests/unit/test_l3_agent.py @@ -574,6 +574,61 @@ class TestBasicRouterOperations(base.BaseTestCase): self._verify_snat_rules(nat_rules_delta, router, negate=True) self.send_arp.assert_called_once() + def test_process_router_internal_network_added_unexpected_error(self): + agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) + router = self._prepare_router_data() + ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper, + self.conf.use_namespaces, router=router) + with mock.patch.object( + l3_agent.L3NATAgent, + 'internal_network_added') as internal_network_added: + # raise RuntimeError to simulate that an unexpected exception + # occurrs + internal_network_added.side_effect = RuntimeError + self.assertRaises(RuntimeError, agent.process_router, ri) + self.assertNotIn( + router[l3_constants.INTERFACE_KEY][0], ri.internal_ports) + + # The unexpected exception has been fixed manually + internal_network_added.side_effect = None + + # _sync_routers_task finds out that _rpc_loop failed to process the + # router last time, it will retry in the next run. + agent.process_router(ri) + # We were able to add the port to ri.internal_ports + self.assertIn( + router[l3_constants.INTERFACE_KEY][0], ri.internal_ports) + + def test_process_router_internal_network_removed_unexpected_error(self): + agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) + router = self._prepare_router_data() + ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper, + self.conf.use_namespaces, router=router) + # add an internal port + agent.process_router(ri) + + with mock.patch.object( + l3_agent.L3NATAgent, + 'internal_network_removed') as internal_net_removed: + # raise RuntimeError to simulate that an unexpected exception + # occurrs + internal_net_removed.side_effect = RuntimeError + ri.internal_ports[0]['admin_state_up'] = False + # The above port is set to down state, remove it. + self.assertRaises(RuntimeError, agent.process_router, ri) + self.assertIn( + router[l3_constants.INTERFACE_KEY][0], ri.internal_ports) + + # The unexpected exception has been fixed manually + internal_net_removed.side_effect = None + + # _sync_routers_task finds out that _rpc_loop failed to process the + # router last time, it will retry in the next run. + agent.process_router(ri) + # We were able to remove the port from ri.internal_ports + self.assertNotIn( + router[l3_constants.INTERFACE_KEY][0], ri.internal_ports) + def test_handle_router_snat_rules_add_back_jump(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) ri = mock.MagicMock()