From 72a5b5b61f532bba6de502feda36dfc7d36cefc7 Mon Sep 17 00:00:00 2001 From: Vasyl Saienko Date: Tue, 24 Mar 2020 16:25:21 +0000 Subject: [PATCH] Fix return correct cache when reusing port The patch fixes issue when the same port is requested for multiple instances and second one can't get metadata due to cached instance_id. Closes-Bug: 1868867 Change-Id: If6a5866e4406c9c6c30e989c79ffb4ee1a88cecf --- neutron/agent/metadata/agent.py | 32 ++++++++++++++----- neutron/common/cache_utils.py | 27 +++++++++------- .../tests/unit/agent/metadata/test_agent.py | 19 +++++++++-- neutron/tests/unit/common/test_cache_utils.py | 7 ++++ 4 files changed, 63 insertions(+), 22 deletions(-) diff --git a/neutron/agent/metadata/agent.py b/neutron/agent/metadata/agent.py index 456689bf521..e71b28bc275 100644 --- a/neutron/agent/metadata/agent.py +++ b/neutron/agent/metadata/agent.py @@ -87,7 +87,15 @@ class MetadataProxyHandler(object): instance_id, tenant_id = self._get_instance_and_tenant_id(req) if instance_id: - return self._proxy_request(instance_id, tenant_id, req) + res = self._proxy_request(instance_id, tenant_id, req) + if isinstance(res, webob.exc.HTTPNotFound): + LOG.info("The instance: %s is not present anymore, " + "skipping cache...", instance_id) + instance_id, tenant_id = self._get_instance_and_tenant_id( + req, skip_cache=True) + if instance_id: + return self._proxy_request(instance_id, tenant_id, req) + return res else: return webob.exc.HTTPNotFound() @@ -118,42 +126,49 @@ class MetadataProxyHandler(object): return filters @cache.cache_method_results - def _get_router_networks(self, router_id): + def _get_router_networks(self, router_id, skip_cache=False): """Find all networks connected to given router.""" internal_ports = self._get_ports_from_server(router_id=router_id) return tuple(p['network_id'] for p in internal_ports) @cache.cache_method_results - def _get_ports_for_remote_address(self, remote_address, networks): + def _get_ports_for_remote_address(self, remote_address, networks, + skip_cache=False): """Get list of ports that has given ip address and are part of given networks. :param networks: list of networks in which the ip address will be searched for + :param skip_cache: when have to skip getting entry from cache """ return self._get_ports_from_server(networks=networks, ip_address=remote_address) - def _get_ports(self, remote_address, network_id=None, router_id=None): + def _get_ports(self, remote_address, network_id=None, router_id=None, + skip_cache=False): """Search for all ports that contain passed ip address and belongs to given network. If no network is passed ports are searched on all networks connected to given router. Either one of network_id or router_id must be passed. + :param skip_cache: when have to skip getting entry from cache + """ if network_id: networks = (network_id,) elif router_id: - networks = self._get_router_networks(router_id) + networks = self._get_router_networks(router_id, + skip_cache=skip_cache) else: raise TypeError(_("Either one of parameter network_id or router_id" " must be passed to _get_ports method.")) - return self._get_ports_for_remote_address(remote_address, networks) + return self._get_ports_for_remote_address(remote_address, networks, + skip_cache=skip_cache) - def _get_instance_and_tenant_id(self, req): + def _get_instance_and_tenant_id(self, req, skip_cache=False): remote_address = req.headers.get('X-Forwarded-For') network_id = req.headers.get('X-Neutron-Network-ID') router_id = req.headers.get('X-Neutron-Router-ID') @@ -165,7 +180,8 @@ class MetadataProxyHandler(object): "dropping") return None, None - ports = self._get_ports(remote_address, network_id, router_id) + ports = self._get_ports(remote_address, network_id, router_id, + skip_cache=skip_cache) LOG.debug("Gotten ports for remote_address %(remote_address)s, " "network_id %(network_id)s, router_id %(router_id)s are: " "%(ports)s", diff --git a/neutron/common/cache_utils.py b/neutron/common/cache_utils.py index c22cf7a1349..f3da54a0dcb 100644 --- a/neutron/common/cache_utils.py +++ b/neutron/common/cache_utils.py @@ -65,7 +65,7 @@ class cache_method_results(object): self._first_call = True self._not_cached = cache.NO_VALUE - def _get_from_cache(self, target_self, *args, **kwargs): + def _get_from_cache(self, target_self, *args, skip_cache=False, **kwargs): target_self_cls_name = reflection.get_class_name(target_self, fully_qualified=False) func_name = "%(module)s.%(class)s.%(func_name)s" % { @@ -78,16 +78,21 @@ class cache_method_results(object): key += helpers.dict2tuple(kwargs) # oslo.cache expects a string or a buffer key = str(key) - try: - item = target_self._cache.get(key) - except TypeError: - LOG.debug("Method %(func_name)s cannot be cached due to " - "unhashable parameters: args: %(args)s, kwargs: " - "%(kwargs)s", - {'func_name': func_name, - 'args': args, - 'kwargs': kwargs}) - return self.func(target_self, *args, **kwargs) + + if not skip_cache: + try: + item = target_self._cache.get(key) + except TypeError: + LOG.debug("Method %(func_name)s cannot be cached due to " + "unhashable parameters: args: %(args)s, kwargs: " + "%(kwargs)s", + {'func_name': func_name, + 'args': args, + 'kwargs': kwargs}) + return self.func(target_self, *args, **kwargs) + else: + LOG.debug('Skipping getting result from cache for %s.', func_name) + item = self._not_cached if item is self._not_cached: item = self.func(target_self, *args, **kwargs) diff --git a/neutron/tests/unit/agent/metadata/test_agent.py b/neutron/tests/unit/agent/metadata/test_agent.py index 80855ce5d74..cfa9fa6f8e8 100644 --- a/neutron/tests/unit/agent/metadata/test_agent.py +++ b/neutron/tests/unit/agent/metadata/test_agent.py @@ -114,6 +114,16 @@ class _TestMetadataProxyHandlerCacheMixin(object): retval = self.handler(req) self.assertEqual('value', retval) + def test_call_skip_cache(self): + req = mock.Mock() + with mock.patch.object(self.handler, + '_get_instance_and_tenant_id') as get_ids: + get_ids.return_value = ('instance_id', 'tenant_id') + with mock.patch.object(self.handler, '_proxy_request') as proxy: + proxy.return_value = webob.exc.HTTPNotFound() + self.handler(req) + get_ids.assert_called_with(req, skip_cache=True) + def test_call_no_instance_match(self): req = mock.Mock() with mock.patch.object(self.handler, @@ -201,7 +211,8 @@ class _TestMetadataProxyHandlerCacheMixin(object): ports = self.handler._get_ports(remote_address, network_id, router_id) mock_get_ip_addr.assert_called_once_with(remote_address, - networks) + networks, + skip_cache=False) self.assertFalse(mock_get_router_networks.called) self.assertEqual(expected, ports) @@ -220,8 +231,10 @@ class _TestMetadataProxyHandlerCacheMixin(object): ) as mock_get_router_networks: ports = self.handler._get_ports(remote_address, router_id=router_id) - mock_get_router_networks.assert_called_once_with(router_id) - mock_get_ip_addr.assert_called_once_with(remote_address, networks) + mock_get_router_networks.assert_called_once_with( + router_id, skip_cache=False) + mock_get_ip_addr.assert_called_once_with( + remote_address, networks, skip_cache=False) self.assertEqual(expected, ports) def test_get_ports_no_id(self): diff --git a/neutron/tests/unit/common/test_cache_utils.py b/neutron/tests/unit/common/test_cache_utils.py index a508e5d21a9..350a71c9a22 100644 --- a/neutron/tests/unit/common/test_cache_utils.py +++ b/neutron/tests/unit/common/test_cache_utils.py @@ -117,3 +117,10 @@ class TestCachingDecorator(base.BaseTestCase): self.decor._cache = False retval = self.decor.func((1, 2)) self.assertEqual(self.decor.func_retval, retval) + + def test_skip_cache(self): + self.decor.func(1, 2, skip_cache=True) + expected_key = (self.func_name, 1, 2) + self.decor._cache.get.assert_not_called() + self.decor._cache.set.assert_called_once_with(str(expected_key), + self.decor.func_retval)