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 @@ def _return_conn(self, server, fp, sock): """ 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 @@ def set(self, key, value, serialize=True, timeout=0): :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 @@ def get(self, key): 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 @@ def incr(self, key, delta=1, timeout=0): :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 @@ def incr(self, key, delta=1, timeout=0): 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 @@ def decr(self, key, delta=1, timeout=0): :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 @@ def delete(self, key): 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 @@ def set_multi(self, mapping, server_key, serialize=True, timeout=0): :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 @@ def __call__(self, env, start_response): 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 @@ def _start_response(status, response_headers, exc_info=None): 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 @@ def _get_container_info(self, env): 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 @@ def handle_get_token(self, req): # 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 @@ def _start_response(status, response_headers, exc_info=None): 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 @@ def account_info(self, account, autocreate=False): 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 @@ def container_info(self, account, container, account_autocreate=False): 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 @@ def GETorHEAD(self, req): 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 @@ def __init__(self): 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 @@ def __init__(self): 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 @@ def incr(self, key, delta=1, timeout=0): 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 @@ def __init__(self): 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 @@ def __init__(self): 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 @@ def __init__(self): 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 @@ def test_set_get(self): 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 @@ def test_incr_w_timeout(self): 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 @@ def test_incr_w_timeout(self): 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 @@ def test_multi(self): 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 @@ def get(self, key): 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]