health check: close conn after timeout/stream rst when grpc hc has conn reuse disabled #11325
Conversation
|
@lizan Similar to #11324 - I could not reproduce the test-failures using the following commands: ./ci/do_ci.sh 'bazel.release' //test/common/config:subscription_factory_impl_test
# or
bazel test //test/common/config:subscription_factory_impl_testThe test-failure is the same as #11324 and doesn't seem related to the change. I'm not sure if this is a common flake or if I am missing something with my test commands. If it's likely flakey, could you kick off the azure pipeline run again? |
…nn reuse disabled Signed-off-by: Joey Muia <joseph.b.muia@gmail.com>
Signed-off-by: Joey Muia <joseph.b.muia@gmail.com>
jmuia
force-pushed
the
jmuia.grpc-hc-reuse-conn-timeout
branch
from
bc253f9 to
a81e1b9
Compare
Signed-off-by: Joey Muia <joseph.b.muia@gmail.com>
There was a problem hiding this comment.
Thanks this seems reasonable to me, just a few notes on the tests.
In the future it would great if you could refrain from rebasing the PR, as per https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#submitting-a-pr.
| @@ -4487,6 +4487,60 @@ TEST_F(GrpcHealthCheckerImplTest, DontReuseConnectionBetweenChecks) { | |||
| expectHostHealthy(true); | |||
| } | |||
|
|
|||
| TEST_F(GrpcHealthCheckerImplTest, DontReuseConnectionTimeout) { | |||
There was a problem hiding this comment.
Could you add a comment to this test case to make it easier to understand which assertion is verifying that we're not reusing the connection? I think its expectClientCreate but it would be nice with a comment alongside it
| test_sessions_[0]->request_encoder_.stream_.resetStream(Http::StreamResetReason::RemoteReset); | ||
| expectHostHealthy(true); | ||
|
|
||
| expectClientCreate(0); |
Signed-off-by: Joey Muia <joseph.b.muia@gmail.com>
There was a problem hiding this comment.
LGTM.
@mattklein123 mind sanity checking that this is the right thing to do?
…nn reuse disabled (envoyproxy#11325) Close connection after timeout/stream reset when grpc health checker has connection reuse disabled. Signed-off-by: Joey Muia <joseph.b.muia@gmail.com> Signed-off-by: Auni Ahsan <auni@google.com>
…nn reuse disabled (envoyproxy#11325) Close connection after timeout/stream reset when grpc health checker has connection reuse disabled. Signed-off-by: Joey Muia <joseph.b.muia@gmail.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
Commit Message:
Close connection after timeout/stream reset when grpc health checker has connection reuse disabled.
Additional Description:
While investigating, #11324, I noticed that it seems connections aren't closed when there is a timeout or stream reset and connection reuse is disabled.
Risk Level: low/medium
Testing: added unit tests
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Joey Muia joseph.b.muia@gmail.com