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
This commit is contained in:
Carl Baldwin
parent
97704f6863
commit
b0a238ea47
@ -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)
|
||||
|
||||
|
||||
@ -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")
|
||||
|
||||
@ -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():
|
||||
|
||||
@ -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])
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
Reference in New Issue
Block a user