From: Carl Baldwin Date: Mon, 23 Feb 2015 20:20:24 +0000 (+0000) Subject: Refactor DVR _arp_entry methods X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=6c5e044ab7ee944cfacf4491b3bc5278d9c04b8f;p=openstack-build%2Fneutron-build.git Refactor DVR _arp_entry methods This should decouple the DVR stuff from the agent. This moves some methods to dvr_router to encapulate them better. It removes a reference to fullsync from DVR to better separate concerns. Change-Id: Ibf0cec4c44576f7c7196c9780a1b12de96a812b3 Partially-Implements: bp/restructure-l3-agent --- diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py index 5abcf88c2..b0651f1cc 100644 --- a/neutron/agent/l3/agent.py +++ b/neutron/agent/l3/agent.py @@ -289,7 +289,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, ri.radvd = ra.DaemonMonitor(router['id'], ri.ns_name, self.process_monitor, - self.get_internal_device_name) + ri.get_internal_device_name) self.event_observers.notify( adv_svc.AdvancedService.before_router_added, ri) @@ -373,7 +373,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, existing_devices = self._get_existing_devices(ri) current_internal_devs = set([n for n in existing_devices if n.startswith(INTERNAL_DEV_PREFIX)]) - current_port_devs = set([self.get_internal_device_name(id) for + current_port_devs = set([ri.get_internal_device_name(id) for id in current_port_ids]) stale_devs = current_internal_devs - current_port_devs for stale_dev in stale_devs: @@ -562,9 +562,6 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, else: return self.get_external_device_name(ex_gw_port['id']) - def get_internal_device_name(self, port_id): - return (INTERNAL_DEV_PREFIX + port_id)[:self.driver.DEV_NAME_LEN] - def get_external_device_name(self, port_id): return (EXTERNAL_DEV_PREFIX + port_id)[:self.driver.DEV_NAME_LEN] @@ -582,7 +579,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, snat_ports = self.get_snat_interfaces(ri) for p in ri.internal_ports: gateway = self._map_internal_interfaces(ri, p, snat_ports) - id_name = self.get_internal_device_name(p['id']) + id_name = ri.get_internal_device_name(p['id']) if gateway: self._snat_redirect_add(ri, gateway['fixed_ips'][0] ['ip_address'], p, id_name) @@ -593,9 +590,10 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, snat_ports) for port in snat_ports: for ip in port['fixed_ips']: - self._update_arp_entry(ri, ip['ip_address'], - port['mac_address'], - ip['subnet_id'], 'add') + ri._update_arp_entry(ip['ip_address'], + port['mac_address'], + ip['subnet_id'], + 'add') return # Compute a list of addresses this router is supposed to have. @@ -665,7 +663,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, self._get_external_device_interface_name(ri, ex_gw_port)) ri.process_floating_ip_addresses(to_fip_interface_name) for p in ri.internal_ports: - internal_interface = self.get_internal_device_name(p['id']) + internal_interface = ri.get_internal_device_name(p['id']) self._snat_redirect_remove(ri, p, internal_interface) if (self.conf.agent_mode == l3_constants.L3_AGENT_MODE_DVR_SNAT @@ -720,7 +718,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, internal_cidr = port['ip_cidr'] mac_address = port['mac_address'] - interface_name = self.get_internal_device_name(port_id) + interface_name = ri.get_internal_device_name(port_id) self._internal_network_added(ri.ns_name, network_id, port_id, internal_cidr, mac_address, @@ -755,7 +753,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, def internal_network_removed(self, ri, port): port_id = port['id'] - interface_name = self.get_internal_device_name(port_id) + interface_name = ri.get_internal_device_name(port_id) if ri.router['distributed'] and ri.ex_gw_port: # DVR handling code for SNAT self._snat_redirect_remove(ri, port, interface_name) @@ -881,7 +879,13 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, router = routers[0] if not router: - self._router_removed(update.id) + try: + self._router_removed(update.id) + except Exception: + # TODO(Carl) Stop this fullsync non-sense. Just retry this + # one router by sticking the update at the end of the queue + # at a lower priority. + self.fullsync = True continue try: diff --git a/neutron/agent/l3/dvr.py b/neutron/agent/l3/dvr.py index 20066e0b9..56460aa44 100644 --- a/neutron/agent/l3/dvr.py +++ b/neutron/agent/l3/dvr.py @@ -73,18 +73,10 @@ class AgentMixin(object): for p in subnet_ports: if p['device_owner'] not in l3_constants.ROUTER_INTERFACE_OWNERS: for fixed_ip in p['fixed_ips']: - self._update_arp_entry(ri, fixed_ip['ip_address'], - p['mac_address'], - subnet_id, 'add') - - def get_internal_port(self, ri, subnet_id): - """Return internal router port based on subnet_id.""" - router_ports = ri.router.get(l3_constants.INTERFACE_KEY, []) - for port in router_ports: - fips = port['fixed_ips'] - for f in fips: - if f['subnet_id'] == subnet_id: - return port + ri._update_arp_entry(fixed_ip['ip_address'], + p['mac_address'], + subnet_id, + 'add') def get_snat_int_device_name(self, port_id): return (SNAT_INT_DEV_PREFIX + @@ -183,43 +175,28 @@ class AgentMixin(object): except Exception: LOG.exception(_LE('DVR: removed snat failed')) - def _update_arp_entry(self, ri, ip, mac, subnet_id, operation): - """Add or delete arp entry into router namespace for the subnet.""" - port = self.get_internal_port(ri, subnet_id) - # update arp entry only if the subnet is attached to the router - if port: - ip_cidr = str(ip) + '/32' - try: - # TODO(mrsmith): optimize the calls below for bulk calls - net = netaddr.IPNetwork(ip_cidr) - interface_name = self.get_internal_device_name(port['id']) - device = ip_lib.IPDevice(interface_name, namespace=ri.ns_name) - if operation == 'add': - device.neigh.add(net.version, ip, mac) - elif operation == 'delete': - device.neigh.delete(net.version, ip, mac) - except Exception: - LOG.exception(_LE("DVR: Failed updating arp entry")) - self.fullsync = True - def add_arp_entry(self, context, payload): """Add arp entry into router namespace. Called from RPC.""" - arp_table = payload['arp_table'] router_id = payload['router_id'] + ri = self.router_info.get(router_id) + if not ri: + return + + arp_table = payload['arp_table'] ip = arp_table['ip_address'] mac = arp_table['mac_address'] subnet_id = arp_table['subnet_id'] - ri = self.router_info.get(router_id) - if ri: - self._update_arp_entry(ri, ip, mac, subnet_id, 'add') + ri._update_arp_entry(ip, mac, subnet_id, 'add') def del_arp_entry(self, context, payload): """Delete arp entry from router namespace. Called from RPC.""" - arp_table = payload['arp_table'] router_id = payload['router_id'] + ri = self.router_info.get(router_id) + if not ri: + return + + arp_table = payload['arp_table'] ip = arp_table['ip_address'] mac = arp_table['mac_address'] subnet_id = arp_table['subnet_id'] - ri = self.router_info.get(router_id) - if ri: - self._update_arp_entry(ri, ip, mac, subnet_id, 'delete') + ri._update_arp_entry(ip, mac, subnet_id, 'delete') diff --git a/neutron/agent/l3/dvr_router.py b/neutron/agent/l3/dvr_router.py index dfb30e704..82371a8a5 100644 --- a/neutron/agent/l3/dvr_router.py +++ b/neutron/agent/l3/dvr_router.py @@ -12,12 +12,19 @@ # License for the specific language governing permissions and limitations # under the License. +import netaddr +from oslo_utils import excutils + from neutron.agent.l3 import dvr_fip_ns from neutron.agent.l3 import dvr_snat_ns from neutron.agent.l3 import router_info as router from neutron.agent.linux import ip_lib from neutron.common import constants as l3_constants from neutron.common import utils as common_utils +from neutron.i18n import _LE +from neutron.openstack.common import log as logging + +LOG = logging.getLogger(__name__) class DvrRouter(router.RouterInfo): @@ -157,3 +164,33 @@ class DvrRouter(router.RouterInfo): # first step is to move the deletion of the snat namespace here self.snat_namespace.delete() self.snat_namespace = None + + def _get_internal_port(self, subnet_id): + """Return internal router port based on subnet_id.""" + router_ports = self.router.get(l3_constants.INTERFACE_KEY, []) + for port in router_ports: + fips = port['fixed_ips'] + for f in fips: + if f['subnet_id'] == subnet_id: + return port + + def _update_arp_entry(self, ip, mac, subnet_id, operation): + """Add or delete arp entry into router namespace for the subnet.""" + port = self._get_internal_port(subnet_id) + # update arp entry only if the subnet is attached to the router + if not port: + return + + ip_cidr = str(ip) + '/32' + try: + # TODO(mrsmith): optimize the calls below for bulk calls + net = netaddr.IPNetwork(ip_cidr) + interface_name = self.get_internal_device_name(port['id']) + device = ip_lib.IPDevice(interface_name, namespace=self.ns_name) + if operation == 'add': + device.neigh.add(net.version, ip, mac) + elif operation == 'delete': + device.neigh.delete(net.version, ip, mac) + except Exception: + with excutils.save_and_reraise_exception(): + LOG.exception(_LE("DVR: Failed updating arp entry")) diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index 51371686c..9e0034ff9 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -24,6 +24,7 @@ from neutron.i18n import _LW from neutron.openstack.common import log as logging LOG = logging.getLogger(__name__) +INTERNAL_DEV_PREFIX = 'qr-' class RouterInfo(object): @@ -83,6 +84,9 @@ class RouterInfo(object): # TODO(Carl) Refactoring should render this obsolete. Remove it. return False + def get_internal_device_name(self, port_id): + return (INTERNAL_DEV_PREFIX + port_id)[:self.driver.DEV_NAME_LEN] + def perform_snat_action(self, snat_callback, *args): # Process SNAT rules for attached subnets if self._snat_action: diff --git a/neutron/tests/functional/agent/test_l3_agent.py b/neutron/tests/functional/agent/test_l3_agent.py index c98f3b246..940091664 100755 --- a/neutron/tests/functional/agent/test_l3_agent.py +++ b/neutron/tests/functional/agent/test_l3_agent.py @@ -162,7 +162,7 @@ class L3AgentTestFramework(base.BaseOVSLinuxTestCase): internal_port = router.router[l3_constants.INTERFACE_KEY][0] int_port_ipv6 = router._get_ipv6_lladdr( internal_port['mac_address']) - internal_device_name = self.agent.get_internal_device_name( + internal_device_name = router.get_internal_device_name( internal_port['id']) internal_device_cidr = internal_port['ip_cidr'] floating_ip_cidr = common_utils.ip_to_cidr( @@ -250,7 +250,7 @@ class L3AgentTestFramework(base.BaseOVSLinuxTestCase): self.assertTrue(len(internal_devices)) for device in internal_devices: self.assertTrue(self.device_exists_with_ip_mac( - device, self.agent.get_internal_device_name, router.ns_name)) + device, router.get_internal_device_name, router.ns_name)) def _assert_extra_routes(self, router): routes = ip_lib.get_routing_table(namespace=router.ns_name) @@ -406,7 +406,7 @@ class L3AgentTestCase(L3AgentTestFramework): device_exists = functools.partial( self.device_exists_with_ip_mac, device, - self.agent.get_internal_device_name, + router.get_internal_device_name, router.ns_name) utils.wait_until_true(device_exists) diff --git a/neutron/tests/unit/agent/l3/test_dvr_router.py b/neutron/tests/unit/agent/l3/test_dvr_router.py index 835e8bc5a..24009d2aa 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_router.py @@ -33,9 +33,11 @@ class TestDvrRouterOperations(base.BaseTestCase): def setUp(self): super(TestDvrRouterOperations, self).setUp() - def _create_router(self, router, **kwargs): + def _create_router(self, router=None, **kwargs): agent_conf = mock.Mock() self.router_id = _uuid() + if not router: + router = mock.MagicMock() return dvr_router.DvrRouter(mock.sentinel.myhost, self.router_id, router, @@ -167,3 +169,17 @@ class TestDvrRouterOperations(base.BaseTestCase): mock.sentinel.device, mock.sentinel.ip_cidr) ri.floating_ip_removed_dist.assert_called_once_with( mock.sentinel.ip_cidr) + + def test__get_internal_port(self): + ri = self._create_router() + port = {'fixed_ips': [{'subnet_id': mock.sentinel.subnet_id}]} + router_ports = [port] + ri.router.get.return_value = router_ports + self.assertEqual(port, ri._get_internal_port(mock.sentinel.subnet_id)) + + def test__get_internal_port_not_found(self): + ri = self._create_router() + port = {'fixed_ips': [{'subnet_id': mock.sentinel.subnet_id}]} + router_ports = [port] + ri.router.get.return_value = router_ports + self.assertEqual(None, ri._get_internal_port(mock.sentinel.subnet_id2)) diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py index c6dea6f62..77bf19002 100644 --- a/neutron/tests/unit/test_l3_agent.py +++ b/neutron/tests/unit/test_l3_agent.py @@ -275,7 +275,7 @@ class BasicRouterOperationsFramework(base.BaseTestCase): 'id': _uuid(), 'device_id': _uuid()}] self.ri_kwargs = {'agent_conf': self.conf, - 'interface_driver': mock.sentinel.interface_driver} + 'interface_driver': self.mock_driver} def _prepare_internal_network_data(self): port_id = _uuid() @@ -300,7 +300,7 @@ class BasicRouterOperationsFramework(base.BaseTestCase): ri.radvd = ra.DaemonMonitor(router['id'], ri.ns_name, agent.process_monitor, - agent.get_internal_device_name) + ri.get_internal_device_name) agent.process_router(ri) @@ -355,7 +355,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): def _test_internal_network_action(self, action): agent, ri, port = self._prepare_internal_network_data() - interface_name = agent.get_internal_device_name(port['id']) + interface_name = ri.get_internal_device_name(port['id']) if action == 'add': self.device_exists.return_value = False @@ -643,40 +643,12 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): self.assertNotEqual(test_port, res_ip) self.assertIsNone(res_ip) - def test_get_internal_port(self): - agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) - router = prepare_router_data(num_internal_ports=4) - subnet_ids = [_get_subnet_id(port) for port in - router[l3_constants.INTERFACE_KEY]] - ri = l3router.RouterInfo(router['id'], router, **self.ri_kwargs) - - # Test Basic cases - port = agent.get_internal_port(ri, subnet_ids[0]) - fips = port.get('fixed_ips', []) - subnet_id = fips[0]['subnet_id'] - self.assertEqual(subnet_ids[0], subnet_id) - port = agent.get_internal_port(ri, subnet_ids[1]) - fips = port.get('fixed_ips', []) - subnet_id = fips[0]['subnet_id'] - self.assertEqual(subnet_ids[1], subnet_id) - port = agent.get_internal_port(ri, subnet_ids[3]) - fips = port.get('fixed_ips', []) - subnet_id = fips[0]['subnet_id'] - self.assertEqual(subnet_ids[3], subnet_id) - - # Test miss cases - no_port = agent.get_internal_port(ri, FAKE_ID) - self.assertIsNone(no_port) - port = agent.get_internal_port(ri, subnet_ids[0]) - fips = port.get('fixed_ips', []) - subnet_id = fips[0]['subnet_id'] - self.assertNotEqual(subnet_ids[3], subnet_id) - def test__set_subnet_arp_info(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) router = prepare_router_data(num_internal_ports=2) router['distributed'] = True - ri = l3router.RouterInfo(router['id'], router, **self.ri_kwargs) + ri = dvr_router.DvrRouter( + HOSTNAME, router['id'], router, **self.ri_kwargs) ports = ri.router.get(l3_constants.INTERFACE_KEY, []) test_ports = [{'mac_address': '00:11:22:33:44:55', 'device_owner': 'network:dhcp', @@ -699,6 +671,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): def test_add_arp_entry(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) router = prepare_router_data(num_internal_ports=2) + router['distributed'] = True subnet_id = _get_subnet_id(router[l3_constants.INTERFACE_KEY][0]) arp_table = {'ip_address': '1.7.23.11', 'mac_address': '00:11:22:33:44:55', @@ -720,24 +693,22 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): 'subnet_id': subnet_id} payload = {'arp_table': arp_table, 'router_id': router['id']} - agent._update_arp_entry = mock.Mock() agent.add_arp_entry(None, payload) - self.assertFalse(agent._update_arp_entry.called) def test__update_arp_entry_with_no_subnet(self): - agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) - ri = l3router.RouterInfo( + ri = dvr_router.DvrRouter( + HOSTNAME, 'foo_router_id', {'distributed': True, 'gw_port_host': HOSTNAME}, **self.ri_kwargs) with mock.patch.object(l3_agent.ip_lib, 'IPDevice') as f: - agent._update_arp_entry(ri, mock.ANY, mock.ANY, - 'foo_subnet_id', 'add') + ri._update_arp_entry(mock.ANY, mock.ANY, 'foo_subnet_id', 'add') self.assertFalse(f.call_count) def test_del_arp_entry(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) router = prepare_router_data(num_internal_ports=2) + router['distributed'] = True subnet_id = _get_subnet_id(router[l3_constants.INTERFACE_KEY][0]) arp_table = {'ip_address': '1.5.25.15', 'mac_address': '00:44:33:22:11:55',