Support Redis cluster topology for the Redis scaler

[mboutet]

Support Redis cluster topology for the Redis scaler

See discussion for context: #872 (comment)

Use-Case

Right now, the Redis scaler only supports non-clustered Redis topology. In our stack, however, we use Redis cluster to provide horizontal scalability and HA. Some cloud providers such as AWS and Azure also offer Redis cluster in their offering.

See https://redis.io/topics/cluster-spec and https://redis.io/topics/cluster-tutorial for more info on the Redis cluster topology.

Specification

• Support specifying multiple addresses in the address field of the trigger spec (one for each node).

Since the cluster topology requires the client to be cluster-aware, I don't know if this would be a separate scaler or if it could be integrated with the current Redis scaler with some more fields in the trigger (perhaps topology = normal (default), OR cluster and addresses (if topology is cluster). In our applications, simply swapping for a cluster-aware client was the only require step in order to support this topology (redis-py-cluster instead of redis-py).

[zroubalik]

@mboutet are you willing to contribute this feature?

[mboutet]

Yes, I could look into it. That would be a great way to start properly leaning Go. I have no definite timeline right now. I will first familiarize myself with the codebase.

[zroubalik]

@mboutet excellent, great! In that case, I'd recommend you to look at upcoming v2 codebase https://github.com/kedacore/keda/tree/v2

[turbaszek]

Hi @mboutet any update on this issue?

[mboutet]

@turbaszek, I haven't done any work on this yet. I was thinking of waiting for the v2 codebase to be officially released to start working on this. Of course, don't hesitate to start the implementation on this if you want.

[goku321]

Can I give it a try if that's okay?

[mboutet]

Yes of course! I haven't done anything on my side.

[goku321]

I did a bit of homework and could think of two approaches to this:

1. Include a boolean field clusterEnabled and a clusterClient field:

diff --git a/pkg/scalers/redis_scaler.go b/pkg/scalers/redis_scaler.go
index bd9da22..1db3459 100644
--- a/pkg/scalers/redis_scaler.go
+++ b/pkg/scalers/redis_scaler.go
@@ -24,8 +24,10 @@ const (
 )
 
 type redisScaler struct {
-       metadata *redisMetadata
-       client   *redis.Client
+       metadata       *redisMetadata
+       client         *redis.Client
+       clusterClient  *redis.ClusterClient
+       clusterEnabled bool
 }

Then, based on clusterEnabled field we would know which client to use for redis related operations..

2. Create separate cluster-aware redis scalers. This would have two implementations for each scaler type.

Would really appreciate any alternatives or suggestions.

[tomkerkhove]

Although I'm not a fan of the name I think a flag sounds ok for me, wdyt @zroubalik? What about isClustered or so?

Are you able to contribute this @goku321?

[goku321]

Definitely @tomkerkhove. Will start working on it.

[mboutet]

What would the trigger's OpenAPI spec looks like? Right now, the parameters are: address, host, port. For the cluster, they should be addresses, hosts, and ports. Unless address, host, and port are also used when in cluster mode, but it would be an inconsistent naming convention in my opinion. Also, the current redis scalers have the optional databaseIndex which is not supported by the cluster configuration, so there should at least be validation if the same scalers spec are used.

Perhaps having different scalers (from the user's perspective at least) would be better/cleaner so that the trigger specs only contain what's relevant for the normal redis configuration or for the clustered configuration.

[tomkerkhove]

I'm not a redis expert so I expected those to be identical, but if it's different than a different scaler feels best indeed

[goku321]

When hosts and ports are given, I'm assuming there will be one to one mapping in the same order. For example: first host inside hosts will be used with the first port inside ports.

[tomkerkhove]

This is supported already.