Using map to store the listener in ConnectionHandlerImpl by soulxu · Pull Request #19362 · envoyproxy/envoy

soulxu · 2021-12-29T07:10:07Z

Commit Message: Using map to store the listener in ConnectionHandlerImpl
Additional Description:
Risk Level: low
Testing: unittest
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: no

Signed-off-by: He Jie Xu hejie.xu@intel.com

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-12-29T12:24:44Z

test/server/connection_handler_test.cc

  EXPECT_CALL(*access_log_, log(_, _, _, _));
 }

-TEST_F(ConnectionHandlerTest, OldBehaviorMatchFirstWildcardListener) {


A note for the reviewer, the original test ensure the first listener matched since the original code using the list. After using the unorder map, so this behavior changed. I assume the MatchFirst isn't part of API contract, so I remove it.

according to this PR #16914, there should be no one depends on the matching order

source/server/connection_handler_impl.cc

wbpcode · 2021-12-30T03:31:19Z

Thanks. It's great and added some comments. cc @rojkov

wbpcode · 2021-12-30T03:31:28Z

/assign

soulxu · 2021-12-30T03:44:49Z

@wbpcode thanks for the review!

wbpcode · 2021-12-30T06:32:04Z

In addition, we may need distinguish the listener of different protocols. Check #17414.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-01-03T13:21:48Z

In addition, we may need distinguish the listener of different protocols. Check #17414.

thanks for another good point!

soulxu · 2022-01-03T13:24:56Z

source/server/connection_handler_impl.cc

 }

 Network::InternalListenerOptRef
 ConnectionHandlerImpl::findByAddress(const Network::Address::InstanceConstSharedPtr& address) {


I'm thinking to change this method name as findInternalListenerByAddress, since the assertion as below, this method is only used for internal listener.

soulxu · 2022-01-03T13:25:32Z

source/server/connection_handler_impl.cc

@@ -224,58 +242,46 @@ ConnectionHandlerImpl::getBalancedHandlerByTag(uint64_t listener_tag) {

 Network::BalancedConnectionHandlerOptRef
 ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Instance& address) {


also think about change this method name to getBalancedTCPHandlerByAddress, since this method is only used for TCP listener

jmarantz · 2022-01-04T14:36:38Z

After @wbpcode approves I'll assign to a maintainer for the next pass. Thanks!

wbpcode

This looks good for me overall. And just some minor comments.

source/server/connection_handler_impl.cc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-01-06T07:09:30Z

This looks good for me overall. And just some minor comments.

thanks for the review!

soulxu · 2022-01-10T02:57:20Z

@wbpcode appreciate if you can take a look at again!

wbpcode

Thanks. LGTM. cc @jmarantz @envoyproxy/envoy-maintainers

Some nit comments. But they are not worth blocking the review.

source/server/connection_handler_impl.cc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway

This looks really good. A few questions, and some comments to add. Thanks for fixing this scalability bottleneck!

/wait

source/server/connection_handler_impl.cc

ggreenway · 2022-01-20T16:16:03Z

source/server/connection_handler_impl.cc

-    if (listener.second.listener_->listenerTag() == listener_tag) {
-      listener.second.listener_->shutdownListener();
+  if (auto iter = listener_map_by_tag_.find(listener_tag); iter != listener_map_by_tag_.end()) {
+    if (iter->second->listener_->listener() != nullptr) {


How can it be in this map, and have a nullptr listener?

We will stop the listener before draining it, the shutdownListener() https://github.com/envoyproxy/envoy/pull/19362/files#diff-fe538327af200510b78cadc759f2c3af989c822a370d3f8a005b92bc59568aa5R153 will reset the pointer. Then it is going to be nullptr. That means if we shutdown twice, then the second time you will see a nullptr.

source/server/connection_handler_impl.cc

test/server/connection_handler_test.cc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-01-24T11:13:54Z

@ggreenway thanks for the review!

I addressed your comments except for #19362 (comment), but let me know if you expect the assertion used there.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway

This looks good. Thanks!

howardjohn · 2022-02-03T15:07:15Z

When upgrading our Istio test suites to this version everything started going a little wonky. I am not 100% sure what's happening but it looks like listeners are getting mixed up - calls are going to the wrong listeners as far as I can tell This is happening with bind_to_port=false. We were able to isolate the regression to this commit.

cc @lambdai

mattklein123 · 2022-02-03T16:08:38Z

It wouldn't surprise me if with bind_to_port = false we now have some ordering variability or something else. Feel free to revert this PR until we sort that out. cc @soulxu

soulxu · 2022-02-03T16:21:00Z

I will do some tests on bind_to_port = false case. Then let you know when I figure out what happened.

soulxu · 2022-02-03T16:44:01Z

When upgrading our Istio test suites to this version everything started going a little wonky. I am not 100% sure what's happening but it looks like listeners are getting mixed up - calls are going to the wrong listeners as far as I can tell This is happening with bind_to_port=false. We were able to isolate the regression to this commit.

cc @lambdai

@howardjohn do you have more failure info, or which testcase are failing. I checked the test, we have few tests on bind_to_port = false case

envoy/test/server/connection_handler_test.cc

Line 959 in a844baf

TEST_F(ConnectionHandlerTest, MatchIPv6WildcardListener) {

envoy/test/server/connection_handler_test.cc

Line 771 in a844baf

TEST_F(ConnectionHandlerTest, MatchLatestListener) {

I only thought the 0 port address can be a problem. But that shouldn't the case, since I can't create multiple 0 port addresses with bind_to_port as false.

howardjohn · 2022-02-03T16:45:45Z

Not Envoy tests, but Istio tests. It's pretty much all of our tests ( istio/istio#37004) since it seems all requests go to random listeners

…

On Thu, Feb 3, 2022 at 8:44 AM xuhj ***@***.***> wrote: When upgrading our Istio test suites to this version everything started going a little wonky. I am not 100% sure what's happening but it looks like listeners are getting mixed up - calls are going to the wrong listeners as far as I can tell This is happening with bind_to_port=false. We were able to isolate the regression to this commit. cc @lambdai <https://github.com/lambdai> @howardjohn <https://github.com/howardjohn> do you have more failure info, or which testcase are failing. I checked the test, we have few tests on bind_to_port = false case https://github.com/envoyproxy/envoy/blob/a844bafc3130667f79fc1846a2e4b25831bbb5c8/test/server/connection_handler_test.cc#L959 https://github.com/envoyproxy/envoy/blob/a844bafc3130667f79fc1846a2e4b25831bbb5c8/test/server/connection_handler_test.cc#L771 I only thought the 0 port address can be a problem. But that shouldn't the case, since I can't create multiple 0 port addresses with bind_to_port as false. — Reply to this email directly, view it on GitHub <#19362 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXKPSFOXMUHPNQFVRW3UZKWF3ANCNFSM5K5NLPWQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

soulxu · 2022-02-03T17:09:05Z

I could be wrong here

envoy/source/server/connection_handler_impl.cc

Lines 245 to 249 in a844baf

    
           if (auto listener_it = tcp_listener_map_by_address_.find(address.asStringView()); 
        
               listener_it != tcp_listener_map_by_address_.end()) { 
        
             return Network::BalancedConnectionHandlerOptRef( 
        
                 listener_it->second->tcpListener().value().get()); 
        
           }

I should check listener_it->second->listener_->listener() != nullptr, otherwise a draining listener could get new connection balanced to.

…oyproxy#19362)" This reverts commit 67c2b72. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

…19362) Switch from list to map to avoid config load performance issues with large numbers of listeners. Signed-off-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: Josh Perry <josh.perry@mx.com>

Using map for connection handler

5529319

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu closed this Dec 29, 2021

soulxu added 4 commits December 29, 2021 09:15

fix test

c492427

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix

f6b5a35

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix

ef4ea00

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix format

6d0f930

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu reopened this Dec 29, 2021

soulxu requested a review from alyssawilk as a code owner December 29, 2021 12:06

soulxu commented Dec 29, 2021

View reviewed changes

wbpcode reviewed Dec 30, 2021

View reviewed changes

source/server/connection_handler_impl.cc Outdated Show resolved Hide resolved

wbpcode reviewed Dec 30, 2021

View reviewed changes

source/server/connection_handler_impl.cc Outdated Show resolved Hide resolved

wbpcode reviewed Dec 30, 2021

View reviewed changes

source/server/connection_handler_impl.cc Outdated Show resolved Hide resolved

wbpcode reviewed Dec 30, 2021

View reviewed changes

source/server/connection_handler_impl.cc Outdated Show resolved Hide resolved

wbpcode reviewed Dec 30, 2021

View reviewed changes

source/server/connection_handler_impl.cc Outdated Show resolved Hide resolved

wbpcode reviewed Dec 30, 2021

View reviewed changes

source/server/connection_handler_impl.cc Outdated Show resolved Hide resolved

wbpcode reviewed Dec 30, 2021

View reviewed changes

source/server/connection_handler_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot assigned wbpcode Dec 30, 2021

soulxu added 2 commits January 3, 2022 13:15

Fix the map

a0907ca

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

using stringview

285a2dc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu commented Jan 3, 2022

View reviewed changes

wbpcode reviewed Jan 4, 2022

View reviewed changes

address comment

0ad3d0d

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu mentioned this pull request Jan 7, 2022

listener: support multiple addresses #19367

Closed

wbpcode previously approved these changes Jan 10, 2022

View reviewed changes

source/server/connection_handler_impl.cc Show resolved Hide resolved

source/server/connection_handler_impl.cc Outdated Show resolved Hide resolved

source/server/connection_handler_impl.cc Outdated Show resolved Hide resolved

alyssawilk assigned ggreenway Jan 10, 2022

alyssawilk added the waiting label Jan 13, 2022

Add more tests

3085f7a

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu dismissed wbpcode’s stale review via 3085f7a January 14, 2022 04:44

repokitteh-read-only bot removed the waiting label Jan 14, 2022

address comments

7cfd406

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway requested changes Jan 20, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 20, 2022

address comments

a2eee35

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jan 24, 2022

soulxu added 2 commits January 24, 2022 15:40

fix the comment

57f33a3

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Merge branch 'main' into using_map_for_conn_handler

9fd3884

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway approved these changes Jan 24, 2022

View reviewed changes

ggreenway enabled auto-merge (squash) January 24, 2022 18:08

ggreenway merged commit 67c2b72 into envoyproxy:main Jan 24, 2022

soulxu mentioned this pull request Feb 3, 2022

Fix ConnectionHandlerImpl matching correct any address #19802

Merged

soulxu mentioned this pull request Feb 4, 2022

Revert "Using map to store the listener in ConnectionHandlerImpl" #19811

Closed

soulxu added a commit to soulxu/envoy that referenced this pull request Feb 4, 2022

Revert "Using map to store the listener in ConnectionHandlerImpl (env…

2d535ed

…oyproxy#19362)" This reverts commit 67c2b72. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

		@@ -224,58 +242,46 @@ ConnectionHandlerImpl::getBalancedHandlerByTag(uint64_t listener_tag) {

		Network::BalancedConnectionHandlerOptRef
		ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Instance& address) {

Conversation

soulxu commented Dec 29, 2021

Uh oh!

soulxu Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wbpcode commented Dec 30, 2021

Uh oh!

wbpcode commented Dec 30, 2021

Uh oh!

soulxu commented Dec 30, 2021

Uh oh!

wbpcode commented Dec 30, 2021

Uh oh!

soulxu commented Jan 3, 2022

Uh oh!

soulxu Jan 3, 2022

Choose a reason for hiding this comment

Uh oh!

soulxu Jan 3, 2022

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Jan 4, 2022

Uh oh!

wbpcode 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!

soulxu commented Jan 6, 2022

Uh oh!

soulxu commented Jan 10, 2022

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggreenway Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

soulxu Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soulxu commented Jan 24, 2022

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

howardjohn commented Feb 3, 2022

soulxu Dec 29, 2021 •

edited

Loading

soulxu Dec 30, 2021 •

edited

Loading

soulxu commented Feb 3, 2022 •

edited

Loading