redis: added support for transactions (#22162)

[asheryerm]

Signed-off-by: asheryer [email protected]

Commit Message: redis: added support for transactions (#22162)
Additional Description:
This has been a feature requested by several large clients who would like to work with envoy+elasticache.
The lack of support for transactions in envoy has prevented them from migrating to envoy thus for.
For certain users, redis transactions are a vital part of their workflow/s.

I implemented transactions by opening a new connection to the primary host of the shard that handles the transaction.
A new connection is needed to prevent contamination of the transaction by other clients.
Once the transaction is over the connection is closed.

In more detail:
When a MULTI command is sent, we mark down that this client has started a transaction and we reply locally with an "OK". We then wait for a command with key KEY, and once it arrives we:

1. Establish a connection with the primary host of the shard that handles KEY.
2. Send a MULTI command on the new connection and ignore the response.
3. Only simple commands (that hash to one slot) can be used within a transaction.
4. Send all commands of the transaction on this connection.
5. Redirection is disabled.
6. When the transaction is terminated with EXEC or DISCARD we close the connection after sending the last command.

Edge cases such as nested MULTI, EXEC without MULTI, etc. are all handled.

As far as the user is concerned, the user experience is almost identical to working with a standalone Redis node.

Risk Level: Medium
Testing: Tested on standalone/cluster/elasticache
Docs Changes: changelog
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[jmarantz]

looks like formatting is still broken.

/wait

[jmarantz]

/wait

[asheryerm]

Working now on fixing the tests

[bpo]

@asheryerm Glad to see you working on this. Did you look into accumulating the commands between multi and exec in a buffer before sending, thereby skipping the need to establish a dedicated connection per-transaction? Just curious if so and what the tradeoffs were versus this design.

[asheryerm]

@bpo Hi Brian, yes, there are two viable options:

1. Accumulating the commands and pipelining them all when EXEC is called.
2. Opening a dedicated connection.

I chose option (2) for two reasons:
a. Redis detects certain errors in the transaction before EXEC is called. Using a dedicated connection allows you to receive feedback from Redis for each command in the transaction, and provides a better user experience.
b. Assuming we accumulate all the commands and then pipeline them all to Redis, it is unclear if I can get Envoy to do this atomically without other clients interleaving commands between the pipelined commands (in the scenario where the pipelining requires more than one packet). This may be possible, but I wasn't 100% sure about it.

[asheryerm]

Hi @asheryerm, could you please check what will happen if the Redis node closes the connection and the client continues to send commands for the transaction? Wanted to make sure that we won't reference an object/pointer after it's destroyed.

Hey @weisisea I tested this by connecting envoy to a redis-server and then killing the server in the middle of the transaction:
I added printouts in the logs to see what is happening. Once the server is killed it leads to the ProxyFilter being destroyed, which destroys the transaction and closes any open connections
proxy_1 | [2022-09-26 13:57:48.331][33][info][redis] [source/extensions/filters/network/redis_proxy/proxy_filter.cc:95] ASHER: Received closing event proxy_1 | [2022-09-26 13:57:48.331][33][info][redis] [source/extensions/filters/network/redis_proxy/proxy_filter.cc:67] ASHER: In ProxyFilter::~ProxyFilter proxy_1 | [2022-09-26 13:57:54.568][16][info][redis] [source/extensions/filters/network/redis_proxy/proxy_filter.cc:95] ASHER: Received closing event proxy_1 | [2022-09-26 13:57:54.568][16][info][redis] [source/extensions/filters/network/redis_proxy/proxy_filter.cc:67] ASHER: In ProxyFilter::~ProxyFilter

If the client continues sending commands past this point they will receive an upstream failure

[weisisea]

Hi @asheryerm, could you please check what will happen if the Redis node closes the connection and the client continues to send commands for the transaction? Wanted to make sure that we won't reference an object/pointer after it's destroyed.

Hey @weisisea I tested this by connecting envoy to a redis-server and then killing the server in the middle of the transaction: I added printouts in the logs to see what is happening. Once the server is killed it leads to the ProxyFilter being destroyed, which destroys the transaction and closes any open connections proxy_1 | [2022-09-26 13:57:48.331][33][info][redis] [source/extensions/filters/network/redis_proxy/proxy_filter.cc:95] ASHER: Received closing event proxy_1 | [2022-09-26 13:57:48.331][33][info][redis] [source/extensions/filters/network/redis_proxy/proxy_filter.cc:67] ASHER: In ProxyFilter::~ProxyFilter proxy_1 | [2022-09-26 13:57:54.568][16][info][redis] [source/extensions/filters/network/redis_proxy/proxy_filter.cc:95] ASHER: Received closing event proxy_1 | [2022-09-26 13:57:54.568][16][info][redis] [source/extensions/filters/network/redis_proxy/proxy_filter.cc:67] ASHER: In ProxyFilter::~ProxyFilter

If the client continues sending commands past this point they will receive an upstream failure

Closing the downstream connection (ProxyFilter object) upon the dedicated connection being closed sounds good to me since we probably don't want the rest of the commands to be run outside of the transaction. Could you please elaborate more on how closing the downstream connection is achieved?

[weisisea]

A follow-up question, is there a plan on incorporating WATCH command support as well to complete the whole transaction story for Redis? Thanks!

[weisisea]

Hi @mattklein123, the changes look good to me. Do you want to take a pass at it?

[mattklein123]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #22729 (comment) was created by @mattklein123.

see: more, trace.

[asheryerm]

Closing the downstream connection (ProxyFilter object) upon the dedicated connection being closed sounds good to me since we probably don't want the rest of the commands to be run outside of the transaction. Could you please elaborate more on how closing the downstream connection is achieved?

When the dedicated connection is closed the dispatcher sends a close event to the dedicated transaction client. However, the dedicated client uses the parent ProxyFilter for the handling of its connection callbacks, so this leads to a termination of the ProxyFilter.

[asheryerm]

A follow-up question, is there a plan on incorporating WATCH command support as well to complete the whole transaction story for Redis? Thanks!

If this current PR is merged and you are more or less happy with the way I did it, then I can definitely look into supporting WATCH as well. It will probably be a smaller change once we have transaction support in place.

[weisisea]

A follow-up question, is there a plan on incorporating WATCH command support as well to complete the whole transaction story for Redis? Thanks!

If this current PR is merged and you are more or less happy with the way I did it, then I can definitely look into supporting WATCH as well. It will probably be a smaller change once we have transaction support in place.

Great, thanks!

[mattklein123]

Code LGTM. One small documentation request. Thank you!

/wait

[mattklein123]

Thanks!