redis: upstream client draining and temp host connection limit and cl…

[msukalski]

…eanup

Signed-off-by: Mitch Sukalski mitch.sukalski@workday.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:

Upstream connection draining has been implemented so cluster topology changes do not cause unnecessary errors for in-flight redis requests. A new connection pool setting, upstream_drain_poll_interval (default 100ms), has been created to control the polling interval to check if draining connections are still active once they've been put aside to drain. Another connection pool setting, max_upstream_unknown_connections (default 100), controls how many connections a worker thread will open to upstream servers that it learns about from redis redirection errors. When hosts are discovered dynamically (e.g., RedisCluster), connections created via redirection are put aside to drain.

Risk Level: low
Testing: unit tests
Docs Changes: new connection pool settings autogenerated from redis_proxy.proto comments
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

[msukalski]

I'm going to add 2 more tests to get full coverage.

[mattklein123]

@HenryYYang for first pass. cc @envoyproxy/redis-dev

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[msukalski]

@HenryYYang this PR has gone stale, help!

[HenryYYang]

Thanks for fixing this, a few questions.

[HenryYYang]

Some small nits and adding stats so that we can track it. Then I think we're good.

[msukalski]

tsan check failed due to some sort of network problem or crash -- I'll try again later with the latest upstream code merged...

[msukalski]

@HenryYYang changes up and all CI checks passed...

[HenryYYang]

LGTM

[mattklein123]

Thanks, some high level questions to get started with before I dig into the code in detail. (Also needs a master merge.)

/wait-any

[msukalski]

@mattklein123 I'll work on the changes, especially RedisCluster specific statistics, and get them up when I figure it out. Thanks!

[msukalski]

ipv6_tests failed due to download failure of six_archive python package; tsan failed due to 1 failing test -- //test/extensions/transport_sockets/tls:ssl_socket_test. I'll have to look into the tsan test failure, but this is not code that I've modified...

[msukalski]

envoy-macos build failed with error: The agent: Hosted macOS 3 lost communication with the server. Verify the machine is running and has a healthy network connection.

//test/extensions/transport_sockets/tls:ssl_socket_test passes locally for me in 22 seconds...

[msukalski]

coverage failed due to g++ internal compiler error compiling test/integration/protocol_integration_test.cc ... will try again with the same code since this all passed earlier yesterday

[msukalski]

now clang-tidy is failing with

ERROR: An error occurred during the fetch of repository 'com_github_pallets_markupsafe':
java.io.IOException: Error downloading [https://github.com/pallets/markupsafe/archive/1.1.1.tar.gz] to /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/external/com_github_pallets_markupsafe/1.1.1.tar.gz: Read timed out

This CI environment is incredibly unstable, and now infuriating. I've spent most of 2 days trying to get a clean CI run with no code changes needed. It's just been a parade of build problems...

[msukalski]

@mattklein123 it doesn't look like it's going to be easy to get a clean CI check. I'm not going to try again until you've had a chance for further review. It's a waste of my time, if I'm going to have to make more changes. Thanks!

[mattklein123]

LGTM with small comment. Thank you!

/wait

[mattklein123]

LGTM, thanks with one small test comment.

/wait

[repokitteh-read-only]

🙀 Error while processing event:

evaluation error
error: cannot load github.com/repokitteh/modules/lib/circleci.star: load("github.com/repokitteh/modules/lib/circleci.star") error: module load error

🐱

Caused by: a #7104 (review) was submitted by @mattklein123.

see: more, trace.

[msukalski]

I'll add a test of the new statistic to the MakeRequestToHostWithZeroMaxUnknownUpstreamConnectionLimit test in conn_pool_impl_test.cc

[mattklein123]

@msukalski can you also merge master? There are some CI flake fixes, etc. Thanks.

/wait

[msukalski]

I've merged the latest upstream master. compile_time_options and tsan have both failed with build "connect timed out" errors pulling down external packages....sigh

[mattklein123]

Thanks!