From 48749c2788396a1dadd57232d18abb7bd131b826 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Mon, 28 Jan 2019 23:53:04 +0100 Subject: [PATCH] Fix update of ports cache in router_info class RouterInfo class has got internal_ports cache which is updated in _process_internal_ports() method. There was an issue in this updates logic because it was iterating through enumerate local variable "internal_ports" which represents current router ports and if such current port was found in updated_ports list it was storred in RouterInfo().internal_ports variable under same index as was found in "internal_ports" local variable. This sometimes leads to an issue because same port can be stored under different index in internal_ports and RouterInfo().internal_ports lists thus wrong port in RouterInfo().internal_ports was overwritten. Such issue leads to problem with generating radvd config file because in ports cache list there was duplicate info about same port so radvd config file contained duplicate interface definitions too. This should be properly fixed by changing RouterInfo.internal_ports to be a dict instead of list of ports but such patch would be much bigger and (possibly) harded to backport to stable branches. Change-Id: I2e38457942518c8a3e07e606091bb6720317b77e Closes-Bug: #1813279 (cherry picked from commit 21cddc47b446fdfb5535b347c1d7825a04e02c62) --- neutron/agent/l3/router_info.py | 41 ++++++++++++------- .../tests/unit/agent/l3/test_router_info.py | 22 ++++++++++ 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index d0438b55db1..dd7fcade88b 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -484,9 +484,23 @@ class RouterInfo(object): ip_devs = ip_wrapper.get_devices() return [ip_dev.name for ip_dev in ip_devs] + def _update_internal_ports_cache(self, port): + # NOTE(slaweq): self.internal_ports is a list of port objects but + # when it is updated in _process_internal_ports() method, + # but it can be based only on indexes of elements in + # self.internal_ports as index of element to updated is unknown. + # It has to be done based on port_id and this method is doing exactly + # that. + for index, p in enumerate(self.internal_ports): + if p['id'] == port['id']: + self.internal_ports[index] = port + break + else: + self.internal_ports.append(port) + @staticmethod def _get_updated_ports(existing_ports, current_ports): - updated_ports = dict() + updated_ports = [] current_ports_dict = {p['id']: p for p in current_ports} for existing_port in existing_ports: current_port = current_ports_dict.get(existing_port['id']) @@ -498,7 +512,7 @@ class RouterInfo(object): key=helpers.safe_sort_key)) mtu_changed = existing_port['mtu'] != current_port['mtu'] if fixed_ips_changed or mtu_changed: - updated_ports[current_port['id']] = current_port + updated_ports.append(current_port) return updated_ports @staticmethod @@ -554,7 +568,7 @@ class RouterInfo(object): for p in new_ports: self.internal_network_added(p) LOG.debug("appending port %s to internal_ports cache", p) - self.internal_ports.append(p) + self._update_internal_ports_cache(p) enable_ra = enable_ra or self._port_has_ipv6_subnet(p) for subnet in p['subnets']: if ipv6_utils.is_ipv6_pd_enabled(subnet): @@ -577,18 +591,15 @@ class RouterInfo(object): del self.pd_subnets[subnet['id']] updated_cidrs = [] - if updated_ports: - for index, p in enumerate(internal_ports): - if not updated_ports.get(p['id']): - continue - self.internal_ports[index] = updated_ports[p['id']] - interface_name = self.get_internal_device_name(p['id']) - ip_cidrs = common_utils.fixed_ip_cidrs(p['fixed_ips']) - LOG.debug("updating internal network for port %s", p) - updated_cidrs += ip_cidrs - self.internal_network_updated( - interface_name, ip_cidrs, p['mtu']) - enable_ra = enable_ra or self._port_has_ipv6_subnet(p) + for p in updated_ports: + self._update_internal_ports_cache(p) + interface_name = self.get_internal_device_name(p['id']) + ip_cidrs = common_utils.fixed_ip_cidrs(p['fixed_ips']) + LOG.debug("updating internal network for port %s", p) + updated_cidrs += ip_cidrs + self.internal_network_updated( + interface_name, ip_cidrs, p['mtu']) + enable_ra = enable_ra or self._port_has_ipv6_subnet(p) # Check if there is any pd prefix update for p in internal_ports: diff --git a/neutron/tests/unit/agent/l3/test_router_info.py b/neutron/tests/unit/agent/l3/test_router_info.py index 364c114251d..52374cadfe7 100644 --- a/neutron/tests/unit/agent/l3/test_router_info.py +++ b/neutron/tests/unit/agent/l3/test_router_info.py @@ -209,6 +209,28 @@ class TestRouterInfo(base.BaseTestCase): p_i_p.assert_called_once_with() p_e_o_d.assert_called_once_with() + def test__update_internal_ports_cache(self): + ri = router_info.RouterInfo(mock.Mock(), _uuid(), {}, **self.ri_kwargs) + ri.internal_ports = [ + {'id': 'port-id-1', 'mtu': 1500}, + {'id': 'port-id-2', 'mtu': 2000}] + initial_internal_ports = ri.internal_ports[:] + + # Test add new element to the cache + new_port = {'id': 'new-port-id', 'mtu': 1500} + ri._update_internal_ports_cache(new_port) + self.assertEqual( + initial_internal_ports + [new_port], + ri.internal_ports) + + # Test update existing port in cache + updated_port = new_port.copy() + updated_port['mtu'] = 2500 + ri._update_internal_ports_cache(updated_port) + self.assertEqual( + initial_internal_ports + [updated_port], + ri.internal_ports) + class BasicRouterTestCaseFramework(base.BaseTestCase): def _create_router(self, router=None, **kwargs):