DVR: fix router scheduling

Fix scheduling of DVR routers to not stop scheduling once
csnat portion was scheduled. See bug report for failing
scenario.

This partially reverts
commit 3794b4a83e68041e24b715135f0ccf09a5631178
and fixes bug 1374473 by moving csnat scheduling
after general dvr router scheduling, so double binding does
not happen.

Closes-Bug: #1472163
Related-Bug: #1374473
Change-Id: I57c06e2be732e47b6cce7c724f6b255ea2d8fa32

diff --git a/neutron/scheduler/l3_agent_scheduler.py b/neutron/scheduler/l3_agent_scheduler.py
index 223a54874425febd8a94dc0e1f89bb6aa63e2684..ae7adbf38ead189550178b594fd59cf0e98b2b93 100644 (file)
--- a/neutron/scheduler/l3_agent_scheduler.py
+++ b/neutron/scheduler/l3_agent_scheduler.py
@@ -232,8 +232,16 @@ class L3Scheduler(object):
     def _schedule_router(self, plugin, context, router_id,
                          candidates=None):
         sync_router = plugin.get_router(context, router_id)
+        candidates = candidates or self._get_candidates(
+            plugin, context, sync_router)
+        if not candidates:
+            return
+
         router_distributed = sync_router.get('distributed', False)
         if router_distributed:
+            for chosen_agent in candidates:
+                self.bind_router(context, router_id, chosen_agent)
+
             # For Distributed routers check for SNAT Binding before
             # calling the schedule_snat_router
             snat_bindings = plugin.get_snat_bindings(context, [router_id])
@@ -241,21 +249,13 @@ 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
-                return plugin.schedule_snat_router(
+                plugin.schedule_snat_router(
                     context, router_id, sync_router)
-            if not router_gw_exists and snat_bindings:
+            elif 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:
-            return
-        if router_distributed:
-            for chosen_agent in candidates:
-                self.bind_router(context, router_id, chosen_agent)
         elif sync_router.get('ha', False):
             chosen_agents = self._bind_ha_router(plugin, context,
                                                  router_id, candidates)

diff --git a/neutron/tests/unit/extensions/test_agent.py b/neutron/tests/unit/extensions/test_agent.py
index 79397a5fe791c818657d9ef1491029f533cb48ac..21fa1022b3952284ab9ac4f4d035bbe8a5a6142f 100644 (file)
--- a/neutron/tests/unit/extensions/test_agent.py
+++ b/neutron/tests/unit/extensions/test_agent.py
@@ -118,6 +118,13 @@ class AgentDBTestMixIn(object):
 
         return res
 
+    def _register_dvr_agents(self):
+        dvr_snat_agent = helpers.register_l3_agent(
+            host=L3_HOSTA, agent_mode=constants.L3_AGENT_MODE_DVR_SNAT)
+        dvr_agent = helpers.register_l3_agent(
+            host=L3_HOSTB, agent_mode=constants.L3_AGENT_MODE_DVR)
+        return [dvr_snat_agent, dvr_agent]
+
 
 class AgentDBTestCase(AgentDBTestMixIn,
                       test_db_base_plugin_v2.NeutronDbPluginV2TestCase):

diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py
index ba391320c2512577f68d31e6380b23f92bbdd40f..77e55682b9773d1d8af45696b6c31a9aeaab424d 100644 (file)
--- a/neutron/tests/unit/extensions/test_l3.py
+++ b/neutron/tests/unit/extensions/test_l3.py
@@ -40,6 +40,7 @@ from neutron.db import l3_agentschedulers_db
 from neutron.db import l3_attrs_db
 from neutron.db import l3_db
 from neutron.db import l3_dvr_db
+from neutron.db import l3_dvrscheduler_db
 from neutron.extensions import external_net
 from neutron.extensions import l3
 from neutron.extensions import portbindings
@@ -301,8 +302,8 @@ class TestL3NatServicePlugin(common_db_mixin.CommonDbMixin,
 # A L3 routing with L3 agent scheduling service plugin class for tests with
 # plugins that delegate away L3 routing functionality
 class TestL3NatAgentSchedulingServicePlugin(TestL3NatServicePlugin,
-                                            l3_agentschedulers_db.
-                                            L3AgentSchedulerDbMixin):
+                                            l3_dvrscheduler_db.
+                                            L3_DVRsch_db_mixin):
 
     supported_extension_aliases = ["router", "l3_agent_scheduler"]
 

diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_agent_scheduler.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_agent_scheduler.py
index dc406d800b23340fa4d63ca2bea6fb9833d1d853..e512b102fb7e81357ac7e7093b3d14e0dcf84fb2 100644 (file)
--- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_agent_scheduler.py
+++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_agent_scheduler.py
@@ -240,9 +240,8 @@ class OvsAgentSchedulerTestCaseBase(test_l3.L3NatTestCaseMixin,
         # the global attribute map
         attributes.RESOURCE_ATTRIBUTE_MAP.update(
             agent.RESOURCE_ATTRIBUTE_MAP)
-        self.l3agentscheduler_dbMinxin = (
-            manager.NeutronManager.get_service_plugins().get(
-                service_constants.L3_ROUTER_NAT))
+        self.l3plugin = manager.NeutronManager.get_service_plugins().get(
+            service_constants.L3_ROUTER_NAT)
         self.l3_notify_p = mock.patch(
             'neutron.extensions.l3agentscheduler.notify')
         self.patched_l3_notify = self.l3_notify_p.start()
@@ -967,11 +966,37 @@ class OvsAgentSchedulerTestCase(OvsAgentSchedulerTestCaseBase):
             res = router_req.get_response(self.ext_api)
             router = self.deserialize(self.fmt, res)
             l3agents = (
-                self.l3agentscheduler_dbMinxin.get_l3_agents_hosting_routers(
+                self.l3plugin.get_l3_agents_hosting_routers(
                     self.adminContext, [router['router']['id']]))
             self._delete('routers', router['router']['id'])
         self.assertEqual(0, len(l3agents))
 
+    def test_dvr_router_scheduling_to_all_needed_agents(self):
+        self._register_dvr_agents()
+        with self.subnet() as s:
+            net_id = s['subnet']['network_id']
+            self._set_net_external(net_id)
+
+            router = {'name': 'router1',
+                      'external_gateway_info': {'network_id': net_id},
+                      'admin_state_up': True,
+                      'distributed': True}
+            r = self.l3plugin.create_router(self.adminContext,
+                                            {'router': router})
+            with mock.patch.object(
+                    self.l3plugin,
+                    'check_ports_exist_on_l3agent') as ports_exist:
+                # emulating dvr serviceable ports exist on compute node
+                ports_exist.return_value = True
+                self.l3plugin.schedule_router(
+                    self.adminContext, r['id'])
+
+        l3agents = self._list_l3_agents_hosting_router(r['id'])
+        self.assertEqual(2, len(l3agents['agents']))
+        self.assertEqual({'dvr', 'dvr_snat'},
+                         set([a['configurations']['agent_mode'] for a in
+                              l3agents['agents']]))
+
     def test_router_sync_data(self):
         with self.subnet() as s1,\
                 self.subnet(cidr='10.0.2.0/24') as s2,\

diff --git a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py
index a8f5c15dd449a3ac50845c869e7a44eb74b5818b..8b4c546a952e2f199e551735277bd386f00b2e8a 100644 (file)
--- a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py
+++ b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py
@@ -489,12 +489,18 @@ class L3SchedulerTestBaseMixin(object):
         sync_router = {'id': 'foo_router_id',
                        'distributed': True}
         plugin.get_router.return_value = sync_router
-        with mock.patch.object(plugin, 'get_snat_bindings', return_value=True):
-                scheduler._schedule_router(
-                    plugin, self.adminContext, 'foo_router_id', None)
+        with mock.patch.object(
+                plugin, 'get_snat_bindings', return_value=True),\
+                mock.patch.object(scheduler, 'bind_router'):
+            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]),
+            mock.call.unbind_snat_servicenode(mock.ANY, 'foo_router_id')
         ]
         plugin.assert_has_calls(expected_calls)
 
@@ -510,11 +516,16 @@ class L3SchedulerTestBaseMixin(object):
         }
         plugin.get_router.return_value = sync_router
         with mock.patch.object(
-            plugin, 'get_snat_bindings', return_value=False):
-                scheduler._schedule_router(
-                    plugin, self.adminContext, 'foo_router_id', None)
+            plugin, 'get_snat_bindings', return_value=False),\
+                mock.patch.object(scheduler, 'bind_router'):
+            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(
+                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]),
             mock.call.schedule_snat_router(
                 mock.ANY, 'foo_router_id', sync_router),
         ]