From 52ea36e71125d26e253eb359a56bd4b069871854 Mon Sep 17 00:00:00 2001 From: Oleg Bondarev Date: Wed, 30 Dec 2015 17:48:49 +0300 Subject: [PATCH] Refactor remove_router_interface() for DVR Currently this method duplicates code from l3_db, looks complicated and hard to read. The patch refactors it to reuse code from l3_db just adding needed dvr related handling. New methods (introduced by parent patch) were used to know which dvr agents should router be deleted from. This will be needed when we remove explicit binding of dvr agents to routers in later patches. The patch also removes useless 'mock all' kind of test and replaces it with a functional test. Partially implements blueprint improve-dvr-l3-agent-binding Change-Id: I6f56b0534e65bf036f4ebe34c51aef52c5859cd3 --- neutron/db/l3_db.py | 10 +- neutron/db/l3_dvr_db.py | 105 +++++++----------- .../l3_router/test_l3_dvr_router_plugin.py | 37 ++++++ neutron/tests/unit/db/test_l3_dvr_db.py | 2 - 4 files changed, 85 insertions(+), 69 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 23ea98c3746..766d1ebc969 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -632,11 +632,12 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): @staticmethod def _make_router_interface_info( - router_id, tenant_id, port_id, subnet_id, subnet_ids): + router_id, tenant_id, port_id, network_id, subnet_id, subnet_ids): return { 'id': router_id, 'tenant_id': tenant_id, 'port_id': port_id, + 'network_id': network_id, 'subnet_id': subnet_id, # deprecated by IPv6 multi-prefix 'subnet_ids': subnet_ids } @@ -668,8 +669,8 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): context.session.add(router_port) return self._make_router_interface_info( - router.id, port['tenant_id'], port['id'], subnets[-1]['id'], - [subnet['id'] for subnet in subnets]) + router.id, port['tenant_id'], port['id'], port['network_id'], + subnets[-1]['id'], [subnet['id'] for subnet in subnets]) def _confirm_router_interface_not_in_use(self, context, router_id, subnet_id): @@ -772,7 +773,8 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase): context, router_id, subnet_id, device_owner) return self._make_router_interface_info(router_id, port['tenant_id'], - port['id'], subnets[0]['id'], + port['id'], port['network_id'], + subnets[0]['id'], [subnet['id'] for subnet in subnets]) diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index 3167fc83b05..9e13a713453 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -298,8 +298,8 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, LOG.debug("CSNAT port updated for IPv6 subnet: " "%s", updated_port) router_interface_info = self._make_router_interface_info( - router_id, port['tenant_id'], port['id'], subnet['id'], - [subnet['id']]) + router_id, port['tenant_id'], port['id'], port['network_id'], + subnet['id'], [subnet['id']]) self.notify_router_interface_action( context, router_interface_info, 'add') return router_interface_info @@ -322,7 +322,7 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, return port def _check_for_multiprefix_csnat_port_and_update( - self, context, router, subnet, port): + self, context, router, network_id, subnet_id): """Checks if the csnat port contains multiple ipv6 prefixes. If the csnat port contains multiple ipv6 prefixes for the given @@ -331,13 +331,11 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, it with the right fixed_ip. This function returns true if it is a multiprefix port. """ - subnet_id = subnet['id'] if router.gw_port: # If router has a gateway port, check if it has IPV6 subnet cs_port = ( self._find_router_port_by_network_and_device_owner( - router, subnet['network_id'], - l3_const.DEVICE_OWNER_ROUTER_SNAT)) + router, network_id, l3_const.DEVICE_OWNER_ROUTER_SNAT)) if cs_port: fixed_ips = ( [fixedip for fixedip in @@ -351,68 +349,49 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, return True return False - def _check_dvr_router_remove_required_and_notify_agent( - self, context, router, port, subnet): - if router.extra_attributes.distributed: - is_multiple_prefix_csport = ( - self._check_for_multiprefix_csnat_port_and_update( - context, router, subnet, port)) - if not is_multiple_prefix_csport: - # Single prefix port - go ahead and delete the port - self.delete_csnat_router_interface_ports( - context.elevated(), router, subnet_id=subnet['id']) - plugin = manager.NeutronManager.get_service_plugins().get( - constants.L3_ROUTER_NAT) - l3_agents = plugin.get_l3_agents_hosting_routers(context, - [router['id']]) - 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: - 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']]) - self.notify_router_interface_action( - context, router_interface_info, 'remove') - return router_interface_info - def remove_router_interface(self, context, router_id, interface_info): - remove_by_port, remove_by_subnet = ( - self._validate_interface_info(interface_info, for_removal=True) - ) - port_id = interface_info.get('port_id') - subnet_id = interface_info.get('subnet_id') router = self._get_router(context, router_id) - device_owner = self._get_device_owner(context, router) + if not router.extra_attributes.distributed: + return super( + L3_NAT_with_dvr_db_mixin, self).remove_router_interface( + context, router_id, interface_info) - if remove_by_port: - port, subnets = self._remove_interface_by_port( - context, router_id, port_id, subnet_id, device_owner) + plugin = manager.NeutronManager.get_service_plugins().get( + constants.L3_ROUTER_NAT) + router_hosts_before = plugin._get_dvr_hosts_for_router( + context, router_id) - # remove_by_subnet is not used here, because the validation logic of - # _validate_interface_info ensures that at least one of remote_by_* - # is True. - else: - port, subnets = self._remove_interface_by_subnet( - context, router_id, subnet_id, device_owner) + interface_info = super( + L3_NAT_with_dvr_db_mixin, self).remove_router_interface( + context, router_id, interface_info) - subnet = subnets[0] - router_interface_info = ( - self._check_dvr_router_remove_required_and_notify_agent( - context, router, port, subnet)) - return router_interface_info + router_hosts_after = plugin._get_dvr_hosts_for_router( + context, router_id) + removed_hosts = set(router_hosts_before) - set(router_hosts_after) + if removed_hosts: + agents = plugin.get_l3_agents(context, + filters={'host': removed_hosts}) + binding_table = l3_dvrsched_db.CentralizedSnatL3AgentBinding + snat_binding = context.session.query(binding_table).filter_by( + router_id=router_id).first() + for agent in agents: + is_this_snat_agent = ( + snat_binding and snat_binding.l3_agent_id == agent['id']) + if not is_this_snat_agent: + plugin.remove_router_from_l3_agent( + context, agent['id'], router_id) + + is_multiple_prefix_csport = ( + self._check_for_multiprefix_csnat_port_and_update( + context, router, interface_info['network_id'], + interface_info['subnet_id'])) + if not is_multiple_prefix_csport: + # Single prefix port - go ahead and delete the port + self.delete_csnat_router_interface_ports( + context.elevated(), router, + subnet_id=interface_info['subnet_id']) + + return interface_info def _get_snat_sync_interfaces(self, context, router_ids): """Query router interfaces that relate to list of router_ids.""" 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 0c91a3fc0c6..c69790fd95e 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 @@ -724,3 +724,40 @@ class L3DvrTestCase(ml2_test_base.ML2TestFramework): self.assertEqual(self.l3_agent['id'], agents['agents'][0]['id']) self.assertFalse(remove_mock.called) + + def test_remove_router_interface(self): + HOST1 = 'host1' + dvr_agent = helpers.register_l3_agent( + host=HOST1, agent_mode=constants.L3_AGENT_MODE_DVR) + router = self._create_router() + arg_list = (portbindings.HOST_ID,) + with self.subnet() as subnet,\ + self.port(subnet=subnet, + device_owner=DEVICE_OWNER_COMPUTE, + arg_list=arg_list, + **{portbindings.HOST_ID: HOST1}): + l3_notifier = mock.Mock() + self.l3_plugin.l3_rpc_notifier = l3_notifier + self.l3_plugin.agent_notifiers[ + constants.AGENT_TYPE_L3] = l3_notifier + + self.l3_plugin.add_router_interface( + self.context, router['id'], + {'subnet_id': subnet['subnet']['id']}) + self.l3_plugin.schedule_router(self.context, router['id']) + + # router should be scheduled to the agent on HOST1 + agents = self.l3_plugin.list_l3_agents_hosting_router( + self.context, router['id'])['agents'] + self.assertEqual(1, len(agents)) + self.assertEqual(dvr_agent['id'], agents[0]['id']) + + 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'])['agents'] + self.assertEqual(0, len(agents)) + l3_notifier.router_removed_from_agent.assert_called_once_with( + self.context, router['id'], HOST1) diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index 750c8ec795d..3833d8db393 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -471,7 +471,6 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): 'distributed': True} router = self._create_router(router_dict) plugin = mock.MagicMock() - plugin.get_subnet_ids_on_router = mock.Mock() with self.network() as net_ext,\ self.subnet() as subnet1,\ self.subnet(cidr='20.0.0.0/24') as subnet2: @@ -517,7 +516,6 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): dvr_ports = self.core_plugin.get_ports( self.ctx, filters=dvr_filters) self.assertEqual(1, len(dvr_ports)) - self.assertEqual(1, plugin.get_subnet_ids_on_router.call_count) def test__validate_router_migration_notify_advanced_services(self): router = {'name': 'foo_router', 'admin_state_up': False}