Cleanup idle connection pools (disabled by default)#17403
Merged
ggreenway merged 16 commits intoenvoyproxy:mainfrom Jul 26, 2021
Merged
Cleanup idle connection pools (disabled by default)#17403ggreenway merged 16 commits intoenvoyproxy:mainfrom
ggreenway merged 16 commits intoenvoyproxy:mainfrom
Conversation
Delete connection pools when they have no connections anymore. This fixes unbounded memory use for cases where a new connection pool is needed for each downstream connection, such as when using upstream PROXY protocol. Fixes envoyproxy#16682 This reverts commit b7bc539. This reverts PR envoyproxy#17319, by re-adding envoyproxy#17302 and envoyproxy#16948. Signed-off-by: Greg Greenway <ggreenway@apple.com> Co-authored-by: Craig Radcliffe <craig.radcliffe@broadcom.com>
Member
Author
|
@bianpengyuan were you able to figure out any more information about the crash you saw here? |
Member
Author
|
/retest |
|
Retrying Azure Pipelines: |
alyssawilk
reviewed
Jul 20, 2021
Contributor
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
one high level comment first - what do you think of landing this default off, making sure it sticks, and then flipping it on after a week or so? If we've verified that the runtime guard works we can simply roll back the runtime default if there are other issues discovered.
fix test failures add new integration test Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
avoid tsan issues. Signed-off-by: Greg Greenway <ggreenway@apple.com>
alyssawilk
reviewed
Jul 22, 2021
Contributor
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Looks pretty solid, though (I'll be the first to admit it's possible there are more bugs that I wouldn't catch with visual inspection =P)
Signed-off-by: Greg Greenway <ggreenway@apple.com>
…cleanup Signed-off-by: Greg Greenway <ggreenway@apple.com>
ggreenway
changed the title
ggreenway
changed the title
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Member
|
We can happily smoke test this, if that's still needed (given it's now disabled by default). |
alyssawilk
approved these changes
Jul 26, 2021
ggreenway
added a commit
to ggreenway/envoy
that referenced
this pull request
Jul 28, 2021
The idle callback was incorrectly going through the ThreadLocalClusterManagerImpl::ClusterEntry to reach the ThreadLocalClusterManager. The pool is owned by the ThreadLocalClusterManager, not by the ClusterEntry, so the reference to the ClusterEntry could be dangling, resulting in a crash. fixes commit envoyproxy#17403 Signed-off-by: Greg Greenway <ggreenway@apple.com>
ggreenway
added a commit
that referenced
this pull request
Jul 28, 2021
#17522) The idle callback was incorrectly going through the ThreadLocalClusterManagerImpl::ClusterEntry to reach the ThreadLocalClusterManager. The pool is owned by the ThreadLocalClusterManager, not by the ClusterEntry, so the reference to the ClusterEntry could be dangling, resulting in a crash. fixes commit #17403 Signed-off-by: Greg Greenway <ggreenway@apple.com>
leyao-daily
pushed a commit
to leyao-daily/envoy
that referenced
this pull request
Sep 30, 2021
Delete connection pools when they have no connections anymore. This fixes unbounded memory use for cases where a new connection pool is needed for each downstream connection, such as when using upstream PROXY protocol. This reverts commit b7bc539. This reverts PR envoyproxy#17319, by re-adding envoyproxy#17302 and envoyproxy#16948. Signed-off-by: Greg Greenway <ggreenway@apple.com> Co-authored-by: Craig Radcliffe <craig.radcliffe@broadcom.com>
leyao-daily
pushed a commit
to leyao-daily/envoy
that referenced
this pull request
Sep 30, 2021
envoyproxy#17522) The idle callback was incorrectly going through the ThreadLocalClusterManagerImpl::ClusterEntry to reach the ThreadLocalClusterManager. The pool is owned by the ThreadLocalClusterManager, not by the ClusterEntry, so the reference to the ClusterEntry could be dangling, resulting in a crash. fixes commit envoyproxy#17403 Signed-off-by: Greg Greenway <ggreenway@apple.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.
Delete connection pools when they have no connections anymore. This
fixes unbounded memory use for cases where a new connection pool is
needed for each downstream connection, such as when using upstream
PROXY protocol.
Fixes #16682
This reverts commit b7bc539.
This reverts PR #17319, by re-adding #17302 and #16948.
Signed-off-by: Greg Greenway ggreenway@apple.com
Co-authored-by: Craig Radcliffe craig.radcliffe@broadcom.com
Risk Level: High! Connection pools used to live until the cluster was destroyed. If anything is caching a pool that now gets deleted sooner, this could crash, corrupt memory, etc.
Testing: Existing and new tests pass
Docs Changes: Updated
Release Notes: Added
Platform Specific Features: None
Runtime guard:
envoy.reloadable_features.conn_pool_delete_when_idle[Optional Deprecated:]
[Optional API Considerations:]