StackExchange / StackExchange.Redis

General purpose redis client
https://stackexchange.github.io/StackExchange.Redis/
Other
5.88k stars 1.51k forks source link

Resolution of DNS address records and client-side load balancing #2294

Open eduardobr opened 1 year ago

eduardobr commented 1 year ago

Hi,

Have we considered implementing resolution of DNS to get multiple addresses (with some periodic updating of list)? In the same spirit of this: https://learn.microsoft.com/en-us/aspnet/core/grpc/loadbalancing?view=aspnetcore-6.0#configure-resolver (It could be highly inspired or copied from this Grpc library implementation)

This would be an amazing addition to anyone using Kubernetes or trying to have a simple setup where all servers are in a single DNS address.

NickCraver commented 1 year ago

Can you describe your layout more? Is it a bunch of A records? CNAMEs? Other?

This isn't something we're considering at the moment, but that's not to say we're not open to it - want to understand your scenario first though. How/when/which TTLs and how Sentinel (if at all) behaves are all factors involved in the approach here.

eduardobr commented 1 year ago

@NickCraver, for the Kubernetes use case it can be either A / AAAA records or CNAMES. See https://kubernetes.io/docs/concepts/services-networking/service/#headless-services

The records would change as needed, in Kubernetes instances exposed can be pretty volatile. For example some patch being applied to some node or autoscaling will reschedule / restart Redis pods, also if some replica enters LOADING state (and we defined it means pod is unready because of that), etc. So let's say some 30 seconds would be a good starting point to keep checking for changes. But also, we can rely on updating as needed (for example when we see an unhealthy / disconnected instance).

Honestly I don't use Sentinels and I think in this use case sentinel is not popular because Kubernetes acts as a monitoring / orchestrating tool and provides service discovery. At the same time, couldn't we simply make it same as what we do when passing multiple addresses to RedisConfig? It seems to me it's the same thing, but list would be auto-updated through a DNS resolver we implement.

nunoguerreirorosa commented 1 year ago

When using Redis Cluster deployed in K8 (not sentinel mode), we are forced on specifying each individual redis cluster node in the connection string so when we scale up / down, the application recovers correctly. If we specify the headless service or the cluster IP in the connection string, downscaling / upscaling is not recovering as when we specify each individual node in the connection string. I assume this is occurring cause of the problem @eduardobr is describing. Internally when there is a failure, it should extract all the redis node IPs from the headless service and reuse those to avoid having to specify always each individual node. Can you confirm that is the behavior @eduardobr , @NickCraver ?

eduardobr commented 1 year ago

@nunoguerreirorosa I think what you describe is similar but not the same. I have little experience with Redis Cluster, but SE.Redis as a client library should get MOVED and ASK status and eventually handle connections to multiple masters even if connected to only one at first. @NickCraver, is MOVED going to add new connections and keep them open, and also eventually get rid of the initial single connection if that doesn't resolve to an existing IP anymore?

Now, if there's a fast change in the redis pods IPs and SE.Redis hang on to the initial IP of the headless service, I think it may take time to recover or never recover.

I think this area deserves attention as K8s use case are growing substantially.

NickCraver commented 1 year ago

A MOVED will cause us to re-evaluate what the state is and re-fetch CLUSTER NODES. We'll connect to what it's telling us to connect to. If CLUSTER NODES is returning endpoints that aren't really what you want people to connect to (e.g. Redis isn't aware of what's in-front of it), that's not something we (or Redis, AFAIK) handles - I think it'd a legit feature request for Redis server to support better in some way, and maybe there's discussion about this somewhere already (wouldn't surprise me).

The alternative thing we could potentially (don't quote me, spit balling ideas here), is to allow a custom plug point that's not CLUSTER NODES or perhaps a follow-up to it, in which you re-map the endpoints coming back from that for what we should connect to. But all of those solutions have inherent state races.

Alternatively, if you always know the cluster endpoints the client should have and they never change, you could disable CLUSTER NODES discovery and we won't even try (e.g. $CLUSTER= in the connection string, or remove it from the CommandMap). This has downsides of course which is: you can't change the cluster, we would never pickup a change. If you wanted for example to add a node, you'd need to push out a client update and suffer the mismatch consequences until both are in sync.

Thoughts?