upstream: add connpool map #5670

klarose · 2019-01-21T17:42:40Z

Description: This introduces a dedicated class -- ConnPoolMap -- to handle the mapping of "key" to connection pool.

The idea here is that very soon (as part of the work for #4128) we will have an unbounded set of mappings, meaning that we need to manage the life-cycle of these mapped connection pools. Rather than try to do so within ClusterManager, it makes sense to push that responsibility into its own class. This PR starts by refactoring ClusterManager's interaction with the mapping so that it happens entirely through the new class. Future PRs will introduce the life-cycle management.

A few things of note:

The class is templated on the key and connection pool type so that it may be easily reused with the tcp connection pool without any changes to it.
The ClusterManager performed a few operations on the entries in the map -- iterating to add callbacks, draining, etc -- that have been refactored into a single operation on the map. This is probably a good thing, but we should make sure that behaviour wasn't changed here. I don't think any has been.
I had to place some accessor methods in from the of the host map because creating the conn pool map now needs access to the Dispatcher, where previously it did not.
I do not defer deletion of any connection pools when the map is deleted. This is consistent with the existing behaviour. I originally had this behaviour, but it failed some of the existing tests, so I backed it out. It's easy to put back in if desired.
I didn't expect this change to be so large. I can break it into two PRs if desired.

Risk Level: High. Integrates into the core upstream behaviour, but I've striven to ensure that no behaviour changed.
Testing: Unit testing, integration testing.

This class allows clients to map keys to connection pools, returning the existing pool if it already exists. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Signed-off-by: Kyle Larose <kyle@agilicus.com>

source/common/upstream/conn_pool_map_impl.h

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2019-01-21T19:55:32Z

/retest

repokitteh-read-only · 2019-01-21T19:55:35Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #5670 (comment) was created by @klarose.

see: more, trace.

klarose · 2019-01-22T20:12:51Z

/retest

repokitteh-read-only · 2019-01-22T20:12:55Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)
🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5670 (comment) was created by @klarose.

see: more, trace.

We were crashing due to the callback firing mid-iteration and clearing the pool. Check for that case and handle it. Signed-off-by: Kyle Larose <kyle@agilicus.com>

source/common/upstream/cluster_manager_impl.cc

klarose · 2019-01-23T21:16:38Z

/retest

repokitteh-read-only · 2019-01-23T21:16:42Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #5670 (comment) was created by @klarose.

see: more, trace.

klarose · 2019-01-29T17:49:34Z

@mattklein123 Any chance you'll be able to take a look at this in the next few days?

Thanks!

mattklein123 · 2019-01-29T23:49:05Z

Yes will do, sorry for the delay.

mattklein123

Thanks and sorry for the review delay. At a high level this looks great. Some comments to get started and then I will take another pass.

/wait

source/common/upstream/cluster_manager_impl.cc

source/common/upstream/cluster_manager_impl.h

source/common/upstream/conn_pool_map_impl.h

source/common/upstream/conn_pool_map.h

source/common/upstream/cluster_manager_impl.cc

Signed-off-by: Kyle Larose <kyle@agilicus.com>

- Fix up comments - Add protection against recursion - Change optional pointer to a reference Signed-off-by: Kyle Larose <kyle@agilicus.com>

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2019-02-01T19:54:15Z

@mattklein123 Changes made! Thanks for the feedback.

mattklein123

Thanks this looks good. A few small comments.

/wait

source/common/common/recursion_checker.h

source/common/upstream/conn_pool_map_impl.h

source/common/upstream/conn_pool_map.h

- General cleanup - Make recursion checker debug only Signed-off-by: Kyle Larose <kyle@agilicus.com>

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2019-02-04T17:53:29Z

/retest

repokitteh-read-only · 2019-02-04T17:53:33Z

🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5670 (comment) was created by @klarose.

see: more, trace.

Signed-off-by: Kyle Larose <kyle@agilicus.com>

mattklein123 · 2019-02-05T05:06:56Z

/retest

repokitteh-read-only · 2019-02-05T05:06:59Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

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

see: more, trace.

klarose · 2019-02-05T13:46:03Z

@mattklein123 Thanks for kicking off the retest. Looks like everything is passing now. I've addressed all your comments.

mattklein123

Looks good, just a few tiny nits then good to go! Thank you!

/wait

source/common/common/BUILD

source/common/common/debug_recursion_checker.h

test/common/upstream/conn_pool_map_impl_test.cc

Signed-off-by: Kyle Larose <kyle@agilicus.com>

mattklein123

Thank you!

This class allows clients to map keys to connection pools, returning the existing pool if it already exists. Signed-off-by: Kyle Larose <kyle@agilicus.com> Signed-off-by: Fred Douglas <fredlas@google.com>

klarose added 5 commits January 18, 2019 15:45

Add ConnPoolMap

a5150d0

This class allows clients to map keys to connection pools, returning the existing pool if it already exists. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Use ConnPoolMap in ClusterManagerImpl for http.

8534755

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Merge branch 'master' into conn_pool_map

2b91fa5

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Remove unnecessary todo

da8544b

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Various bits of cleanup

d2da030

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose commented Jan 21, 2019

View reviewed changes

source/common/upstream/conn_pool_map_impl.h Show resolved Hide resolved

klarose added 2 commits January 21, 2019 13:19

Fix clang errors

1863ff8

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Fix capture in UT

b903bbf

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Do not clear out the pool until finished

4807a5f

We were crashing due to the callback firing mid-iteration and clearing the pool. Check for that case and handle it. Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose commented Jan 23, 2019

View reviewed changes

source/common/upstream/cluster_manager_impl.cc Show resolved Hide resolved

mattklein123 self-assigned this Jan 24, 2019

klarose changed the title ~~[upstream] add connpool map~~ upstream: add connpool map Jan 24, 2019

mattklein123 requested changes Jan 31, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 31, 2019

klarose added 5 commits January 31, 2019 15:47

Merge branch 'master' into conn_pool_map

11dbe57

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Add a helpe class to fail on recursion

85dcf41

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Various code review fixes

6743f7c

- Fix up comments - Add protection against recursion - Change optional pointer to a reference Signed-off-by: Kyle Larose <kyle@agilicus.com>

Code review fixes for cluster_manager_impl

aa18650

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Use flat_hash_map rather than map

c3fc41a

Signed-off-by: Kyle Larose <kyle@agilicus.com>

repokitteh-read-only bot removed the waiting label Feb 1, 2019

klarose added 2 commits February 1, 2019 09:51

Fix typo in comment

f74d8cf

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Fix coverage expect death tests

e5216eb

Signed-off-by: Kyle Larose <kyle@agilicus.com>

mattklein123 requested changes Feb 3, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 3, 2019

donbowman mentioned this pull request Feb 4, 2019

End-to-End IP transparency using Proxy Protocol #4128

Closed

klarose added 3 commits February 4, 2019 11:59

Code review fixes

1b50fd1

- General cleanup - Make recursion checker debug only Signed-off-by: Kyle Larose <kyle@agilicus.com>

Rename recursion_checker to debug_recursion_checker

1112b90

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Merge branch 'master' into conn_pool_map

92afa73

Signed-off-by: Kyle Larose <kyle@agilicus.com>

repokitteh-read-only bot removed the waiting label Feb 4, 2019

Derive from TestBase

25b58b2

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose added 2 commits February 4, 2019 14:47

Don't run assert tests in debug

5ba7ae4

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Merge branch 'master' into conn_pool_map

5d8536e

Signed-off-by: Kyle Larose <kyle@agilicus.com>

mattklein123 requested changes Feb 5, 2019

View reviewed changes

source/common/common/BUILD Outdated Show resolved Hide resolved

source/common/common/debug_recursion_checker.h Outdated Show resolved Hide resolved

test/common/upstream/conn_pool_map_impl_test.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Feb 5, 2019

klarose added 2 commits February 5, 2019 11:56

Nit fixes.

4b6e8ec

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Merge branch 'master' into conn_pool_map

5887fec

Signed-off-by: Kyle Larose <kyle@agilicus.com>

repokitteh-read-only bot removed the waiting label Feb 5, 2019

mattklein123 approved these changes Feb 5, 2019

View reviewed changes

mattklein123 merged commit 6b216b3 into envoyproxy:master Feb 5, 2019

upstream: add connpool map #5670

upstream: add connpool map #5670

Uh oh!

Conversation

klarose commented Jan 21, 2019

Uh oh!

Uh oh!

klarose commented Jan 21, 2019

Uh oh!

repokitteh-read-only bot commented Jan 21, 2019

Uh oh!

klarose commented Jan 22, 2019

Uh oh!

repokitteh-read-only bot commented Jan 22, 2019

Uh oh!

Uh oh!

klarose commented Jan 23, 2019

Uh oh!

repokitteh-read-only bot commented Jan 23, 2019

Uh oh!

klarose commented Jan 29, 2019

Uh oh!

mattklein123 commented Jan 29, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klarose commented Feb 1, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klarose commented Feb 4, 2019

Uh oh!

repokitteh-read-only bot commented Feb 4, 2019

Uh oh!

mattklein123 commented Feb 5, 2019

Uh oh!

repokitteh-read-only bot commented Feb 5, 2019

Uh oh!

klarose commented Feb 5, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants