Skip to content

http2: drain only once when reached max_requests_per_connection#19078

Merged
RyanTheOptimist merged 3 commits intoenvoyproxy:mainfrom
YaoZengzeng:max-requests
Nov 29, 2021
Merged

http2: drain only once when reached max_requests_per_connection#19078
RyanTheOptimist merged 3 commits intoenvoyproxy:mainfrom
YaoZengzeng:max-requests

Conversation

@YaoZengzeng
Copy link
Copy Markdown
Member

@YaoZengzeng YaoZengzeng commented Nov 22, 2021

Commit Message: drain only once when reached max_requests_per_connection
Additional Description: fixes #19045
Risk Level: low
Testing: unit test
Docs Changes: n/a

@YaoZengzeng
http: drain only once when reached max_requests_per_connection
93878fc 
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Nov 22, 2021

YaoZengzeng is not allowed to assign users.

🐱

Caused by: a #19078 (comment) was created by @YaoZengzeng.

see: more, trace.

@YaoZengzeng
Copy link
Copy Markdown
Member Author

YaoZengzeng commented Nov 22, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Nov 22, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19078 (comment) was created by @YaoZengzeng.

see: more, trace.

@RyanTheOptimist
Copy link
Copy Markdown
Contributor

RyanTheOptimist commented Nov 22, 2021

/assign @RyanTheOptimist

RyanTheOptimist
RyanTheOptimist reviewed Nov 22, 2021
View reviewed changes
test/common/http/conn_manager_impl_test_2.cc
}

// Regression test for https://github.com/envoyproxy/envoy/issues/19045
TEST_F(HttpConnectionManagerImplTest, MaxRequests) {
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test! Just to make sure I'm clear, does this test fail without the changes to source/common/http/conn_manager_impl.cc? (I assume it does)

Copy link
Copy Markdown
Member Author

@YaoZengzeng YaoZengzeng Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would crash actually, because function startDrainSequence() will be called multiple times, ref: https://github.com/envoyproxy/envoy/blob/main/source/common/http/conn_manager_impl.cc#L1162

Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

RyanTheOptimist
RyanTheOptimist previously approved these changes Nov 23, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/assign @alyssawilk

@RyanTheOptimist
Copy link
Copy Markdown
Contributor

RyanTheOptimist commented Nov 23, 2021

/assign @alyssawilk

@kfaseela
Copy link
Copy Markdown
Contributor

kfaseela commented Nov 23, 2021

Thanks for the fix @YaoZengzeng

alyssawilk
alyssawilk reviewed Nov 23, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick debugging and fix!
The one thing I'd request is documenting this in docs/root/version_history/current.rst as a behavioral change. I think we don't need to runtime guard because if folks relied on this behavior, they can up the (long standing) drain timeout on their instances, but we should give them a heads up of the behavioral change.
LGTM modulo that addition

@YaoZengzeng
add a behavior change
97cb233 
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
@YaoZengzeng
alphabetical order
22fc8fd 
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
@YaoZengzeng
Copy link
Copy Markdown
Member Author

YaoZengzeng commented Nov 24, 2021

@alyssawilk udpated!

alyssawilk
alyssawilk approved these changes Nov 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. thanks for the fix and apologies for the delay (was out most of last week)

@YaoZengzeng
Copy link
Copy Markdown
Member Author

YaoZengzeng commented Nov 29, 2021

@alyssawilk thanks for your review:) So this PR could get merged?

@RyanTheOptimist RyanTheOptimist merged commit b432368 into envoyproxy:main Nov 29, 2021
mpuncel added a commit to mpuncel/envoy that referenced this pull request Nov 30, 2021
@mpuncel
Merge branch 'main' into mpuncel/hc-after-sds-init-manager
09ddc81 
* main: (77 commits)
  Fix verify_and_print_latest_release logic (envoyproxy#19111)
  http2: drain only once when reached max_requests_per_connection (envoyproxy#19078)
  Overload: Reset H2 server stream only use codec level reset mechanism (envoyproxy#18895)
  Update QUICHE from c2ddf95dc to 7f2d442e3 (envoyproxy#19095)
  tools: Fix dependency checker release dates bug (envoyproxy#19109)
  cve_scan: Use `envoy.dependency.cve_scan` (envoyproxy#19047)
  tcp: fix overenthusiastic bounds on the new pool (envoyproxy#19036)
  dep: update Proxy-Wasm C++ host (2021-11-18). (envoyproxy#19074)
  build(deps): bump frozendict from 2.0.7 to 2.1.0 in /tools/base (envoyproxy#19080)
  kafka: dependency upgrades (envoyproxy#18995)
  build(deps): bump charset-normalizer in /tools/dependency (envoyproxy#19105)
  build(deps): bump slack-sdk in /.github/actions/pr_notifier (envoyproxy#19093)
  dep: Remove dependency - six (envoyproxy#19085)
  Remove requested_server_name_ field from StreamInfo (envoyproxy#19102)
  broken link path fix for items http_filters/grpc_json_transcoder_filter (envoyproxy#19101)
  quic: turn off GRO (envoyproxy#19088)
  Listener: Add global conn limit opt out. (envoyproxy#18876)
  Specify type for matching Subject Alternative Name. (envoyproxy#18628)
  Fix a broken example in Lua filter docs (envoyproxy#19086)
  Fix a small typo (envoyproxy#19058)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
jiangshantao-dbg pushed a commit to istio-mt/envoy that referenced this pull request Jun 21, 2022
@YaoZengzeng
http2: drain only once when reached max_requests_per_connection (envo…
17f836b 
…yproxy#19078)

Commit Message: drain only once when reached max_requests_per_connection
Additional Description: fixes envoyproxy#19045
Risk Level: low
Testing: unit test
Docs Changes: n/a

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
jiangshantao-dbg added a commit to istio-mt/envoy that referenced this pull request Jun 22, 2022
@jiangshantao-dbg
http2: drain only once when reached max_requests_per_connection (envo…
52a65ab 
…yproxy#19078) (#14)

Commit Message: drain only once when reached max_requests_per_connection
Additional Description: fixes envoyproxy#19045
Risk Level: low
Testing: unit test
Docs Changes: n/a

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RyanTheOptimist RyanTheOptimist RyanTheOptimist left review comments

+2 more reviewers

@kfaseela kfaseela kfaseela approved these changes

@alyssawilk alyssawilk alyssawilk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alyssawilk alyssawilk

@RyanTheOptimist RyanTheOptimist

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

HttpConnectionManager max_requests_per_connection behaving different from the documented behaviour

4 participants

@YaoZengzeng @RyanTheOptimist @kfaseela @alyssawilk