(ClusterClient).ForEachNode does not cover all nodes

[gbbr]

In a cluster with 3 masters (ports 7000-7002) and 3 slaves (ports 7003-7005) connected as:

c := redis.NewClusterClient(&redis.ClusterOptions{
	Addrs: []string{":7000", ":7001", ":7002"},
})

Using WrapProcess inside ForEachNode does not wrap each client, leaving gaps. ForEachNode uses a for loop to go through each master (state.masters) and slave (state.slaves), but omits some nodes (state.nodes.nodes).

In the above setup, the state field contains:

state.masters:
172.17.0.2:7002
172.17.0.2:7000
172.17.0.2:7001

state.slaves:
172.17.0.2:7005
172.17.0.2:7003
172.17.0.2:7004

state.nodes.nodes:
172.17.0.2:7002
172.17.0.2:7005
172.17.0.2:7000
172.17.0.2:7003
172.17.0.2:7004
:7000
:7001
:7002
172.17.0.2:7001

Even though ForEachNode goes as expected through each unique node, it does not offer the possibility to tap into every client. One possible fix could be:

diff --git a/cluster.go b/cluster.go
index a2c18b3..8761000 100644
--- a/cluster.go
+++ b/cluster.go
@@ -816,11 +816,7 @@ func (c *ClusterClient) ForEachNode(fn func(client *Client) error) error {
 		}
 	}
 
-	for _, node := range state.masters {
-		wg.Add(1)
-		go worker(node)
-	}
-	for _, node := range state.slaves {
+	for _, node := range state.nodes.nodes {
 		wg.Add(1)
 		go worker(node)
 	}

Would this change or a similar one make sense? Or am I doing something stupid?

[vmihailenco]

Why do you want to tap every client? AFAIR go-redis only uses nodes in state.masters and state.slaves. Unused nodes should be eventually removed by clusterNodes.GC.

Probably :7000 :7001 :7002 should not be added to the nodes in first place, but I don't see what harm it causes...

[gbbr]

I want to wrap each client to find out which one of them has successfully executed a command (as you have suggested here). In the current setup, each wrapped process command returns an error because the one that succeeds is not wrapped (the for loop doesn't catch it because it's not present in masters/slaves).

---

Some potentially unnecessary context (as to why those ports are there, and why I'm not sure if the GC would remove them - they are valid): the redis cluster runs in a Docker container where the ports are exposed as :7000-:7005, but inside the container the nodes find each other using that IP address (172.17.0.2). masters and slaves registers the nodes returned by the cluster with the (wrong, in my case) IP, but nodes.nodes also contains the ports used when setting up the cluster, which the system can connect to.

I hope what I'm saying makes sense. I do realize however that my setup is odd due to this IP/Docker thing, but I wonder if my suggested change to the code would not make everything a bit more correct.

What are your thoughts?

[vmihailenco]

I want to wrap each client to find out which one of them has successfully executed a command (as you have suggested here).

My guess was that you want to measure latency to every node in the cluster, which can be achieved with ForEachNode and WrapProcess.

What do you really try to achieve?

What are your thoughts?

If 172.17.0.2 in your case is a loopback address then go-redis can be improved to strip loopback ip address, e.g. 172.17.0.2:7004 becomes :7004. AFAIK we already have code that does this for single IP address at https://github.com/go-redis/redis/blob/master/cluster.go#L341-L348

[gbbr]

If 172.17.0.2 in your case is a loopback address then go-redis can be improved to strip loopback ip address, e.g. 172.17.0.2:7004 becomes :7004.

The problem is that it's a loopback address in the Docker container, but not on my local machine. The cluster advertises it. When the library tries to connect to it, it fails, but when it tries to connect to simply :7004, that works fine. Do you see the issue?

[gbbr]

Sorry, forgot to answer your first question.

What do you really try to achieve?

TL;DR I want to know which node in the cluster ran a command.

I'm trying to wrap the cluster process to apply tracing to each command that runs on it, using our tracing library (dd-trace-go). As it works now, I am not able to tell which node runs the command and that is kind of essential when tracing Redis calls. So I was hoping to somehow work with WrapClient to be able to tell which is the client that ran my command successfully (returning a non-nil error) and use that.

[gbbr]

AFAIR go-redis only uses nodes in state.masters and state.slaves. Unused nodes should be eventually removed by clusterNodes.GC.

Actually it seems that go-redis uses the nodes property. So it might make sense after all to iterate through that when running ForEachNode.

See here.

[gbbr]

Resolved in PR.