From b1ec8d523d4c45616dd71016f7e218b4b732c2ee Mon Sep 17 00:00:00 2001 From: John Schwarz Date: Fri, 19 Aug 2016 15:17:21 +0100 Subject: [PATCH] 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 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 | 49 +++++++- .../alembic_migrations/versions/CONTRACT_HEAD | 2 +- ...d_binding_index_to_routerl3agentbinding.py | 76 +++++++++++ neutron/scheduler/l3_agent_scheduler.py | 93 +++++++++----- ...d_binding_index_to_routerl3agentbinding.py | 87 +++++++++++++ .../scheduler/test_l3_agent_scheduler.py | 53 ++++---- .../unit/scheduler/test_l3_agent_scheduler.py | 119 +++++++++++++++++- 7 files changed, 414 insertions(+), 65 deletions(-) create mode 100644 neutron/db/migration/alembic_migrations/versions/newton/contract/2e0d7a8a1586_add_binding_index_to_routerl3agentbinding.py create mode 100644 neutron/tests/functional/db/migrations/test_2e0d7a8a1586_add_binding_index_to_routerl3agentbinding.py diff --git a/neutron/db/l3_agentschedulers_db.py b/neutron/db/l3_agentschedulers_db.py index 1e4c75a6b01..4eccde58cbd 100644 --- a/neutron/db/l3_agentschedulers_db.py +++ b/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): diff --git a/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD b/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD index 1b0ccbcc77b..77434118dfc 100644 --- a/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD +++ b/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD @@ -1 +1 @@ -97c25b0d2353 +2e0d7a8a1586 diff --git a/neutron/db/migration/alembic_migrations/versions/newton/contract/2e0d7a8a1586_add_binding_index_to_routerl3agentbinding.py b/neutron/db/migration/alembic_migrations/versions/newton/contract/2e0d7a8a1586_add_binding_index_to_routerl3agentbinding.py new file mode 100644 index 00000000000..16961cd2430 --- /dev/null +++ b/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']) diff --git a/neutron/scheduler/l3_agent_scheduler.py b/neutron/scheduler/l3_agent_scheduler.py index e9046dcfe09..45b39cbf7bd 100644 --- a/neutron/scheduler/l3_agent_scheduler.py +++ b/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.', - {'agent_id': chosen_agent.id, - 'router_id': router_id}) + 'to L3 agent %(agent_id)s (tried to bind with ' + 'binding_index %(binding_index)d). The conflict was ' + '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': chosen_agent.id}) + '%(agent_id)s with binding_index %(binding_index)d', + {'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, diff --git a/neutron/tests/functional/db/migrations/test_2e0d7a8a1586_add_binding_index_to_routerl3agentbinding.py b/neutron/tests/functional/db/migrations/test_2e0d7a8a1586_add_binding_index_to_routerl3agentbinding.py new file mode 100644 index 00000000000..d9316ca6475 --- /dev/null +++ b/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 diff --git a/neutron/tests/functional/scheduler/test_l3_agent_scheduler.py b/neutron/tests/functional/scheduler/test_l3_agent_scheduler.py index 01523f0d107..9ad8c1ae659 100644 --- a/neutron/tests/functional/scheduler/test_l3_agent_scheduler.py +++ b/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.l3_plugin, self.adminContext, - host, [self.router_to_schedule['id']]) - if did_it_schedule: - break + self.scheduler.auto_schedule_routers( + self.l3_plugin, self.adminContext, + host, [self.router_to_schedule['id']]) + + 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, - 'Agent pre scheduled, yet no binding found!') + self.assertEqual(hosting_before, hosting_after, + 'Agent pre scheduled, yet no binding found!') elif self.expected_router_scheduled: - self.assertTrue(did_it_schedule, - 'Agent not scheduled, not expected') + self.assertNotEqual(hosting_before, hosting_after, + '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() diff --git a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py index 82057ee2a12..5362d38831c 100644 --- a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py +++ b/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: - result = self.scheduler.auto_schedule_routers( + '_get_routers_can_schedule',) as gr,\ + 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):