From 3794b4a83e68041e24b715135f0ccf09a5631178 Mon Sep 17 00:00:00 2001 From: Swaminathan Vasudevan Date: Wed, 17 Dec 2014 17:15:07 -0800 Subject: [PATCH] Fixes Multiple External Networks issue with DVR Current L3 agents can support more than one external network when configured properly. On DVR routers, router-gateway-set was returning a 500 error, when two external networks were configured in the system. The problem resides in the scheduler where the bind_router is called twice when the reschedule_router is called from update_router. The _schedule_router binds the snat and the qrouter with the respective agents. But after scheduling it does not return agent. And in the case of two external networks, the get_candidates always returns a valid candidate to be processed and hence the bind_router is called twice. This patch fixes the _schedule_router function and hence avoids the multiple calls to bind_router. This prevents the update_router from failing and causing the nested rollback for the transactions. Change-Id: I24d44c60a3ea5bbc9e3f44aa5191deff315723ca Closes-Bug: #1374473 --- neutron/db/l3_dvrscheduler_db.py | 1 + neutron/scheduler/l3_agent_scheduler.py | 4 ++- neutron/tests/unit/test_l3_schedulers.py | 34 +++++++++++++----------- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/neutron/db/l3_dvrscheduler_db.py b/neutron/db/l3_dvrscheduler_db.py index e751c44abc4..1b0a981a8f5 100644 --- a/neutron/db/l3_dvrscheduler_db.py +++ b/neutron/db/l3_dvrscheduler_db.py @@ -308,3 +308,4 @@ class L3_DVRsch_db_mixin(l3agent_sch_db.L3AgentSchedulerDbMixin): return self.bind_dvr_router_servicenode( context, router_id, chosen_agent) + return chosen_agent diff --git a/neutron/scheduler/l3_agent_scheduler.py b/neutron/scheduler/l3_agent_scheduler.py index 14aae7c114e..edcc5de14d4 100644 --- a/neutron/scheduler/l3_agent_scheduler.py +++ b/neutron/scheduler/l3_agent_scheduler.py @@ -241,12 +241,14 @@ class L3Scheduler(object): 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) + return 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) + return candidates = candidates or self.get_candidates( plugin, context, sync_router) if not candidates: diff --git a/neutron/tests/unit/test_l3_schedulers.py b/neutron/tests/unit/test_l3_schedulers.py index 9e603425111..bc3d0d118a6 100644 --- a/neutron/tests/unit/test_l3_schedulers.py +++ b/neutron/tests/unit/test_l3_schedulers.py @@ -473,18 +473,12 @@ class L3SchedulerTestBaseMixin(object): sync_router = {'id': 'foo_router_id', 'distributed': True} plugin.get_router.return_value = sync_router - with contextlib.nested( - mock.patch.object(scheduler, 'bind_router'), - mock.patch.object(plugin, 'get_snat_bindings', return_value=True)): + with 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), - mock.call.get_l3_agent_candidates(mock.ANY, sync_router, [agent]), ] plugin.assert_has_calls(expected_calls) @@ -499,21 +493,14 @@ class L3SchedulerTestBaseMixin(object): } } plugin.get_router.return_value = sync_router - with contextlib.nested( - mock.patch.object(scheduler, 'bind_router'), - mock.patch.object( - plugin, 'get_snat_bindings', return_value=False) - ): + with 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.schedule_snat_router( mock.ANY, 'foo_router_id', sync_router), - 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.ANY, sync_router, [agent]), ] plugin.assert_has_calls(expected_calls) @@ -1040,6 +1027,21 @@ class L3DvrSchedulerTestCase(testlib_api.SqlTestCase, self.assertTrue(mock_bind_snat.called) self.assertFalse(mock_bind_dvr.called) + def test_schedule_snat_router_return_value(self): + agent, router = self._prepare_schedule_snat_tests() + with contextlib.nested( + mock.patch.object(self.dut, 'get_l3_agents'), + mock.patch.object(self.dut, 'get_snat_candidates'), + mock.patch.object(self.dut, 'bind_snat_servicenode'), + mock.patch.object(self.dut, 'bind_dvr_router_servicenode') + ) as (mock_gl3, mock_snat_canidates, mock_bind_snat, mock_bind_dvr): + mock_snat_canidates.return_value = [agent] + mock_bind_snat.return_value = [agent] + mock_bind_dvr.return_value = [agent] + chosen_agent = self.dut.schedule_snat_router( + self.adminContext, 'foo_router_id', router) + self.assertEqual(chosen_agent, [agent]) + def test_schedule_router_unbind_snat_servicenode_negativetest(self): router = { 'id': 'foo_router_id',