From f98f239a15d68344f84ca755dd8a55698d528b1c Mon Sep 17 00:00:00 2001 From: Swaminathan Vasudevan Date: Mon, 4 Jun 2018 16:57:00 +0000 Subject: [PATCH] Revert "DVR: Fix allowed_address_pair IP, ARP table update by neutron agent" This reverts commit fbe308bdc12191c187343b5ef103dea9af738380. This does not help the ARP update for the unbound Allowed-address-pair IP, since the temporary ARP update (NUD: reachable) goes to incomplete state when the router tries to re-ARP for the IP, before it responds to a VM, since DVR routers does not allow the ARP requests to flow through the br-tun. Closes-bug: #1773999 Change-Id: I9977c8cbbbc1e68565249e7f80c59319fe967300 --- neutron/agent/l3/dvr.py | 4 +- neutron/agent/l3/dvr_local_router.py | 9 +-- neutron/db/l3_dvr_db.py | 12 ++-- neutron/privileged/agent/linux/ip_lib.py | 3 +- .../l3_router/test_l3_dvr_router_plugin.py | 12 ++-- .../unit/agent/l3/test_dvr_local_router.py | 67 +------------------ neutron/tests/unit/agent/linux/test_ip_lib.py | 17 ----- 7 files changed, 17 insertions(+), 107 deletions(-) diff --git a/neutron/agent/l3/dvr.py b/neutron/agent/l3/dvr.py index ba59d5f684d..44e840621e9 100644 --- a/neutron/agent/l3/dvr.py +++ b/neutron/agent/l3/dvr.py @@ -53,8 +53,8 @@ class AgentMixin(object): ip = arp_table['ip_address'] mac = arp_table['mac_address'] subnet_id = arp_table['subnet_id'] - nud_state = arp_table.get('nud_state') - ri._update_arp_entry(ip, mac, subnet_id, action, nud_state=nud_state) + + ri._update_arp_entry(ip, mac, subnet_id, action) def add_arp_entry(self, context, payload): """Add arp entry into router namespace. Called from RPC.""" diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index 031d886fb3b..f02890cb081 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -239,8 +239,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): arp_delete.add(arp_entry) self._pending_arp_set -= arp_delete - def _update_arp_entry( - self, ip, mac, subnet_id, operation, nud_state='permanent'): + 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 @@ -253,7 +252,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): device = ip_lib.IPDevice(interface_name, namespace=self.ns_name) if device.exists(): if operation == 'add': - device.neigh.add(ip, mac, nud_state=nud_state) + device.neigh.add(ip, mac) elif operation == 'delete': device.neigh.delete(ip, mac) return True @@ -280,14 +279,12 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): tuple(common_utils.get_dvr_allowed_address_pair_device_owners())) for p in subnet_ports: - nud_state = 'permanent' if p.get('device_owner') else 'reachable' if p['device_owner'] not in ignored_device_owners: for fixed_ip in p['fixed_ips']: self._update_arp_entry(fixed_ip['ip_address'], p['mac_address'], subnet_id, - 'add', - nud_state=nud_state) + 'add') self._process_arp_cache_for_internal_port(subnet_id) @staticmethod diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index e462ae00eb3..539618e7dcb 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -892,7 +892,7 @@ class _DVRAgentInterfaceMixin(object): return f_port def _generate_arp_table_and_notify_agent( - self, context, fixed_ip, mac_address, notifier, nud_state='permanent'): + self, context, fixed_ip, mac_address, notifier): """Generates the arp table entry and notifies the l3 agent.""" ip_address = fixed_ip['ip_address'] subnet = fixed_ip['subnet_id'] @@ -904,8 +904,7 @@ class _DVRAgentInterfaceMixin(object): return arp_table = {'ip_address': ip_address, 'mac_address': mac_address, - 'subnet_id': subnet, - 'nud_state': nud_state} + 'subnet_id': subnet} notifier(context, router_id, arp_table) def _get_subnet_id_for_given_fixed_ip( @@ -949,14 +948,11 @@ class _DVRAgentInterfaceMixin(object): return allowed_address_pair_fixed_ips = ( self._get_allowed_address_pair_fixed_ips(context, port_dict)) - for fixed_ip in fixed_ips: + changed_fixed_ips = fixed_ips + allowed_address_pair_fixed_ips + for fixed_ip in changed_fixed_ips: self._generate_arp_table_and_notify_agent( context, fixed_ip, port_dict['mac_address'], self.l3_rpc_notifier.add_arp_entry) - for fixed_ip in allowed_address_pair_fixed_ips: - self._generate_arp_table_and_notify_agent( - context, fixed_ip, port_dict['mac_address'], - self.l3_rpc_notifier.add_arp_entry, nud_state='reachable') def delete_arp_entry_for_dvr_service_port( self, context, port_dict, fixed_ips_to_delete=None): diff --git a/neutron/privileged/agent/linux/ip_lib.py b/neutron/privileged/agent/linux/ip_lib.py index 878dd3573d6..3da6ec7ee29 100644 --- a/neutron/privileged/agent/linux/ip_lib.py +++ b/neutron/privileged/agent/linux/ip_lib.py @@ -176,14 +176,13 @@ def add_neigh_entry(ip_version, ip_address, mac_address, device, namespace, :param namespace: The name of the namespace in which to add the entry """ family = _IP_VERSION_FAMILY_MAP[ip_version] - state = kwargs.get('nud_state', 'permanent') _run_iproute_neigh('replace', device, namespace, dst=ip_address, lladdr=mac_address, family=family, - state=ndmsg.states[state], + state=ndmsg.states['permanent'], **kwargs) diff --git a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py index 292f1344d11..29786ca1c48 100644 --- a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py +++ b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py @@ -1045,8 +1045,7 @@ class L3DvrTestCase(L3DvrTestCaseBase): vm_arp_table = { 'ip_address': vm_port_fixed_ips[0]['ip_address'], 'mac_address': vm_port_mac, - 'subnet_id': vm_port_subnet_id, - 'nud_state': 'permanent'} + 'subnet_id': vm_port_subnet_id} vm_port2 = self.core_plugin.update_port( self.context, int_port2['port']['id'], {'port': {portbindings.HOST_ID: HOST2}}) @@ -1098,8 +1097,7 @@ class L3DvrTestCase(L3DvrTestCaseBase): vrrp_arp_table1 = { 'ip_address': vrrp_port_fixed_ips[0]['ip_address'], 'mac_address': vm_port_mac, - 'subnet_id': vrrp_port_subnet_id, - 'nud_state': 'reachable'} + 'subnet_id': vrrp_port_subnet_id} expected_calls = [ mock.call(self.context, @@ -1214,8 +1212,7 @@ class L3DvrTestCase(L3DvrTestCaseBase): vm_arp_table = { 'ip_address': vm_port_fixed_ips[0]['ip_address'], 'mac_address': vm_port_mac, - 'subnet_id': vm_port_subnet_id, - 'nud_state': 'permanent'} + 'subnet_id': vm_port_subnet_id} self.assertEqual(1, l3_notifier.add_arp_entry.call_count) floating_ip = {'floating_network_id': ext_net['network']['id'], 'router_id': router['id'], @@ -1244,8 +1241,7 @@ class L3DvrTestCase(L3DvrTestCaseBase): vrrp_arp_table1 = { 'ip_address': vrrp_port_fixed_ips[0]['ip_address'], 'mac_address': vm_port_mac, - 'subnet_id': vrrp_port_subnet_id, - 'nud_state': 'reachable'} + 'subnet_id': vrrp_port_subnet_id} expected_calls = [ mock.call(self.context, diff --git a/neutron/tests/unit/agent/l3/test_dvr_local_router.py b/neutron/tests/unit/agent/l3/test_dvr_local_router.py index 6663c0a42f8..4a2df133186 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py @@ -521,49 +521,7 @@ class TestDvrRouterOperations(base.BaseTestCase): ri._set_subnet_arp_info(subnet_id) self.assertEqual(1, parp.call_count) self.mock_ip_dev.neigh.add.assert_called_once_with( - '1.2.3.4', '00:11:22:33:44:55', nud_state='permanent') - - # Test negative case - router['distributed'] = False - ri._set_subnet_arp_info(subnet_id) - self.mock_ip_dev.neigh.add.never_called() - - def test__set_subnet_arp_info_with_allowed_address_pair_port(self): - agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) - router = l3_test_common.prepare_router_data(num_internal_ports=2) - router['distributed'] = True - self._set_ri_kwargs(agent, router['id'], router) - ri = dvr_router.DvrLocalRouter(HOSTNAME, **self.ri_kwargs) - ports = ri.router.get(lib_constants.INTERFACE_KEY, []) - subnet_id = l3_test_common.get_subnet_id(ports[0]) - test_ports = [{'mac_address': '00:11:22:33:44:55', - 'device_owner': '', - 'fixed_ips': [{'ip_address': '1.2.3.4', - 'prefixlen': 24, - 'subnet_id': subnet_id}]}, - {'mac_address': '11:22:33:44:55:66', - 'device_owner': lib_constants.DEVICE_OWNER_LOADBALANCER, - 'fixed_ips': [{'ip_address': '1.2.3.5', - 'prefixlen': 24, - 'subnet_id': subnet_id}]}, - {'mac_address': '22:33:44:55:66:77', - 'device_owner': - lib_constants.DEVICE_OWNER_LOADBALANCERV2, - 'fixed_ips': [{'ip_address': '1.2.3.6', - 'prefixlen': 24, - 'subnet_id': subnet_id}]}] - - self.plugin_api.get_ports_by_subnet.return_value = test_ports - - # Test basic case - ports[0]['subnets'] = [{'id': subnet_id, - 'cidr': '1.2.3.0/24'}] - with mock.patch.object(ri, - '_process_arp_cache_for_internal_port') as parp: - ri._set_subnet_arp_info(subnet_id) - self.assertEqual(1, parp.call_count) - self.mock_ip_dev.neigh.add.assert_called_once_with( - '1.2.3.4', '00:11:22:33:44:55', nud_state='reachable') + '1.2.3.4', '00:11:22:33:44:55') # Test negative case router['distributed'] = False @@ -578,33 +536,14 @@ class TestDvrRouterOperations(base.BaseTestCase): router[lib_constants.INTERFACE_KEY][0]) arp_table = {'ip_address': '1.7.23.11', 'mac_address': '00:11:22:33:44:55', - 'subnet_id': subnet_id, - 'nud_state': 'permanent'} + 'subnet_id': subnet_id} payload = {'arp_table': arp_table, 'router_id': router['id']} agent._router_added(router['id'], router) agent.add_arp_entry(None, payload) agent.router_deleted(None, router['id']) self.mock_ip_dev.neigh.add.assert_called_once_with( - '1.7.23.11', '00:11:22:33:44:55', nud_state='permanent') - - def test_add_arp_entry_with_nud_state_reachable(self): - agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) - router = l3_test_common.prepare_router_data(num_internal_ports=2) - router['distributed'] = True - subnet_id = l3_test_common.get_subnet_id( - router[lib_constants.INTERFACE_KEY][0]) - arp_table = {'ip_address': '1.7.23.11', - 'mac_address': '00:11:22:33:44:55', - 'subnet_id': subnet_id, - 'nud_state': 'reachable'} - - payload = {'arp_table': arp_table, 'router_id': router['id']} - agent._router_added(router['id'], router) - agent.add_arp_entry(None, payload) - agent.router_deleted(None, router['id']) - self.mock_ip_dev.neigh.add.assert_called_once_with( - '1.7.23.11', '00:11:22:33:44:55', nud_state='reachable') + '1.7.23.11', '00:11:22:33:44:55') def test_add_arp_entry_no_routerinfo(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index 4827eec8c4c..4d9c6eeea94 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -1681,23 +1681,6 @@ class TestIpNeighCommand(TestIPCmdBase): ifindex=1, state=ndmsg.states['permanent']) - @mock.patch.object(pyroute2, 'NetNS') - def test_add_entry_with_state_override(self, mock_netns): - mock_netns_instance = mock_netns.return_value - mock_netns_enter = mock_netns_instance.__enter__.return_value - mock_netns_enter.link_lookup.return_value = [1] - self.neigh_cmd.add( - '192.168.45.100', 'cc:dd:ee:ff:ab:cd', nud_state='reachable') - mock_netns_enter.link_lookup.assert_called_once_with(ifname='tap0') - mock_netns_enter.neigh.assert_called_once_with( - 'replace', - dst='192.168.45.100', - lladdr='cc:dd:ee:ff:ab:cd', - family=2, - ifindex=1, - state=ndmsg.states['reachable'], - nud_state='reachable') - @mock.patch.object(pyroute2, 'NetNS') def test_add_entry_nonexistent_namespace(self, mock_netns): mock_netns.side_effect = OSError(errno.ENOENT, None)