Add binding_index to RouterL3AgentBinding

The patch proposes adding a new binding_index to the
RouterL3AgentBinding table, with an additional Unique Constraint that
enforces a single <router_id, binding_id> per router. This goes a long
way into fixing 2 issues:

1. When scheduling a non-HA router, we only use binding_index=1. This
   means that only a single row containing that router_id can be
   committed into the database. This in fact prevents over-scheduling of
   non-HA routers. Note that for the HA router case, the binding_index
   is simply copied from the L3HARouterAgentPortBinding (since they are
   always created together they should always match).

2. This sets the ground-work for a refactor of the l3 scheduler - by
   using this binding and db-based limitation, we can schedule a router
   to agents using the RouterL3AgentBinding, while postponing the
   creation of L3HARouterAgentPortBinding objects for the agents until
   they ask for it (using sync_routers). This will be a major
   improvement over todays "everything can create
   L3HARouterAgentPortBinding" way of things).

Closes-Bug: #1535557
Change-Id: I3447ea5bcb7c57365c6f50efe12a1671e86588b3

neutron/db/l3_agentschedulers_db.py

@@ -41,6 +41,8 @@ from neutron.plugins.common import constants as service_constants
 
 LOG = logging.getLogger(__name__)
 
+LOWEST_BINDING_INDEX = 1
+
 L3_AGENTS_SCHEDULER_OPTS = [
     cfg.StrOpt('router_scheduler_driver',
                default='neutron.scheduler.l3_agent_scheduler.'
@@ -60,6 +62,13 @@ cfg.CONF.register_opts(L3_AGENTS_SCHEDULER_OPTS)
 class RouterL3AgentBinding(model_base.BASEV2):
     """Represents binding between neutron routers and L3 agents."""
 
+    __table_args__ = (
+        sa.UniqueConstraint(
+            'router_id', 'binding_index',
+            name='uniq_router_l3_agent_binding0router_id0binding_index0'),
+        model_base.BASEV2.__table_args__
+    )
+
     router_id = sa.Column(sa.String(36),
                           sa.ForeignKey("routers.id", ondelete='CASCADE'),
                           primary_key=True)
@@ -67,6 +76,8 @@ class RouterL3AgentBinding(model_base.BASEV2):
     l3_agent_id = sa.Column(sa.String(36),
                             sa.ForeignKey("agents.id", ondelete='CASCADE'),
                             primary_key=True)
+    binding_index = sa.Column(sa.Integer, nullable=False,
+                              server_default=str(LOWEST_BINDING_INDEX))
 
 
 class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase,
@@ -194,7 +205,7 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase,
                         service_constants.L3_ROUTER_NAT)
                     self.router_scheduler.create_ha_port_and_bind(
                         plugin, context, router['id'],
-                        router['tenant_id'], agent)
+                        router['tenant_id'], agent, is_manual_scheduling=True)
                 else:
                     self.router_scheduler.bind_router(
                         context, router_id, agent)
@@ -489,7 +500,7 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase,
 
     def auto_schedule_routers(self, context, host, router_ids):
         if self.router_scheduler:
-            return self.router_scheduler.auto_schedule_routers(
+            self.router_scheduler.auto_schedule_routers(
                 self, context, host, router_ids)
 
     def schedule_router(self, context, router, candidates=None):
@@ -523,6 +534,40 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase,
             context, [router_id], admin_state_up=state, active=True)
         return [a.host for a in agents]
 
+    def get_vacant_binding_index(self, context, router_id,
+                                 is_manual_scheduling=False):
+        """Return a vacant binding_index to use and whether or not it exists.
+
+        Each RouterL3AgentBinding has a binding_index which is unique per
+        router_id, and when creating a single binding we require to find a
+        'vacant' binding_index which isn't yet used - for example if we have
+        bindings with indices 1 and 3, then clearly binding_index == 2 is free.
+
+        :returns: binding_index.
+        """
+        num_agents = self.get_number_of_agents_for_scheduling(context)
+
+        query = context.session.query(RouterL3AgentBinding)
+        query = query.filter(
+            RouterL3AgentBinding.router_id == router_id)
+        query = query.order_by(RouterL3AgentBinding.binding_index.asc())
+
+        bindings = query.all()
+        binding_indices = [b.binding_index for b in bindings]
+        all_indicies = set(range(LOWEST_BINDING_INDEX, num_agents + 1))
+        open_slots = sorted(list(all_indicies - set(binding_indices)))
+
+        if open_slots:
+            return open_slots[0]
+
+        # Last chance: if this is a manual scheduling, we're gonna allow
+        # creation of a binding_index even if it will exceed
+        # max_l3_agents_per_router.
+        if is_manual_scheduling:
+            return max(all_indicies) + 1
+
+        return -1
+
 
 class AZL3AgentSchedulerDbMixin(L3AgentSchedulerDbMixin,
                                 router_az.RouterAvailabilityZonePluginBase):

neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD

@@ -1 +1 @@
-97c25b0d2353
+2e0d7a8a1586

neutron/db/migration/alembic_migrations/versions/newton/contract/2e0d7a8a1586_add_binding_index_to_routerl3agentbinding.py

@@ -0,0 +1,76 @@
+# Copyright 2016 OpenStack Foundation
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+#
+
+"""Add binding index to RouterL3AgentBinding
+
+Revision ID: 2e0d7a8a1586
+Revises: 97c25b0d2353
+Create Date: 2016-09-01 14:01:57.263289
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '2e0d7a8a1586'
+down_revision = '97c25b0d2353'
+
+from collections import defaultdict
+
+from alembic import op
+import sqlalchemy as sa
+
+
+ROUTER_L3_AGENT_BINDING = 'routerl3agentbindings'
+
+
+def contract_creation_exceptions():
+    """Add a new binding_index to ensure that no over-creation of the bindings
+    is possible.
+    """
+    return {
+        sa.Column: ['%s.binding_index' % ROUTER_L3_AGENT_BINDING]
+    }
+
+
+def upgrade():
+    op.add_column(ROUTER_L3_AGENT_BINDING,
+                  sa.Column('binding_index', sa.Integer(), nullable=False,
+                           server_default='1'))
+
+    bindings_table = sa.Table(
+        ROUTER_L3_AGENT_BINDING,
+        sa.MetaData(),
+        sa.Column('router_id', sa.String(36)),
+        sa.Column('l3_agent_id', sa.String(36)),
+        sa.Column('binding_index', sa.Integer,
+                  nullable=False, server_default='1'),
+    )
+
+    routers_to_bindings = defaultdict(list)
+    session = sa.orm.Session(bind=op.get_bind())
+    with session.begin(subtransactions=True):
+        for result in session.query(bindings_table):
+            routers_to_bindings[result.router_id].append(result)
+
+        for bindings in routers_to_bindings.values():
+            for index, result in enumerate(bindings):
+                session.execute(bindings_table.update().values(
+                    binding_index=index + 1).where(
+                    bindings_table.c.router_id == result.router_id).where(
+                    bindings_table.c.l3_agent_id == result.l3_agent_id))
+    session.commit()
+
+    op.create_unique_constraint(
+        'uniq_router_l3_agent_binding0router_id0binding_index0',
+        ROUTER_L3_AGENT_BINDING, ['router_id', 'binding_index'])

neutron/scheduler/l3_agent_scheduler.py

@@ -138,25 +138,25 @@ class L3Scheduler(object):
         l3_agent = plugin.get_enabled_agent_on_host(
             context, lib_const.AGENT_TYPE_L3, host)
         if not l3_agent:
-            return False
+            return
 
         unscheduled_routers = self._get_routers_to_schedule(
             context, plugin, router_ids)
         if not unscheduled_routers:
             if utils.is_extension_supported(
                     plugin, lib_const.L3_HA_MODE_EXT_ALIAS):
-                return self._schedule_ha_routers_to_additional_agent(
+                self._schedule_ha_routers_to_additional_agent(
                     plugin, context, l3_agent)
+                return
 
         target_routers = self._get_routers_can_schedule(
             context, plugin, unscheduled_routers, l3_agent)
         if not target_routers:
             LOG.warning(_LW('No routers compatible with L3 agent '
                             'configuration on host %s'), host)
-            return False
+            return
 
         self._bind_routers(context, plugin, target_routers, l3_agent)
-        return True
 
     def _get_candidates(self, plugin, context, sync_router):
         """Return L3 agents where a router could be scheduled."""
@@ -198,19 +198,30 @@ class L3Scheduler(object):
             else:
                 self.bind_router(context, router['id'], l3_agent)
 
-    def bind_router(self, context, router_id, chosen_agent):
+    def bind_router(self, context, router_id, chosen_agent,
+                    binding_index=l3_agentschedulers_db.LOWEST_BINDING_INDEX):
         """Bind the router to the l3 agent which has been chosen."""
+        # Pre-cache the agent's id so that if an exception is raised we can
+        # safely access its value. Otherwise, sqlalchemy will try to fetch it
+        # from the database during a rollback, which is bad for us.
+        agent_id = chosen_agent.id
+
         try:
             with context.session.begin(subtransactions=True):
                 binding = l3_agentschedulers_db.RouterL3AgentBinding()
                 binding.l3_agent = chosen_agent
                 binding.router_id = router_id
+                binding.binding_index = binding_index
                 context.session.add(binding)
-        except db_exc.DBDuplicateEntry:
+        except db_exc.DBDuplicateEntry as error:
             LOG.debug('Router %(router_id)s has already been scheduled '
-                      'to L3 agent %(agent_id)s.',
+                      'to L3 agent %(agent_id)s (tried to bind with '
-                      {'agent_id': chosen_agent.id,
+                      'binding_index %(binding_index)d). The conflict was '
-                       'router_id': router_id})
+                      'with columns %(columns)r.',
+                      {'agent_id': agent_id,
+                       'router_id': router_id,
+                       'binding_index': binding_index,
+                       'columns': error.columns})
             return
         except db_exc.DBReferenceError:
             LOG.debug('Router %s has already been removed '
@@ -218,8 +229,11 @@ class L3Scheduler(object):
             return
 
         LOG.debug('Router %(router_id)s is scheduled to L3 agent '
-                  '%(agent_id)s', {'router_id': router_id,
+                  '%(agent_id)s with binding_index %(binding_index)d',
-                                   'agent_id': chosen_agent.id})
+                  {'router_id': router_id,
+                   'agent_id': agent_id,
+                   'binding_index': binding_index})
+        return binding
 
     def _schedule_router(self, plugin, context, router_id,
                          candidates=None):
@@ -267,7 +281,7 @@ class L3Scheduler(object):
                                   tenant_id)
 
     def create_ha_port_and_bind(self, plugin, context, router_id,
-                                tenant_id, agent):
+                                tenant_id, agent, is_manual_scheduling=False):
         """Creates and binds a new HA port for this agent."""
         ctxt = context.elevated()
         creator = functools.partial(self._add_port_from_net,
@@ -277,20 +291,36 @@ class L3Scheduler(object):
                                         ctxt, tenant_id)
         dep_deleter = functools.partial(plugin._delete_ha_network, ctxt)
         dep_id_attr = 'network_id'
-        try:
-            port_binding = utils.create_object_with_dependency(
-                creator, dep_getter, dep_creator, dep_id_attr, dep_deleter)[0]
-            with db_api.autonested_transaction(context.session):
-                port_binding.l3_agent_id = agent['id']
-        except db_exc.DBDuplicateEntry:
-            LOG.debug("Router %(router)s already scheduled for agent "
-                      "%(agent)s", {'router': router_id, 'agent': agent['id']})
-        except l3.RouterNotFound:
-            LOG.debug('Router %s has already been removed '
-                      'by concurrent operation', router_id)
-            return
 
-        self.bind_router(context, router_id, agent)
+        for attempts in range(1, db_api.MAX_RETRIES + 1):
+            binding_index = plugin.get_vacant_binding_index(
+                context, router_id, is_manual_scheduling)
+            if binding_index == -1:
+                LOG.debug("Couldn't find a vacant binding_index for router %s",
+                          router_id)
+                return
+
+            # This might fail in case of concurrent calls, which is good for us
+            # as we can skip the rest of this function.
+            if not self.bind_router(context, router_id, agent, binding_index):
+                return
+
+            try:
+                port_binding = utils.create_object_with_dependency(
+                    creator, dep_getter, dep_creator,
+                    dep_id_attr, dep_deleter)[0]
+                with db_api.autonested_transaction(context.session):
+                    port_binding.l3_agent_id = agent['id']
+                return
+            except db_exc.DBDuplicateEntry:
+                LOG.debug("Router %(router)s already scheduled for agent "
+                          "%(agent)s", {'router': router_id,
+                                        'agent': agent['id']})
+                return
+            except l3.RouterNotFound:
+                LOG.debug('Router %s has already been removed '
+                          'by concurrent operation', router_id)
+                return
 
     def get_ha_routers_l3_agents_counts(self, context, plugin, filters=None):
         """Return a mapping (router, # agents) matching specified filters."""
@@ -306,7 +336,6 @@ class L3Scheduler(object):
 
         routers_agents = self.get_ha_routers_l3_agents_counts(context, plugin,
                                                               agent)
-        scheduled = False
         admin_ctx = context.elevated()
         underscheduled_routers = [router for router, agents in routers_agents
                                   if (not self.max_ha_agents or
@@ -320,19 +349,21 @@ class L3Scheduler(object):
                                              router['id'],
                                              router['tenant_id'],
                                              agent)
-                scheduled = True
-
-        return scheduled
 
     def _bind_ha_router_to_agents(self, plugin, context, router_id,
                                  chosen_agents):
         port_bindings = plugin.get_ha_router_port_bindings(context,
                                                            [router_id])
-        for port_binding, agent in zip(port_bindings, chosen_agents):
+        binding_indices = range(l3_agentschedulers_db.LOWEST_BINDING_INDEX,
+                                len(port_bindings) + 1)
+        for port_binding, agent, binding_index in zip(
+            port_bindings, chosen_agents, binding_indices):
+
+            if not self.bind_router(context, router_id, agent, binding_index):
+                continue
             try:
                 with db_api.autonested_transaction(context.session):
                     port_binding.l3_agent_id = agent.id
-                    self.bind_router(context, router_id, agent)
             except db_exc.DBDuplicateEntry:
                 LOG.debug("Router %(router)s already scheduled for agent "
                           "%(agent)s", {'router': router_id,

neutron/tests/functional/db/migrations/test_2e0d7a8a1586_add_binding_index_to_routerl3agentbinding.py

@@ -0,0 +1,87 @@
+# Copyright 2016 Business Cat is Very Serious 13.37
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+#
+
+import collections
+
+from oslo_db.sqlalchemy import utils as db_utils
+from oslo_utils import uuidutils
+
+from neutron.tests.functional.db import test_migrations
+
+
+class HARouterPortMigrationMixin(object):
+    """Validates binding_index for RouterL3AgentBinding migration."""
+
+    def _create_so(self, o_type, values):
+        """create standard attr object."""
+        stan = db_utils.get_table(self.engine, 'standardattributes')
+        # find next available id taking into account existing records
+        rec_ids = [r.id for r in self.engine.execute(stan.select()).fetchall()]
+        next_id = max([0] + rec_ids) + 1
+        self.engine.execute(stan.insert().values({'id': next_id,
+                                                  'resource_type': o_type}))
+        values['standard_attr_id'] = next_id
+        return self._create_rec(o_type, values)
+
+    def _create_rec(self, o_type, values):
+        otable = db_utils.get_table(self.engine, o_type)
+        self.engine.execute(otable.insert().values(values))
+
+    def _make_router_agents_and_bindings(self, router_id):
+        self._create_so('routers', {'id': router_id})
+        # each router gets a couple of agents
+        for _ in range(2):
+            agent_id = uuidutils.generate_uuid()
+            timestamp = '2000-04-06T14:34:23'
+            self._create_rec('agents', {'id': agent_id,
+                                        'topic': 'x',
+                                        'agent_type': 'L3',
+                                        'binary': 'x',
+                                        'host': agent_id,
+                                        'created_at': timestamp,
+                                        'started_at': timestamp,
+                                        'heartbeat_timestamp': timestamp,
+                                        'configurations': ''})
+            self._create_rec('routerl3agentbindings',
+                             {'router_id': router_id, 'l3_agent_id': agent_id})
+
+    def _create_ha_routers(self, engine):
+        for rid in [uuidutils.generate_uuid() for i in range(10)]:
+            self._make_router_agents_and_bindings(rid)
+
+    def _pre_upgrade_2e0d7a8a1586(self, engine):
+        self._create_ha_routers(engine)
+        return True  # return True so check function is invoked after migrate
+
+    def _check_2e0d7a8a1586(self, engine, data):
+        bindings_table = db_utils.get_table(engine, 'routerl3agentbindings')
+        rows = engine.execute(bindings_table.select()).fetchall()
+
+        routers_to_bindings = collections.defaultdict(list)
+        for router_id, agent_id, binding_index in rows:
+            routers_to_bindings[router_id].append(binding_index)
+
+        for binding_indices in routers_to_bindings.values():
+            self.assertEqual(list(range(1, 3)), sorted(binding_indices))
+
+
+class TestHARouterPortMigrationMysql(HARouterPortMigrationMixin,
+                                     test_migrations.TestWalkMigrationsMysql):
+    pass
+
+
+class TestHARouterPortMigrationPsql(HARouterPortMigrationMixin,
+                                    test_migrations.TestWalkMigrationsPsql):
+    pass

neutron/tests/functional/scheduler/test_l3_agent_scheduler.py

@@ -96,22 +96,26 @@ class L3SchedulerBaseTest(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
     def _test_auto_schedule(self, expected_count):
         router_ids = [rtr['id'] for rtr in self.routers]
 
-        did_it_schedule = False
+        hosting_before = self.l3_plugin.get_l3_agents_hosting_routers(
+            self.adminContext, router_ids)
 
         # Try scheduling on each host
         for host in self.hosts:
-            did_it_schedule = self.scheduler.auto_schedule_routers(
+            self.scheduler.auto_schedule_routers(
                 self.l3_plugin,
                 self.adminContext,
                 host,
                 router_ids)
-            if did_it_schedule:
+
-                break
+        hosting_after = self.l3_plugin.get_l3_agents_hosting_routers(
+            self.adminContext, router_ids)
 
         if expected_count:
-            self.assertTrue(did_it_schedule, 'Failed to schedule agent')
+            self.assertNotEqual(hosting_before, hosting_after,
+                                'Failed to schedule agent')
         else:
-            self.assertFalse(did_it_schedule, 'Agent scheduled, not expected')
+            self.assertEqual(hosting_before, hosting_after,
+                             'Agent scheduled, not expected')
 
 
 class L3ChanceSchedulerTestCase(L3SchedulerBaseTest):
@@ -500,16 +504,6 @@ class L3AZAutoScheduleTestCaseBase(L3AZSchedulerBaseTest):
               scheduled_agent_count=[0, 0, 0],
               expected_scheduled_agent_count=[0, 0, 0])),
 
-        ('HA router, partial scheduled, agent in specified AZ activated',
-         dict(az_count=3,
-              router_az_hints=2,
-              agent_az='az1',
-              agent_count=[1, 1, 1],
-              max_l3_agents_per_router=2,
-              min_l3_agents_per_router=2,
-              down_agent_count=[0, 1, 0],
-              scheduled_agent_count=[1, 0, 0],
-              expected_scheduled_agent_count=[1, 1, 0])),
     ]
 
     def test_auto_schedule_router(self):
@@ -794,24 +788,27 @@ class L3DVRSchedulerTestCase(L3DVRSchedulerBaseTest):
             self.scheduler.bind_router(self.adminContext,
                                        self.router_to_schedule['id'],
                                        self.l3_agents[0])
-        did_it_schedule = False
-
         # schedule:
+        hosting_before = self.l3_plugin.get_l3_agents_hosting_routers(
+            self.adminContext, [self.router_to_schedule['id']])
+
         for host in self.hosts:
-            did_it_schedule = self.scheduler.auto_schedule_routers(
+            self.scheduler.auto_schedule_routers(
-                                  self.l3_plugin, self.adminContext,
+                self.l3_plugin, self.adminContext,
-                                  host, [self.router_to_schedule['id']])
+                host, [self.router_to_schedule['id']])
-            if did_it_schedule:
+
-                break
+        hosting_after = self.l3_plugin.get_l3_agents_hosting_routers(
+            self.adminContext, [self.router_to_schedule['id']])
 
         if self.router_already_hosted:
-            self.assertFalse(did_it_schedule,
+            self.assertEqual(hosting_before, hosting_after,
-                            'Agent pre scheduled, yet no binding found!')
+                             'Agent pre scheduled, yet no binding found!')
         elif self.expected_router_scheduled:
-            self.assertTrue(did_it_schedule,
+            self.assertNotEqual(hosting_before, hosting_after,
-                            'Agent not scheduled, not expected')
+                                'Agent not scheduled, not expected')
         else:
-            self.assertFalse(did_it_schedule, 'Agent scheduled, not expected')
+            self.assertEqual(hosting_before, hosting_after,
+                             'Agent scheduled, not expected')
 
     def test_least_routers_schedule_router(self):
         self.scheduler = l3_agent_scheduler.LeastRoutersScheduler()

neutron/tests/unit/scheduler/test_l3_agent_scheduler.py

@@ -135,13 +135,15 @@ class L3SchedulerBaseTestCase(base.BaseTestCase):
         with mock.patch.object(self.scheduler,
                                '_get_routers_to_schedule') as gs,\
                 mock.patch.object(self.scheduler,
-                                  '_get_routers_can_schedule') as gr:
+                                  '_get_routers_can_schedule',) as gr,\
-            result = self.scheduler.auto_schedule_routers(
+                mock.patch.object(self.scheduler,
+                                  '_bind_routers') as gb:
+            self.scheduler.auto_schedule_routers(
                 self.plugin, mock.ANY, mock.ANY, mock.ANY)
             self.assertTrue(self.plugin.get_enabled_agent_on_host.called)
-            self.assertTrue(result)
             self.assertTrue(gs.called)
             self.assertTrue(gr.called)
+            self.assertTrue(gb.called)
 
     def test_auto_schedule_routers_no_agents(self):
         self.plugin.get_enabled_agent_on_host.return_value = None
@@ -1428,6 +1430,18 @@ class L3HATestCaseMixin(testlib_api.SqlTestCase,
 
         self._register_l3_agents()
 
+    @staticmethod
+    def get_router_l3_agent_binding(context, router_id, l3_agent_id=None,
+                                    binding_index=None):
+        model = l3_agentschedulers_db.RouterL3AgentBinding
+        query = context.session.query(model)
+        query = query.filter(model.router_id == router_id)
+        if l3_agent_id:
+            query = query.filter(model.l3_agent_id == l3_agent_id)
+        if binding_index:
+            query = query.filter(model.binding_index == binding_index)
+        return query
+
     def _create_ha_router(self, ha=True, tenant_id='tenant1', az_hints=None):
         self.adminContext.tenant_id = tenant_id
         router = {'name': 'router1', 'admin_state_up': True,
@@ -1477,6 +1491,74 @@ class L3HATestCaseMixin(testlib_api.SqlTestCase,
                 self.assertFalse(bind_router.called)
 
 
+class VacantBindingIndexTestCase(L3HATestCaseMixin):
+    """Test various scenarios for get_vacant_binding_index().
+
+        binding_index
+            The binding_index we want to delete/unschedule.
+
+        is_manual_scheduling
+            Whether or not this is a scheduling requested by the user
+            (`neutron l3-agent-router-add`) or by some worker (scheduler or RPC
+            from agent). If this is a manual scheduling we should always
+            comply.
+    """
+
+    binding_scenarios = [
+        ('Delete first binding_index',
+         dict(binding_index=1)),
+
+        ('Delete middle binding_index',
+         dict(binding_index=2)),
+
+        ('Delete last binding_index',
+         dict(binding_index=3)),
+
+        ('Do not remove any bindings',
+         dict(binding_index=None)),
+    ]
+
+    manual_scheduling_scenarios = [
+        ('with manual scheduling',
+         dict(is_manual_scheduling=True)),
+
+        ('without manual scheduling',
+         dict(is_manual_scheduling=False)),
+    ]
+
+    scenarios = testscenarios.multiply_scenarios(
+        binding_scenarios, manual_scheduling_scenarios)
+
+    def test_get_vacant_binding_index(self):
+        helpers.register_l3_agent('host_3')
+        cfg.CONF.set_override('max_l3_agents_per_router', 3)
+        cfg.CONF.set_override('min_l3_agents_per_router', 3)
+        router = self._create_ha_router()
+
+        if self.binding_index:
+            binding = self.get_router_l3_agent_binding(
+                self.adminContext, router['id'],
+                binding_index=self.binding_index)
+            self.assertEqual(1, binding.count())
+            with self.adminContext.session.begin():
+                self.adminContext.session.delete(binding.first())
+
+        vacant_binding_index = self.plugin.get_vacant_binding_index(
+                self.adminContext, router['id'], self.is_manual_scheduling)
+
+        if self.binding_index:
+            self.assertEqual(self.binding_index, vacant_binding_index)
+        else:
+            if self.is_manual_scheduling:
+                # If this is a manual scheduling, the user requested the
+                # binding so we should always provide a new one.
+                self.assertEqual(cfg.CONF.max_l3_agents_per_router + 1,
+                                 vacant_binding_index)
+            else:
+                # Else, we already have 3 so -1 is the 'error' value.
+                self.assertEqual(-1, vacant_binding_index)
+
+
 class L3_HA_scheduler_db_mixinTestCase(L3HATestCaseMixin):
 
     def _register_l3_agents(self, plugin=None):
@@ -1635,6 +1717,37 @@ class L3AgentSchedulerDbMixinTestCase(L3HATestCaseMixin):
                                                            [router['id']])
         self.assertIn(agent.id, [ha_port.l3_agent_id for ha_port in ha_ports])
 
+    def test_schedule_routers_unique_binding_indices(self):
+        cfg.CONF.set_override('max_l3_agents_per_router', 2)
+        router = self._create_ha_router()
+
+        bindings = self.get_router_l3_agent_binding(self.adminContext,
+                                                    router['id']).all()
+        binding_indices = [binding.binding_index for binding in bindings]
+        self.assertEqual(list(range(1, cfg.CONF.max_l3_agents_per_router + 1)),
+                         binding_indices)
+
+    def test_bind_router_twice_for_non_ha(self):
+        router = self._create_ha_router(ha=False)
+        self.plugin.router_scheduler.bind_router(self.adminContext,
+                                                 router['id'],
+                                                 self.agent1)
+        self.plugin.router_scheduler.bind_router(self.adminContext,
+                                                 router['id'],
+                                                 self.agent2)
+
+        # Make sure the second bind_router call didn't schedule the router to
+        # more agents than allowed.
+        agents = self.plugin.get_l3_agents_hosting_routers(self.adminContext,
+                                                           [router['id']])
+        self.assertEqual(1, len(agents))
+
+        # Moreover, make sure that the agent that did get bound, only got bound
+        # once.
+        bindings = self.get_router_l3_agent_binding(
+            self.adminContext, router['id'], l3_agent_id=agents[0]['id'])
+        self.assertEqual(1, bindings.count())
+
 
 class L3HAChanceSchedulerTestCase(L3HATestCaseMixin):