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

@@ -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(
+            elif (is_router_distributed and subnet_ids and
-                constants.L3_AGENT_MODE_DVR) and (
+                    agent_mode.startswith(constants.L3_AGENT_MODE_DVR) and (
-                self.check_ports_exist_on_l3agent(
+                        self.check_ports_exist_on_l3agent(
-                    context, l3_agent, sync_router['id'])):
+                            context, l3_agent, subnet_ids))):
                 candidates.append(l3_agent)
         return candidates
 

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:
+            subnet_ids = plugin.get_subnet_ids_on_router(
-                if not plugin.check_ports_exist_on_l3agent(context, l3_agent,
+                context, router['id'])
-                                                           router['id']):
+            if subnet_ids:
-                    plugin.remove_router_from_l3_agent(
+                for l3_agent in l3_agents:
-                        context, l3_agent['id'], router['id'])
+                    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']])

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'])

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}

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
-        return l3_agent, router
 
     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,
+        val = self.check_ports_exist_on_l3agent(
-                                                l3_agent, router['id'])
+            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):