From 2267b030bfe5921f0cdde2facff284b4ceba3839 Mon Sep 17 00:00:00 2001 From: Tong Li Date: Wed, 13 Feb 2013 13:54:51 -0500 Subject: [PATCH] Swift MemcacheRing (set) interface is incompatible fixes This patch fixes the Swift MemcacheRing set and set_multi interface incompatible problem with python memcache. The fix added two extra named parameters to both set and set_multi method. When only time or timeout parameter is present, then one of the value will be used. When both time and timeout are present, the time parameter will be used. Named parameter min_compress_len is added for pure compatibility purposes. The current implementation ignores this parameter. To make swift memcached methods all consistent cross the board, method incr and decr have also been changed to include a new named parameter time. In future OpenStack releases, the named parameter timeout will be removed, keep the named parameter timeout around for now is to make sure that mismatched releases between client and server will still work. From now on, when a call is made to set, set_multi, decr, incr by using timeout parametner, a warning message will be logged to indicate the deprecation of the parameter. Fixes: bug #1095730 Change-Id: I07af784a54d7d79395fc3265e74145f92f38a893 --- swift/common/memcached.py | 64 +++++++++++++++---- swift/common/middleware/cname_lookup.py | 2 +- swift/common/middleware/formpost.py | 2 +- swift/common/middleware/staticweb.py | 2 +- swift/common/middleware/tempauth.py | 4 +- swift/common/middleware/tempurl.py | 2 +- swift/proxy/controllers/base.py | 6 +- swift/proxy/controllers/container.py | 2 +- test/unit/common/middleware/test_formpost.py | 2 +- test/unit/common/middleware/test_ratelimit.py | 8 +-- test/unit/common/middleware/test_staticweb.py | 4 +- test/unit/common/middleware/test_tempauth.py | 4 +- test/unit/common/middleware/test_tempurl.py | 4 +- test/unit/common/test_memcached.py | 17 ++++- test/unit/proxy/test_server.py | 4 +- 15 files changed, 89 insertions(+), 38 deletions(-) mode change 100644 => 100755 swift/common/memcached.py mode change 100644 => 100755 swift/common/middleware/cname_lookup.py mode change 100644 => 100755 swift/common/middleware/formpost.py mode change 100644 => 100755 swift/common/middleware/staticweb.py mode change 100644 => 100755 swift/common/middleware/tempauth.py mode change 100644 => 100755 swift/common/middleware/tempurl.py mode change 100644 => 100755 swift/proxy/controllers/base.py mode change 100644 => 100755 swift/proxy/controllers/container.py mode change 100644 => 100755 test/unit/common/middleware/test_formpost.py mode change 100644 => 100755 test/unit/common/middleware/test_ratelimit.py mode change 100644 => 100755 test/unit/common/middleware/test_staticweb.py mode change 100644 => 100755 test/unit/common/middleware/test_tempauth.py mode change 100644 => 100755 test/unit/common/middleware/test_tempurl.py mode change 100644 => 100755 test/unit/common/test_memcached.py diff --git a/swift/common/memcached.py b/swift/common/memcached.py old mode 100644 new mode 100755 index 123f5beb97..86ce8b6bc6 --- a/swift/common/memcached.py +++ b/swift/common/memcached.py @@ -142,7 +142,8 @@ class MemcacheRing(object): """ Returns a server connection to the pool """ self._client_cache[server].append((fp, sock)) - def set(self, key, value, serialize=True, timeout=0): + def set(self, key, value, serialize=True, timeout=0, time=0, + min_compress_len=0): """ Set a key/value pair in memcache @@ -151,10 +152,22 @@ class MemcacheRing(object): :param serialize: if True, value is serialized with JSON before sending to memcache, or with pickle if configured to use pickle instead of JSON (to avoid cache poisoning) - :param timeout: ttl in memcache + :param timeout: ttl in memcache, this parameter is now deprecated. It + will be removed in next release of OpenStack, + use time parameter instead in the future + :time: equivalent to timeout, this parameter is added to keep the + signature compatible with python-memcached interface. This + implementation will take this value and sign it to the + parameter timeout + :min_compress_len: minimum compress length, this parameter was added + to keep the signature compatible with + python-memcached interface. This implementation + ignores it. """ key = md5hash(key) - timeout = sanitize_timeout(timeout) + if timeout: + logging.warn("parameter timeout has been deprecated, use time") + timeout = sanitize_timeout(time or timeout) flags = 0 if serialize and self._allow_pickle: value = pickle.dumps(value, PICKLE_PROTOCOL) @@ -204,7 +217,7 @@ class MemcacheRing(object): except Exception, e: self._exception_occurred(server, e) - def incr(self, key, delta=1, timeout=0): + def incr(self, key, delta=1, time=0, timeout=0): """ Increments a key which has a numeric value by delta. If the key can't be found, it's added as delta or 0 if delta < 0. @@ -217,15 +230,21 @@ class MemcacheRing(object): :param key: key :param delta: amount to add to the value of key (or set as the value if the key is not found) will be cast to an int - :param timeout: ttl in memcache + :param time: the time to live. This parameter deprecates parameter + timeout. The addition of this parameter is to make the + interface consistent with set and set_multi methods + :param timeout: ttl in memcache, deprecated, will be removed in future + OpenStack releases :raises MemcacheConnectionError: """ + if timeout: + logging.warn("parameter timeout has been deprecated, use time") key = md5hash(key) command = 'incr' if delta < 0: command = 'decr' delta = str(abs(int(delta))) - timeout = sanitize_timeout(timeout) + timeout = sanitize_timeout(time or timeout) for (server, fp, sock) in self._get_conns(key): try: sock.sendall('%s %s %s\r\n' % (command, key, delta)) @@ -251,7 +270,7 @@ class MemcacheRing(object): self._exception_occurred(server, e) raise MemcacheConnectionError("No Memcached connections succeeded.") - def decr(self, key, delta=1, timeout=0): + def decr(self, key, delta=1, time=0, timeout=0): """ Decrements a key which has a numeric value by delta. Calls incr with -delta. @@ -260,10 +279,17 @@ class MemcacheRing(object): :param delta: amount to subtract to the value of key (or set the value to 0 if the key is not found) will be cast to an int - :param timeout: ttl in memcache + :param time: the time to live. This parameter depcates parameter + timeout. The addition of this parameter is to make the + interface consistent with set and set_multi methods + :param timeout: ttl in memcache, deprecated, will be removed in future + OpenStack releases :raises MemcacheConnectionError: """ - self.incr(key, delta=-delta, timeout=timeout) + if timeout: + logging.warn("parameter timeout has been deprecated, use time") + + self.incr(key, delta=-delta, time=(time or timeout)) def delete(self, key): """ @@ -280,7 +306,8 @@ class MemcacheRing(object): except Exception, e: self._exception_occurred(server, e) - def set_multi(self, mapping, server_key, serialize=True, timeout=0): + def set_multi(self, mapping, server_key, serialize=True, timeout=0, + time=0, min_compress_len=0): """ Sets multiple key/value pairs in memcache. @@ -290,10 +317,23 @@ class MemcacheRing(object): :param serialize: if True, value is serialized with JSON before sending to memcache, or with pickle if configured to use pickle instead of JSON (to avoid cache poisoning) - :param timeout: ttl for memcache + :param timeout: ttl for memcache. This parameter is now deprecated, it + will be removed in next release of OpenStack, use time + parameter instead in the future + :time: equalvent to timeout, this parameter is added to keep the + signature compatible with python-memcached interface. This + implementation will take this value and sign it to parameter + timeout + :min_compress_len: minimum compress length, this parameter was added + to keep the signature compatible with + python-memcached interface. This implementation + ignores it """ + if timeout: + logging.warn("parameter timeout has been deprecated, use time") + server_key = md5hash(server_key) - timeout = sanitize_timeout(timeout) + timeout = sanitize_timeout(time or timeout) msg = '' for key, value in mapping.iteritems(): key = md5hash(key) diff --git a/swift/common/middleware/cname_lookup.py b/swift/common/middleware/cname_lookup.py old mode 100644 new mode 100755 index 86f7d9e15e..e6d8717c88 --- a/swift/common/middleware/cname_lookup.py +++ b/swift/common/middleware/cname_lookup.py @@ -108,7 +108,7 @@ class CNAMELookupMiddleware(object): if self.memcache: memcache_key = ''.join(['cname-', given_domain]) self.memcache.set(memcache_key, found_domain, - timeout=ttl) + time=ttl) if found_domain is None or found_domain == a_domain: # no CNAME records or we're at the last lookup error = True diff --git a/swift/common/middleware/formpost.py b/swift/common/middleware/formpost.py old mode 100644 new mode 100755 index 92042052ec..9c4a9a0b36 --- a/swift/common/middleware/formpost.py +++ b/swift/common/middleware/formpost.py @@ -487,7 +487,7 @@ class FormPost(object): pass key = key[0] if key and memcache: - memcache.set('temp-url-key/%s' % account, key, timeout=60) + memcache.set('temp-url-key/%s' % account, key, time=60) return key diff --git a/swift/common/middleware/staticweb.py b/swift/common/middleware/staticweb.py old mode 100644 new mode 100755 index 9963395c0a..23945cfb6c --- a/swift/common/middleware/staticweb.py +++ b/swift/common/middleware/staticweb.py @@ -213,7 +213,7 @@ class _StaticWebContext(WSGIContext): memcache_client.set(memcache_key, (self._index, self._error, self._listings, self._listings_css), - timeout=self.cache_timeout) + time=self.cache_timeout) def _listing(self, env, start_response, prefix=None): """ diff --git a/swift/common/middleware/tempauth.py b/swift/common/middleware/tempauth.py old mode 100644 new mode 100755 index 12987564be..c21ce71381 --- a/swift/common/middleware/tempauth.py +++ b/swift/common/middleware/tempauth.py @@ -441,12 +441,12 @@ class TempAuth(object): # Save token memcache_token_key = '%s/token/%s' % (self.reseller_prefix, token) memcache_client.set(memcache_token_key, (expires, groups), - timeout=float(expires - time())) + time=float(expires - time())) # Record the token with the user info for future use. memcache_user_key = \ '%s/user/%s' % (self.reseller_prefix, account_user) memcache_client.set(memcache_user_key, token, - timeout=float(expires - time())) + time=float(expires - time())) resp = Response(request=req, headers={ 'x-auth-token': token, 'x-storage-token': token}) url = self.users[account_user]['url'].replace('$HOST', resp.host_url) diff --git a/swift/common/middleware/tempurl.py b/swift/common/middleware/tempurl.py old mode 100644 new mode 100755 index b5988b0d5d..2cda87756e --- a/swift/common/middleware/tempurl.py +++ b/swift/common/middleware/tempurl.py @@ -343,7 +343,7 @@ class TempURL(object): pass key = key[0] if key and memcache: - memcache.set('temp-url-key/%s' % account, key, timeout=60) + memcache.set('temp-url-key/%s' % account, key, time=60) return key def _get_hmac(self, env, expires, key, request_method=None): diff --git a/swift/proxy/controllers/base.py b/swift/proxy/controllers/base.py old mode 100644 new mode 100755 index 1d5dcc24c7..f4ed3f5af6 --- a/swift/proxy/controllers/base.py +++ b/swift/proxy/controllers/base.py @@ -401,7 +401,7 @@ class Controller(object): self.app.memcache.set(cache_key, {'status': result_code, 'container_count': container_count}, - timeout=cache_timeout) + time=cache_timeout) if result_code == HTTP_OK: return partition, nodes, container_count return None, None, None @@ -472,11 +472,11 @@ class Controller(object): if container_info['status'] == HTTP_OK: self.app.memcache.set( cache_key, container_info, - timeout=self.app.recheck_container_existence) + time=self.app.recheck_container_existence) elif container_info['status'] == HTTP_NOT_FOUND: self.app.memcache.set( cache_key, container_info, - timeout=self.app.recheck_container_existence * 0.1) + time=self.app.recheck_container_existence * 0.1) if container_info['status'] == HTTP_OK: container_info['partition'] = part container_info['nodes'] = nodes diff --git a/swift/proxy/controllers/container.py b/swift/proxy/controllers/container.py old mode 100644 new mode 100755 index 3369c4b2c6..90a8861d56 --- a/swift/proxy/controllers/container.py +++ b/swift/proxy/controllers/container.py @@ -79,7 +79,7 @@ class ContainerController(Controller): self.app.memcache.set( cache_key, headers_to_container_info(resp.headers, resp.status_int), - timeout=self.app.recheck_container_existence) + time=self.app.recheck_container_existence) if 'swift.authorize' in req.environ: req.acl = resp.headers.get('x-container-read') diff --git a/test/unit/common/middleware/test_formpost.py b/test/unit/common/middleware/test_formpost.py old mode 100644 new mode 100755 index c4067f5918..e16db7e291 --- a/test/unit/common/middleware/test_formpost.py +++ b/test/unit/common/middleware/test_formpost.py @@ -32,7 +32,7 @@ class FakeMemcache(object): def get(self, key): return self.store.get(key) - def set(self, key, value, timeout=0): + def set(self, key, value, time=0): self.store[key] = value return True diff --git a/test/unit/common/middleware/test_ratelimit.py b/test/unit/common/middleware/test_ratelimit.py old mode 100644 new mode 100755 index 82aef84541..2d504fee5c --- a/test/unit/common/middleware/test_ratelimit.py +++ b/test/unit/common/middleware/test_ratelimit.py @@ -36,11 +36,11 @@ class FakeMemcache(object): def get(self, key): return self.store.get(key) - def set(self, key, value, serialize=False, timeout=0): + def set(self, key, value, serialize=False, time=0): self.store[key] = value return True - def incr(self, key, delta=1, timeout=0): + def incr(self, key, delta=1, time=0): if self.error_on_incr: raise MemcacheConnectionError('Memcache restarting') if self.init_incr_return_neg: @@ -52,8 +52,8 @@ class FakeMemcache(object): self.store[key] = 0 return int(self.store[key]) - def decr(self, key, delta=1, timeout=0): - return self.incr(key, delta=-delta, timeout=timeout) + def decr(self, key, delta=1, time=0): + return self.incr(key, delta=-delta, time=time) @contextmanager def soft_lock(self, key, timeout=0, retries=5): diff --git a/test/unit/common/middleware/test_staticweb.py b/test/unit/common/middleware/test_staticweb.py old mode 100644 new mode 100755 index 00ad6773db..c573c802c7 --- a/test/unit/common/middleware/test_staticweb.py +++ b/test/unit/common/middleware/test_staticweb.py @@ -33,11 +33,11 @@ class FakeMemcache(object): def get(self, key): return self.store.get(key) - def set(self, key, value, timeout=0): + def set(self, key, value, time=0): self.store[key] = value return True - def incr(self, key, timeout=0): + def incr(self, key, time=0): self.store[key] = self.store.setdefault(key, 0) + 1 return self.store[key] diff --git a/test/unit/common/middleware/test_tempauth.py b/test/unit/common/middleware/test_tempauth.py old mode 100644 new mode 100755 index c2fc48f353..07b9dd5873 --- a/test/unit/common/middleware/test_tempauth.py +++ b/test/unit/common/middleware/test_tempauth.py @@ -29,11 +29,11 @@ class FakeMemcache(object): def get(self, key): return self.store.get(key) - def set(self, key, value, timeout=0): + def set(self, key, value, time=0): self.store[key] = value return True - def incr(self, key, timeout=0): + def incr(self, key, time=0): self.store[key] = self.store.setdefault(key, 0) + 1 return self.store[key] diff --git a/test/unit/common/middleware/test_tempurl.py b/test/unit/common/middleware/test_tempurl.py old mode 100644 new mode 100755 index 7031152970..944532c406 --- a/test/unit/common/middleware/test_tempurl.py +++ b/test/unit/common/middleware/test_tempurl.py @@ -31,11 +31,11 @@ class FakeMemcache(object): def get(self, key): return self.store.get(key) - def set(self, key, value, timeout=0): + def set(self, key, value, time=0): self.store[key] = value return True - def incr(self, key, timeout=0): + def incr(self, key, time=0): self.store[key] = self.store.setdefault(key, 0) + 1 return self.store[key] diff --git a/test/unit/common/test_memcached.py b/test/unit/common/test_memcached.py old mode 100644 new mode 100755 index 7272cdaee3..f5b04f9e88 --- a/test/unit/common/test_memcached.py +++ b/test/unit/common/test_memcached.py @@ -179,10 +179,15 @@ class TestMemcached(unittest.TestCase): self.assert_(float(mock.cache.values()[0][1]) == 0) memcache_client.set('some_key', [1, 2, 3], timeout=10) self.assertEquals(mock.cache.values()[0][1], '10') + memcache_client.set('some_key', [1, 2, 3], time=20) + self.assertEquals(mock.cache.values()[0][1], '20') + sixtydays = 60 * 24 * 60 * 60 esttimeout = time.time() + sixtydays memcache_client.set('some_key', [1, 2, 3], timeout=sixtydays) self.assert_(-1 <= float(mock.cache.values()[0][1]) - esttimeout <= 1) + memcache_client.set('some_key', [1, 2, 3], time=sixtydays) + self.assert_(-1 <= float(mock.cache.values()[0][1]) - esttimeout <= 1) def test_incr(self): memcache_client = memcached.MemcacheRing(['1.2.3.4:11211']) @@ -206,14 +211,14 @@ class TestMemcached(unittest.TestCase): memcache_client = memcached.MemcacheRing(['1.2.3.4:11211']) mock = MockMemcached() memcache_client._client_cache['1.2.3.4:11211'] = [(mock, mock)] * 2 - memcache_client.incr('some_key', delta=5, timeout=55) + memcache_client.incr('some_key', delta=5, time=55) self.assertEquals(memcache_client.get('some_key'), '5') self.assertEquals(mock.cache.values()[0][1], '55') memcache_client.delete('some_key') self.assertEquals(memcache_client.get('some_key'), None) fiftydays = 50 * 24 * 60 * 60 esttimeout = time.time() + fiftydays - memcache_client.incr('some_key', delta=5, timeout=fiftydays) + memcache_client.incr('some_key', delta=5, time=fiftydays) self.assertEquals(memcache_client.get('some_key'), '5') self.assert_(-1 <= float(mock.cache.values()[0][1]) - esttimeout <= 1) memcache_client.delete('some_key') @@ -221,7 +226,7 @@ class TestMemcached(unittest.TestCase): memcache_client.incr('some_key', delta=5) self.assertEquals(memcache_client.get('some_key'), '5') self.assertEquals(mock.cache.values()[0][1], '0') - memcache_client.incr('some_key', delta=5, timeout=55) + memcache_client.incr('some_key', delta=5, time=55) self.assertEquals(memcache_client.get('some_key'), '10') self.assertEquals(mock.cache.values()[0][1], '0') @@ -278,6 +283,12 @@ class TestMemcached(unittest.TestCase): timeout=10) self.assertEquals(mock.cache.values()[0][1], '10') self.assertEquals(mock.cache.values()[1][1], '10') + memcache_client.set_multi( + {'some_key1': [1, 2, 3], 'some_key2': [4, 5, 6]}, 'multi_key', + time=20) + self.assertEquals(mock.cache.values()[0][1], '20') + self.assertEquals(mock.cache.values()[1][1], '20') + fortydays = 50 * 24 * 60 * 60 esttimeout = time.time() + fortydays memcache_client.set_multi( diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 62e4f1d16a..c01b4cbb3c 100755 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -358,11 +358,11 @@ class FakeMemcache(object): def keys(self): return self.store.keys() - def set(self, key, value, timeout=0): + def set(self, key, value, time=0): self.store[key] = value return True - def incr(self, key, timeout=0): + def incr(self, key, time=0): self.store[key] = self.store.setdefault(key, 0) + 1 return self.store[key]