From 89e76a8afae19c5f66d532538607aedaac59722a Mon Sep 17 00:00:00 2001 From: Eugene Nikanorov Date: Tue, 19 Aug 2014 23:01:11 +0400 Subject: [PATCH] Fix l3 agent scheduling logic to avoid unwanted failures In case router is being added to l3 agent which is already hosting the router it is fine to let such a request to succeed. This patch also adds a check for unnecessary scheduling that might happen twice in described case and lead to unwanted messages in the logs. Change-Id: Id104b36ba7e1e6f6a9378ee600c33e9962230521 Closes-Bug: #1358636 --- neutron/db/l3_agentschedulers_db.py | 40 +++++++++-------- .../unit/openvswitch/test_agent_scheduler.py | 4 +- neutron/tests/unit/test_l3_schedulers.py | 44 +++++++++++++++++++ 3 files changed, 67 insertions(+), 21 deletions(-) diff --git a/neutron/db/l3_agentschedulers_db.py b/neutron/db/l3_agentschedulers_db.py index 1543779de..c937bc742 100644 --- a/neutron/db/l3_agentschedulers_db.py +++ b/neutron/db/l3_agentschedulers_db.py @@ -152,30 +152,29 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase, if is_wrong_type_or_unsuitable_agent: raise l3agentscheduler.InvalidL3Agent(id=agent['id']) - def validate_agent_router_existing_binding(self, context, agent, router): - """Validate if the router is already assigned to the agent. + def check_agent_router_scheduling_needed(self, context, agent, router): + """Check if the router scheduling is needed. :raises: RouterHostedByL3Agent if router is already assigned + to a different agent. + :returns: True if scheduling is needed, otherwise False """ router_id = router['id'] agent_id = agent['id'] query = context.session.query(RouterL3AgentBinding) + bindings = query.filter_by(router_id=router_id).all() + if not bindings: + return True + for binding in bindings: + if binding.l3_agent_id == agent_id: + # router already bound to the agent we need + return False if router.get('distributed'): - binding = query.filter_by(router_id=router_id, - l3_agent_id=agent_id).first() - if binding: - raise l3agentscheduler.RouterHostedByL3Agent( - router_id=router_id, - agent_id=binding.l3_agent_id) - else: - try: - binding = query.filter_by(router_id=router_id).one() - - raise l3agentscheduler.RouterHostedByL3Agent( - router_id=router_id, - agent_id=binding.l3_agent_id) - except exc.NoResultFound: - pass + return False + # non-dvr case: centralized router is already bound to some agent + raise l3agentscheduler.RouterHostedByL3Agent( + router_id=router_id, + agent_id=bindings[0].l3_agent_id) def create_router_to_agent_binding(self, context, agent, router): """Create router to agent binding.""" @@ -192,8 +191,11 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase, router = self.get_router(context, router_id) agent = self._get_agent(context, agent_id) self.validate_agent_router_combination(context, agent, router) - self.validate_agent_router_existing_binding(context, agent, router) - self.create_router_to_agent_binding(context, agent, router) + if self.check_agent_router_scheduling_needed( + context, agent, router): + self.create_router_to_agent_binding(context, agent, router) + else: + return l3_notifier = self.agent_notifiers.get(constants.AGENT_TYPE_L3) if l3_notifier: diff --git a/neutron/tests/unit/openvswitch/test_agent_scheduler.py b/neutron/tests/unit/openvswitch/test_agent_scheduler.py index cce5552fe..26e4cace3 100644 --- a/neutron/tests/unit/openvswitch/test_agent_scheduler.py +++ b/neutron/tests/unit/openvswitch/test_agent_scheduler.py @@ -1055,9 +1055,9 @@ class OvsAgentSchedulerTestCase(OvsAgentSchedulerTestCaseBase): L3_HOSTA) self._add_router_to_l3_agent(hosta_id, router1['router']['id']) + # scheduling twice on the same agent is fine self._add_router_to_l3_agent(hosta_id, - router1['router']['id'], - expected_code=exc.HTTPConflict.code) + router1['router']['id']) def test_router_add_to_two_l3_agents(self): with self.router() as router1: diff --git a/neutron/tests/unit/test_l3_schedulers.py b/neutron/tests/unit/test_l3_schedulers.py index 86d7c0d0a..9cfcaf657 100644 --- a/neutron/tests/unit/test_l3_schedulers.py +++ b/neutron/tests/unit/test_l3_schedulers.py @@ -28,6 +28,7 @@ from neutron.common import constants from neutron.common import topics from neutron import context as q_context from neutron.db import agents_db +from neutron.db import common_db_mixin from neutron.db import l3_agentschedulers_db from neutron.db import l3_db from neutron.db import l3_dvrscheduler_db @@ -232,6 +233,8 @@ class L3SchedulerTestExtensionManager(object): class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin, + l3_db.L3_NAT_db_mixin, + common_db_mixin.CommonDbMixin, test_db_plugin.NeutronDbPluginV2TestCase, test_l3_plugin.L3NatTestCaseMixin): @@ -301,6 +304,47 @@ class L3SchedulerTestCase(l3_agentschedulers_db.L3AgentSchedulerDbMixin, router['router']['id'], subnet['subnet']['network_id']) self._delete('routers', router['router']['id']) + def _test_add_router_to_l3_agent(self, + distributed=False, + already_scheduled=False): + agent_id = self.agent_id1 + agent = self.agent1 + if distributed: + self._register_l3_dvr_agents() + agent_id = self.l3_dvr_snat_id + agent = self.l3_dvr_snat_agent + router = self._make_router(self.fmt, + tenant_id=str(uuid.uuid4()), + name='r1') + router['router']['distributed'] = distributed + router['router']['external_gateway_info'] = None + if already_scheduled: + self._test_schedule_bind_router(agent, router) + with contextlib.nested( + mock.patch.object(self, "create_router_to_agent_binding"), + mock.patch('neutron.db.l3_db.L3_NAT_db_mixin.get_router', + return_value=router['router']) + ) as (auto_s, gr): + self.add_router_to_l3_agent(self.adminContext, agent_id, + router['router']['id']) + self.assertNotEqual(already_scheduled, auto_s.called) + + def test_add_router_to_l3_agent(self): + self._test_add_router_to_l3_agent(distributed=False, + already_scheduled=False) + + def test_add_distributed_router_to_l3_agent(self): + self._test_add_router_to_l3_agent(distributed=True, + already_scheduled=False) + + def test_add_router_to_l3_agent_already_scheduled(self): + self._test_add_router_to_l3_agent(distributed=False, + already_scheduled=True) + + def test_add_distributed_router_to_l3_agent_already_scheduled(self): + self._test_add_router_to_l3_agent(distributed=True, + already_scheduled=True) + def test_schedule_router_distributed(self): scheduler = l3_agent_scheduler.ChanceScheduler() agent = agents_db.Agent() -- 2.45.2