From a0b45359fa34fc495813650f971371a9c1acb72b Mon Sep 17 00:00:00 2001 From: Grokzen Date: Thu, 13 Oct 2016 21:54:44 +0200 Subject: [PATCH 1/6] Fix the broken method from_url that did not setup and connect to the cluster in a correct way. Method is now compliant with known use-cases from redis-py way of using that method. --- docs/release-notes.rst | 7 +++++++ ptp-debug.py | 1 + rediscluster/client.py | 24 ++++++++++++++++++++++++ rediscluster/connection.py | 12 ++++++++++++ 4 files changed, 44 insertions(+) diff --git a/docs/release-notes.rst b/docs/release-notes.rst index be4108ae..a68be2e5 100644 --- a/docs/release-notes.rst +++ b/docs/release-notes.rst @@ -1,6 +1,13 @@ Release Notes ============= +Next release (??? ??, 201?) +--------------------------- + + * Fix a bug where from_url was not possible to use without passing in additional variables. Now it works as the same method from redis-py. + Note that the same rules that is currently in place for passing ip addresses/dns names into startup_nodes variable apply the same way through + the from_url method. + 1.3.1 (Oct 13, 2016) -------------------- diff --git a/ptp-debug.py b/ptp-debug.py index 6785b007..0c4b6e31 100644 --- a/ptp-debug.py +++ b/ptp-debug.py @@ -4,5 +4,6 @@ # Note: decode_responses must be set to True when used with python3 rc = StrictRedisCluster(startup_nodes=startup_nodes, decode_responses=True) +url_client = StrictRedisCluster.from_url('http://127.0.0.1:7000') __import__('ptpdb').set_trace() diff --git a/rediscluster/client.py b/rediscluster/client.py index 855e59e1..6be0be4f 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -167,6 +167,30 @@ def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, self.response_callbacks = self.__class__.RESPONSE_CALLBACKS.copy() self.response_callbacks = dict_merge(self.response_callbacks, self.CLUSTER_COMMANDS_RESPONSE_CALLBACKS) + @classmethod + def from_url(cls, url, db=None, **kwargs): + """ + Return a Redis client object configured from the given URL, which must + use either `the ``redis://`` scheme + `_ for RESP + connections or the ``unix://`` scheme for Unix domain sockets. + For example:: + redis://[:password]@localhost:6379/0 + unix://[:password]@/path/to/socket.sock?db=0 + There are several ways to specify a database number. The parse function + will return the first specified option: + 1. A ``db`` querystring option, e.g. redis://localhost?db=0 + 2. If using the redis:// scheme, the path argument of the url, e.g. + redis://localhost/0 + 3. The ``db`` argument to this function. + If none of these options are specified, db=0 is used. + Any additional querystring arguments and keyword arguments will be + passed along to the ConnectionPool class's initializer. In the case + of conflicting arguments, querystring arguments always win. + """ + connection_pool = ClusterConnectionPool.from_url(url, db=db, **kwargs) + return cls(connection_pool=connection_pool) + def __repr__(self): """ """ diff --git a/rediscluster/connection.py b/rediscluster/connection.py index 813ba083..8d78e7d1 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -75,6 +75,18 @@ def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=Cl """ super(ClusterConnectionPool, self).__init__(connection_class=connection_class, max_connections=max_connections) + # Special case to make from_url method compliant with cluster setting. + # from_url method will send in the ip and port through a different variable then the + # regular startup_nodes variable. + if startup_nodes is None: + if 'port' in connection_kwargs and 'host' in connection_kwargs: + startup_nodes = [{ + 'host': connection_kwargs.pop('host'), + 'port': str(connection_kwargs.pop('port')), + }] + + print(startup_nodes) + self.max_connections = max_connections or 2 ** 31 self.max_connections_per_node = max_connections_per_node From de2a354ea586b3d7089b115df5e90f81f1dfab40 Mon Sep 17 00:00:00 2001 From: Diogo Albuquerque Date: Wed, 19 Oct 2016 15:43:49 -0200 Subject: [PATCH 2/6] Added options to skip full coverage check --- docs/authors.rst | 1 + docs/release-notes.rst | 1 + rediscluster/client.py | 10 +++++++--- rediscluster/connection.py | 9 +++++++-- rediscluster/nodemanager.py | 11 +++++++++-- tests/test_cluster_obj.py | 14 +++++++++++++- 6 files changed, 38 insertions(+), 8 deletions(-) diff --git a/docs/authors.rst b/docs/authors.rst index 2d64a2c7..35087e31 100644 --- a/docs/authors.rst +++ b/docs/authors.rst @@ -20,3 +20,4 @@ Authors who contributed code or testing: - gmolight - https://github.com/gmolight - baranbartu - https://github.com/baranbartu - monklof - https://github.com/monklof + - dutradda - https://github.com/dutradda diff --git a/docs/release-notes.rst b/docs/release-notes.rst index a68be2e5..d602034e 100644 --- a/docs/release-notes.rst +++ b/docs/release-notes.rst @@ -7,6 +7,7 @@ Next release (??? ??, 201?) * Fix a bug where from_url was not possible to use without passing in additional variables. Now it works as the same method from redis-py. Note that the same rules that is currently in place for passing ip addresses/dns names into startup_nodes variable apply the same way through the from_url method. + * Added options to skip full coverage check. This flag is useful when the CONFIG redis command is disabled by the server. 1.3.1 (Oct 13, 2016) diff --git a/rediscluster/client.py b/rediscluster/client.py index 6be0be4f..a4e388d5 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -113,7 +113,7 @@ class StrictRedisCluster(StrictRedis): } def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, max_connections_per_node=False, init_slot_cache=True, - readonly_mode=False, reinitialize_steps=None, **kwargs): + readonly_mode=False, reinitialize_steps=None, skip_full_coverage_check=False, **kwargs): """ :startup_nodes: List of nodes that initial bootstrapping can be done from @@ -125,6 +125,9 @@ def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, Maximum number of connections that should be kept open at one time :readonly_mode: enable READONLY mode. You can read possibly stale data from slave. + :skip_full_coverage_check: + Skips the check of cluster-require-full-coverage config, useful for clusters + without the CONFIG command (like aws) :**kwargs: Extra arguments that will be sent into StrictRedis instance when created (See Official redis-py doc for supported kwargs @@ -156,6 +159,7 @@ def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, max_connections=max_connections, reinitialize_steps=reinitialize_steps, max_connections_per_node=max_connections_per_node, + skip_full_coverage_check=skip_full_coverage_check, **kwargs ) @@ -168,7 +172,7 @@ def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, self.response_callbacks = dict_merge(self.response_callbacks, self.CLUSTER_COMMANDS_RESPONSE_CALLBACKS) @classmethod - def from_url(cls, url, db=None, **kwargs): + def from_url(cls, url, db=None, skip_full_coverage_check=False, **kwargs): """ Return a Redis client object configured from the given URL, which must use either `the ``redis://`` scheme @@ -189,7 +193,7 @@ def from_url(cls, url, db=None, **kwargs): of conflicting arguments, querystring arguments always win. """ connection_pool = ClusterConnectionPool.from_url(url, db=db, **kwargs) - return cls(connection_pool=connection_pool) + return cls(connection_pool=connection_pool, skip_full_coverage_check=skip_full_coverage_check) def __repr__(self): """ diff --git a/rediscluster/connection.py b/rediscluster/connection.py index 8d78e7d1..da21c97b 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -70,8 +70,12 @@ class ClusterConnectionPool(ConnectionPool): RedisClusterDefaultTimeout = None def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterConnection, - max_connections=None, max_connections_per_node=False, reinitialize_steps=None, **connection_kwargs): + max_connections=None, max_connections_per_node=False, reinitialize_steps=None, + skip_full_coverage_check=False, **connection_kwargs): """ + :skip_full_coverage_check: + Skips the check of cluster-require-full-coverage config, useful for clusters + without the CONFIG command (like aws) """ super(ClusterConnectionPool, self).__init__(connection_class=connection_class, max_connections=max_connections) @@ -90,7 +94,8 @@ def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=Cl self.max_connections = max_connections or 2 ** 31 self.max_connections_per_node = max_connections_per_node - self.nodes = NodeManager(startup_nodes, reinitialize_steps=reinitialize_steps, **connection_kwargs) + self.nodes = NodeManager(startup_nodes, reinitialize_steps=reinitialize_steps, + skip_full_coverage_check=skip_full_coverage_check, **connection_kwargs) if init_slot_cache: self.nodes.initialize() diff --git a/rediscluster/nodemanager.py b/rediscluster/nodemanager.py index 0c950f21..6cf318f3 100644 --- a/rediscluster/nodemanager.py +++ b/rediscluster/nodemanager.py @@ -19,8 +19,11 @@ class NodeManager(object): """ RedisClusterHashSlots = 16384 - def __init__(self, startup_nodes=None, reinitialize_steps=None, **connection_kwargs): + def __init__(self, startup_nodes=None, reinitialize_steps=None, skip_full_coverage_check=False, **connection_kwargs): """ + :skip_full_coverage_check: + Skips the check of cluster-require-full-coverage config, useful for clusters + without the CONFIG command (like aws) """ self.connection_kwargs = connection_kwargs self.nodes = {} @@ -29,6 +32,7 @@ def __init__(self, startup_nodes=None, reinitialize_steps=None, **connection_kwa self.orig_startup_nodes = [node for node in self.startup_nodes] self.reinitialize_counter = 0 self.reinitialize_steps = reinitialize_steps or 25 + self._skip_full_coverage_check = skip_full_coverage_check if not self.startup_nodes: raise RedisClusterException("No startup nodes provided") @@ -218,7 +222,10 @@ def initialize(self): self.populate_startup_nodes() self.refresh_table_asap = False - need_full_slots_coverage = self.cluster_require_full_coverage(nodes_cache) + if self._skip_full_coverage_check: + need_full_slots_coverage = False + else: + need_full_slots_coverage = self.cluster_require_full_coverage(nodes_cache) # Validate if all slots are covered or if we should try next startup node for i in range(0, self.RedisClusterHashSlots): diff --git a/tests/test_cluster_obj.py b/tests/test_cluster_obj.py index 611bcf48..c9b08084 100644 --- a/tests/test_cluster_obj.py +++ b/tests/test_cluster_obj.py @@ -15,8 +15,9 @@ from tests.conftest import _get_client, skip_if_server_version_lt, skip_if_not_password_protected_nodes # 3rd party imports -from mock import patch, Mock +from mock import patch, Mock, MagicMock from redis._compat import b, unicode +from redis import StrictRedis import pytest pytestmark = skip_if_server_version_lt('2.9.0') @@ -107,6 +108,17 @@ def test_custom_connectionpool(): assert {"host": h, "port": p} in c.connection_pool.nodes.startup_nodes +@patch('rediscluster.nodemanager.StrictRedis', new=MagicMock()) +def test_skip_full_coverage_check(): + """ + Test if the cluster_require_full_coverage NodeManager method was not called with the flag activated + """ + c = StrictRedisCluster("192.168.0.1", 7001, init_slot_cache=False, skip_full_coverage_check=True) + c.connection_pool.nodes.cluster_require_full_coverage = MagicMock() + c.connection_pool.nodes.initialize() + assert not c.connection_pool.nodes.cluster_require_full_coverage.called + + def test_blocked_commands(r): """ These commands should be blocked and raise RedisClusterException From 0875754c2638f5d155c15e987b18441c81e2ef90 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sat, 12 Nov 2016 21:06:46 +0100 Subject: [PATCH 3/6] Add a line in upgrading.rst that describes the new skip_full_coverage_check option into the client. --- docs/upgrading.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/upgrading.rst b/docs/upgrading.rst index 447398e1..0af71896 100644 --- a/docs/upgrading.rst +++ b/docs/upgrading.rst @@ -3,6 +3,14 @@ Upgrading redis-py-cluster This document describes what must be done when upgrading between different versions to ensure that code still works. + +1.3.1 --> Next release +---------------------- + +If your redis instance is configured to not have the `CONFIG ...` comannds enabled due to security reasons you need to pass this into the client object `skip_full_coverage_check=True`. Benefits is that the client class no longer requires the `CONFIG ...` commands to be enabled on the server. Downsides is that you can't use the option in your redis server and still use the same feature in this client. + + + 1.3.0 --> 1.3.1 --------------- From 9f9baeced12b7abac53b3680ea3f5a1b16a38038 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sat, 12 Nov 2016 21:29:04 +0100 Subject: [PATCH 4/6] Fix broken command CLUSTER SLOTS that would break with newer redis versions that include cluster node ID in reponse. --- docs/release-notes.rst | 1 + rediscluster/utils.py | 4 ++-- tests/test_utils.py | 50 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 2 deletions(-) diff --git a/docs/release-notes.rst b/docs/release-notes.rst index d602034e..0f65cd54 100644 --- a/docs/release-notes.rst +++ b/docs/release-notes.rst @@ -8,6 +8,7 @@ Next release (??? ??, 201?) Note that the same rules that is currently in place for passing ip addresses/dns names into startup_nodes variable apply the same way through the from_url method. * Added options to skip full coverage check. This flag is useful when the CONFIG redis command is disabled by the server. + * Fixed a bug where method *CLUSTER SLOTS* would break in newer redis versions where node id is included in the reponse. Method is not compatible with both old and new redis versions. 1.3.1 (Oct 13, 2016) diff --git a/rediscluster/utils.py b/rediscluster/utils.py index b275d6db..f5475018 100644 --- a/rediscluster/utils.py +++ b/rediscluster/utils.py @@ -125,8 +125,8 @@ def parse_cluster_slots(resp, **options): """ current_host = options.get('current_host', '') - def fix_server(host, port): - return (host or current_host, port) + def fix_server(*args): + return (args[0] or current_host, args[1]) slots = {} for slot in resp: diff --git a/tests/test_utils.py b/tests/test_utils.py index a1df4a60..6b11f71e 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -14,6 +14,7 @@ merge_result, first_key, clusterdown_wrapper, + parse_cluster_slots, ) # 3rd party imports @@ -21,6 +22,55 @@ from redis._compat import unicode +def test_parse_cluster_slots(): + """ + Example raw output from redis cluster. Output is form a redis 3.2.x node + that includes the id in the reponse. The test below that do not include the id + is to validate that the code is compatible with redis versions that do not contain + that value in the response from the server. + + 127.0.0.1:10000> cluster slots + 1) 1) (integer) 5461 + 2) (integer) 10922 + 3) 1) "10.0.0.1" + 2) (integer) 10000 + 3) "3588b4cf9fc72d57bb262a024747797ead0cf7ea" + 4) 1) "10.0.0.4" + 2) (integer) 10000 + 3) "a72c02c7d85f4ec3145ab2c411eefc0812aa96b0" + 2) 1) (integer) 10923 + 2) (integer) 16383 + 3) 1) "10.0.0.2" + 2) (integer) 10000 + 3) "ffd36d8d7cb10d813f81f9662a835f6beea72677" + 4) 1) "10.0.0.5" + 2) (integer) 10000 + 3) "5c15b69186017ddc25ebfac81e74694fc0c1a160" + 3) 1) (integer) 0 + 2) (integer) 5460 + 3) 1) "10.0.0.3" + 2) (integer) 10000 + 3) "069cda388c7c41c62abe892d9e0a2d55fbf5ffd5" + 4) 1) "10.0.0.6" + 2) (integer) 10000 + 3) "dc152a08b4cf1f2a0baf775fb86ad0938cb907dc" + """ + mock_response = [ + [0, 5460, ['172.17.0.2', 7000], ['172.17.0.2', 7003]], + [5461, 10922, ['172.17.0.2', 7001], ['172.17.0.2', 7004]], + [10923, 16383, ['172.17.0.2', 7002], ['172.17.0.2', 7005]] + ] + parse_cluster_slots(mock_response) + + extended_mock_response = [ + [0, 5460, ['172.17.0.2', 7000, 'ffd36d8d7cb10d813f81f9662a835f6beea72677'], ['172.17.0.2', 7003, '5c15b69186017ddc25ebfac81e74694fc0c1a160']], + [5461, 10922, ['172.17.0.2', 7001, '069cda388c7c41c62abe892d9e0a2d55fbf5ffd5'], ['172.17.0.2', 7004, 'dc152a08b4cf1f2a0baf775fb86ad0938cb907dc']], + [10923, 16383, ['172.17.0.2', 7002, '3588b4cf9fc72d57bb262a024747797ead0cf7ea'], ['172.17.0.2', 7005, 'a72c02c7d85f4ec3145ab2c411eefc0812aa96b0']] + ] + + parse_cluster_slots(extended_mock_response) + + def test_string_keys_to(): def mock_true(): return True From 20440c57821fae1a76372aa07c194cf22b10c10c Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sun, 27 Nov 2016 12:52:24 +0100 Subject: [PATCH 5/6] Bump version to 1.3.2 to prepare for release --- docs/release-notes.rst | 4 ++-- docs/upgrading.rst | 4 ++-- rediscluster/__init__.py | 2 +- setup.py | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/release-notes.rst b/docs/release-notes.rst index 0f65cd54..9bf53d9e 100644 --- a/docs/release-notes.rst +++ b/docs/release-notes.rst @@ -1,8 +1,8 @@ Release Notes ============= -Next release (??? ??, 201?) ---------------------------- +1.3.2 (Nov 27, 2016) +-------------------- * Fix a bug where from_url was not possible to use without passing in additional variables. Now it works as the same method from redis-py. Note that the same rules that is currently in place for passing ip addresses/dns names into startup_nodes variable apply the same way through diff --git a/docs/upgrading.rst b/docs/upgrading.rst index 0af71896..70096e29 100644 --- a/docs/upgrading.rst +++ b/docs/upgrading.rst @@ -4,8 +4,8 @@ Upgrading redis-py-cluster This document describes what must be done when upgrading between different versions to ensure that code still works. -1.3.1 --> Next release ----------------------- +1.3.1 --> 1.3.2 +--------------- If your redis instance is configured to not have the `CONFIG ...` comannds enabled due to security reasons you need to pass this into the client object `skip_full_coverage_check=True`. Benefits is that the client class no longer requires the `CONFIG ...` commands to be enabled on the server. Downsides is that you can't use the option in your redis server and still use the same feature in this client. diff --git a/rediscluster/__init__.py b/rediscluster/__init__.py index a909d9b2..85757601 100644 --- a/rediscluster/__init__.py +++ b/rediscluster/__init__.py @@ -16,7 +16,7 @@ setattr(redis, "StrictClusterPipeline", StrictClusterPipeline) # Major, Minor, Fix version -__version__ = (1, 3, 1) +__version__ = (1, 3, 2) if sys.version_info[0:3] == (3, 4, 0): raise RuntimeError("CRITICAL: rediscluster do not work with python 3.4.0. Please use 3.4.1 or higher.") diff --git a/setup.py b/setup.py index 06072c9e..4a5e890a 100644 --- a/setup.py +++ b/setup.py @@ -20,7 +20,7 @@ setup( name="redis-py-cluster", - version="1.3.1", + version="1.3.2", description="Cluster library for redis 3.0.0 built on top of redis-py lib", long_description=readme + '\n\n' + history, author="Johan Andersson", From 4fb6b474a90516ba68d6dd64a2b8a2f2fed36a0c Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sun, 27 Nov 2016 13:05:08 +0100 Subject: [PATCH 6/6] Change travis tests to test against the latest commit in 3.0 and 3.2 branch and not some version that is not the latest --- .travis.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3dbae178..a8594fe6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,19 +9,19 @@ python: services: - redis-server install: - - "if [[ $REDIS_VERSION == '3.0' ]]; then REDIS_VERSION=3.0.7 make redis-install; fi" - - "if [[ $REDIS_VERSION == '3.2' ]]; then REDIS_VERSION=3.2.0-rc3 make redis-install; fi" + - "if [[ $REDIS_VERSION == '3.0' ]]; then REDIS_VERSION=3.0 make redis-install; fi" + - "if [[ $REDIS_VERSION == '3.2' ]]; then REDIS_VERSION=3.2 make redis-install; fi" - pip install -r dev-requirements.txt - pip install -e . - "if [[ $HIREDIS == '1' ]]; then pip install hiredis; fi" env: - # Redis 3.0.7 + # Redis 3.0 - HIREDIS=0 REDIS_VERSION=3.0 - # Redis 3.0.7 and HIREDIS + # Redis 3.0 and HIREDIS - HIREDIS=1 REDIS_VERSION=3.0 - # Redis 3.2.0-rc3 + # Redis 3.2 - HIREDIS=0 REDIS_VERSION=3.2 - # Redis 3.2.0-rc3 and HIREDIS + # Redis 3.2 and HIREDIS - HIREDIS=1 REDIS_VERSION=3.2 script: - make start