Envoy support for stackexchange

[rkarthick]

Envoy proxy for redis: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/other_protocols/redis

Envoy does the heavy lifting of redis cluster instance discovery and partitioning commands among redis cluster instances from the clients to a set of proxy instances. Clients can round robin or randomly select a proxy to connect to and the proxy will do the right thing of picking the right cluster instance for a given command.

Apart from the glue of wiring up the new proxy in Enums and disabling pub/sub for the proxy (similar to Twemproxy), this PR contains the following major change in the primary node selection; When an envoy proxy mode is selected for connection, the check for verifying if the node is master or replica is disabled. In the envoy proxy world we can connect to any proxy instance. All we care about here is ensuring that the load is balanced between the proxy instances (which is accomplished through the current round-robin logic).

@mgravell raised a valid point offline that the order of operations wouldn't be guaranteed in this mode and this is indeed one of the limitation of this approach. The commands that form the foundation for redis transactions (exec, discard, multi, watch, unwatch) (https://redis.io/topics/transactions) are not supported by Envoy (https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/network/common/redis/supported_commands.h) and hence added to the exclusion list in PR (https://github.com/StackExchange/StackExchange.Redis/pull/1977/files#diff-d76e63e6d287770f768927dfdc3b25d17fc14d4413e0562140a86a6426cac7b4R59).

The testing is inspired from this PR: #1425. Open to suggestions on that.

Thanks in advance.

[mgravell]

The most important part of an inbound PR is: that box above ^^^^

What is envoy proxy? Maybe a link. What does it apply to? Any particular server variants? Cluster? Vanilla? Versions? Anything especially interesting about it that is relevant to reviewers? What testing approach, if any, has been applied?

Without context, it is easy to say "yes, that's definitely code", but much harder to say whether it is the appropriate code for the situation.

Can you help us out with some input above please?

[rkarthick]

The most important part of an inbound PR is: that box above ^^^^

What is envoy proxy? Maybe a link. What does it apply to? Any particular server variants? Cluster? Vanilla? Versions? Anything especially interesting about it that is relevant to reviewers? What testing approach, if any, has been applied?

Without context, it is easy to say "yes, that's definitely code", but much harder to say whether it is the appropriate code for the situation.

Can you help us out with some input above please?

Furnished the PR with the details. We synced briefly over this last year about adding Envoy support, and you did raise the following concern:

Your input would be welcome. I see what you mean about the primary node se oflection, when in reality we could round-robin load to any proxy, similar to how we do with replicas - although that may raise some complications around "unawaited async calls", in terms of guaranteed order (order goes out the window if we send commands to different proxies). But then: that is also a problem we have for a planned pooled mode... maybe ultimately, if people do unawaited async calls without using the batch/transaction APIs, we just have to say: order is a bit wobbly.

[mgravell]

Awesome; thanks for the extra context.

[NickCraver]

Thanks for this! I'd like to change a few implementation details (thinking some .SupportsX extension methods on the proxy type) but awesome pass at this. I already see several conflicts from PRs in pipe right now, so let us get those in and then I can edit on this branch if that's alright. I'm not sure what to do about the testing - is this available in Docker form somewhere? If so we can add it to the container suite and do some connection testing at least :)

[rkarthick]

Thank you so much for the initial comments. Yeah I can definitely get working on a docker connection test, in the meanwhile, while we get the other Prs in.

I am not super familiar with .SupportsX extension methods. I will be more than happy to take a stab at it too.
Are you thinking of introducing a generic proxy type, with options to enable or disable a particular feature through .SupportsX method? If you have link to any sample code that has a similar pattern, that would be super helpful.

[NickCraver]

@rkarthick I coded up what I was thinking here in a branch (you could PR against this branch if you wanted): 87bf9bb - does that make sense? Thoughts for/against?

[NickCraver]

...and looks like we have docker templates in https://github.com/envoyproxy/envoy/tree/main/examples/redis - excellent, I can take a stab at setting this up one evening this week I hope! (I want to shuffle where our Docker files are so they make more sense - they're a bit nested atm)

[rkarthick]

@NickCraver This the main commit for envoy support created over your craver/proxy-extensions branch. Do you prefer me closing this PR out and we can continue discussion over a new PR?

[NickCraver]

@rkarthick hmmm, yeah the diff is tricky because that PR is on "latest" which is on top of craver/docs-work - let me see if I can get some of those merged today then all you'd need to do here is merge main in.

[NickCraver]

@rkarthick git has lost it's mind here, or I have - I'm not sure what happened with branches but it's a royal mess - what I'm trying to do is lower the bar as much as possible. I've opened #1982 to get the extensibility into main to hopefully make your PR on top of that as painless as possible. Sorry this is a commit mess :(

[rkarthick]

Now worries @NickCraver :). I spent some time fighting with the git the get the changes over your branch and finally gave up and opened a new branch rkarthick@3d8c72b I will wait for #1982 to land and I apply my changes over it.

[NickCraver]

@rkarthick perfect - thanks for bearing with me on this one!

[rkarthick]

closing this PR out for a fresh one over master.