diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index 5d34bb8b0d1..e9bef3f33b8 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -368,11 +368,19 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, subnet_ids = plugin.get_subnet_ids_on_router( context, router['id']) if subnet_ids: + binding_table = l3_dvrsched_db.CentralizedSnatL3AgentBinding + snat_binding = context.session.query(binding_table).filter_by( + router_id=router['id']).first() for l3_agent in l3_agents: - if not plugin.check_dvr_serviceable_ports_on_host( - context, l3_agent['host'], subnet_ids): - plugin.remove_router_from_l3_agent( - context, l3_agent['id'], router['id']) + is_this_snat_agent = ( + snat_binding and + snat_binding.l3_agent_id == l3_agent['id']) + if (is_this_snat_agent or + plugin.check_dvr_serviceable_ports_on_host( + context, l3_agent['host'], subnet_ids)): + continue + plugin.remove_router_from_l3_agent( + context, l3_agent['id'], router['id']) router_interface_info = self._make_router_interface_info( router['id'], port['tenant_id'], port['id'], subnet['id'], [subnet['id']]) diff --git a/neutron/db/l3_dvrscheduler_db.py b/neutron/db/l3_dvrscheduler_db.py index b24039dfd11..4113748b70d 100644 --- a/neutron/db/l3_dvrscheduler_db.py +++ b/neutron/db/l3_dvrscheduler_db.py @@ -178,8 +178,17 @@ class L3_DVRsch_db_mixin(l3agent_sch_db.L3AgentSchedulerDbMixin): 'on host %(host)s', {'port': port_id, 'host': port_host}) return [] + agent = self._get_agent_by_type_and_host( + context, n_const.AGENT_TYPE_L3, port_host) removed_router_info = [] for router_id in router_ids: + snat_binding = context.session.query( + CentralizedSnatL3AgentBinding).filter_by( + router_id=router_id).filter_by( + l3_agent_id=agent.id).first() + if snat_binding: + # not removing from the agent hosting SNAT for the router + continue subnet_ids = self.get_subnet_ids_on_router(admin_context, router_id) if self.check_dvr_serviceable_ports_on_host( @@ -199,9 +208,7 @@ class L3_DVRsch_db_mixin(l3agent_sch_db.L3AgentSchedulerDbMixin): # unbind this port from router dvr_binding['router_id'] = None dvr_binding.update(dvr_binding) - agent = self._get_agent_by_type_and_host(context, - n_const.AGENT_TYPE_L3, - port_host) + info = {'router_id': router_id, 'host': port_host, 'agent_id': str(agent.id)} removed_router_info.append(info) diff --git a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py index 7bc01d3d5cf..6cc146ef24b 100644 --- a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py +++ b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py @@ -654,3 +654,82 @@ class L3DvrTestCase(ml2_test_base.ML2TestFramework): topic=topics.L3_AGENT, version='1.1')] mock_prepare.assert_has_calls(expected, any_order=True) + + def test_router_is_not_removed_from_snat_agent_on_interface_removal(self): + """Check that dvr router is not removed from l3 agent hosting + SNAT for it on router interface removal + """ + router = self._create_router() + kwargs = {'arg_list': (external_net.EXTERNAL,), + external_net.EXTERNAL: True} + host = self.l3_agent['host'] + with self.subnet() as subnet,\ + self.network(**kwargs) as ext_net,\ + self.subnet(network=ext_net, cidr='20.0.0.0/24'): + self.l3_plugin._update_router_gw_info( + self.context, router['id'], + {'network_id': ext_net['network']['id']}) + self.l3_plugin.add_router_interface( + self.context, router['id'], + {'subnet_id': subnet['subnet']['id']}) + + agents = self.l3_plugin.list_l3_agents_hosting_router( + self.context, router['id']) + self.assertEqual(1, len(agents['agents'])) + csnat_agent_host = self.l3_plugin.get_snat_bindings( + self.context, [router['id']])[0]['l3_agent']['host'] + self.assertEqual(host, csnat_agent_host) + with mock.patch.object(self.l3_plugin, + '_l3_rpc_notifier') as l3_notifier: + self.l3_plugin.remove_router_interface( + self.context, router['id'], + {'subnet_id': subnet['subnet']['id']}) + agents = self.l3_plugin.list_l3_agents_hosting_router( + self.context, router['id']) + self.assertEqual(1, len(agents['agents'])) + self.assertFalse(l3_notifier.router_removed_from_agent.called) + + def test_router_is_not_removed_from_snat_agent_on_dhcp_port_deletion(self): + """Check that dvr router is not removed from l3 agent hosting + SNAT for it on DHCP port removal + """ + router = self._create_router() + kwargs = {'arg_list': (external_net.EXTERNAL,), + external_net.EXTERNAL: True} + with self.network(**kwargs) as ext_net,\ + self.subnet(network=ext_net),\ + self.subnet(cidr='20.0.0.0/24') as subnet,\ + self.port(subnet=subnet, + device_owner=constants.DEVICE_OWNER_DHCP) as port: + self.core_plugin.update_port( + self.context, port['port']['id'], + {'port': {'binding:host_id': self.l3_agent['host']}}) + self.l3_plugin._update_router_gw_info( + self.context, router['id'], + {'network_id': ext_net['network']['id']}) + self.l3_plugin.add_router_interface( + self.context, router['id'], + {'subnet_id': subnet['subnet']['id']}) + + # router should be scheduled to the dvr_snat l3 agent + csnat_agent_host = self.l3_plugin.get_snat_bindings( + self.context, [router['id']])[0]['l3_agent']['host'] + self.assertEqual(self.l3_agent['host'], csnat_agent_host) + agents = self.l3_plugin.list_l3_agents_hosting_router( + self.context, router['id']) + self.assertEqual(1, len(agents['agents'])) + self.assertEqual(self.l3_agent['id'], agents['agents'][0]['id']) + + notifier = self.l3_plugin.agent_notifiers[ + constants.AGENT_TYPE_L3] + with mock.patch.object( + notifier, 'router_removed_from_agent') as remove_mock: + self._delete('ports', port['port']['id']) + # now when port is deleted the router still has external + # gateway and should still be scheduled to the snat agent + agents = self.l3_plugin.list_l3_agents_hosting_router( + self.context, router['id']) + self.assertEqual(1, len(agents['agents'])) + self.assertEqual(self.l3_agent['id'], + agents['agents'][0]['id']) + self.assertFalse(remove_mock.called) diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index 84b772f8358..c03b20b4b81 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -479,47 +479,6 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): fip, floatingip, router)) self.assertFalse(create_fip.called) - def test_remove_router_interface_delete_router_l3agent_binding(self): - interface_info = {'subnet_id': '123'} - router = mock.MagicMock() - router.extra_attributes.distributed = True - plugin = mock.MagicMock() - plugin.get_l3_agents_hosting_routers = mock.Mock( - return_value=[mock.MagicMock()]) - plugin.get_subnet_ids_on_router = mock.Mock( - return_value=interface_info) - plugin.check_dvr_serviceable_ports_on_host = mock.Mock( - return_value=False) - plugin.remove_router_from_l3_agent = mock.Mock( - return_value=None) - with mock.patch.object(self.mixin, '_get_router') as grtr,\ - mock.patch.object(self.mixin, '_get_device_owner') as gdev,\ - mock.patch.object(self.mixin, - '_remove_interface_by_subnet') as rmintf,\ - mock.patch.object( - self.mixin, - 'delete_csnat_router_interface_ports') as delintf,\ - mock.patch.object(manager.NeutronManager, - 'get_service_plugins') as gplugin,\ - mock.patch.object(self.mixin, - '_make_router_interface_info') as mkintf,\ - mock.patch.object(self.mixin, - 'notify_router_interface_action') as notify: - grtr.return_value = router - gdev.return_value = mock.Mock() - rmintf.return_value = (mock.MagicMock(), mock.MagicMock()) - mkintf.return_value = mock.Mock() - gplugin.return_value = {plugin_const.L3_ROUTER_NAT: plugin} - delintf.return_value = None - notify.return_value = None - - self.mixin.manager = manager - self.mixin.remove_router_interface( - self.ctx, mock.Mock(), interface_info) - self.assertTrue(plugin.get_l3_agents_hosting_routers.called) - self.assertTrue(plugin.check_dvr_serviceable_ports_on_host.called) - self.assertTrue(plugin.remove_router_from_l3_agent.called) - def test_remove_router_interface_csnat_ports_removal(self): router_dict = {'name': 'test_router', 'admin_state_up': True, 'distributed': True}