support for envoyproxy #1989

rkarthick · 2022-02-11T01:17:00Z

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.

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

NickCraver

Looking good! I put some tidy suggestions inline - want to add testing here as well but initial is 👍

src/StackExchange.Redis/CommandMap.cs

src/StackExchange.Redis/Enums/Proxy.cs

src/StackExchange.Redis/Enums/ServerType.cs

src/StackExchange.Redis/ServerEndPoint.cs

src/StackExchange.Redis/ServerSelectionStrategy.cs

Co-authored-by: Nick Craver <[email protected]>

src/StackExchange.Redis/Enums/ServerType.cs

rkarthick · 2022-02-11T04:01:10Z

Ah, thanks for catching this, I incorrectly made the change in this version.

…

On Thu, Feb 10, 2022 at 5:30 PM Nick Craver ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/StackExchange.Redis/Enums/ServerType.cs <#1989 (comment)> : > @@ -30,6 +34,7 @@ internal static class ServerTypeExtensions /// </summary> public static bool HasSinglePrimary(this ServerType type) => type switch { + ServerType.Envoyproxy => true, Is this one true or false? I got the impression this was a round-robin desired and not sure about envoy and wasn't sure what *should* happen. Thoughts? — Reply to this email directly, view it on GitHub <#1989 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH4RGVNCOGNM5UDXO3VSCTU2RRDJANCNFSM5OCU76XA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Karthick Ramachandran

…ick/StackExchange.Redis into rkarthick/envoy-support-1

rkarthick · 2022-02-14T18:27:44Z

Interesting, the failing testcase (ServerTakesPrecendenceOverSnapshot) is working locally. @NickCraver do you know if SimulateConnectionFailure can be flaky for any reason?

I will also make changes to docker files to test basic envoy connection this week :)

rkarthick · 2022-02-17T17:57:27Z

@NickCraver Envoy doesnt seem to have windows binary, that we could run locally using *.cmd file. Do you have any thoughts on how we can get the environment going in Windows Server? The tests are passing in Ubuntu, except for some random flakiness with others.

ravgupms · 2022-02-17T19:18:37Z

@rkarthick you can download Envoy for Windows from the following link:
https://archive.tetratelabs.io/envoy/download/v1.21.0/envoy-v1.21.0-windows-amd64.tar.xz

NickCraver · 2022-02-18T00:10:18Z

@rkarthick Sorry it's a crazy week this week in particular (bootcamp at work), nice progress here! I think we can use the envoy image instead of installing for better caching/re-use/speed here and only mount our yaml - I think? Is there a reason that approach doesn't work?

I'm not too worried about Windows - can help with skipping tests if we don't have the server as an approach there (based on TestConfig)

rkarthick · 2022-02-18T05:33:42Z

No worries @NickCraver and thank you for the reply.

@rkarthick Sorry it's a crazy week this week in particular (bootcamp at work), nice progress here! I think we can use the envoy image instead of installing for better caching/re-use/speed here and only mount our yaml - I think? Is there a reason that approach doesn't work?

The usage of envoy proxy docker container within docker-compose will involve the following changes:

Make redis in docker-compose.yaml use a fixed ipv4_address
Use that ipv4_address in the Cluster/node-700*.conf files instead of 127.0.0.1
Use the ipv4_address in envoy.yaml

This is because envoy redis cluster instance discovery relies on cluster slots redis command. cluster slots command returns the IP address in nodes.conf file (hostnames are supported in Redis 7.0, however envoy still has to support this discovery). If the IP address is not reachable (such as 127.0.0.1 from a different envoy container), envoy will not be able to communicate with the redis cluster instance.

Hence I decided to install the envoy within the docker image to make the change minimal. Mostly because I havent worked within sentinel before and wasnt sure if modifying the IP address from 127.0.0.1 in nodes.conf to a fixed ipv4 address will have a side effect in it.

I could potentially make these changes to see if affects sentinel or other testcases. But it would be great to get your thoughts before I go down that route :).

NickCraver · 2022-02-18T22:09:21Z

@rkarthick I think I follow - I agree current is likely the better path, let me get a free evening (been a busy week) to poke at this and get things green. I really appreciate all the effort here, will help get some final polish ideas tested for the build pipeline :)

NickCraver · 2022-02-18T23:18:29Z

I noticed the tests took 10 seconds, then noticed it took 2 seconds....whatever the connect timeout is. Will dig into why, but it's indeed exhausting timeouts:

23:15:10.3019: 127.0.0.1:7015: Starting read
23:15:10.3140: Response from 127.0.0.1:7015/Interactive / EXISTS: Integer: 0
23:15:12.2798: Not all available tasks completed cleanly (from ReconfigureAsync#1760, timeout 2000ms), IOCP: (Busy=0,Free=1000,Min=16,Max=1000), WORKER: (Busy=3,Free=2044,Min=16,Max=2047)

In waiting for the subscriber connection to connect _when it's not supported_, we were being dumb and waiting the full timeout to proceed. So...don't do that, do the right thing.

NickCraver · 2022-02-18T23:51:52Z

Well I found a few issues here:

The command map wasn't right...but it wasn't right for Twemproxy either :)
We were always waiting the full timeout when subscriptions aren't supported at all (via command map) - changed the logic to behave properly.

I'll split the above fixes into their own PR as they are existing bugs in connection logic, but getting there! These tests now run instantly - we should probably add some more command testing but looking good on actually connecting now.

In investigating an issue in #1989, I found a few gaps. Overall: 1. Twemproxy has an out of date CommandMap, which propagated to Envoy. 2. We were expecting both interactive and subscription connections to complete to complete the async handler...but we shouldn't because subscriptions may be disabled. 3. RedisSubscriber changes on the sync path weren't validating the message (asserting the command map has it enabled). This fixes all of the above and adds another test considering all 3.

NickCraver · 2022-02-19T01:09:30Z

Prepping the fixes to the underlying issues in #2001 - I'll merge main in here once it's in place.

rkarthick · 2022-02-19T03:51:02Z

src/StackExchange.Redis/CommandMap.cs


            RedisCommand.SCRIPT,

-            RedisCommand.ECHO, RedisCommand.PING, RedisCommand.QUIT, RedisCommand.SELECT,


Ah, thanks for catching this

I think PING is support by Envoy. https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/other_protocols/redis.html?highlight=PING

rkarthick · 2022-02-19T04:06:11Z

Thank you for making these fixes and catching the subscription functionality issue :). I can add a few more command tests over the weekend. Is there a reason why we removed PING though in envoy. Its supported by envoy.

NickCraver · 2022-02-19T04:11:19Z

Yep, look at the argument name ;) Please check me for the rest though, it definitely needs more eyes and is probably missing more.

In investigating an issue in #1989, I found a few gaps. Overall: 1. Twemproxy has an out of date CommandMap, which propagated to Envoy. 2. We were expecting both interactive and subscription connections to complete to complete the async handler...but we shouldn't because subscriptions may be disabled. 3. RedisSubscriber changes on the sync path weren't validating the message (asserting the command map has it enabled). This fixes all of the above and adds another test considering all 3.

NickCraver · 2022-02-22T13:35:39Z

src/StackExchange.Redis/CommandMap.cs

            RedisCommand.SCRIPT,

-            RedisCommand.ECHO, RedisCommand.PING, RedisCommand.SELECT,
+            RedisCommand.ECHO, RedisCommand.SELECT,


From Nick: Twemproxy supports PING: https://github.com/twitter/twemproxy/blob/master/notes/redis.md#connection

NickCraver

Looking good now - thanks for this! We can tweak the command map if needed :)

rkarthick · 2022-02-22T17:23:54Z

This is great. Thank you for getting this into the master :).

NickCraver · 2022-02-22T20:19:38Z

@rkarthick Thanks for getting this done! I appreciate the iteration as we found existing issues here <3

support for envoyproxy

2028e39

NickCraver requested changes Feb 11, 2022

View reviewed changes

rkarthick and others added 5 commits February 10, 2022 17:25

Update src/StackExchange.Redis/CommandMap.cs

833002d

Co-authored-by: Nick Craver <[email protected]>

Update src/StackExchange.Redis/Enums/ServerType.cs

7a79c0e

Co-authored-by: Nick Craver <[email protected]>

Update src/StackExchange.Redis/Enums/Proxy.cs

04b4e0f

Co-authored-by: Nick Craver <[email protected]>

Update src/StackExchange.Redis/ServerEndPoint.cs

a9ad8a1

Co-authored-by: Nick Craver <[email protected]>

Update src/StackExchange.Redis/ServerSelectionStrategy.cs

11b3a05

Co-authored-by: Nick Craver <[email protected]>

NickCraver reviewed Feb 11, 2022

View reviewed changes

src/StackExchange.Redis/Enums/ServerType.cs Outdated Show resolved Hide resolved

fixed incorrect singleprimary setting for envoy

a34574b

Merge branch 'rkarthick/envoy-support-1' of https://github.com/rkarth…

800a441

…ick/StackExchange.Redis into rkarthick/envoy-support-1

rkarthick added 7 commits February 16, 2022 22:47

added testcases for envoy testing

abacb9c

added missing files

84fbc1a

replace copy with add

cd64621

empty commit

caaae52

renamed envoy file

98728de

fixed config file

39a00dd

empty commit

f5759ec

rkarthick requested a review from NickCraver February 17, 2022 17:41

NickCraver self-assigned this Feb 18, 2022

Simplify test config

b8be69f

Fixing command maps and secondary connection issues

9b6ab09

In waiting for the subscriber connection to connect _when it's not supported_, we were being dumb and waiting the full timeout to proceed. So...don't do that, do the right thing.

Envoy: skip when unavailable

681afbe

NickCraver mentioned this pull request Feb 19, 2022

Misc connection/subscription fixes #2001

Merged

NickCraver added ⚙️ area:connection ➕ enhancement labels Feb 19, 2022

rkarthick commented Feb 19, 2022

View reviewed changes

NickCraver added 2 commits February 22, 2022 08:33

Merge branch 'main' into pr/1989

d6f72e8

Fix merge

23b837b

NickCraver reviewed Feb 22, 2022

View reviewed changes

NickCraver approved these changes Feb 22, 2022

View reviewed changes

NickCraver merged commit 8197e97 into StackExchange:main Feb 22, 2022

NickCraver added a commit that referenced this pull request Feb 22, 2022

Add #1989 to release notes!

7cff161


		RedisCommand.SCRIPT,

		RedisCommand.ECHO, RedisCommand.PING, RedisCommand.QUIT, RedisCommand.SELECT,

support for envoyproxy #1989

support for envoyproxy #1989

Uh oh!

Conversation

rkarthick commented Feb 11, 2022

Uh oh!

NickCraver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkarthick commented Feb 11, 2022 via email

Uh oh!

rkarthick commented Feb 14, 2022

Uh oh!

rkarthick commented Feb 17, 2022

Uh oh!

ravgupms commented Feb 17, 2022

Uh oh!

NickCraver commented Feb 18, 2022

Uh oh!

rkarthick commented Feb 18, 2022

Uh oh!

NickCraver commented Feb 18, 2022

Uh oh!

NickCraver commented Feb 18, 2022

Uh oh!

NickCraver commented Feb 18, 2022

Uh oh!

NickCraver commented Feb 19, 2022

Uh oh!

rkarthick Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

rkarthick Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

rkarthick commented Feb 19, 2022

Uh oh!

NickCraver commented Feb 19, 2022

Uh oh!

NickCraver Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

NickCraver left a comment

Choose a reason for hiding this comment

Uh oh!

rkarthick commented Feb 22, 2022

Uh oh!

NickCraver commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants