Remove subnet_id from check_ports_exist_on_l3agent

Refactor check_ports_exist_on_l3agent so that subnet_id no longer
needs to be passed.  Instead it calls get_subnet_ids_on_router.  This
helps to pave the way for removing hints from schedule router.

Partial-bug: #1353266
Partial-bug: #1356639
Co-Authored-By: Swaminathan Vasudevan <swaminathan.vasudevan@hp.com>

Change-Id: I6e9dcb0b899294bb4cf3e3d616a0a690049c338e

neutron/db/l3_agentschedulers_db.py

@@ -354,17 +354,15 @@ 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, router_id):
-                                     subnet_id):
         """
         This function checks for existence of dvr serviceable
         ports on the host, running the input l3agent.
         """
-        if not subnet_id:
+        subnet_ids = self.get_subnet_ids_on_router(context, router_id)
-            return True
 
         core_plugin = manager.NeutronManager.get_plugin()
-        filter = {'fixed_ips': {'subnet_id': [subnet_id]}}
+        filter = {'fixed_ips': {'subnet_id': subnet_ids}}
         ports = core_plugin.get_ports(context, filters=filter)
         for port in ports:
             if (n_utils.is_dvr_serviced(port['device_owner']) and
@@ -407,8 +405,7 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase,
             candidates.append(l3_agent)
         return candidates
 
-    def get_l3_agent_candidates(self, context, sync_router, l3_agents,
+    def get_l3_agent_candidates(self, context, sync_router, l3_agents):
-                                subnet_id=None):
         """Get the valid l3 agents for the router from a list of l3_agents."""
         candidates = []
         for l3_agent in l3_agents:
@@ -436,7 +433,7 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase,
                 candidates.append(l3_agent)
             elif is_router_distributed and agent_mode.startswith('dvr') and (
                 self.check_ports_exist_on_l3agent(
-                    context, l3_agent, sync_router['id'], subnet_id)):
+                    context, l3_agent, sync_router['id'])):
                 candidates.append(l3_agent)
         return candidates
 

neutron/scheduler/l3_agent_scheduler.py

@@ -136,7 +136,7 @@ class L3Scheduler(object):
         self.bind_routers(context, target_routers, l3_agent)
         return True
 
-    def get_candidates(self, plugin, context, sync_router, subnet_id):
+    def get_candidates(self, plugin, context, sync_router):
         """Return L3 agents where a router could be scheduled."""
         with context.session.begin(subtransactions=True):
             # allow one router is hosted by just
@@ -158,8 +158,7 @@ class L3Scheduler(object):
                 return
             new_l3agents = plugin.get_l3_agent_candidates(context,
                                                           sync_router,
-                                                          active_l3_agents,
+                                                          active_l3_agents)
-                                                          subnet_id)
             old_l3agentset = set(l3_agents)
             if sync_router.get('distributed', False):
                 new_l3agentset = set(new_l3agents)
@@ -201,13 +200,12 @@ class L3Scheduler(object):
     def _schedule_router(self, plugin, context, router_id,
                          candidates=None, hints=None):
         sync_router = plugin.get_router(context, router_id)
-        subnet_id = hints.get('subnet_id') if hints else None
         if (hints and 'gw_exists' in hints
             and sync_router.get('distributed', False)):
             plugin.schedule_snat_router(
                 context, router_id, sync_router, hints['gw_exists'])
         candidates = candidates or self.get_candidates(
-            plugin, context, sync_router, subnet_id)
+            plugin, context, sync_router)
         if not candidates:
             return
         if sync_router.get('distributed', False):

neutron/tests/unit/test_l3_schedulers.py

@@ -320,10 +320,11 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
         if already_scheduled:
             self._test_schedule_bind_router(agent, router)
         with contextlib.nested(
+            mock.patch.object(self, "validate_agent_router_combination"),
             mock.patch.object(self, "create_router_to_agent_binding"),
             mock.patch('neutron.db.l3_db.L3_NAT_db_mixin.get_router',
                        return_value=router['router'])
-        ) as (auto_s, gr):
+        ) as (valid, auto_s, gr):
             self.add_router_to_l3_agent(self.adminContext, agent_id,
                                         router['router']['id'])
             self.assertNotEqual(already_scheduled, auto_s.called)
@@ -372,8 +373,7 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
             mock.call.get_l3_agents_hosting_routers(
                 mock.ANY, ['foo_router_id'], admin_state_up=True),
             mock.call.get_l3_agents(mock.ANY, active=True),
-            mock.call.get_l3_agent_candidates(
+            mock.call.get_l3_agent_candidates(mock.ANY, sync_router, [agent]),
-                mock.ANY, sync_router, [agent], None),
         ]
         plugin.assert_has_calls(expected_calls)
 
@@ -391,8 +391,7 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
             mock.call.get_l3_agents_hosting_routers(
                 mock.ANY, ['foo_router_id'], admin_state_up=True),
             mock.call.get_l3_agents(mock.ANY, active=True),
-            mock.call.get_l3_agent_candidates(
+            mock.call.get_l3_agent_candidates(mock.ANY, sync_router, [agent]),
-                mock.ANY, sync_router, [agent], None),
         ]
         plugin.assert_has_calls(expected_calls)
 
@@ -416,8 +415,7 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
             mock.call.get_l3_agents_hosting_routers(
                 mock.ANY, ['foo_router_id'], admin_state_up=True),
             mock.call.get_l3_agents(mock.ANY, active=True),
-            mock.call.get_l3_agent_candidates(
+            mock.call.get_l3_agent_candidates(mock.ANY, sync_router, [agent]),
-                mock.ANY, sync_router, [agent], None),
         ]
         plugin.assert_has_calls(expected_calls)
 
@@ -454,14 +452,15 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
             args, kwargs = flog.call_args
             self.assertIn('has already been scheduled', args[0])
 
-    def _check_get_l3_agent_candidates(self, router, agent_list, exp_host):
+    def _check_get_l3_agent_candidates(
+            self, router, agent_list, exp_host, count=1):
         candidates = self.get_l3_agent_candidates(self.adminContext,
-                                                  router, agent_list,
+                                                  router, agent_list)
-                                                  subnet_id=None)
+        self.assertEqual(len(candidates), count)
-        self.assertEqual(len(candidates), 1)
+        if count:
-        self.assertEqual(candidates[0]['host'], exp_host)
+            self.assertEqual(candidates[0]['host'], exp_host)
 
-    def test_get_l3_agent_candidates(self):
+    def test_get_l3_agent_candidates_legacy(self):
         self._register_l3_dvr_agents()
         router = self._make_router(self.fmt,
                                    tenant_id=str(uuid.uuid4()),
@@ -475,19 +474,130 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
         exp_host = FIRST_L3_AGENT.get('host')
         self._check_get_l3_agent_candidates(router, agent_list, exp_host)
 
+    def test_get_l3_agent_candidates_dvr(self):
+        self._register_l3_dvr_agents()
+        router = self._make_router(self.fmt,
+                                   tenant_id=str(uuid.uuid4()),
+                                   name='r2')
+        router['external_gateway_info'] = None
+        router['id'] = str(uuid.uuid4())
+        agent_list = [self.agent1, self.l3_dvr_agent]
         # test dvr agent_mode case only dvr agent should be candidate
         router['distributed'] = True
         exp_host = DVR_L3_AGENT.get('host')
+        self.check_ports_exist_on_l3agent = mock.Mock(return_value=True)
         self._check_get_l3_agent_candidates(router, agent_list, exp_host)
 
-        # test dvr_snat agent_mode cases: dvr_snat agent can host
+    def test_get_l3_agent_candidates_dvr_no_vms(self):
-        # centralized and distributed routers
+        self._register_l3_dvr_agents()
+        router = self._make_router(self.fmt,
+                                   tenant_id=str(uuid.uuid4()),
+                                   name='r2')
+        router['external_gateway_info'] = None
+        router['id'] = str(uuid.uuid4())
+        agent_list = [self.agent1, self.l3_dvr_agent]
+        exp_host = DVR_L3_AGENT.get('host')
+        router['distributed'] = True
+        # Test no VMs present case
+        self.check_ports_exist_on_l3agent = mock.Mock(return_value=False)
+        self._check_get_l3_agent_candidates(
+            router, agent_list, exp_host, count=0)
+
+    def test_get_l3_agent_candidates_dvr_snat(self):
+        self._register_l3_dvr_agents()
+        router = self._make_router(self.fmt,
+                                   tenant_id=str(uuid.uuid4()),
+                                   name='r2')
+        router['external_gateway_info'] = None
+        router['id'] = str(uuid.uuid4())
+        router['distributed'] = True
+
         agent_list = [self.l3_dvr_snat_agent]
         exp_host = DVR_SNAT_L3_AGENT.get('host')
+        self.check_ports_exist_on_l3agent = mock.Mock(return_value=True)
         self._check_get_l3_agent_candidates(router, agent_list, exp_host)
+
+    def test_get_l3_agent_candidates_dvr_snat_no_vms(self):
+        self._register_l3_dvr_agents()
+        router = self._make_router(self.fmt,
+                                   tenant_id=str(uuid.uuid4()),
+                                   name='r2')
+        router['external_gateway_info'] = None
+        router['id'] = str(uuid.uuid4())
+        router['distributed'] = True
+
+        agent_list = [self.l3_dvr_snat_agent]
+        exp_host = DVR_SNAT_L3_AGENT.get('host')
+        self.check_ports_exist_on_l3agent = mock.Mock(return_value=False)
+        # Test no VMs present case
+        self.check_ports_exist_on_l3agent.return_value = False
+        self._check_get_l3_agent_candidates(
+            router, agent_list, exp_host, count=0)
+
+    def test_get_l3_agent_candidates_centralized(self):
+        self._register_l3_dvr_agents()
+        router = self._make_router(self.fmt,
+                                   tenant_id=str(uuid.uuid4()),
+                                   name='r2')
+        router['external_gateway_info'] = None
+        router['id'] = str(uuid.uuid4())
+        # check centralized test case
         router['distributed'] = False
+        exp_host = DVR_SNAT_L3_AGENT.get('host')
+        agent_list = [self.l3_dvr_snat_agent]
         self._check_get_l3_agent_candidates(router, agent_list, exp_host)
 
+    def _prepare_check_ports_exist_tests(self):
+        l3_agent = agents_db.Agent()
+        l3_agent.admin_state_up = True
+        l3_agent.host = HOST
+        router = self._make_router(self.fmt,
+                                   tenant_id=str(uuid.uuid4()),
+                                   name='r2')
+        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
+
+    def test_check_ports_exist_on_l3agent_no_subnets(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
+            val = self.check_ports_exist_on_l3agent(self.adminContext,
+                                                    l3_agent, router['id'])
+            self.assertFalse(val)
+
+    def test_check_ports_exist_on_l3agent_no_subnet_match(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 matching subnet
+            self.get_subnet_ids_on_router.return_value = [str(uuid.uuid4())]
+            val = self.check_ports_exist_on_l3agent(self.adminContext,
+                                                    l3_agent, router['id'])
+            self.assertFalse(val)
+
+    def test_check_ports_exist_on_l3agent_subnet_match(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
+            # matching subnet
+            port = {'subnet_id': str(uuid.uuid4()),
+                    'binding:host_id': HOST,
+                    'device_owner': 'compute:',
+                    'id': 1234}
+            self.plugin.get_ports.return_value = [port]
+            self.plugin.get_subnet_ids_on_router = mock.Mock(
+                return_value=[port['subnet_id']])
+            val = self.check_ports_exist_on_l3agent(self.adminContext,
+                                                    l3_agent, router['id'])
+            self.assertTrue(val)
+
 
 class L3AgentChanceSchedulerTestCase(L3SchedulerTestCase):