From 671e3ffa468974bf3b52f787e865eba2b6c97ffe Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 2 Sep 2014 17:07:04 +0000 Subject: [PATCH] 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 Change-Id: I062d8cc3cb870bbaa033d5b107a7dd868dfafa19 --- neutron/db/l3_dvrscheduler_db.py | 55 +++++++++++++----------- neutron/scheduler/l3_agent_scheduler.py | 22 +++++++--- neutron/tests/unit/test_l3_schedulers.py | 44 +++++++++++-------- 3 files changed, 75 insertions(+), 46 deletions(-) diff --git a/neutron/db/l3_dvrscheduler_db.py b/neutron/db/l3_dvrscheduler_db.py index e0e0f6da923..d26dae2026f 100644 --- 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 9a140d23f2c..f13ae8db333 100644 --- 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 f6dc98d8c0a..2a8890784bd 100644 --- 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])