From 062ad0a0a62ab49bd0c27e73fbe93d739f82f410 Mon Sep 17 00:00:00 2001 From: Swaminathan Vasudevan Date: Wed, 4 Nov 2015 18:02:09 -0800 Subject: [PATCH] Change check_ports_exist_on_l3agent to pass the subnet_ids The get_subnet_ids_on_router is called for every available l3agent in check_ports_exist_on_l3agent. This introduces un-necessary call to the same function multiple times which is expensive since it calls get_ports internally. In large scale the time taken to schedule a VM on a given N-Node increases. By passing the subnet_ids to check_ports_exist_on_l3agent we would be only calling once get_subnet_ids_on_router in the get_l3_agent_candidates. This patch addresses the above problem by calling get_subnet_ids_on_router just once. Change-Id: I9d130f98e07bfe571eee32b827ff9af4010ff0fb Related-Bug: #1513678 --- neutron/db/l3_agentschedulers_db.py | 20 +++++----- neutron/db/l3_dvr_db.py | 13 ++++--- .../tests/unit/db/test_agentschedulers_db.py | 7 +++- neutron/tests/unit/db/test_l3_dvr_db.py | 8 +++- .../unit/scheduler/test_l3_agent_scheduler.py | 39 +++++++------------ 5 files changed, 44 insertions(+), 43 deletions(-) diff --git a/neutron/db/l3_agentschedulers_db.py b/neutron/db/l3_agentschedulers_db.py index cceb33d3d43..e2a01bb829c 100644 --- a/neutron/db/l3_agentschedulers_db.py +++ b/neutron/db/l3_agentschedulers_db.py @@ -439,15 +439,12 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase, if agentschedulers_db.AgentSchedulerDbMixin.is_eligible_agent( active, l3_agent)] - def check_ports_exist_on_l3agent(self, context, l3_agent, router_id): + def check_ports_exist_on_l3agent( + self, context, l3_agent, subnet_ids): """ This function checks for existence of dvr serviceable ports on the host, running the input l3agent. """ - subnet_ids = self.get_subnet_ids_on_router(context, router_id) - if not subnet_ids: - return False - core_plugin = manager.NeutronManager.get_plugin() # NOTE(swami):Before checking for existence of dvr # serviceable ports on the host managed by the l3 @@ -479,6 +476,10 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase, ignore_admin_state=False): """Get the valid l3 agents for the router from a list of l3_agents.""" candidates = [] + is_router_distributed = sync_router.get('distributed', False) + if is_router_distributed: + subnet_ids = self.get_subnet_ids_on_router( + context, sync_router['id']) for l3_agent in l3_agents: if not ignore_admin_state and not l3_agent.admin_state_up: # ignore_admin_state True comes from manual scheduling @@ -500,16 +501,15 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase, (ex_net_id and gateway_external_network_id and ex_net_id != gateway_external_network_id)): continue - is_router_distributed = sync_router.get('distributed', False) if agent_mode in ( constants.L3_AGENT_MODE_LEGACY, constants.L3_AGENT_MODE_DVR_SNAT) and ( not is_router_distributed): candidates.append(l3_agent) - elif is_router_distributed and agent_mode.startswith( - constants.L3_AGENT_MODE_DVR) and ( - self.check_ports_exist_on_l3agent( - context, l3_agent, sync_router['id'])): + elif (is_router_distributed and subnet_ids and + agent_mode.startswith(constants.L3_AGENT_MODE_DVR) and ( + self.check_ports_exist_on_l3agent( + context, l3_agent, subnet_ids))): candidates.append(l3_agent) return candidates diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index 67d8a786dcb..3a4fcfb3f10 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -375,11 +375,14 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, constants.L3_ROUTER_NAT) l3_agents = plugin.get_l3_agents_hosting_routers(context, [router['id']]) - for l3_agent in l3_agents: - if not plugin.check_ports_exist_on_l3agent(context, l3_agent, - router['id']): - plugin.remove_router_from_l3_agent( - context, l3_agent['id'], router['id']) + subnet_ids = plugin.get_subnet_ids_on_router( + context, router['id']) + if subnet_ids: + for l3_agent in l3_agents: + if not plugin.check_ports_exist_on_l3agent( + context, l3_agent, subnet_ids): + 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/tests/unit/db/test_agentschedulers_db.py b/neutron/tests/unit/db/test_agentschedulers_db.py index c3e4bc02371..eafb2235ab3 100644 --- a/neutron/tests/unit/db/test_agentschedulers_db.py +++ b/neutron/tests/unit/db/test_agentschedulers_db.py @@ -758,13 +758,18 @@ class OvsAgentSchedulerTestCase(OvsAgentSchedulerTestCaseBase): router = {'name': 'router1', 'admin_state_up': True, 'distributed': True} + subnet_ids = {'id': '1234'} r = self.l3plugin.create_router( self.adminContext, {'router': router}) dvr_agent = self._register_dvr_agents()[1] with mock.patch.object( self.l3plugin, - 'check_ports_exist_on_l3agent') as port_exists: + 'check_ports_exist_on_l3agent') as port_exists,\ + mock.patch.object( + self.l3plugin, + 'get_subnet_ids_on_router') as rtr_subnets: + rtr_subnets.return_value = [subnet_ids] port_exists.return_value = True self.l3plugin.schedule_router( self.adminContext, r['id']) diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index 80cf11017c1..df603f48169 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -483,6 +483,8 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): 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_ports_exist_on_l3agent = mock.Mock( return_value=False) plugin.remove_router_from_l3_agent = mock.Mock( @@ -519,6 +521,8 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): router_dict = {'name': 'test_router', 'admin_state_up': True, '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: @@ -550,7 +554,8 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): with mock.patch.object(manager.NeutronManager, 'get_service_plugins') as get_svc_plugin: get_svc_plugin.return_value = { - plugin_const.L3_ROUTER_NAT: self.mixin} + plugin_const.L3_ROUTER_NAT: plugin} + self.mixin.manager = manager self.mixin.remove_router_interface( self.ctx, router['id'], {'port_id': dvr_ports[0]['id']}) @@ -563,6 +568,7 @@ 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} diff --git a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py index 3be8c872390..670e5a0a0ce 100644 --- a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py +++ b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py @@ -616,6 +616,7 @@ class L3SchedulerTestBaseMixin(object): agent_list = [self.agent1, self.l3_dvr_agent] # test dvr agent_mode case only dvr agent should be candidate router['distributed'] = True + self.get_subnet_ids_on_router = mock.Mock() self.check_ports_exist_on_l3agent = mock.Mock(return_value=True) self._check_get_l3_agent_candidates(router, agent_list, HOST_DVR) @@ -629,6 +630,7 @@ class L3SchedulerTestBaseMixin(object): agent_list = [self.agent1, self.l3_dvr_agent] router['distributed'] = True # Test no VMs present case + self.get_subnet_ids_on_router = mock.Mock() self.check_ports_exist_on_l3agent = mock.Mock(return_value=False) self._check_get_l3_agent_candidates( router, agent_list, HOST_DVR, count=0) @@ -643,6 +645,7 @@ class L3SchedulerTestBaseMixin(object): router['distributed'] = True agent_list = [self.l3_dvr_snat_agent] + self.get_subnet_ids_on_router = mock.Mock() self.check_ports_exist_on_l3agent = mock.Mock(return_value=True) self._check_get_l3_agent_candidates(router, agent_list, HOST_DVR_SNAT) @@ -658,6 +661,7 @@ class L3SchedulerTestBaseMixin(object): agent_list = [self.l3_dvr_snat_agent] self.check_ports_exist_on_l3agent = mock.Mock(return_value=False) # Test no VMs present case + self.get_subnet_ids_on_router = mock.Mock() self.check_ports_exist_on_l3agent.return_value = False self._check_get_l3_agent_candidates( router, agent_list, HOST_DVR_SNAT, count=0) @@ -684,14 +688,13 @@ class L3SchedulerTestBaseMixin(object): router['external_gateway_info'] = None router['id'] = str(uuid.uuid4()) self.plugin.get_ports = mock.Mock(return_value=[]) - self.get_subnet_ids_on_router = mock.Mock(return_value=[]) - return l3_agent, router + return l3_agent def test_check_ports_exist_on_l3agent_no_subnets(self): - l3_agent, router = self._prepare_check_ports_exist_tests() + l3_agent = self._prepare_check_ports_exist_tests() # no subnets - val = self.check_ports_exist_on_l3agent(self.adminContext, - l3_agent, router['id']) + val = self.check_ports_exist_on_l3agent( + self.adminContext, l3_agent, []) self.assertFalse(val) def test_check_ports_exist_on_l3agent_with_dhcp_enabled_subnets(self): @@ -707,38 +710,24 @@ class L3SchedulerTestBaseMixin(object): subnet = {'id': str(uuid.uuid4()), 'enable_dhcp': True} - self.get_subnet_ids_on_router = mock.Mock( - return_value=[subnet['id']]) - self.plugin.get_subnet = mock.Mock(return_value=subnet) self.plugin.get_ports = mock.Mock() val = self.check_ports_exist_on_l3agent( - self.adminContext, agent_list[0], router['id']) + self.adminContext, agent_list[0], [subnet['id']]) self.assertTrue(val) self.assertFalse(self.plugin.get_ports.called) - def test_check_ports_exist_on_l3agent_if_no_subnets_then_return(self): - l3_agent, router = self._prepare_check_ports_exist_tests() - with mock.patch.object(manager.NeutronManager, - 'get_plugin') as getp: - getp.return_value = self.plugin - # no subnets and operation is remove_router_interface, - # so return immediately without calling get_ports - self.check_ports_exist_on_l3agent(self.adminContext, - l3_agent, router['id']) - self.assertFalse(self.plugin.get_ports.called) - def test_check_ports_exist_on_l3agent_no_subnet_match(self): - l3_agent, router = self._prepare_check_ports_exist_tests() + l3_agent = self._prepare_check_ports_exist_tests() # no matching subnet self.plugin.get_subnet_ids_on_router = mock.Mock( return_value=[str(uuid.uuid4())]) val = self.check_ports_exist_on_l3agent(self.adminContext, - l3_agent, router['id']) + l3_agent, []) self.assertFalse(val) def test_check_ports_exist_on_l3agent_subnet_match(self): - l3_agent, router = self._prepare_check_ports_exist_tests() + l3_agent = self._prepare_check_ports_exist_tests() # matching subnet port = {'subnet_id': str(uuid.uuid4()), 'binding:host_id': 'host_1', @@ -747,11 +736,9 @@ class L3SchedulerTestBaseMixin(object): subnet = {'id': str(uuid.uuid4()), 'enable_dhcp': False} self.plugin.get_ports.return_value = [port] - self.get_subnet_ids_on_router = mock.Mock( - return_value=[port['subnet_id']]) self.plugin.get_subnet = mock.Mock(return_value=subnet) val = self.check_ports_exist_on_l3agent(self.adminContext, - l3_agent, router['id']) + l3_agent, [port['subnet_id']]) self.assertTrue(val) def test_get_l3_agents_hosting_routers(self):