From: Eugene Nikanorov Date: Tue, 19 Aug 2014 19:01:11 +0000 (+0400) Subject: Fix l3 agent scheduling logic to avoid unwanted failures X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=89e76a8afae19c5f66d532538607aedaac59722a;p=openstack-build%2Fneutron-build.git 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 --- 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()