Fix DB Duplicate error when scheduling distributed routers

The error was caused by binding the router to an agent
candidate that was already selected during the scheduling
process.

A DB lookup was also saved by passing the router object
around; this led to a minor style cleanup.

Closes-bug: #1351123

Change-Id: Ib71a0140c8a7fbd5b230609d33487f8adba252e7

neutron/db/l3_dvrscheduler_db.py

@@ -248,27 +248,25 @@ 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, gw_exists):
+    def schedule_snat_router(self, context, router_id, sync_router, gw_exists):
         """Schedule the snat router on l3 service agent."""
         if gw_exists:
-            query = (context.session.
-                     query(CentralizedSnatL3AgentBinding).
-                     filter_by(router_id=router_id))
-            for bind in query:
-                agt_id = bind.l3_agent_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 '
-                          '%(agent_id)s', {'router_id': router_id,
-                                           'agent_id': agt_id})
-                self.bind_dvr_router_servicenode(context,
-                                                 router_id,
-                                                 bind.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
-            sync_router = self.get_router(context, router_id)
             snat_candidates = self.get_snat_candidates(sync_router,
                                                        active_l3_agents)
             if snat_candidates:

neutron/scheduler/l3_agent_scheduler.py

@@ -196,11 +196,12 @@ class L3Scheduler(object):
                          candidates=None, hints=None):
         sync_router = plugin.get_router(context, router_id)
         subnet_id = hints.get('subnet_id') if hints else None
-        candidates = candidates or self.get_candidates(
-            plugin, context, sync_router, subnet_id)
         if (hints and 'gw_exists' in hints
             and sync_router.get('distributed', False)):
-            plugin.schedule_snat_router(context, router_id, sync_router)
+            plugin.schedule_snat_router(
+                context, router_id, sync_router, hints['gw_exists'])
+        candidates = candidates or self.get_candidates(
+            plugin, context, sync_router, subnet_id)
         if not candidates:
             return
         if sync_router.get('distributed', False):

neutron/tests/unit/test_l3_schedulers.py

@@ -21,6 +21,7 @@ import uuid
 
 import mock
 from oslo.config import cfg
+from sqlalchemy.orm import query
 
 from neutron.api.v2 import attributes as attr
 from neutron.common import constants
@@ -129,6 +130,36 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin,
             router['router']['id'], subnet['subnet']['network_id'])
         self._delete('routers', router['router']['id'])
 
+    def test_schedule_router_distributed(self):
+        scheduler = l3_agent_scheduler.ChanceScheduler()
+        agent = agents_db.Agent()
+        agent.admin_state_up = True
+        agent.heartbeat_timestamp = timeutils.utcnow()
+        sync_router = {
+            'id': 'foo_router_id',
+            'distributed': True
+        }
+        plugin = mock.Mock()
+        plugin.get_router.return_value = sync_router
+        plugin.get_l3_agents_hosting_routers.return_value = []
+        plugin.get_l3_agents.return_value = [agent]
+        plugin.get_l3_agent_candidates.return_value = [agent]
+        with mock.patch.object(scheduler, 'bind_router'):
+            scheduler._schedule_router(
+                plugin, self.adminContext,
+                'foo_router_id', None, {'gw_exists': True})
+        expected_calls = [
+            mock.call.get_router(mock.ANY, 'foo_router_id'),
+            mock.call.schedule_snat_router(
+                mock.ANY, 'foo_router_id', sync_router, True),
+            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], None),
+        ]
+        plugin.assert_has_calls(expected_calls)
+
     def _test_schedule_bind_router(self, agent, router):
         ctx = self.adminContext
         session = ctx.session
@@ -381,3 +412,21 @@ class L3DvrSchedulerTestCase(base.BaseTestCase):
                                                     'thisHost', 'dvr_port1',
                                                     sub_ids)
             self.assertFalse(result)
+
+    def test_schedule_snat_router_with_snat_candidates(self):
+        agent = agents_db.Agent()
+        agent.admin_state_up = True
+        agent.heartbeat_timestamp = timeutils.utcnow()
+        with contextlib.nested(
+            mock.patch.object(query.Query, 'first'),
+            mock.patch.object(self.dut, 'get_l3_agents'),
+            mock.patch.object(self.dut, 'get_snat_candidates'),
+            mock.patch.object(self.dut, 'bind_snat_servicenode')) as (
+                mock_query, mock_agents, mock_candidates, mock_bind):
+            mock_query.return_value = []
+            mock_agents.return_value = [agent]
+            mock_candidates.return_value = [agent]
+            self.dut.schedule_snat_router(
+                self.adminContext, 'foo_router_id', mock.ANY, True)
+        mock_bind.assert_called_once_with(
+            self.adminContext, 'foo_router_id', [agent])