Wip: Add multi-connection support in H2 conn pool. #7852

conqerAtapple · 2019-08-07T15:03:18Z

Signed-off-by: Jojy G Varghese [email protected]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Currently H2 connection pool can create only ONE upstream connection. There are use cases that demand a policy based approach to connections in the H2 pool More background can be found at:
https://github.com/envoyproxy/envoy/issues/7403

The policy could be described as below example :

 clusters:                                                                      
    - name: cluster-name                                                   
      connect_timeout: 0.25s       
      connection_policy:
           max_requests_per_connection: 1  
           drain_on_overflow: yes // Should drain or keep in overflow list
           idle_timeout: 500ms     // Time after which the connection can be closed.                   
                                                  
      ...
      ...

Risk Level: High
Testing: Unit tests, Integration tests to be added
Docs Changes: This change will require new configuration for describing the policy.
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

conqerAtapple · 2019-08-07T15:04:11Z

@mattklein123 @alyssawilk This is a first take on the issue #7403.

mattklein123

Thanks for taking this on. In order to aid in review, can you potentially add some high level comments for all the interfaces and functions in the header files? That will help me give some initial feedback on the implementation. Thank you!

/wait

mattklein123 · 2019-08-07T18:00:26Z

include/envoy/upstream/upstream.h

@@ -646,6 +646,25 @@ using ProtocolOptionsConfigConstSharedPtr = std::shared_ptr<const ProtocolOption
 */
 class ClusterTypedMetadataFactory : public Envoy::Config::TypedMetadataFactory {};

+class ConnectionRequestPolicySubscriber {


Can you add interface comments for these classes and functions? It's not immediately clear to me why these policies need to live in upstream.h. Can we potentially move them to a new file? I'm not sure how they are going to get configured.

I have added comments and TODOs hoping to make the intent clearer. There is no strong reason for having the policy interfaces in upstream.h. The only reason i went with it for now is due to its proximity to ClusterInfo. Since the policies are related (1:1) to ClusterInfo, they could be declared together. I am open to suggestions.

OK thanks this is more clear. IMO I would simplify this for right now and not worry about a public interface, cluster info, etc. I would probably just build this config directly into the a new http2 connection pool options config:

envoy/api/envoy/api/v2/cds.proto

Line 252 in e03936e

core.Http2ProtocolOptions http2_protocol_options = 14;

. You can then plumb that config directly from cluster info -> the http2 connection pool much like we do for the existing http2 protocol options.

In terms of these interfaces and implementation, I would either just define them right now directly in the http2 conn pool code, or make new files in that area to contain them. We can always make this more generic later but IMO it's fine to define this where we are going to use this for now.

@mattklein123 Thanks! How do you visualize the Htt2ConnectionPolicy config? I suppose something like

clusters: - name: cluster-name connect_timeout: 0.25s connection_policy: max_requests_per_connection: 1 drain_on_overflow: yes // Should drain or keep in overflow list idle_timeout: 500ms // Time after which the connection can be closed. http2_protocol_options: {...} ...

Something like this yes, but per @alyssawilk we should see if we can also unify with HTTP/1.

mattklein123 · 2019-08-07T18:00:57Z

source/common/http/http2/conn_pool.h

+  // list when drain clients does not have any more requests being served.
+  // Connections remain in this list till:
+  //  - Connection is closed.
+  std::list<ActiveClientPtr> to_close_clients_;
  ActiveClientPtr primary_client_;


Presumably primary/draining clients are no longer needed?

That is correct. I will remove them.

mattklein123 · 2019-08-07T18:01:30Z

source/common/upstream/upstream_impl.cc

@@ -651,6 +651,14 @@ ClusterInfoImpl::extensionProtocolOptions(const std::string& name) const {
  return nullptr;
 }

+const ConnectionRequestPolicy& ClusterInfoImpl::connectionPolicy() const {
+  if (!connection_policy_) {


I doubt this is thread safe? But it's clear to me yet where this is used and whether it needs to live here.

Not in its current form. The idea is that the Policy implementation would be injected but I am not sure how that would be implemented. I might need help with that.

Signed-off-by: Jojy G Varghese <[email protected]>

conqerAtapple · 2019-08-08T14:56:29Z

@mattklein123 PTAL. Thank you for initial feedback. Looking forward to more feedback.

conqerAtapple · 2019-08-08T15:46:40Z

@mattklein123 I just rebased with master . I am getting link errors in protobuf:

bazel-out/host/bin/external/com_google_protobuf/_objs/protoc/main.o:main.cc:function _GLOBAL__sub_I__ZN6google8protobuf8compiler12ProtobufMainEiPPc: error: undefined reference to 'std::ios_base::Init::~Init()'
bazel-out/host/bin/external/com_google_protobuf/_objs/protoc/main.o:main.cc:function google::protobuf::compiler::ProtobufMain(int, char**): error: undefined reference to 'operator delete(void*)'
bazel-out/host/bin/external/com_google_protobuf/_objs/protoc/main.o:main.cc:function google::protobuf::compiler::ProtobufMain(int, char**): error: undefined reference to 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
bazel-out/host/bin/external/com_google_protobuf/_objs/protoc/main.o:main.cc:function google::protobuf::compiler::ProtobufMain(int, char**): error: undefined reference to 'operator delete(void*)'
bazel-out/host/bin/external/com_google_protobuf/_objs/protoc/main.o:main.cc:function google::protobuf::compiler::ProtobufMain(int, char**): error: undefined reference to 'operator delete(void*)'
bazel-out/host/bin/external/com_google_protobuf/_objs/protoc/main.o:main.cc:function google::protobuf::compiler::ProtobufMain(int, char**): error: undefined reference to 'operator delete(void*)'
bazel-out/host/bin/external/com_google_protobuf/_objs/protoc/main.o:main.cc:function google::protobuf::compiler::ProtobufMain(int, char**): error: undefined reference to 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
bazel-out/host/bin/external/com_google_protobuf/_objs/protoc/main.o:main.cc:function google::protobuf::compiler::ProtobufMain(int, char**): error: undefined reference to 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
bazel-out/host/bin/external/com_google_protobuf/_objs/protoc/main.o:main.cc:function google::protobuf::compiler::ProtobufMain(int, char**): error: undefined reference to 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'

Wondering what i am missing.

mattklein123

Thanks for working on this. This is going to need very detailed review, but in general this seems like it's on the right track. I think modulo my high level comments that I added, it's probably fine to start actually getting tests passing, add new tests, etc. Thank you!

/wait

mattklein123 · 2019-08-08T21:32:32Z

include/envoy/upstream/upstream.h

@@ -646,6 +646,25 @@ using ProtocolOptionsConfigConstSharedPtr = std::shared_ptr<const ProtocolOption
 */
 class ClusterTypedMetadataFactory : public Envoy::Config::TypedMetadataFactory {};

+class ConnectionRequestPolicySubscriber {


OK thanks this is more clear. IMO I would simplify this for right now and not worry about a public interface, cluster info, etc. I would probably just build this config directly into the a new http2 connection pool options config:

envoy/api/envoy/api/v2/cds.proto

Line 252 in e03936e

core.Http2ProtocolOptions http2_protocol_options = 14;

. You can then plumb that config directly from cluster info -> the http2 connection pool much like we do for the existing http2 protocol options.

In terms of these interfaces and implementation, I would either just define them right now directly in the http2 conn pool code, or make new files in that area to contain them. We can always make this more generic later but IMO it's fine to define this where we are going to use this for now.

mattklein123 · 2019-08-08T21:34:24Z

source/common/http/http2/conn_pool.h

@@ -81,19 +94,63 @@ class ConnPoolImpl : public ConnectionPool::Instance, public ConnPoolImplBase {

  virtual CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData& data) PURE;
  virtual uint32_t maxTotalStreams() PURE;
+  /*


nit: please clean out the commented out code here and elsewhere.

mattklein123 · 2019-08-08T21:36:00Z

source/common/http/http2/conn_pool.h

+  // Connections that are waiting to be closed. Connections are moved to this
+  // list when drain clients does not have any more requests being served.
+  // Connections remain in this list till:
+  //  - Connection is closed.


Why is this state needed? Can't we just close them directly vs. putting them on a new list?

The idea is that a busy/active client could be transitioned to drained if the policy dictates a) to not accept any more requests on that client b) close the client once all current requests are finished.

Sorry I don't follow. Once a client can be closed why does it need to be on a list?

mattklein123 · 2019-08-08T21:37:57Z

source/common/http/http2/conn_pool.cc


-  // Make sure all clients are destroyed before we are destroyed.
+ConnPoolImpl::~ConnPoolImpl() {
+  drainConnections();


All connections need to be closed here in this case like happened previously, not just put in the draining state.

It happens in the method drainConnections. It in turn calls checkForDrained which then calls close on all closable clients. I thought once we move all the clients to drain state, it implies that we dont accept any new requests and just wait for existing requests to finish .

mattklein123 · 2019-08-08T21:40:42Z

source/common/http/http2/conn_pool.cc

@@ -259,17 +388,60 @@ void ConnPoolImpl::onStreamReset(ActiveClient& client, Http::StreamResetReason r
  }
 }

-void ConnPoolImpl::onUpstreamReady() {
-  // Establishes new codec streams for each pending request.
-  while (!pending_requests_.empty()) {


In the HTTP/2 case I think we can still have multiple pending requests per connection created, right? Don't we still need a loop to deal with one we get a new connection?

conqerAtapple · 2019-08-08T22:23:26Z

@mattklein123 Thanks for reviewing. I wanted to check once again what i might be missing. After rebasing with master, i am unable to build:

ERROR: /home/jojy/.cache/bazel/_bazel_jojy/d11de52fa5a5d3ec50a154631da518da/external/com_github_grpc_grpc/BUILD:470:1: Linking of rule '@com_github_grpc_grpc//:grpc_cpp_plugin' failed (Exit 1) gcc failed: error executing command /usr/bin/gcc -o bazel-out/host/bin/external/com_github_grpc_grpc/grpc_cpp_plugin -pthread
 -pthread -Wl,-S '-fuse-ld=gold' -Wl,-no-as-needed -Wl,-z,relro,-z,now -B/usr/bin -pass-exit-codes ... (remaining 3 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
bazel-out/host/bin/external/com_github_grpc_grpc/_objs/grpc_cpp_plugin/cpp_plugin.o:cpp_plugin.cc:function ProtoBufService::method(int) const: error: undefined reference to 'operator new(unsigned long)'
bazel-out/host/bin/external/com_github_grpc_grpc/_objs/grpc_cpp_plugin/cpp_plugin.o:cpp_plugin.cc:function ProtoBufFile::service(int) const: error: undefined reference to 'operator new(unsigned long)'
bazel-out/host/bin/external/com_github_grpc_grpc/_objs/grpc_cpp_plugin/cpp_plugin.o:cpp_plugin.cc:function google::protobuf::io::StringOutputStream::~StringOutputStream(): error: undefined reference to 'operator delete(void*)'
bazel-out/host/bin/external/com_github_grpc_grpc/_objs/grpc_cpp_plugin/cpp_plugin.o:cpp_plugin.cc:function ProtoBufMethod::~ProtoBufMethod(): error: undefined reference to 'operator delete(void*)'
bazel-out/host/bin/external/com_github_grpc_grpc/_objs/grpc_cpp_plugin/cpp_plugin.o:cpp_plugin.cc:function ProtoBufService::~ProtoBufService(): error: undefined reference to 'operator delete(void*)'
bazel-out/host/bin/external/com_github_grpc_grpc/_objs/grpc_cpp_plugin/cpp_plugin.o:cpp_plugin.cc:function ProtoBufFile::~ProtoBufFile(): error: undefined reference to 'operator delete(void*)'
bazel-out/host/bin/external/com_github_grpc_grpc/_objs/grpc_cpp_plugin/cpp_plugin.o:cpp_plugin.cc:function ProtoBufFile::CreatePrinter(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) const: error: undefined reference to 'operator new(unsigned long)'
bazel-out/host/bin/external/com_github_grpc_grpc/_objs/grpc_cpp_plugin/cpp_plugin.o:cpp_plugin.cc:function void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*, std::forward_iterator_tag) [clone .isra.92]: error: undefined reference to 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
bazel-out/host/bin/external/com_github_grpc_grpc/_objs/grpc_cpp_plugin/cpp_plugin.o:cpp_plugin.cc:function void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*, std::forward_iterator_tag) [clone .isra.92]: error: undefined reference to 'std::__throw_logic_error(char const*)'

it looks like a missing ldflag(libstdc++)?

mattklein123 · 2019-08-08T22:36:18Z

@conqerAtapple sorry not sure I would hop into #envoy-dev and ask there. cc @lizan @PiotrSikora

lizan · 2019-08-09T00:14:45Z

@conqerAtapple make sure you have Bazel >= 0.28.0, ideally what .bazelversion file says.

- removed old commented reside - added override keyword. Signed-off-by: Jojy G Varghese <[email protected]>

conqerAtapple · 2019-08-09T18:14:52Z

@lizan thanks! that worked.

alyssawilk · 2019-08-12T20:15:15Z

include/envoy/upstream/upstream.h

+ * this interface as a contract between the policy implementation and policy
+ * user(subscriber).
+ */
+class ConnectionRequestPolicySubscriber {


I took a super cursory pass at this and while I'm really happy to get this work into HTTP/2 I'm worried about the code overlap between the HTTP/1 multiple-connection management and HTTP/2.

I think we might do better here if we tried to factor out / enhance the connection management we already have, and then use it in the H2 connection pool. WDYT?

+1 to this. This was going to be my suggestion as well once we get the code a bit more stabilized. I think there will be significant duplication. Up to you whether you want to try to do it in HTTP/2 first and then figure out the overlap or try to factor out first... (There already is a common base class you can work from.)

- Added ConnectionPolicy interface in Mock. - Fixed conn pool cleanup. Signed-off-by: Jojy G Varghese <[email protected]>

stale · 2019-08-21T14:02:13Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-10-10T21:07:16Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-10-17T21:47:10Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

mattklein123 assigned mattklein123 and alyssawilk Aug 7, 2019

mattklein123 requested changes Aug 7, 2019

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Aug 7, 2019

conqerAtapple added 3 commits August 7, 2019 22:39

Wip: Add multi-connection support in H2 conn pool.

eb7a9b8

Signed-off-by: Jojy G Varghese <[email protected]>

Added comments and TODOs.

e266525

Signed-off-by: Jojy G Varghese <[email protected]>

Removed references to old clients.

ce362fb

Signed-off-by: Jojy G Varghese <[email protected]>

conqerAtapple force-pushed the wip_h2_conn_policy branch from abfca1c to e266525 Compare August 8, 2019 15:05

mattklein123 requested changes Aug 8, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 8, 2019

Cleanup.

ee3fe0c

- removed old commented reside - added override keyword. Signed-off-by: Jojy G Varghese <[email protected]>

repokitteh-read-only bot removed the waiting label Aug 9, 2019

mattklein123 added the waiting label Aug 10, 2019

alyssawilk reviewed Aug 12, 2019

View reviewed changes

Changes:

63004e2

- Added ConnectionPolicy interface in Mock. - Fixed conn pool cleanup. Signed-off-by: Jojy G Varghese <[email protected]>

repokitteh-read-only bot removed the waiting label Aug 14, 2019

mattklein123 added the waiting label Aug 14, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 21, 2019

mattklein123 added the no stalebot Disables stalebot from closing an issue label Aug 23, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 23, 2019

mattklein123 mentioned this pull request Aug 23, 2019

Client side ALPN support (upstream cluster) #3431

Closed

PiotrSikora mentioned this pull request Sep 30, 2019

http: option to limit the maximum number of requests on a downstream connection #8443

Open

alyssawilk requested a review from antoniovicente October 2, 2019 18:51

alyssawilk removed the no stalebot Disables stalebot from closing an issue label Oct 10, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 10, 2019

stale bot closed this Oct 17, 2019

conqerAtapple mentioned this pull request Oct 31, 2019

max_requests_per_connection=1 causes gRPC connections reset #8817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wip: Add multi-connection support in H2 conn pool. #7852

Wip: Add multi-connection support in H2 conn pool. #7852

conqerAtapple commented Aug 7, 2019

conqerAtapple commented Aug 7, 2019

mattklein123 left a comment

mattklein123 Aug 7, 2019

conqerAtapple Aug 8, 2019

mattklein123 Aug 8, 2019

conqerAtapple Aug 12, 2019

mattklein123 Aug 12, 2019

mattklein123 Aug 7, 2019

conqerAtapple Aug 8, 2019

mattklein123 Aug 7, 2019

conqerAtapple Aug 8, 2019

conqerAtapple commented Aug 8, 2019

conqerAtapple commented Aug 8, 2019

mattklein123 left a comment

mattklein123 Aug 8, 2019

mattklein123 Aug 8, 2019

mattklein123 Aug 8, 2019

conqerAtapple Aug 12, 2019

mattklein123 Aug 12, 2019

mattklein123 Aug 8, 2019

conqerAtapple Aug 12, 2019

mattklein123 Aug 8, 2019

conqerAtapple commented Aug 8, 2019 •

edited

Loading

mattklein123 commented Aug 8, 2019

lizan commented Aug 9, 2019

conqerAtapple commented Aug 9, 2019

alyssawilk Aug 12, 2019

mattklein123 Aug 12, 2019

stale bot commented Aug 21, 2019

stale bot commented Oct 10, 2019

stale bot commented Oct 17, 2019

Wip: Add multi-connection support in H2 conn pool. #7852

Wip: Add multi-connection support in H2 conn pool. #7852

Conversation

conqerAtapple commented Aug 7, 2019

conqerAtapple commented Aug 7, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conqerAtapple commented Aug 8, 2019

conqerAtapple commented Aug 8, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conqerAtapple commented Aug 8, 2019 • edited Loading

mattklein123 commented Aug 8, 2019

lizan commented Aug 9, 2019

conqerAtapple commented Aug 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Aug 21, 2019

stale bot commented Oct 10, 2019

stale bot commented Oct 17, 2019

conqerAtapple commented Aug 8, 2019 •

edited

Loading