Error when using with `redis-clustering`

[joshuay03]

We use a connection pool of Redis::Cluster as the client and started seeing some errors when we upgraded to v5.3.0 of that gem (while using the latest version of faulty):

Faulty::CircuitFailureError: circuit error for "Faulty::Storage::CircuitProxy": Given block needs an argument if you use the cluster client.

This is the source of the error. It seems some conditional logic might be necessary when calling #watch depending on what the client class is.

I would have sent a PR to fix, but I noticed specs aren't run against redis-clustering. Is this something you would be open to officially supporting?

[justinhoward]

Hey @joshuay03! Thanks for the question. I'm curious what your experience is with running Faulty on a Redis cluster. Do you actually have multiple nodes configured? The reason I ask, is that Faulty isn't compatible with Redis clusters. You can see some discussion in #53 and in #51. In particular, my conclusion was:

For anyone coming across this issue. I think we could implement an alternate redis cluster backend that would support clusters. My thought is that it would likely use Redis hash tags to ensure that pipelined commands always apply a single cluster node.

As mentioned, this would have performance tradeoffs. For example, the circuit list key would always be located on a single node (unless we implemented that a different way, or chose not to support it). Also, since a circuit would always be located on a single node, it would likely create uneven node distribution. That said, if someone is willing to accept those tradeoffs, I'd be glad to accept such a PR!

[joshuay03]

Hey @joshuay03! Thanks for the question. I'm curious what your experience is with running Faulty on a Redis cluster. Do you actually have multiple nodes configured? The reason I ask, is that Faulty isn't compatible with Redis clusters. You can see some discussion in #53 and in #51.

Ah lol that was my teammate (before my time). We're using a single shard with two nodes, and everything seems to be working fine. It seems we've been running this setup for a while now. I will however note that we've got storage.disable_warnings = true configured as @JuanitoFatas has mentioned in those issues.

[justinhoward]

@joshuay03 Ah, well thanks for your feedback and contribution! I'm open to ideas if you are. At this time though, I'm not convinced it's a good idea to support redis-clustering without also supporting a multi shard configuration.

[joshuay03]

@justinhoward Fair enough, we'll figure out a way to patch this on our end. Do you think it's worth explicitly mentioning in the README that this isn't supported?

[justinhoward]

@joshuay03 It sounds like you're not necessarily using the features of the clustering client if you have only one shard. Is using a normal Redis client an option?

[joshuay03]

@joshuay03 It sounds like you're not necessarily using the features of the clustering client if you have only one shard.

Not at the moment.

Is using a normal Redis client an option?

I don't think so... I'm not completely across all our infra, but my understanding is that we default to a single shard cluster setup and prefer to use the cluster client due to it's interoperability with the standalone client, and then scale as needed without the need to change anything on the application side. This cluster is also used for other rate limiting purposes.

[justinhoward]

@joshuay03 With the redis-clustering client, is there a way to detect that there is only one shard? If there is, then it might be possible to support it in that configuration without actually supporting sharding with hash tags.

[joshuay03]

@joshuay03 With the redis-clustering client, is there a way to detect that there is only one shard? If there is, then it might be possible to support it in that configuration without actually supporting sharding with hash tags.

I think we can either dig into the config and check that it's configured with one node (i.e. one shard) e.g. Redis::Cluster.new(nodes: [ENV["REDIS_SHARD_URL"]]), or we ask the server with redis.config("get", "cluster-enabled") ignore, it's possible for cluster mode to be enabled with a single shard.

[pda]

@justinhoward You're quite right, this wasn't working in cluster mode 😅

General operation was fine (creating the circuit, calling #try_run { … } etc, but it would crash with cross-slot MULTI errors when opening/closing/reset etc.

We've worked around this for now by putting a static hash key into the options.storage.key_prefix, like circuit_breaker{noshard}. This of course means that all circuits will hash to a single slot and use a single shard of the cluster. This is fine for us right now, but it's not a good long-term solution.

Longer term, it would be great to get the circuit name wrapped in a hash tag. Perhaps we could do this only when the connection is detected to be cluster-mode Redis 🤔 Then we wouldn't be changing keys for existing (non-cluster-mode) users, and it would be a smaller / safer / backwards-compatible change.

[justinhoward]

get the circuit name wrapped in a hash tag

@pda That sounds like a reasonable solution to me. After reviewing my previous thoughts, I don't think adding hash tags would necessarily cause any performance penalty for non-clustered Redis installations. I still think sharded clusters are perhaps not a great fit for Faulty, but I think it's okay to leave that up to the user.

If you decide to look into this, a few considerations:

• The circuit list will be on a single, likely arbitrary shard
• Individual circuits will likely be "hot", so it'll potentially cause uneven shard load
• This would technically be a backwards-incompatible change, since it will create new database entries for every circuit. We'll want to think about how to handle that. It might be fine to accept that for the sake of simplicity.

[pda]

One interesting note is that the Redis client detects and fails MULTI operations when the keys involved map to different hash slots, of which there are always 16,384, regardless of how many shards are in the cluster.

In our case, for whatever reason, we're pointing Faulty at a cluster-mode Redis with a single shard. The cluster was previously created for, and continues to be used for, a separate workload. It's probably cluster-mode with shards=1 to allow for easy future scaling of that other cluster-mode-capable workload.

This doesn't really change anything in this discussion, it's just interesting that even a single-shard cluster-mode redis encounters these cross-slot errors, and in that specific shards=1 scenario, statically hash-tagging the entire key prefix to a single hash slot fixes it without any downside.

[joshuay03]

@justinhoward Just an FYI, I'll be looking into this over the course of this week. I'll take into account everything that's been discussed here and send a PR where we can further discuss anything that's up in the air.

[joshuay03]

@justinhoward Just an FYI, I'll be looking into this over the course of this week. I'll take into account everything that's been discussed here and send a PR where we can further discuss anything that's up in the air.

I've implemented this in our fork so we can (temporarily) use and test it out while I work on upstreaming.

Edit: Forgot to link the PR 🤦‍♂ buildkite#1