Refactor _process_routers to handle a single router

The method _process_routers no longer handles multiple routers.  The
only caller of this method would construct a list of exactly one
router in order to make the call.  This made the for loop unnecessary.
The method's logic is too heavy for its current purpose.  This commit
removes much of the weight.

The use of the sets in this method is also no longer necessary.  It
became clear that all of it boiled down to "if the router is not
compatible with with this agent but it is known in router_info from
before then we need to remove it."  This is an exceptional condition
that shouldn't be handled in this method so I raise an exception and
handle it in process_router_update where other router removal is
handled.  Logging was added for this exceptional condition.

The eventlet pool was also obsolete.  It was used to spawn two methods
and there was a waitall at the end.  The other refactoring made it
clear that the two spawns were mutually exclusive.  There was only one
thread spawned for any given invocation of the method and the eventlet
pool is overkill.

Change-Id: Ibeac591b08565d10b2a9730e25a54f2cd11fc2bc
Closes-Bug: #1378398

diff --git a/neutron/agent/l3_agent.py b/neutron/agent/l3_agent.py
index e37a1fbc986ead7658853c5629360625f9b09139..829f7dd532ef145c89409d4a6ac293a4046386c7 100644 (file)
--- a/neutron/agent/l3_agent.py
+++ b/neutron/agent/l3_agent.py
@@ -35,6 +35,7 @@ from neutron.agent.linux import ra
 from neutron.agent import rpc as agent_rpc
 from neutron.common import config as common_config
 from neutron.common import constants as l3_constants
+from neutron.common import exceptions as n_exc
 from neutron.common import ipv6_utils
 from neutron.common import rpc as n_rpc
 from neutron.common import topics
@@ -42,7 +43,7 @@ from neutron.common import utils as common_utils
 from neutron import context
 from neutron import manager
 from neutron.openstack.common import excutils
-from neutron.openstack.common.gettextutils import _LW
+from neutron.openstack.common.gettextutils import _LE, _LW
 from neutron.openstack.common import importutils
 from neutron.openstack.common import log as logging
 from neutron.openstack.common import loopingcall
@@ -1779,48 +1780,38 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback,
         LOG.debug(_('Got router added to agent :%r'), payload)
         self.routers_updated(context, payload)
 
-    def _process_routers(self, routers):
-        pool = eventlet.GreenPool()
+    def _process_router_if_compatible(self, router):
         if (self.conf.external_network_bridge and
             not ip_lib.device_exists(self.conf.external_network_bridge)):
             LOG.error(_("The external network bridge '%s' does not exist"),
                       self.conf.external_network_bridge)
             return
 
+        # If namespaces are disabled, only process the router associated
+        # with the configured agent id.
+        if (not self.conf.use_namespaces and
+            router['id'] != self.conf.router_id):
+            raise n_exc.RouterNotCompatibleWithAgent(router_id=router['id'])
+
+        # Either ex_net_id or handle_internal_only_routers must be set
+        ex_net_id = (router['external_gateway_info'] or {}).get('network_id')
+        if not ex_net_id and not self.conf.handle_internal_only_routers:
+            raise n_exc.RouterNotCompatibleWithAgent(router_id=router['id'])
+
+        # If target_ex_net_id and ex_net_id are set they must be equal
         target_ex_net_id = self._fetch_external_net_id()
-        # if routers are all the routers we have (They are from router sync on
-        # starting or when error occurs during running), we seek the
-        # routers which should be removed.
-        # If routers are from server side notification, we seek them
-        # from subset of incoming routers and ones we have now.
-        prev_router_ids = set(self.router_info) & set(
-            [router['id'] for router in routers])
-        cur_router_ids = set()
-        for r in routers:
-            # If namespaces are disabled, only process the router associated
-            # with the configured agent id.
-            if (not self.conf.use_namespaces and
-                r['id'] != self.conf.router_id):
-                continue
-            ex_net_id = (r['external_gateway_info'] or {}).get('network_id')
-            if not ex_net_id and not self.conf.handle_internal_only_routers:
-                continue
-            if (target_ex_net_id and ex_net_id and
-                ex_net_id != target_ex_net_id):
-                # Double check that our single external_net_id has not changed
-                # by forcing a check by RPC.
-                if (ex_net_id != self._fetch_external_net_id(force=True)):
-                    continue
-            cur_router_ids.add(r['id'])
-            if r['id'] not in self.router_info:
-                self._router_added(r['id'], r)
-            ri = self.router_info[r['id']]
-            ri.router = r
-            pool.spawn_n(self.process_router, ri)
-        # identify and remove routers that no longer exist
-        for router_id in prev_router_ids - cur_router_ids:
-            pool.spawn_n(self._router_removed, router_id)
-        pool.waitall()
+        if (target_ex_net_id and ex_net_id and ex_net_id != target_ex_net_id):
+            # Double check that our single external_net_id has not changed
+            # by forcing a check by RPC.
+            if ex_net_id != self._fetch_external_net_id(force=True):
+                raise n_exc.RouterNotCompatibleWithAgent(
+                    router_id=router['id'])
+
+        if router['id'] not in self.router_info:
+            self._router_added(router['id'], router)
+        ri = self.router_info[router['id']]
+        ri.router = router
+        self.process_router(ri)
 
     def _process_router_update(self):
         for rp, update in self._queue.each_update_to_next_router():
@@ -1844,7 +1835,15 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback,
                 self._router_removed(update.id)
                 continue
 
-            self._process_routers([router])
+            try:
+                self._process_router_if_compatible(router)
+            except n_exc.RouterNotCompatibleWithAgent as e:
+                LOG.exception(e.msg)
+                # Was the router previously handled by this agent?
+                if router['id'] in self.router_info:
+                    LOG.error(_LE("Removing incompatible router '%s'"),
+                              router['id'])
+                    self._router_removed(router['id'])
             LOG.debug("Finished a router update for %s", update.id)
             rp.fetched_and_processed(update.timestamp)
 

diff --git a/neutron/common/exceptions.py b/neutron/common/exceptions.py
index be62388aa6e5d62488cd9089134dfc093f386572..1fd83e46ee3d50e2ea14ce71ebb786b9bdf72bed 100644 (file)
--- a/neutron/common/exceptions.py
+++ b/neutron/common/exceptions.py
@@ -335,3 +335,7 @@ class DeviceIDNotOwnedByTenant(Conflict):
 
 class InvalidCIDR(BadRequest):
     message = _("Invalid CIDR %(input)s given as IP prefix")
+
+
+class RouterNotCompatibleWithAgent(NeutronException):
+    message = _("Router '%(router_id)s' is not compatible with this agent")

diff --git a/neutron/services/vpn/agent.py b/neutron/services/vpn/agent.py
index 12e969df9b504d0cbd53215c67c474eecb329d92..6f67ccf1f007e09e1128cb3988d2220d6436155f 100644 (file)
--- a/neutron/services/vpn/agent.py
+++ b/neutron/services/vpn/agent.py
@@ -130,15 +130,15 @@ class VPNAgent(l3_agent.L3NATAgentWithStateReport):
         for device in self.devices:
             device.destroy_router(router_id)
 
-    def _process_routers(self, routers):
+    def _process_router_if_compatible(self, router):
         """Router sync event.
 
         This method overwrites parent class method.
-        :param routers: list of routers
+        :param router: a router
         """
-        super(VPNAgent, self)._process_routers(routers)
+        super(VPNAgent, self)._process_router_if_compatible(router)
         for device in self.devices:
-            device.sync(self.context, routers)
+            device.sync(self.context, [router])
 
 
 def main():

diff --git a/neutron/tests/unit/services/vpn/test_vpn_agent.py b/neutron/tests/unit/services/vpn/test_vpn_agent.py
index 4cde577d236fdb8a01ba81c356ce340b9c5eb19d..2a2a9a14f8dea0f87007f0333a3e75ab67f7f8e8 100644 (file)
--- a/neutron/tests/unit/services/vpn/test_vpn_agent.py
+++ b/neutron/tests/unit/services/vpn/test_vpn_agent.py
@@ -183,15 +183,14 @@ class TestVPNAgent(base.BaseTestCase):
         self.agent._router_removed(router_id)
         device.destroy_router.assert_called_once_with(router_id)
 
-    def test_process_routers(self):
+    def test_process_router_if_compatible(self):
         self.plugin_api.get_external_network_id.return_value = None
-        routers = [
-            {'id': _uuid(),
-             'admin_state_up': True,
-             'routes': [],
-             'external_gateway_info': {}}]
+        router = {'id': _uuid(),
+                  'admin_state_up': True,
+                  'routes': [],
+                  'external_gateway_info': {}}
 
         device = mock.Mock()
         self.agent.devices = [device]
-        self.agent._process_routers(routers)
-        device.sync.assert_called_once_with(mock.ANY, routers)
+        self.agent._process_router_if_compatible(router)
+        device.sync.assert_called_once_with(mock.ANY, [router])

diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py
index 8de7922d21027a0d3d78b8a265772674384c820d..264804a596e1d2d637358ac52ba1f7e46bbee92f 100644 (file)
--- a/neutron/tests/unit/test_l3_agent.py
+++ b/neutron/tests/unit/test_l3_agent.py
@@ -1870,108 +1870,94 @@ vrrp_instance VR_1 {
         self.assertEqual(['1234'], agent._router_ids())
         self.assertFalse(agent._clean_stale_namespaces)
 
-    def test_process_routers_with_no_ext_net_in_conf(self):
+    def test_process_router_if_compatible_with_no_ext_net_in_conf(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         self.plugin_api.get_external_network_id.return_value = 'aaa'
 
-        routers = [
-            {'id': _uuid(),
-             'routes': [],
-             'admin_state_up': True,
-             'external_gateway_info': {'network_id': 'aaa'}}]
+        router = {'id': _uuid(),
+                  'routes': [],
+                  'admin_state_up': True,
+                  'external_gateway_info': {'network_id': 'aaa'}}
 
-        agent._process_routers(routers)
-        self.assertIn(routers[0]['id'], agent.router_info)
+        agent._process_router_if_compatible(router)
+        self.assertIn(router['id'], agent.router_info)
         self.plugin_api.get_external_network_id.assert_called_with(
             agent.context)
 
-    def test_process_routers_with_cached_ext_net(self):
+    def test_process_router_if_compatible_with_cached_ext_net(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         self.plugin_api.get_external_network_id.return_value = 'aaa'
         agent.target_ex_net_id = 'aaa'
 
-        routers = [
-            {'id': _uuid(),
-             'routes': [],
-             'admin_state_up': True,
-             'external_gateway_info': {'network_id': 'aaa'}}]
+        router = {'id': _uuid(),
+                  'routes': [],
+                  'admin_state_up': True,
+                  'external_gateway_info': {'network_id': 'aaa'}}
 
-        agent._process_routers(routers)
-        self.assertIn(routers[0]['id'], agent.router_info)
+        agent._process_router_if_compatible(router)
+        self.assertIn(router['id'], agent.router_info)
         self.assertFalse(self.plugin_api.get_external_network_id.called)
 
-    def test_process_routers_with_stale_cached_ext_net(self):
+    def test_process_router_if_compatible_with_stale_cached_ext_net(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         self.plugin_api.get_external_network_id.return_value = 'aaa'
         agent.target_ex_net_id = 'bbb'
 
-        routers = [
-            {'id': _uuid(),
-             'routes': [],
-             'admin_state_up': True,
-             'external_gateway_info': {'network_id': 'aaa'}}]
+        router = {'id': _uuid(),
+                  'routes': [],
+                  'admin_state_up': True,
+                  'external_gateway_info': {'network_id': 'aaa'}}
 
-        agent._process_routers(routers)
-        self.assertIn(routers[0]['id'], agent.router_info)
+        agent._process_router_if_compatible(router)
+        self.assertIn(router['id'], agent.router_info)
         self.plugin_api.get_external_network_id.assert_called_with(
             agent.context)
 
-    def test_process_routers_with_no_ext_net_in_conf_and_two_net_plugin(self):
+    def test_process_router_if_compatible_w_no_ext_net_and_2_net_plugin(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
 
-        routers = [
-            {'id': _uuid(),
-             'routes': [],
-             'admin_state_up': True,
-             'external_gateway_info': {'network_id': 'aaa'}}]
+        router = {'id': _uuid(),
+                  'routes': [],
+                  'admin_state_up': True,
+                  'external_gateway_info': {'network_id': 'aaa'}}
 
         agent.router_info = {}
         self.plugin_api.get_external_network_id.side_effect = (
             n_exc.TooManyExternalNetworks())
         self.assertRaises(n_exc.TooManyExternalNetworks,
-                          agent._process_routers,
-                          routers)
-        self.assertNotIn(routers[0]['id'], agent.router_info)
+                          agent._process_router_if_compatible,
+                          router)
+        self.assertNotIn(router['id'], agent.router_info)
 
-    def test_process_routers_with_ext_net_in_conf(self):
+    def test_process_router_if_compatible_with_ext_net_in_conf(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         self.plugin_api.get_external_network_id.return_value = 'aaa'
 
-        routers = [
-            {'id': _uuid(),
-             'routes': [],
-             'admin_state_up': True,
-             'external_gateway_info': {'network_id': 'aaa'}},
-            {'id': _uuid(),
-             'routes': [],
-             'admin_state_up': True,
-             'external_gateway_info': {'network_id': 'bbb'}}]
+        router = {'id': _uuid(),
+                  'routes': [],
+                  'admin_state_up': True,
+                  'external_gateway_info': {'network_id': 'bbb'}}
 
         agent.router_info = {}
         self.conf.set_override('gateway_external_network_id', 'aaa')
-        agent._process_routers(routers)
-        self.assertIn(routers[0]['id'], agent.router_info)
-        self.assertNotIn(routers[1]['id'], agent.router_info)
+        self.assertRaises(n_exc.RouterNotCompatibleWithAgent,
+                          agent._process_router_if_compatible,
+                          router)
+        self.assertNotIn(router['id'], agent.router_info)
 
-    def test_process_routers_with_no_bridge_no_ext_net_in_conf(self):
+    def test_process_router_if_compatible_with_no_bridge_no_ext_net(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         self.plugin_api.get_external_network_id.return_value = 'aaa'
 
-        routers = [
-            {'id': _uuid(),
-             'routes': [],
-             'admin_state_up': True,
-             'external_gateway_info': {'network_id': 'aaa'}},
-            {'id': _uuid(),
-             'routes': [],
-             'admin_state_up': True,
-             'external_gateway_info': {'network_id': 'bbb'}}]
+        router = {'id': _uuid(),
+                  'routes': [],
+                  'admin_state_up': True,
+                  'external_gateway_info': {'network_id': 'aaa'}}
 
         agent.router_info = {}
         self.conf.set_override('external_network_bridge', '')
-        agent._process_routers(routers)
-        self.assertIn(routers[0]['id'], agent.router_info)
-        self.assertIn(routers[1]['id'], agent.router_info)
+        agent._process_router_if_compatible(router)
+        self.assertIn(router['id'], agent.router_info)
 
     def test_nonexistent_interface_driver(self):
         self.conf.set_override('interface_driver', None)