Removing a router twice from the same agent shouldn't cause an error

When we remove a router from an agent that has already been
unscheduled from we raise an exception that eventually causes an error.
The method '_unbind_router' raises a 'RouterNotHostedByL3Agent' exception
on failure. In both cases the actual removal of the router
from the same agent has no effect.

The solution is to stop raising 'RouterNotHostedByL3Agent' so
that _unbind_router() being invoked without error can indicate that
the router is no longer bound.

This solution matches the behaviour found when trying
to schedule a router to the same agent twice.

This change is a result of the discussion in:

https://review.openstack.org/#/c/144681/2

Closes-Bug: #1406705

Change-Id: I015bfc0fde11ba4f39417e4c134faa8132cb3eac

diff --git a/neutron/db/l3_agentschedulers_db.py b/neutron/db/l3_agentschedulers_db.py
index 444a724a697482398bb7a71394982006ece289bf..88da19aed0c915531c1731928674b410b76997c4 100644 (file)
--- a/neutron/db/l3_agentschedulers_db.py
+++ b/neutron/db/l3_agentschedulers_db.py
@@ -19,7 +19,6 @@ import sqlalchemy as sa
 from sqlalchemy import func
 from sqlalchemy import or_
 from sqlalchemy import orm
-from sqlalchemy.orm import exc
 from sqlalchemy.orm import joinedload
 from sqlalchemy import sql
 
@@ -228,12 +227,7 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase,
             query = query.filter(
                 RouterL3AgentBinding.router_id == router_id,
                 RouterL3AgentBinding.l3_agent_id == agent_id)
-            try:
-                binding = query.one()
-            except exc.NoResultFound:
-                raise l3agentscheduler.RouterNotHostedByL3Agent(
-                    router_id=router_id, agent_id=agent_id)
-            context.session.delete(binding)
+            query.delete()
 
     def reschedule_router(self, context, router_id, candidates=None):
         """Reschedule router to a new l3 agent

diff --git a/neutron/extensions/l3agentscheduler.py b/neutron/extensions/l3agentscheduler.py
index d9beb6e88356249b22b625d42f022982f36256a8..f18a7abdb314c3732d1f3150db09fe8a1cd24810 100644 (file)
--- a/neutron/extensions/l3agentscheduler.py
+++ b/neutron/extensions/l3agentscheduler.py
@@ -172,11 +172,6 @@ class RouterReschedulingFailed(exceptions.Conflict):
                 "no eligible l3 agent found.")
 
 
-class RouterNotHostedByL3Agent(exceptions.Conflict):
-    message = _("The router %(router_id)s is not hosted"
-                " by L3 agent %(agent_id)s.")
-
-
 class RouterL3AgentMismatch(exceptions.Conflict):
     message = _("Cannot host %(router_type)s router %(router_id)s "
                 "on %(agent_mode)s L3 agent %(agent_id)s.")

diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py
index 58787aba9f1181e42b0b97be55e2e38cd2fb422e..7f273a36d26c7c92b365929d3bc62514fa5a4587 100644 (file)
--- a/neutron/plugins/ml2/plugin.py
+++ b/neutron/plugins/ml2/plugin.py
@@ -51,7 +51,6 @@ from neutron.db import quota_db  # noqa
 from neutron.db import securitygroups_rpc_base as sg_db_rpc
 from neutron.extensions import allowedaddresspairs as addr_pair
 from neutron.extensions import extra_dhcp_opt as edo_ext
-from neutron.extensions import l3agentscheduler
 from neutron.extensions import portbindings
 from neutron.extensions import providernet as provider
 from neutron.i18n import _LE, _LI, _LW
@@ -1149,14 +1148,8 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
                 l3plugin.dvr_vmarp_table_update(context, port, "del")
             l3plugin.notify_routers_updated(context, router_ids)
             for router in removed_routers:
-                try:
-                    l3plugin.remove_router_from_l3_agent(
-                        context, router['agent_id'], router['router_id'])
-                except l3agentscheduler.RouterNotHostedByL3Agent:
-                    # router may have been removed by another process
-                    LOG.debug("Router %(id)s not hosted by L3 agent %(agent)s",
-                        {'id': router['router_id'],
-                         'agent': router['agent_id']})
+                l3plugin.remove_router_from_l3_agent(
+                    context, router['agent_id'], router['router_id'])
         try:
             # Note that DVR Interface ports will have bindings on
             # multiple hosts, and so will have multiple mech_contexts,

diff --git a/neutron/tests/unit/ml2/test_ml2_plugin.py b/neutron/tests/unit/ml2/test_ml2_plugin.py
index 2b07e9eb775fa4c7b9356d3dfddced23515e26c1..5dec58e49d6002d2305639a923c22c607be06476 100644 (file)
--- a/neutron/tests/unit/ml2/test_ml2_plugin.py
+++ b/neutron/tests/unit/ml2/test_ml2_plugin.py
@@ -27,7 +27,6 @@ from neutron import context
 from neutron.db import db_base_plugin_v2 as base_plugin
 from neutron.db import l3_db
 from neutron.extensions import external_net as external_net
-from neutron.extensions import l3agentscheduler
 from neutron.extensions import multiprovidernet as mpnet
 from neutron.extensions import portbindings
 from neutron.extensions import providernet as pnet
@@ -325,10 +324,7 @@ class TestMl2DvrPortsV2(TestMl2PortsV2):
                       device_owner='compute:None'),
             mock.patch.object(self.l3plugin, 'dvr_deletens_if_no_port',
                               return_value=[ns_to_delete]),
-            mock.patch.object(self.l3plugin, 'remove_router_from_l3_agent',
-                side_effect=l3agentscheduler.RouterNotHostedByL3Agent(
-                            router_id=ns_to_delete['router_id'],
-                            agent_id=ns_to_delete['agent_id']))
+            mock.patch.object(self.l3plugin, 'remove_router_from_l3_agent')
         ) as (get_service_plugin, port, dvr_delns_ifno_port,
               remove_router_from_l3_agent):
 

diff --git a/neutron/tests/unit/test_l3_schedulers.py b/neutron/tests/unit/test_l3_schedulers.py
index 9e6034251116cbe6dd948ca6f9458131056729ee..110b3c57df35f7e91e0ff26a170eb2a6d860d222 100644 (file)
--- a/neutron/tests/unit/test_l3_schedulers.py
+++ b/neutron/tests/unit/test_l3_schedulers.py
@@ -341,6 +341,20 @@ class L3SchedulerTestBaseMixin(object):
                                         router['router']['id'])
             self.assertNotEqual(already_scheduled, auto_s.called)
 
+    def test__unbind_router_removes_binding(self):
+        agent_id = self.agent_id1
+        agent = self.agent1
+        router = self._make_router(self.fmt,
+                                   tenant_id=str(uuid.uuid4()),
+                                   name='r1')
+        self._test_schedule_bind_router(agent, router)
+        self._unbind_router(self.adminContext,
+                            router['router']['id'],
+                            agent_id)
+        bindings = self._get_l3_bindings_hosting_routers(
+            self.adminContext, [router['router']['id']])
+        self.assertEqual(0, len(bindings))
+
     def _create_router_for_l3_agent_dvr_test(self,
                                              distributed=False,
                                              external_gw=None):