thrift: supported setting max requests for per downstream connection by wbpcode · Pull Request #15125 · envoyproxy/envoy

wbpcode · 2021-02-20T11:54:50Z

Signed-off-by: wbpcode comems@msn.com

Commit Message: thrift: supported setting max requests for per downstream connection
Additional Description:

Supported setting max requests for per downstream connection. By setting max_requests_per_connection, Envoy can actively disconnect the thrift client after reaching a certain number of requests.
Due to the limitations of the thrift protocol itself, we cannot achieve a clean disconnection.

Check #14560 get more information.

Risk Level: Normal
Testing: Added
Docs Changes: N/A
Release Notes: Added

Signed-off-by: wbpcode <comems@msn.com>

repokitteh-read-only · 2021-02-20T11:54:55Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15125 was opened by wbpcode.

see: more, trace.

zyfjeff · 2021-02-20T13:51:56Z

I feel that this requirement is not universal. Why not do this by removing the listener from the control plane? @wbpcode

wbpcode · 2021-02-22T06:39:09Z

I feel that this requirement is not universal. Why not do this by removing the listener from the control plane? @wbpcode

That's also an option. However, if there are multiple dubbo services in the cluster, although some of them have generated the corresponding listener for it, such as 1.2.3.4:10000, there may still be traffic accessing other dubbo services that need to be proxied through 0.0.0.0:10000.

In addition, we have a similar configuration for upstream connections, and I personally don't see any harm in making the same restrictions for downstream connections. 😄

wbpcode · 2021-02-22T06:43:14Z

@rgs1 Would you mind helping to give this pr a review or some suggestions? 😃

rgs1 · 2021-02-22T13:44:34Z

source/extensions/filters/network/thrift_proxy/config.h

  std::list<ThriftFilters::FilterFactoryCb> filter_factories_;
  const bool payload_passthrough_;
+
+  uint64_t max_requests_per_connection_ = UINT64_MAX;


nit:

uint64_t max_requests_per_connection_{};

initializing this to zero and skipping the limit check when zero is probably easier (and more common across the code base).

rgs1 · 2021-02-22T13:46:04Z

source/extensions/filters/network/thrift_proxy/conn_manager.h

  TimeSource& time_source_;
+
+  // The number of requests accumulated on the current connection. A connection is processed by only
+  // one thread, so there is no need to consider the thread safety of the count.


I think we can drop the 2nd sentence since it's a common assumption that conn managers are per thread.

rgs1 · 2021-02-22T13:48:13Z

source/extensions/filters/network/thrift_proxy/conn_manager.cc

+
+  if (parent_.accumulated_requests_ >= parent_.config_.maxRequestsPerConnection()) {
+    parent_.read_callbacks_->connection().readDisable(true);
+    parent_.requests_overflow_ = true;


can we add a stat for this and increment it here?

rgs1 · 2021-02-22T13:49:28Z

test/extensions/filters/network/thrift_proxy/conn_manager_test.cc

+  EXPECT_EQ(0U, store_.counter("test.response_invalid_type").value());
+  EXPECT_EQ(1U, store_.counter("test.response_success").value());
+  EXPECT_EQ(0U, store_.counter("test.response_error").value());
+}


Can we also test:

limit set but not reached

limit set, reached, cleared and next request goes through

And also test the suggested stat from above is properly incremented in each case.

rgs1

Thanks! We are going to use this too, so excited to see this feature.

Could you also add a comment about this new feature in docs/root/configuration/listeners/network_filters/thrift_proxy_filter.rst?

cc: @fishcakez to get additional feedback

htuch · 2021-02-23T03:00:43Z

/lgtm api

Signed-off-by: wbpcode <comems@msn.com>

docs/root/configuration/listeners/network_filters/thrift_proxy_filter.rst

Signed-off-by: wbpcode <comems@msn.com>

rgs1 · 2021-02-26T01:36:16Z

source/extensions/filters/network/thrift_proxy/config.h

@@ -1,5 +1,6 @@
 #pragma once

+#include <cstdint>


not needed anymore i think

👍 yes, it is not needed. I just forget to remove it.

rgs1 · 2021-02-26T01:42:14Z

source/extensions/filters/network/thrift_proxy/conn_manager.h

  TimeSource& time_source_;
+
+  // The maximum number of requests remaining to be processed on the current connection.
+  uint64_t remaining_streams_{};


Can we do this the other way around? Having a counter of seen requests and a the limit in a separate var? E.g.:

uint64_t requests_{}; uint64_t max_requests_{};

If max_requests_ is 0, there are no limits. I rather avoid depending on std::numeric_limits<uint64_t>::max().

I have no objection. The reason for choosing current method is to reduce one if judgment. If using this method, we need to determine whether max_requests is 0, and then determine whether the accumulated request value is greater than max_requests.

But it should be ok.

rgs1 · 2021-02-26T01:44:41Z

test/extensions/filters/network/thrift_proxy/conn_manager_test.cc

+  for (size_t i = 0; i < 4; i++) {
+    mock_new_connection();
+    EXPECT_EQ(5, sendSomeThriftRequest(6));
+  }


Why is this loop needed?

In order to simulate the situation of multiple disconnections and observe whether the stat value grows correctly.

rgs1

Thanks for the updates, left a few more comments.

Signed-off-by: wbpcode <comems@msn.com>

rgs1 · 2021-02-26T15:06:55Z

test/extensions/filters/network/thrift_proxy/conn_manager_test.cc

  }

+  // Return the number of requests actually sent.
+  uint32_t sendSomeThriftRequest(uint32_t request_number) {


nit: uint32_t sendRequests(uint32_t count) {

rgs1 · 2021-02-26T15:10:57Z

test/extensions/filters/network/thrift_proxy/conn_manager_test.cc

+  EXPECT_EQ(0U, store_.counter("test.response_exception").value());
+  EXPECT_EQ(0U, store_.counter("test.response_invalid_type").value());
+  EXPECT_EQ(50U, store_.counter("test.response_success").value());
+  EXPECT_EQ(0U, store_.counter("test.response_error").value());


should we check here if cx_destroy_local_with_active_rq was incremented? I think it probably is incremented when a close happens after max requests is reached.

rgs1

This looks great, thanks! I have one final nit and one question about whether we should check if cx_destroy_local_with_active_rq is incremented after a local connection close. Other than that, LGTM.

(I also added some comments previously and deleted them, since I realized I was wrong)

rgs1 · 2021-02-26T15:13:59Z

The linux multiarch failure is transient, happened on other PRs too.

wbpcode · 2021-03-01T05:46:07Z

@rgs1
I think cx_destroy_local_with_active_rq will never increase. After we get the complete request (in finalizeRequest), we will increase the accumulated_requests count without waiting for the response to end. When the count is greater than max_requests, the request_overflow flag will be set. No new requests will be accepted after that. After all accepted requests are over, we will actively disconnect.

The advantage of this is that when the connection is disconnected, we don't have to worry about those requests that exceed the limit being reset and introducing idempotent problems. Because those requests that exceed the limit will not be forwarded by the thrift proxy at all.

Signed-off-by: wbpcode <comems@msn.com>

…tion

rgs1 · 2021-03-01T15:20:39Z

@rgs1
I think cx_destroy_local_with_active_rq will never increase. After we get the complete request (in finalizeRequest), we will increase the accumulated_requests count without waiting for the response to end. When the count is greater than max_requests, the request_overflow flag will be set. No new requests will be accepted after that. After all accepted requests are over, we will actively disconnect.

The advantage of this is that when the connection is disconnected, we don't have to worry about those requests that exceed the limit being reset and introducing idempotent problems. Because those requests that exceed the limit will not be forwarded by the thrift proxy at all.

Ok makes sense.

rgs1 · 2021-03-01T15:22:25Z

You'll want to merge main once #15238 lands.

rgs1

Thanks!

…tion

wbpcode · 2021-03-02T05:31:49Z

@rgs1 main merged.

rgs1 · 2021-03-02T13:45:04Z

@wbpcode LGTM, thanks!

@zuercher over to you for merging.

thrift: supported setting max requests for per downstream connection

f8538bf

Signed-off-by: wbpcode <comems@msn.com>

wbpcode requested a review from zuercher as a code owner February 20, 2021 11:54

repokitteh-read-only bot added the api label Feb 20, 2021

repokitteh-read-only bot assigned htuch Feb 20, 2021

rgs1 reviewed Feb 22, 2021

View reviewed changes

rgs1 suggested changes Feb 22, 2021

View reviewed changes

htuch assigned zuercher Feb 23, 2021

repokitteh-read-only bot removed the api label Feb 23, 2021

wbpcode added 3 commits February 23, 2021 20:35

update some docs and some test

cb664b0

Signed-off-by: wbpcode <comems@msn.com>

fix asan error

3446082

Signed-off-by: wbpcode <comems@msn.com>

fix asan error

aa038d4

Signed-off-by: wbpcode <comems@msn.com>

wbpcode requested a review from rgs1 February 24, 2021 11:04

zuercher assigned rgs1 Feb 24, 2021

zuercher reviewed Feb 24, 2021

View reviewed changes

docs/root/configuration/listeners/network_filters/thrift_proxy_filter.rst Outdated Show resolved Hide resolved

fix docs

7f5b510

Signed-off-by: wbpcode <comems@msn.com>

rgs1 reviewed Feb 26, 2021

View reviewed changes

rgs1 suggested changes Feb 26, 2021

View reviewed changes

minor update

1c99671

Signed-off-by: wbpcode <comems@msn.com>

rgs1 reviewed Feb 26, 2021

View reviewed changes

wbpcode added 2 commits March 1, 2021 13:47

update method name

d3e44e3

Signed-off-by: wbpcode <comems@msn.com>

Merge branch 'main' of github.com:envoyproxy/envoy into thrift-connec…

fa47f36

…tion

rgs1 approved these changes Mar 1, 2021

View reviewed changes

Merge branch 'main' of github.com:envoyproxy/envoy into thrift-connec…

8a832d9

…tion

rgs1 approved these changes Mar 2, 2021

View reviewed changes

zuercher approved these changes Mar 2, 2021

View reviewed changes

zuercher merged commit 8a2685e into envoyproxy:main Mar 2, 2021

wbpcode deleted the thrift-connection branch April 6, 2021 07:00

Conversation

wbpcode commented Feb 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 20, 2021

Uh oh!

zyfjeff commented Feb 20, 2021

Uh oh!

wbpcode commented Feb 22, 2021

Uh oh!

wbpcode commented Feb 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Feb 23, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

rgs1 commented Feb 26, 2021

Uh oh!

wbpcode commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgs1 commented Mar 1, 2021

Uh oh!

rgs1 commented Mar 1, 2021

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Mar 2, 2021

Uh oh!

rgs1 commented Mar 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

wbpcode commented Feb 20, 2021 •

edited

Loading

wbpcode Feb 26, 2021 •

edited

Loading

wbpcode Feb 26, 2021 •

edited

Loading

wbpcode commented Mar 1, 2021 •

edited

Loading