http2: allows the capacity to increase with SETTINGS frame by YaoZengzeng · Pull Request #19161 · envoyproxy/envoy

YaoZengzeng · 2021-12-01T12:46:51Z

Signed-off-by: YaoZengzeng [email protected]

Commit Message: allows the capacity of http2 connection to increase with SETTINGS frame
Additional Description: fixes #18880
Risk Level: medium
Testing: unit
Docs Changes: n/a

YaoZengzeng · 2021-12-01T15:34:42Z

@alyssawilk PTAL:)

alyssawilk

Huzzah! Thanks so much for picking up this series of changes. Couple of ideas below
/wait

source/common/conn_pool/conn_pool_base.h

alyssawilk · 2021-12-01T19:24:13Z

source/common/http/conn_pool_base.cc

    // currentUnusedCapacity() to be explicit int32_t
    ASSERT(std::numeric_limits<int32_t>::max() >= old_unused_capacity);
-    concurrent_stream_limit_ = settings.maxConcurrentStreams().value();
+    if (configured_stream_limit_ > settings.maxConcurrentStreams().value()) {


any reason to not set
concurrent_stream_limit_ = std::min(settings.maxConcurrentStreams().value(), configured_stream_limit_) ? I think it's a bit easier to read?

alyssawilk · 2021-12-01T19:26:47Z

source/common/http/conn_pool_base.cc

 void MultiplexedActiveClientBase::onSettings(ReceivedSettings& settings) {
-  if (settings.maxConcurrentStreams().has_value() &&
-      settings.maxConcurrentStreams().value() < concurrent_stream_limit_) {
+  if (settings.maxConcurrentStreams().has_value()) {


I think we should runtime guard this change as described in CONTRIBUTING.md

alyssawilk · 2021-12-01T19:31:27Z

test/common/http/http2/conn_pool_test.cc

  closeAllClients();
 }

+TEST_F(Http2ConnPoolImplTest, IncreaseCapacityWithSettingsFrame) {


it would be good to integration test this as well.
I think you could move TEST_P(QuicHttpIntegrationTest, NoInitialStreams) { to the protocol integration test, and updateConcurrentStreams / sendSettings based on HTTP/3 vs HTTP/3?

If I understand correctly, do you mean move NoInitialStreams to protocol_integration_test.cc and let it test both HTTP2 and HTTP3? It seems a bit complicated, could I open another PR to implement it after dig a little deeper :)

I'd definitely like to make sure we integration test incrementing settings with this change. Alternately you can land this false by default with a TODO to flip true with integration testing, but I don't think adapting the test is too complicated so I think it makes more sense to land the test now. I'm happy to help if you get stuck!

Get it, let me do some research to get familiar with the integration test framework :)

YaoZengzeng · 2021-12-06T14:31:49Z

@alyssawilk updated! But I only write a integration test for http2.

There two issues for NoInitialStreams of http2:

The valid range of max concurrent streams is from 1 to 2147483647 (2^31 - 1) and defaults to 2147483647. Also for http2, client always launch a request first, then get the SETTINGS frame from server.
I don't know how to adjust concurrent streams of nghttp2 like quic does: quic::test::QuicSessionPeer::SetMaxOpenIncomingBidirectionalStreams(...);

alyssawilk · 2021-12-06T16:21:10Z

Thanks for the integration test!
you can change the streams by writing a sendSettings utility for the test upstream, similar to the updateConcurrentStreams in https://github.com/envoyproxy/envoy/pull/18775/files or alternately you can copy the "send a settings frame" from say UpstreamSettingsMaxStreamsAfterGoAway and use it to update the settings on the connection.

YaoZengzeng · 2021-12-27T06:19:29Z

@alyssawilk Already use NoInitialStreams to test both HTTP2 & HTTP3.

What weired is the test cases never fail in my local environment with test command bazel test --config=docker-clang --test_output=streamed //test/integration:multiplexed_upstream_integration_test --cache_test_results=no , but it never succeed in CI.

Have any ideas?

alyssawilk · 2022-01-04T17:15:08Z

hm, not sure. the test/integration/README file has instructions which can help if test failures are due to slow CI machines - you could try that?
/wait

YaoZengzeng · 2022-01-05T10:25:44Z

hm, not sure. the test/integration/README file has instructions which can help if test failures are due to slow CI machines - you could try that? /wait

Still don't know why :(

Is there any way to pass "-l trace" to CI script, so I could get the verbose log from CI output rather than just error output.

alyssawilk · 2022-01-05T20:36:10Z

well you can tweak the CI scripts to do that but honestly I usually use the test mock code to only beef up logging in the test case which fails.

like if you know what part of the test fails you can put that code in
EXPECT_LOG_CONTAINS("debug", "foobareep", {
your statements here
})
like I did over in https://github.com/envoyproxy/envoy/pull/19234/files

YaoZengzeng · 2022-01-11T15:42:03Z

@alyssawilk PTAL! I think the NoInitialStreams for http2 will not fail anymore :)

alyssawilk

Looks great! I think the 2 test failures aren't yours :-)

Just because the logic is finicky I'm going to hold this until the release is cut tomorrow (which will require one more merge since the version history file will change)

alyssawilk · 2022-01-12T19:53:03Z

release is in! one final sync and you should be good to go. Thanks again for your patience here!

Signed-off-by: YaoZengzeng <[email protected]>

YaoZengzeng · 2022-01-13T06:01:46Z

@alyssawilk synced!

alyssawilk

Awesome! Thanks again both for tackling this change and all your patience getting the tests working and code merged!

…y#19161) Commit Message: allows the capacity of http2 connection to increase with SETTINGS frame Additional Description: fixes envoyproxy#18880 Risk Level: medium Testing: unit Docs Changes: n/a Signed-off-by: YaoZengzeng <[email protected]> Signed-off-by: Josh Perry <[email protected]>

ggreenway assigned alyssawilk Dec 1, 2021

alyssawilk reviewed Dec 1, 2021

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Dec 1, 2021

alyssawilk added the waiting label Dec 6, 2021

repokitteh-read-only bot removed the waiting label Dec 22, 2021

YaoZengzeng force-pushed the settings branch 6 times, most recently from 322734a to 21f83e0 Compare December 25, 2021 10:16

repokitteh-read-only bot added waiting and removed waiting labels Jan 4, 2022

YaoZengzeng force-pushed the settings branch from a5fd3ae to 0269822 Compare January 5, 2022 09:41

YaoZengzeng force-pushed the settings branch 4 times, most recently from 5fc69c2 to 401cacb Compare January 6, 2022 13:32

alyssawilk added the waiting label Jan 6, 2022

YaoZengzeng force-pushed the settings branch from 401cacb to 3ad8858 Compare January 7, 2022 03:40

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

YaoZengzeng force-pushed the settings branch from 3ad8858 to 382b503 Compare January 7, 2022 06:57

YaoZengzeng force-pushed the settings branch 6 times, most recently from fd4d950 to 217b9f8 Compare January 8, 2022 06:00

alyssawilk added the waiting label Jan 10, 2022

YaoZengzeng force-pushed the settings branch from 217b9f8 to 56194d1 Compare January 11, 2022 07:18

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

YaoZengzeng force-pushed the settings branch 3 times, most recently from bdc5223 to 316f20f Compare January 11, 2022 14:34

alyssawilk previously approved these changes Jan 11, 2022

View reviewed changes

YaoZengzeng added 7 commits January 13, 2022 09:39

http2: allows the capacity to increase with SETTINGS frame

6e6f85c

Signed-off-by: YaoZengzeng <[email protected]>

spell check

a7ffc07

Signed-off-by: YaoZengzeng <[email protected]>

runtime guard

5d924ff

Signed-off-by: YaoZengzeng <[email protected]>

alphabetical

1668f9f

Signed-off-by: YaoZengzeng <[email protected]>

spelling

3cd8314

Signed-off-by: YaoZengzeng <[email protected]>

integration test

83b9f05

Signed-off-by: YaoZengzeng <[email protected]>

no initial streams

715e15a

Signed-off-by: YaoZengzeng <[email protected]>

YaoZengzeng dismissed alyssawilk’s stale review via 715e15a January 13, 2022 01:59

YaoZengzeng force-pushed the settings branch from 316f20f to 715e15a Compare January 13, 2022 01:59

alyssawilk approved these changes Jan 13, 2022

View reviewed changes

alyssawilk merged commit 86cbd31 into envoyproxy:main Jan 13, 2022

adisuissa mentioned this pull request Jul 19, 2022

envoy_reloadable_features_http2_allow_capacity_increase_by_settings deprecation #22289

Closed

Conversation

YaoZengzeng commented Dec 1, 2021

Uh oh!

YaoZengzeng commented Dec 1, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

YaoZengzeng Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

YaoZengzeng Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

YaoZengzeng commented Dec 6, 2021

Uh oh!

alyssawilk commented Dec 6, 2021

Uh oh!

YaoZengzeng commented Dec 27, 2021

Uh oh!

alyssawilk commented Jan 4, 2022

Uh oh!

YaoZengzeng commented Jan 5, 2022

Uh oh!

alyssawilk commented Jan 5, 2022

Uh oh!

YaoZengzeng commented Jan 11, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jan 12, 2022

Uh oh!

YaoZengzeng commented Jan 13, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants