Combine H1 and H2 conn_pool implementations by ggreenway · Pull Request #9668 · envoyproxy/envoy

ggreenway · 2020-01-13T22:11:42Z

This allows the H2 connection pool to have more than 2 connections (1 active, 1 draining), which allows things like very low values of max_requests_per_connection to work because there can be multiple draining connections concurrently, as well as supporting multiple active connections.

Risk Level: High
Testing: Unit, integration
Docs Changes: Added to .proto; updated conn pool and circuit breaker docs; added old behavior to deprecation list
Release Notes: added
Fixes #7403
Fixes #9215

This extracts shared functionality (connection timeouts, drain callbacks) into the base class. It also introduces a base class for the ActiveClient, opening up for moving shared client functionality into the base class as well. Moves the handling of the ready/busy lists up to the base class, setting us up for reusing these for HTTP/2. Signed-off-by: Snow Pettersen <snowp@squareup.com>

Signed-off-by: Greg Greenway <ggreenway@apple.com>

This can be achieved by using the existing cluster config mocks. Signed-off-by: Greg Greenway <ggreenway@apple.com>

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Create additional connections when the limit is reached. Signed-off-by: Greg Greenway <ggreenway@apple.com>

It was used in an intermediate version of the change. Signed-off-by: Greg Greenway <ggreenway@apple.com>

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only · 2020-01-13T22:11:47Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9668 was opened by ggreenway.

see: more, trace.

ggreenway · 2020-01-13T22:14:05Z

cc @snowp

ggreenway · 2020-01-13T22:18:14Z

I have 2 big concerns with this PR:

I messed up the implementation and it completely breaks the world until the bug is fixed
Subtle behavior changes to existing configurations. Behavior is mostly the same for both H1 and H2 pools, with the exception of circuit breakers. The old behavior was to only look at connection circuit breakers for H1 pools, and only look at request circuit breakers for H2 pools. Now, both types are looked at in all cases. Will this break someone?

snowp · 2020-01-13T22:25:03Z

Re 2. I have written code myself that doesn't touch the unused circuit breaker for H1/H2 only clusters, so I'm sure there are several cases where people have bumped the max connection CB for a HTTP/1.1 only cluster to permit some concurrency but left the max request CB limit at the default. I'm wondering if we should be runtime guarding the new behavior and then communicate the change of behavior with a timeline for changing the default. Thoughts @mattklein123 ?

Signed-off-by: Greg Greenway <ggreenway@apple.com>

snowp

Nice, I think this approach makes sense. Might be good to add some test coverage to the new circuit breaking behavior for H1/H2

source/common/http/conn_pool_base.cc

test/test_common/utility.cc

ggreenway · 2020-01-27T18:35:27Z

But it passed on my machine.

When I ran the tests against the wrong branch. Oops.

The last batch of changes I pushed definitely has a bug, I'm trying to figure out the correct way to fix it.

When there is a pending request, and a connection in the process of connecting, if drainConnection() or addDrainedCallback() is called, should the pending connection be closed, or kept open until it is connected to try to serve the pending request? @mattklein123

mattklein123 · 2020-01-27T21:54:26Z

When there is a pending request, and a connection in the process of connecting, if drainConnection() or addDrainedCallback() is called, should the pending connection be closed, or kept open until it is connected to try to serve the pending request? @mattklein123

As long as there are pending requests I think we have to allow them to be served, which includes waiting for connection?

a CONNECTING client into DRAINING, which doesn't make sense. Also minor comment improvements. Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2020-01-28T01:15:52Z

That makes sense. I hadn't realized that the two functions serve very different use cases. I think I have the desired behavior for both now.

ggreenway · 2020-01-28T18:23:39Z

@envoyproxy/api-shepherds PTAL at this. It's not a change to the protos, just a doc change and applying an existing field in a new context.

mattklein123

Thanks, awesome work. Just a few doc comments and then I think we can ship and hopefully get some bake time!

/wait

docs/root/intro/arch_overview/upstream/circuit_breaking.rst

docs/root/intro/deprecated.rst

docs/root/intro/version_history.rst

source/common/http/http1/conn_pool.cc

Signed-off-by: Greg Greenway <ggreenway@apple.com>

mattklein123

Thanks, awesome work. @alyssawilk any further comments on this?

comments addressed

alyssawilk

LGTM - I had a draft with one comment about documenting the overrides in the release notes and you beat me to it :-P

…pplied to certain HTTP version The circuit-breaker connection pool settings `MaxRequests`and `MaxPendingRequests` apply to HTTP/1.1 as well as HTTP/2 since [1] and [2]. This was actually changed in the Istio docs via [3], but these source code comments were left, potentially causing confusion. [1] envoyproxy/envoy#9215 [2] envoyproxy/envoy#9668 [3] istio/api#2428 Signed-off-by: Christian Rohmann <christian.rohmann@inovex.de>

…pplying to particular HTTP version The circuit-breaker connection pool settings `MaxRequests`and `MaxPendingRequests` apply to HTTP/1.1 as well as HTTP/2 since [1] and [2]. This was actually changed in the Istio docs via [3], but these source code comments were left, potentially causing confusion. [1] envoyproxy/envoy#9215 [2] envoyproxy/envoy#9668 [3] istio/api#2428 Signed-off-by: Christian Rohmann <christian.rohmann@inovex.de>

…pplying to particular HTTP version (#57705) The circuit-breaker connection pool settings `MaxRequests`and `MaxPendingRequests` apply to HTTP/1.1 as well as HTTP/2 since [1] and [2]. This was actually changed in the Istio docs via [3], but these source code comments were left, potentially causing confusion. [1] envoyproxy/envoy#9215 [2] envoyproxy/envoy#9668 [3] istio/api#2428 Signed-off-by: Christian Rohmann <christian.rohmann@inovex.de>

Snow Pettersen and others added 19 commits December 13, 2019 22:53

Merge branch 'master' into h2-combined

77f16dd

Fix build: logger

c6ca9b7

Signed-off-by: Greg Greenway <ggreenway@apple.com>

fix tests build

249770b

Signed-off-by: Greg Greenway <ggreenway@apple.com>

more build fixes

579dbef

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Improve docs for LinkedObject::moveBetweenLists

e7a0587

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Phase one: moved some logic to base class.

4e2c147

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into h2-combined

75bdc34

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Working version; tests pass

3a35fa6

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into h2-combined

379cdb1

fix_format

6c5f650

Signed-off-by: Greg Greenway <ggreenway@apple.com>

cleanup: remove maxTotalStreams(), used only by tests.

206c6f3

This can be achieved by using the existing cluster config mocks. Signed-off-by: Greg Greenway <ggreenway@apple.com>

cleanup: translate 0 to MAX so that special code for zero isn't needed

30c38e9

Signed-off-by: Greg Greenway <ggreenway@apple.com>

upstream: Enforce max_concurrent_streams on http2 upstream pools.

85b0e6e

Create additional connections when the limit is reached. Signed-off-by: Greg Greenway <ggreenway@apple.com>

Remove now-unneeded ENVOY_CONN_ID_LOG.

8a53fd5

It was used in an intermediate version of the change. Signed-off-by: Greg Greenway <ggreenway@apple.com>

grammar/whitespace

107abcb

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Fix ASAN error when destructing a pool with connections still in it.

6fb21fd

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Fix ASAN failure in test code (mutate vector while iterating)

f145d10

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into h2-combined

c792feb

repokitteh-read-only bot added the api label Jan 13, 2020

htuch requested a review from mattklein123 January 13, 2020 22:26

htuch assigned mattklein123 Jan 13, 2020

apply api change to v3alpha to fix CI

32252a6

Signed-off-by: Greg Greenway <ggreenway@apple.com>

mattklein123 assigned alyssawilk and snowp Jan 13, 2020

snowp suggested changes Jan 13, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 27, 2020

Fix a bug introduced in last change that could put

7c5d261

a CONNECTING client into DRAINING, which doesn't make sense. Also minor comment improvements. Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Jan 28, 2020

mattklein123 requested changes Jan 28, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 28, 2020

doc fixes

dab1c60

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Jan 29, 2020

ggreenway requested a review from mattklein123 January 29, 2020 17:56

mattklein123 approved these changes Jan 29, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 29, 2020

alyssawilk approved these changes Jan 29, 2020

View reviewed changes

mattklein123 merged commit be8d4ca into envoyproxy:master Jan 29, 2020

ramaraochavali mentioned this pull request Feb 8, 2020

Fix load balancing of pilot connections istio/istio#20957

Merged

AndresGuedez mentioned this pull request Feb 14, 2020

http2: support custom SETTINGS parameters #9964

Merged

snowp mentioned this pull request Apr 15, 2020

Client side ALPN support (upstream cluster) #3431

Closed

ramaraochavali mentioned this pull request May 18, 2020

incorrect stats increment for streaming requests #11237

Closed

ramaraochavali mentioned this pull request Sep 23, 2020

http2MaxRequests seems incorrectly named istio/istio#27473

Closed

ramaraochavali mentioned this pull request Mar 18, 2021

Does http1MaxPendingRequests misnamed ? istio/istio#31548

Closed

ramaraochavali mentioned this pull request Jul 23, 2022

clarify destination rule docs istio/api#2428

Merged

frittentheke mentioned this pull request Sep 19, 2025

chore: remove comments about MaxRequests or MaxPendingRequests only applying to particular HTTP version istio/istio#57705

Merged

Conversation

ggreenway commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 13, 2020

Uh oh!

ggreenway commented Jan 13, 2020

Uh oh!

ggreenway commented Jan 13, 2020

Uh oh!

snowp commented Jan 13, 2020

Uh oh!

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

ggreenway commented Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Jan 27, 2020

Uh oh!

ggreenway commented Jan 28, 2020

Uh oh!

ggreenway commented Jan 28, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ggreenway commented Jan 13, 2020 •

edited

Loading

ggreenway commented Jan 27, 2020 •

edited

Loading