Remove slave to jedisPool mapping

[wxjackie]

Recently occurred such a problem in our project: There are some redis clusters on K8s deployed and managed by us, after the business service runs for a period of time using one of the RedisCluster (cluster A), one of the shards of the cluster fails, and then the business service connects to another cluster(cluster B) through the JedisCluster.

In some cases, such as redis on K8s or some cloud service platforms, there may be situations where the redis node address has been reused by another redis cluster, for example:

Cluster A:
192.168.1.100:6379 master		192.168.1.103:6379 slave
192.168.1.101:6379 master		192.168.1.104:6379 slave
192.168.1.102:6379 master		192.168.1.105:6379 slave (down and change to another IP)
Cluster B:
192.168.1.200:6379 master		192.168.1.203:6379 slave
192.168.1.201:6379 master		192.168.1.204:6379 slave
192.168.1.202:6379 master		192.168.1.105:6379 slave (reuse the ip)

But the nodes map in JedisInfoCache saves the mapping of all master and slave nodes to JedisPool. However, the slave->JedisPool has not been updated or reset in any way during the long-running after the initialization of JedisCluster. If the address of the slave (who has down and change to another address) is reused by other clusters and this cluster fails, JedisPool is randomly selected from the nodes map for service discovery, and another cluster may be discoverd, causing read and write requests to be sent to other clusters incorrectly.

So I think that the slave->JedisPool mapping should not be saved, because they are not checked after initialization but may be selected for disovery after failure.

[sazzad16]

@wxjackie Methods like getNodes(), getShuffledNodesPool() are available to help users to fulfil their own requirement. There are applications that use these methods and some of those do some operations entirely on slave nodes. So I don't agree with the idea of skipping slave nodes from the mentioned methods.

[wxjackie]

The problem is that the slave nodes in the nodes are never checked and updated after initialization, so that getShuffledNodesPool() used in renewClusterSlots, the failed slave may be selected for discovery. If the address has been used by other Redis clusters, It will cause JedisCluster read and write requests to be sent to a wrong redis cluster.

I did the following operations, this phenomenon can occur:
1. create a redis cluster.
2. start jedisCluster and print the cached nodes information in a loop.
3. shutdown 127.0.0.1:6384 redis server
4. cluster forget it.

Thanks reply @sazzad16 , Please read my code test screenshot below, JedisCluster does cache the invalid slave information, and may still continue to use it，if the address127.0.0.1:6384 has been used by another redis cluster, may lead to serious wrong discovery problems:

[rustlingwind]

The JedisInfoCache never get refreshed automatically even there's a failover / resharding / cluster vertical upgrade (bluegreen) / auto-scaling occurred for the Redis cluster. E.g. when a failover occurred, a slave will be promoted as a master node, and there would be a new slave node initialized and added to the shard. Then the cluster node IPs would be changed.
I once met this problem, the redis cluster auto-scaled (AWS Elasticache), and the JedisCluster was always connecting to old IPs. After max attempts of reconnections, exception raised and never had opportunity to reconnect to new IPs of cluster nodes.
This is a serious problem in production environment, not align with the SLO/SLA requirements of many companies.
I just noticed that The JedisClusterInfoCache field is protected in class
JedisClusterConnectionHandler and cannot invoke the discoverClusterNodesAndSlots func of cache to refresh nodes info from JedisCluster. But even it can be invoked, still lack of some out-of-box mechanisms to auto refresh the Redis cluster topology and it's expected that the Redis cluster node IP changes can be transparent to users and needn't implement by themselves to refresh the cluster node IPs.

[quinlam]

This is marked as "will not fix" but can we get clarity on what that means?

• No bandwidth to investigate?
• Issue not reproduced?
• Issue acknowledged but not a concern of the library?

Right now Jedis in cluster mode does not seem to handle failover as described by @rustlingwind

[sazzad16]

@quinlam Not sure if you noticed but this is submitted as a pull request (PR) not as an issue. Irrespective of the issue, the change that this PR has proposed is not accepted.

Ideally there should be separate issue and PR. In that case we could just close the PR. But this specific PR is kept open to attract more able eyes and brain for better solution, even another PR. That's where the label comes which is not ideal but the situation is not ideal in the first place.