From d034532d376b6ca2309d20f59ccc060a762ff12c Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Thu, 16 Jun 2016 18:40:08 +0200 Subject: [PATCH] cache_utils: fixed cache misses for the new (oslo.cache) configuration When the new (oslo.cache) way of configuring the cache is used, cache is never hit, because self._cache.get() consistently raises exceptions: TypeError: 'sha1() argument 1 must be string or buffer, not tuple' It occurs because the key passed into the oslo.cache region does not conform to oslo.cache requirements. The library enforces the key to be compatible with sha1_mangle_key() function: http://git.openstack.org/cgit/openstack/oslo.cache/tree/oslo_cache/core.py?id=8b8a718507b30a4a2fd36e6c14d1071bd6cca878#n140 With this patch, we transform the key to a string, to conform to the requirements. The bug sneaked into the tree unnoticed because of two reasons: - there were no unit tests to validate the new way of cache configuration. - the 'legacy' code path was configuring the cache in a slightly different way, omitting some oslo.cache code. For the former, new unit tests were introduced that cover the cache on par with the legacy mode. For the latter, the legacy code path was modified to rely on the same configuration path as for the new way. Closes-Bug: #1593342 Change-Id: I2724aa21f66f0fb69147407bfcf3184585d7d5cd --- neutron/common/cache_utils.py | 17 ++++++++---- .../tests/unit/agent/metadata/test_agent.py | 26 ++++++++++++++++--- neutron/tests/unit/common/test_cache_utils.py | 6 ++--- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/neutron/common/cache_utils.py b/neutron/common/cache_utils.py index 97c46110b1a..0e7d6518a21 100644 --- a/neutron/common/cache_utils.py +++ b/neutron/common/cache_utils.py @@ -65,7 +65,6 @@ def _get_cache_region_for_legacy(url): backend = parsed.scheme if backend == 'memory': - backend = 'oslo_cache.dict' query = parsed.query # NOTE(fangzhen): The following NOTE and code is from legacy # oslo-incubator cache module. Previously reside in neutron at @@ -78,11 +77,17 @@ def _get_cache_region_for_legacy(url): if not query and '?' in parsed.path: query = parsed.path.split('?', 1)[-1] parameters = parse.parse_qs(query) - expiration_time = int(parameters.get('default_ttl', [0])[0]) - region = cache.create_region() - region.configure(backend, expiration_time=expiration_time) - return region + conf = cfg.ConfigOpts() + register_oslo_configs(conf) + cache_conf_dict = { + 'enabled': True, + 'backend': 'oslo_cache.dict', + 'expiration_time': int(parameters.get('default_ttl', [0])[0]), + } + for k, v in cache_conf_dict.items(): + conf.set_override(k, v, group='cache') + return _get_cache_region(conf) else: raise RuntimeError(_('Old style configuration can use only memory ' '(dict) backend')) @@ -108,6 +113,8 @@ class cache_method_results(object): key = (func_name,) + args if kwargs: key += utils.dict2tuple(kwargs) + # oslo.cache expects a string or a buffer + key = str(key) try: item = target_self._cache.get(key) except TypeError: diff --git a/neutron/tests/unit/agent/metadata/test_agent.py b/neutron/tests/unit/agent/metadata/test_agent.py index 7e872b9154f..d82d6599ade 100644 --- a/neutron/tests/unit/agent/metadata/test_agent.py +++ b/neutron/tests/unit/agent/metadata/test_agent.py @@ -51,6 +51,16 @@ class CacheConfFixture(ConfFixture): self.config(cache_url='memory://?default_ttl=5') +class NewCacheConfFixture(ConfFixture): + def setUp(self): + super(NewCacheConfFixture, self).setUp() + self.config( + group='cache', + enabled=True, + backend='oslo_cache.dict', + expiration_time=5) + + class TestMetadataProxyHandlerBase(base.BaseTestCase): fake_conf = cfg.CONF fake_conf_fixture = ConfFixture(fake_conf) @@ -96,9 +106,7 @@ class TestMetadataProxyHandlerRpc(TestMetadataProxyHandlerBase): self.assertEqual(expected, ports) -class TestMetadataProxyHandlerCache(TestMetadataProxyHandlerBase): - fake_conf = cfg.CONF - fake_conf_fixture = CacheConfFixture(fake_conf) +class _TestMetadataProxyHandlerCacheMixin(object): def test_call(self): req = mock.Mock() @@ -411,6 +419,18 @@ class TestMetadataProxyHandlerCache(TestMetadataProxyHandlerBase): ) +class TestMetadataProxyHandlerCache(TestMetadataProxyHandlerBase, + _TestMetadataProxyHandlerCacheMixin): + fake_conf = cfg.CONF + fake_conf_fixture = CacheConfFixture(fake_conf) + + +class TestMetadataProxyHandlerNewCache(TestMetadataProxyHandlerBase, + _TestMetadataProxyHandlerCacheMixin): + fake_conf = cfg.CONF + fake_conf_fixture = NewCacheConfFixture(fake_conf) + + class TestMetadataProxyHandlerNoCache(TestMetadataProxyHandlerCache): fake_conf = cfg.CONF fake_conf_fixture = ConfFixture(fake_conf) diff --git a/neutron/tests/unit/common/test_cache_utils.py b/neutron/tests/unit/common/test_cache_utils.py index d1e20dc8c88..4c4c334bf65 100644 --- a/neutron/tests/unit/common/test_cache_utils.py +++ b/neutron/tests/unit/common/test_cache_utils.py @@ -100,7 +100,7 @@ class TestCachingDecorator(base.BaseTestCase): self.decor._cache.get.return_value = self.not_cached retval = self.decor.func(*args, **kwargs) self.decor._cache.set.assert_called_once_with( - expected_key, self.decor.func_retval) + str(expected_key), self.decor.func_retval) self.assertEqual(self.decor.func_retval, retval) def test_cache_hit(self): @@ -110,7 +110,7 @@ class TestCachingDecorator(base.BaseTestCase): retval = self.decor.func(*args, **kwargs) self.assertFalse(self.decor._cache.set.called) self.assertEqual(self.decor._cache.get.return_value, retval) - self.decor._cache.get.assert_called_once_with(expected_key) + self.decor._cache.get.assert_called_once_with(str(expected_key)) def test_get_unhashable(self): expected_key = (self.func_name, [1], 2) @@ -118,7 +118,7 @@ class TestCachingDecorator(base.BaseTestCase): retval = self.decor.func([1], 2) self.assertFalse(self.decor._cache.set.called) self.assertEqual(self.decor.func_retval, retval) - self.decor._cache.get.assert_called_once_with(expected_key) + self.decor._cache.get.assert_called_once_with(str(expected_key)) def test_missing_cache(self): delattr(self.decor, '_cache')