Fix close_connections_on_host_health_failure#11241
Merged
snowp merged 5 commits intoJun 15, 2020
Merged
Conversation
close_connections_on_host_health_failure was disabled by the refactoring to make tcp_proxy use the TCP connection pool implementation. To fix it we: * Add a method to immediately close all active connections on a TCP conn pool. * Invoke this method instead of drainConnections() if close_connections_on_host_health_failure is set. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
Contributor
|
/retest |
|
🤷♀️ nothing to rebuild. |
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 2 pipeline(s). |
snowp
suggested changes
May 19, 2020
Contributor
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks for working on this. Do you think you could come up with an integration test that verifies this behavior? Mostly so we can prevent this kind of regression from happening again.
|
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! |
stale
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Test to show connections remain open if the flag is false. Test to show connections are closed if the flag is true. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
Contributor
Author
|
@snowp Sorry for the delay, I've added some tests to the branch. |
stale
Bot
removed
the
stale
Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
snowp
suggested changes
Jun 12, 2020
Contributor
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks for adding a test! One question
@ggreenway mind taking a look as well?
ggreenway
reviewed
Jun 12, 2020
Member
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
This LGTM, aside from the comment and semicolon
Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
yashwant121
pushed a commit
to yashwant121/envoy
that referenced
this pull request
Jun 24, 2020
close_connections_on_host_health_failure was disabled by the refactoring to make tcp_proxy use the TCP connection pool implementation. To fix it we: * Add a method to immediately close all active connections on a TCP conn pool. * Invoke this method instead of drainConnections() if close_connections_on_host_health_failure is set. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
yashwant121
pushed a commit
to yashwant121/envoy
that referenced
this pull request
Jun 24, 2020
close_connections_on_host_health_failure was disabled by the refactoring to make tcp_proxy use the TCP connection pool implementation. To fix it we: * Add a method to immediately close all active connections on a TCP conn pool. * Invoke this method instead of drainConnections() if close_connections_on_host_health_failure is set. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
yashwant121
pushed a commit
to yashwant121/envoy
that referenced
this pull request
Jun 24, 2020
close_connections_on_host_health_failure was disabled by the refactoring to make tcp_proxy use the TCP connection pool implementation. To fix it we: * Add a method to immediately close all active connections on a TCP conn pool. * Invoke this method instead of drainConnections() if close_connections_on_host_health_failure is set. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
songhu
pushed a commit
to songhu/envoy
that referenced
this pull request
Jun 25, 2020
close_connections_on_host_health_failure was disabled by the refactoring to make tcp_proxy use the TCP connection pool implementation. To fix it we: * Add a method to immediately close all active connections on a TCP conn pool. * Invoke this method instead of drainConnections() if close_connections_on_host_health_failure is set. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
yashwant121
pushed a commit
to yashwant121/envoy
that referenced
this pull request
Jul 24, 2020
close_connections_on_host_health_failure was disabled by the refactoring to make tcp_proxy use the TCP connection pool implementation. To fix it we: * Add a method to immediately close all active connections on a TCP conn pool. * Invoke this method instead of drainConnections() if close_connections_on_host_health_failure is set. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close_connections_on_host_health_failure was disabled by the refactoring to
make tcp_proxy use the TCP connection pool implementation.
To fix it we:
Additional Notes:
The breakage was introduced in the release immediately after the flag was introduced. It has been broken for some time (since v1.8). This fix may present some risk to users who currently have the flag enabled even though it doesn't work as documented, as the observed behaviour will change.
Risk Level: Low
Testing: Manual
Docs Changes: None