Listener: Add global conn limit opt out. by wez470 · Pull Request #18876 · envoyproxy/envoy

wez470 · 2021-11-03T01:36:26Z

Commit Message: Add listener global conn limit opt out.
Additional Description: Allows listeners (including the admin) to opt out of the globally set max downstream connections limit. Further info on why this is desired can be read in the linked issue. Individual listeners that have opted out of the global connection limit can still set a local limit using https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/runtime.
Risk Level: small
Testing: function, unit, integration
Docs Changes: Added api docs + mention in overload manager docs
Release Notes:
Fixes #18678

repokitteh-read-only · 2021-11-03T01:36:29Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18876 was opened by wez470.

see: more, trace.

repokitteh-read-only · 2021-11-03T01:36:33Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18876 was opened by wez470.

see: more, trace.

Signed-off-by: Weston Carlson <wez470@gmail.com>

adisuissa

Thanks for working on this.
Left a couple of comments on API and docs.

api/envoy/config/listener/v3/listener.proto

docs/root/configuration/operations/overload_manager/overload_manager.rst

Signed-off-by: Weston Carlson <wez470@gmail.com>

wez470 · 2021-11-08T15:20:47Z

@adisuissa PTAL, thanks.

adisuissa

I just realized there are 3 things in play after this PR:

The global connections# limit (runtime flag)
The per-listener connections# limit (runtime flag)
The proposed ignore global-connections# limit field (for both the admin and general listeners).
It may be somewhat hard to understand which of these override the others.

Question: would it make sense to have just the global and local limits, where the local limit overrides the global, and if set to 0, it is unlimited?
I guess these can also be fields instead of runtime flags.

envoy/event/dispatcher.h

envoy/network/listener.h

envoy/server/configuration.h

wez470 · 2021-11-08T22:15:06Z

Thanks for the comments @adisuissa!

Question: would it make sense to have just the global and local limits, where the local limit overrides the global, and if set to 0, it is unlimited?

This would satisfy the behaviour we want but I believe it's a question more so for maintainers. Making local limits override the global one is a breaking change. Today they can be used in tandem and a listener will be limited if it hits either limit. Further, AFAICT there's no way to set a local limit on the admin listener. So there's no way it could opt out without work to introduce some sort of local limit setting there.

Signed-off-by: Weston Carlson <wez470@gmail.com>

yanavlasov · 2021-11-10T15:25:45Z

@wez470 would you be able to

Thanks for the comments @adisuissa!

Question: would it make sense to have just the global and local limits, where the local limit overrides the global, and if set to 0, it is unlimited?

This would satisfy the behaviour we want but I believe it's a question more so for maintainers. Making local limits override the global one is a breaking change. Today they can be used in tandem and a listener will be limited if it hits either limit. Further, AFAICT there's no way to set a local limit on the admin listener. So there's no way it could opt out without work to introduce some sort of local limit setting there.

Listener connection limit allows overriding global limit down. Having a separate flag to override up makes sense to me. I think we could make it so that if the listener limit is larger than global limit it acts as override up, but it may just make config more complicated.

wez470 · 2021-11-11T17:02:04Z

Listener connection limit allows overriding global limit down. Having a separate flag to override up makes sense to me. I think we could make it so that if the listener limit is larger than global limit it acts as override up, but it may just make config more complicated.

I'll look into doing this. For the override up flag, were you thinking this would be a runtime flag?

wez470 · 2021-11-11T20:09:07Z

I've realized that Listener connection limit allows overriding global limit down. Having a separate flag to override up makes sense to me. still misses some cases that the ignore global flag supports. If I want a listener that ignores the global limit, I have to make it handle more connections than the global limit. Although for my use case it would be okay to have a setup like this, there could be a case where you want a listener to only handle a handful of connections but still not be affected by the global limit. There would be no way to configure this with that approach. Thoughts?

KBaichoo · 2021-11-12T17:30:17Z

Given the below diagram:

I wonder if we'd want to end up with something more granular than global limits, but provide a grouping mechanism among subsets of listeners. e.g. Implement listener sets as in the middle of the diagram where listener 2 and 3 are limited by some maximum number, while listener 1 is not.

This would be a more generalized approach than global limit opt out (solving this use case of opting out the admin, and some particular listeners), and enable cases such as multi-tenant connection limits.

I think it would add some additional configuration complexity (e.g. to set up groups), but the behavior seems more predictable / intuitive than overriding possibly occurring in both directions. Thoughts?

yanavlasov

LGTM, modulo small const correctness nit.

/wait

source/common/network/tcp_listener_impl.h

wez470 · 2021-11-14T20:05:18Z

@KBaichoo I like this idea. It is a little more work than I'd hoped to do to solve this issue personally, but I'm open to it if it's agreed that's the directions we should go.

KBaichoo

@wez470 I'm also happy with the current solution (opt out) as it seems to be heading in the direction where we might eventually want to go and is more intuitive than overriding bidirectionally based on implicit values or implicit comparisons.

test/integration/cx_limit_integration_test.cc

docs/root/configuration/operations/overload_manager/overload_manager.rst

Signed-off-by: Weston Carlson <wez470@gmail.com>

KBaichoo · 2021-11-15T21:19:26Z

You might want to merge again since #19007 disabled a flaky QUIC test.

wez470 · 2021-11-15T21:53:16Z

Thanks

Signed-off-by: Weston Carlson <wez470@gmail.com>

KBaichoo

@adisuissa can you take a look as API Sheppard?

adisuissa

/lgtm api

wez470 · 2021-11-22T15:05:30Z

@yanavlasov Can you PTAL?

wez470 · 2021-11-24T15:39:49Z

Thanks all for reviewing!

* 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>

repokitteh-read-only bot added the api label Nov 3, 2021

repokitteh-read-only bot assigned adisuissa Nov 3, 2021

Add listener global conn limit opt out.

8c82329

Signed-off-by: Weston Carlson <wez470@gmail.com>

wez470 force-pushed the listener-global-conn-opt-out branch from 927965c to 8c82329 Compare November 3, 2021 01:42

wez470 added 5 commits November 2, 2021 19:54

Update version history.

dd69f18

Signed-off-by: Weston Carlson <wez470@gmail.com>

Fix spelling.

78135f9

Signed-off-by: Weston Carlson <wez470@gmail.com>

Add new listener field to various tests.

5e24f3d

Signed-off-by: Weston Carlson <wez470@gmail.com>

Fix admin related tests.

83351d1

Signed-off-by: Weston Carlson <wez470@gmail.com>

Update func doc string.

3a5311e

Signed-off-by: Weston Carlson <wez470@gmail.com>

wez470 marked this pull request as ready for review November 4, 2021 00:01

wez470 requested review from alyssawilk, asraa, ggreenway, junr03, lizan and yanavlasov as code owners November 4, 2021 00:01

adisuissa reviewed Nov 4, 2021

View reviewed changes

api/envoy/config/listener/v3/listener.proto Outdated Show resolved Hide resolved

docs/root/configuration/operations/overload_manager/overload_manager.rst Outdated Show resolved Hide resolved

Improve docs.

377b1e9

Signed-off-by: Weston Carlson <wez470@gmail.com>

adisuissa reviewed Nov 8, 2021

View reviewed changes

envoy/event/dispatcher.h Outdated Show resolved Hide resolved

envoy/event/dispatcher.h Outdated Show resolved Hide resolved

envoy/network/listener.h Outdated Show resolved Hide resolved

envoy/server/configuration.h Outdated Show resolved Hide resolved

Address doc related PR feedback.

906e73f

Signed-off-by: Weston Carlson <wez470@gmail.com>

yanavlasov assigned yanavlasov and KBaichoo Nov 10, 2021

yanavlasov requested changes Nov 12, 2021

View reviewed changes

source/common/network/tcp_listener_impl.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 12, 2021

KBaichoo reviewed Nov 15, 2021

View reviewed changes

test/integration/cx_limit_integration_test.cc Outdated Show resolved Hide resolved

test/integration/cx_limit_integration_test.cc Outdated Show resolved Hide resolved

docs/root/configuration/operations/overload_manager/overload_manager.rst Outdated Show resolved Hide resolved

Make field const. Cleanup tests.

a696219

Signed-off-by: Weston Carlson <wez470@gmail.com>

repokitteh-read-only bot removed the waiting label Nov 15, 2021

wez470 added 2 commits November 15, 2021 15:53

Merge branch 'main' into listener-global-conn-opt-out

c3de373

Signed-off-by: Weston Carlson <wez470@gmail.com>

Kick CI

9036f88

Signed-off-by: Weston Carlson <wez470@gmail.com>

Merge branch 'main' into listener-global-conn-opt-out

9b96a53

Signed-off-by: Weston Carlson <wez470@gmail.com>

KBaichoo approved these changes Nov 16, 2021

View reviewed changes

adisuissa reviewed Nov 16, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Nov 16, 2021

wez470 requested a review from yanavlasov November 16, 2021 18:30

yanavlasov approved these changes Nov 24, 2021

View reviewed changes

yanavlasov merged commit e903a6e into envoyproxy:main Nov 24, 2021

wez470 deleted the listener-global-conn-opt-out branch November 24, 2021 15:25

Conversation

wez470 commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Nov 3, 2021

Uh oh!

repokitteh-read-only bot commented Nov 3, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wez470 commented Nov 8, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wez470 commented Nov 8, 2021

Uh oh!

yanavlasov commented Nov 10, 2021

Uh oh!

wez470 commented Nov 11, 2021

Uh oh!

wez470 commented Nov 11, 2021

Uh oh!

KBaichoo commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wez470 commented Nov 14, 2021

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KBaichoo commented Nov 15, 2021

Uh oh!

wez470 commented Nov 15, 2021

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

wez470 commented Nov 22, 2021

Uh oh!

wez470 commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wez470 commented Nov 3, 2021 •

edited

Loading

KBaichoo commented Nov 12, 2021 •

edited

Loading