In case of a non clustered redis, avoid checking masters and slaves

[mbaroukhcw]

Hi.

I'm using keydb (https://docs.keydb.dev/) instead of redis.

KeyDB has a multi master mode.

It was not working here because although info all returns cluster_enabled:0, this file expect role:master.
And on keydb, as multi-master is not standard, rhe returned value for role is active-replica with cluster_enabled: 0

So this patch does not check master/slave config if cluster_enabled:0 and use the host as master.

This patch has no impact on redis and I'm able to use keydb instead with a simple configuration : Each server has nginx+keydb and each nginx configuration instruct to use the keydb instance on localhost. Then, all keydb instance are connected together in master-master mode.

So I can add/remove servers without any impact on nchan.

[slact]

Valid use case, but I can't accept this patch as is. Nchan needs to know about masters and slaves to load-balance its SUBSCRIBE requests.

I might be okay with adding active-replica as another alias for master, but in this case I think discovering other nodes should be turned off. This is just off the top of my head, I need to think on this a bit more. Your suggestions are most welcome.

[slact]

I see no mention of active-replica mode in the KeyDB docs on the info command:

https://docs.keydb.dev/docs/commands#info

Here is the meaning of all fields in the replication section:

role: Value is "master" if the instance is replica of no one, or "slave" if the instance is a replica of some master instance. Note that a replica can be master of another replica (chained replication).

[mbaroukhcw]

Hi @slact

Thanks for your comment :).

Effectively, their doc does not mention it. That is just what I see when running info all :

# Replication
role:active-replica
master_global_link_status:up
connected_masters:2
master_host:keydb2
master_port:6379
master_link_status:up
master_last_io_seconds_ago:6
master_sync_in_progress:0
...

# Cluster
cluster_enabled:0

My goal was just to have the minimum impact on existing code : if cluster is disabled, as I understand, it seems not really useful to try to find a master. We can use the first node that is not in a cluster. I understand that it might lead to inconsistancy if the user use multiple node and some are part of a cluster and some not. So maybe we can fail if there is more than 1 node and one of them is not part of a cluster ?

imho, adding "active-repilca" should be avoided if we can as it is vendor specific. And we can here. May be that another redis compatible alternative (DragonFly ?) works differently and adding vendor specific code for all alternatives could make the code more complicated.

At first I thought I'll make a patch on KeyDB but this mode master-master does not exists on redis so it seems legit for them to returns something different. And as,in this case, they report cluster as disabled, it works.

[mbaroukhcw]

Also, on keydb, subscribe actually works on any host. So on keydb subscribe will work if we use the first node.

On second thought, I suppose that if the node get down we should choose another. (I suppose the module alredy does this) So we should not fail if there is more than one node with cluster_disabled. (as wrongly said in my previous comment).

[slact]

Well, my concern is that this is an undocumented reply for the INFO command on their part. A better alternative might be a setting to skip autodiscovery of the network topology and just use the hosts provided, with the roles explicitly specified.

[mbaroukhcw]

I agree totally that allowing to explicitly define which host is read-write and which is read-only is the best option. That would be great. Maybe by adding a second parameter to "nchan_redis_server".

But it seems a lot more work.

[slact]

Ok, I've added a feature to manually set a Redis server to "master" or "slave":

Please test it with KeyDB on your install.

[mbaroukhcw]

Nice ! I'll check tomorrow

[andryyy]

Works for me. :) But can I disable auto-discovering of other nodes I do not define in the config?

[mbaroukhcw]

Hi. Sorry I was not able to test becuase for now I was using the debian source module to rebuild a .deb. And now, using sources from this repo, building the .deb fail

/libnginx-mod-nchan-1.3.6+dfsg/./src/store/redis/redis_nodeset.c:1053:9: error: 'cluster_nodes_line_t' has no member named 'maybe_failed'
 1053 |     if(l->maybe_failed) {
      |         ^~
make[1]: *** [/libnginx-mod-nchan-1.3.6+dfsg/obj-x86_64-linux-gnu/Makefile:1946: /libnginx-mod-nchan-1.3.6+dfsg/obj-x86_64-linux-gnu/addon/redis/redis_nodeset.o] Error 1

So I have to try to build nginx with the module I suppose.