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

diff --git a/neutron/db/l3_dvrscheduler_db.py b/neutron/db/l3_dvrscheduler_db.py
index e751c44abc4268a454e7d69da9cc9d1fb713d478..1b0a981a8f5f2a8b3029d9bd20fa4c4b8359640d 100644 (file)
--- 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 14aae7c114e5056765c5ced563e96b327820d22e..edcc5de14d40ba5787a2497f8f40b958dc90922a 100644 (file)
--- 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 9e6034251116cbe6dd948ca6f9458131056729ee..bc3d0d118a672199464bb51254969c254a7d7557 100644 (file)
--- 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',