From fbe308bdc12191c187343b5ef103dea9af738380 Mon Sep 17 00:00:00 2001 From: Swaminathan Vasudevan Date: Wed, 7 Mar 2018 19:03:42 -0800 Subject: [PATCH] DVR: Fix allowed_address_pair IP, ARP table update by neutron agent Allowed_address_pair IP when associated with a network port will inherit the services MAC. Right now the ARP entry is updated with the last MAC that it is associated with. But when allowed_address_pair IPs are used in the context of VRRP the MAC keeps switching between the MASTER and SLAVE. VRRP instance sends out GARP, but the ARP entry in the router namespace is not getting updated based on the GARP. This might cause the VRRP IP and the service using the IP to fail. Since we having been adding the ARP entry with NUD state as PERMANENT, the ARP entries are set for ever and does not adopt the GARP sent out by the VRRP instance. This will cause instances associated with DVR routers to have a service interruption. So the proposed patch will add the ARP entry for the Allowed address pair with NUD for 'REACHABLE'. This allows the Allowed_address_pair IP MAC to be updated on the fly. Change-Id: I43c3471f5d259e8c2ee1685398a06a4680c0bfcd Closes-Bug: #1608400 --- 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, 107 insertions(+), 17 deletions(-) diff --git a/neutron/agent/l3/dvr.py b/neutron/agent/l3/dvr.py index 44e840621e9..ba59d5f684d 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'] - - ri._update_arp_entry(ip, mac, subnet_id, action) + nud_state = arp_table.get('nud_state') + ri._update_arp_entry(ip, mac, subnet_id, action, nud_state=nud_state) 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 f02890cb081..031d886fb3b 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -239,7 +239,8 @@ 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): + def _update_arp_entry( + self, ip, mac, subnet_id, operation, nud_state='permanent'): """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 @@ -252,7 +253,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) + device.neigh.add(ip, mac, nud_state=nud_state) elif operation == 'delete': device.neigh.delete(ip, mac) return True @@ -279,12 +280,14 @@ 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') + 'add', + nud_state=nud_state) 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 539618e7dcb..e462ae00eb3 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): + self, context, fixed_ip, mac_address, notifier, nud_state='permanent'): """Generates the arp table entry and notifies the l3 agent.""" ip_address = fixed_ip['ip_address'] subnet = fixed_ip['subnet_id'] @@ -904,7 +904,8 @@ class _DVRAgentInterfaceMixin(object): return arp_table = {'ip_address': ip_address, 'mac_address': mac_address, - 'subnet_id': subnet} + 'subnet_id': subnet, + 'nud_state': nud_state} notifier(context, router_id, arp_table) def _get_subnet_id_for_given_fixed_ip( @@ -948,11 +949,14 @@ class _DVRAgentInterfaceMixin(object): return allowed_address_pair_fixed_ips = ( self._get_allowed_address_pair_fixed_ips(context, port_dict)) - changed_fixed_ips = fixed_ips + allowed_address_pair_fixed_ips - for fixed_ip in changed_fixed_ips: + for fixed_ip in 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 3da6ec7ee29..878dd3573d6 100644 --- a/neutron/privileged/agent/linux/ip_lib.py +++ b/neutron/privileged/agent/linux/ip_lib.py @@ -176,13 +176,14 @@ 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['permanent'], + state=ndmsg.states[state], **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 29786ca1c48..292f1344d11 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,7 +1045,8 @@ 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} + 'subnet_id': vm_port_subnet_id, + 'nud_state': 'permanent'} vm_port2 = self.core_plugin.update_port( self.context, int_port2['port']['id'], {'port': {portbindings.HOST_ID: HOST2}}) @@ -1097,7 +1098,8 @@ 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} + 'subnet_id': vrrp_port_subnet_id, + 'nud_state': 'reachable'} expected_calls = [ mock.call(self.context, @@ -1212,7 +1214,8 @@ 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} + 'subnet_id': vm_port_subnet_id, + 'nud_state': 'permanent'} self.assertEqual(1, l3_notifier.add_arp_entry.call_count) floating_ip = {'floating_network_id': ext_net['network']['id'], 'router_id': router['id'], @@ -1241,7 +1244,8 @@ 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} + 'subnet_id': vrrp_port_subnet_id, + 'nud_state': 'reachable'} 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 4a2df133186..6663c0a42f8 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py @@ -521,7 +521,49 @@ 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') + '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') # Test negative case router['distributed'] = False @@ -536,14 +578,33 @@ 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} + 'subnet_id': subnet_id, + 'nud_state': 'permanent'} 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') + '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') 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 4d9c6eeea94..4827eec8c4c 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -1681,6 +1681,23 @@ 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)