Call unbind_snat_servicenode from schedule router

Refactor to move the call to plugin.unbind_snat_servicenode from
schedule_snat_router to _schedule_router.  This is a move 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: I062d8cc3cb870bbaa033d5b107a7dd868dfafa19

diff --git a/neutron/db/l3_dvrscheduler_db.py b/neutron/db/l3_dvrscheduler_db.py
index e0e0f6da9238b9649ad03b7de684787da8056f8d..d26dae2026f75f97056cac59cfa44676e842bd87 100644 (file)
--- a/neutron/db/l3_dvrscheduler_db.py
+++ b/neutron/db/l3_dvrscheduler_db.py
@@ -18,6 +18,7 @@ import random
 import sqlalchemy as sa
 from sqlalchemy import orm
 from sqlalchemy.orm import exc
+from sqlalchemy.orm import joinedload
 
 from neutron.common import constants as q_const
 from neutron.common import utils as n_utils
@@ -278,28 +279,34 @@ class L3_DVRsch_db_mixin(l3agent_sch_db.L3AgentSchedulerDbMixin):
         LOG.debug('Removed binding for router %(router_id)s and '
                   'agent %(id)s', {'router_id': router_id, 'id': agent_id})
 
-    def schedule_snat_router(self, context, router_id, sync_router, gw_exists):
+    def get_snat_bindings(self, context, router_ids):
+        """ Retrieves the dvr snat bindings for a router."""
+        if not router_ids:
+            return []
+        query = context.session.query(CentralizedSnatL3AgentBinding)
+        query = query.options(joinedload('l3_agent')).filter(
+            CentralizedSnatL3AgentBinding.router_id.in_(router_ids))
+        return query.all()
+
+    def schedule_snat_router(self, context, router_id, sync_router):
         """Schedule the snat router on l3 service agent."""
-        if gw_exists:
-            binding = (context.session.
-                       query(CentralizedSnatL3AgentBinding).
-                       filter_by(router_id=router_id).first())
-            if binding:
-                l3_agent_id = binding.l3_agent_id
-                l3_agent = binding.l3_agent
-                LOG.debug('SNAT Router %(router_id)s has already been '
-                          'hosted by L3 agent '
-                          '%(l3_agent_id)s', {'router_id': router_id,
-                                              'l3_agent_id': l3_agent_id})
-                self.bind_dvr_router_servicenode(context, router_id, l3_agent)
-                return
-            active_l3_agents = self.get_l3_agents(context, active=True)
-            if not active_l3_agents:
-                LOG.warn(_('No active L3 agents'))
-                return
-            snat_candidates = self.get_snat_candidates(sync_router,
-                                                       active_l3_agents)
-            if snat_candidates:
-                self.bind_snat_servicenode(context, router_id, snat_candidates)
-        else:
-            self.unbind_snat_servicenode(context, router_id)
+        binding = (context.session.
+                   query(CentralizedSnatL3AgentBinding).
+                   filter_by(router_id=router_id).first())
+        if binding:
+            l3_agent_id = binding.l3_agent_id
+            l3_agent = binding.l3_agent
+            LOG.debug('SNAT Router %(router_id)s has already been '
+                      'hosted by L3 agent '
+                      '%(l3_agent_id)s', {'router_id': router_id,
+                                          'l3_agent_id': l3_agent_id})
+            self.bind_dvr_router_servicenode(context, router_id, l3_agent)
+            return
+        active_l3_agents = self.get_l3_agents(context, active=True)
+        if not active_l3_agents:
+            LOG.warn(_('No active L3 agents found for SNAT'))
+            return
+        snat_candidates = self.get_snat_candidates(sync_router,
+                                                   active_l3_agents)
+        if snat_candidates:
+            self.bind_snat_servicenode(context, router_id, snat_candidates)

diff --git a/neutron/scheduler/l3_agent_scheduler.py b/neutron/scheduler/l3_agent_scheduler.py
index 9a140d23f2cb761aecfbbdba119a6bf6ec413569..f13ae8db333133ec4978ed8e092114b55d3acad6 100644 (file)
--- a/neutron/scheduler/l3_agent_scheduler.py
+++ b/neutron/scheduler/l3_agent_scheduler.py
@@ -200,15 +200,27 @@ class L3Scheduler(object):
     def _schedule_router(self, plugin, context, router_id,
                          candidates=None, hints=None):
         sync_router = plugin.get_router(context, router_id)
-        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'])
+        router_distributed = sync_router.get('distributed', False)
+        if router_distributed:
+            # For Distributed routers check for SNAT Binding before
+            # calling the schedule_snat_router
+            snat_bindings = plugin.get_snat_bindings(context, [router_id])
+            router_gw_exists = (hints and 'gw_exists' in hints
+                                and hints['gw_exists'])
+            if not snat_bindings and router_gw_exists:
+                # If GW exists for DVR routers and no SNAT binding
+                # call the schedule_snat_router
+                plugin.schedule_snat_router(context, router_id, sync_router)
+            if not router_gw_exists and snat_bindings:
+                # If DVR router and no Gateway but SNAT Binding exists then
+                # call the unbind_snat_servicenode to unbind the snat service
+                # from agent
+                plugin.unbind_snat_servicenode(context, router_id)
         candidates = candidates or self.get_candidates(
             plugin, context, sync_router)
         if not candidates:
             return
-        if sync_router.get('distributed', False):
+        if router_distributed:
             for chosen_agent in candidates:
                 self.bind_router(context, router_id, chosen_agent)
         else:

diff --git a/neutron/tests/unit/test_l3_schedulers.py b/neutron/tests/unit/test_l3_schedulers.py
index f6dc98d8c0a4587495c6b6e691a731f0707312fa..2a8890784bdbd5b1ba35e2672f143ecd0eaa621f 100644 (file)
--- a/neutron/tests/unit/test_l3_schedulers.py
+++ b/neutron/tests/unit/test_l3_schedulers.py
@@ -364,10 +364,13 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
             'distributed': True
         }
         plugin.get_router.return_value = sync_router
-        with mock.patch.object(scheduler, 'bind_router'):
-            scheduler._schedule_router(
-                plugin, self.adminContext,
-                'foo_router_id', None)
+        with contextlib.nested(
+            mock.patch.object(scheduler, 'bind_router'),
+            mock.patch.object(
+                plugin, 'get_snat_bindings', return_value=False)
+        ):
+                scheduler._schedule_router(
+                    plugin, self.adminContext, 'foo_router_id', None)
         expected_calls = [
             mock.call.get_router(mock.ANY, 'foo_router_id'),
             mock.call.get_l3_agents_hosting_routers(
@@ -382,12 +385,14 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
         sync_router = {'id': 'foo_router_id',
                        'distributed': True}
         plugin.get_router.return_value = sync_router
-        with mock.patch.object(scheduler, 'bind_router'):
-            scheduler._schedule_router(
-                plugin, self.adminContext,
-                'foo_router_id', None)
+        with contextlib.nested(
+            mock.patch.object(scheduler, 'bind_router'),
+            mock.patch.object(plugin, 'get_snat_bindings', return_value=True)):
+                scheduler._schedule_router(
+                    plugin, self.adminContext, 'foo_router_id', None)
         expected_calls = [
             mock.call.get_router(mock.ANY, 'foo_router_id'),
+            mock.call.unbind_snat_servicenode(mock.ANY, 'foo_router_id'),
             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),
@@ -406,10 +411,13 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
             }
         }
         plugin.get_router.return_value = sync_router
-        with mock.patch.object(scheduler, 'bind_router'):
-            scheduler._schedule_router(
-                plugin, self.adminContext,
-                'foo_router_id', None)
+        with contextlib.nested(
+            mock.patch.object(scheduler, 'bind_router'),
+            mock.patch.object(
+                plugin, 'get_snat_bindings', return_value=False)
+        ):
+                scheduler._schedule_router(
+                    plugin, self.adminContext, 'foo_router_id', None)
         expected_calls = [
             mock.call.get_router(mock.ANY, 'foo_router_id'),
             mock.call.get_l3_agents_hosting_routers(
@@ -877,7 +885,7 @@ class L3DvrSchedulerTestCase(testlib_api.SqlTestCase,
             mock_agents.return_value = [agent]
             mock_candidates.return_value = [agent]
             self.dut.schedule_snat_router(
-                self.adminContext, 'foo_router_id', router, True)
+                self.adminContext, 'foo_router_id', router)
         self.assertFalse(mock_dvr.called)
 
     def test_schedule_router_unbind_snat_servicenode_negativetest(self):
@@ -887,11 +895,13 @@ class L3DvrSchedulerTestCase(testlib_api.SqlTestCase,
         }
         with contextlib.nested(
             mock.patch.object(self.dut, 'get_router'),
-            mock.patch.object(self.dut, 'unbind_snat_servicenode')) as (
-                mock_rd, mock_unbind):
+            mock.patch.object(self.dut, 'get_snat_bindings'),
+            mock.patch.object(self.dut, 'unbind_snat_servicenode')
+        ) as (mock_rd, mock_snat_bind, mock_unbind):
             mock_rd.return_value = router
+            mock_snat_bind.return_value = False
             self.dut.schedule_snat_router(
-                self.adminContext, 'foo_router_id', router, True)
+                self.adminContext, 'foo_router_id', router)
         self.assertFalse(mock_unbind.called)
 
     def test_schedule_snat_router_with_snat_candidates(self):
@@ -910,7 +920,7 @@ class L3DvrSchedulerTestCase(testlib_api.SqlTestCase,
             mock_agents.return_value = [agent]
             mock_candidates.return_value = [agent]
             self.dut.schedule_snat_router(
-                self.adminContext, 'foo_router_id', mock.ANY, True)
+                self.adminContext, 'foo_router_id', mock.ANY)
         mock_bind.assert_called_once_with(
             self.adminContext, 'foo_router_id', [agent])