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 1543779de03b27702f7dccc9d662fe185b7d2904..c937bc74281daefffd44575253ffca68854efd52 100644 (file)
--- 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 cce5552feb1fba5f8ecf9d3ca3485961f5e11b01..26e4cace32f313d01eca6ca48c32f6cebe909968 100644 (file)
--- 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 86d7c0d0a0004c3df21be407d5ebee4afffd28c7..9cfcaf657c5007cb786d71f18fb8a2503e29c98f 100644 (file)
--- 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()