add rps_threshold & max_rejection_probability by WeavingGao · Pull Request #16742 · envoyproxy/envoy

WeavingGao · 2021-06-01T07:48:20Z

Signed-off-by: gaoweiwen whereisgww@outlook.com

Commit Message:
Add two configuration items to the admission_control filter.

rps_threshold indicates the minimum RPS threshold.
max_rejection_probability indicates the maximum throttling percentage.

details in issue: #16392

Additional Description:
Risk Level: Medium
Testing: Unit test and integration test
Docs Changes: admission_control_filter.rst
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

repokitteh-read-only · 2021-06-01T07:48:30Z

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

🐱

Caused by: #16742 was opened by WeavingGao.

see: more, trace.

WeavingGao · 2021-06-01T07:55:13Z

I'm sorry that this PR came a little late since the last discussion in the related issue(#16392) , because some other things have filled up my time recently. Anyway, it's ready to be reviewed.
cc @tonya11en @mattklein123

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

mattklein123 · 2021-06-01T15:48:19Z

Assigning over to @tonya11en for first pass.

tonya11en

Included some high-level comments on the approach to the traffic threshold.

tonya11en · 2021-06-01T19:20:37Z

api/envoy/extensions/filters/http/admission_control/v3alpha/admission_control.proto

+  // If the number of requests in the last second is less than this threshold, the request
+  // will not be rejected, even if the success rate is lower than sr_threshold.
+  // Defaults to 0.
+  config.core.v3.RuntimeUInt32 rps_threshold = 6;


I don't think this should only consider the last 1 second. It would make more sense to look at something like the minimum number of samples in the sampling window. It would still accomplish the same thing, but give better behavior for bursty request patterns.

Would that work for your particular use-case?

To be more clear, you can still represent what I'm suggesting as an RPS threshold. It would just be an "average RPS" threshold across the entirety of the sampling window.

tonya11en · 2021-06-01T19:23:59Z

docs/root/configuration/http/http_filters/admission_control_filter.rst

+  P_{reject} = \left\{
+  \begin{array}{cl}
+  0 & \ (rps < rps\_threshold) \\
+  min({(\frac{n_{total} - s}{n_{total} + 1})}^\frac{1}{aggression}\ ,\ max\_reject\_probability) & \ (rps \geq rps\_threshold)
+  \end{array}
+  \right.


Can you include a screenshot of this rendered in the docs?

I also don't think it's necessary to change the function here as you're introducing circumstances under which this function applies, not changes to the behavior of the function.

screenshot here

Your comment reminds me that this does make the formula more complicated. It may be more appropriate to explain these two configuration items after the original formula.

tonya11en · 2021-06-01T22:22:41Z

/wait

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

WeavingGao · 2021-06-02T10:50:41Z

Changed LastSampleCounts to averageRps and updated the documentation.

tonya11en

Thanks for doing this. Tests are very thorough!

The approach makes sense, so there's just a few minor comments.

tonya11en · 2021-06-03T18:15:33Z

docs/root/configuration/http/http_filters/admission_control_filter.rst

  rejection probability will be higher for higher success rates. See `Aggression`_ for a more
  detailed explanation.

+In addition, there are two more configuration item:


Suggested change

In addition, there are two more configuration item:

Note that there are additional parameters that affect the rejection probability:

tonya11en · 2021-06-03T18:21:38Z

source/extensions/filters/http/admission_control/admission_control.cc

  }

+  if (config_->getController().averageRps() < config_->rpsThreshold()) {
+    return Http::FilterHeadersStatus::Continue;


Consider throwing a debug log in here.

tonya11en · 2021-06-03T18:31:19Z

source/extensions/filters/http/admission_control/thread_local_controller.cc

    : time_source_(time_source), sampling_window_(sampling_window) {}

+uint32_t ThreadLocalControllerImpl::averageRps() const {
+  // In the very beginning of sampling, the average rps might be very big, so just skipping from it.


I don't quite understand what you mean here. Why do you want to do this?

The first few requests in the first second may have a short interval, resulting in a big average rps. So skip the calculation of the first second.

I see, because you're allowing fractional seconds due to the age being measured in microseconds.

You can just set the min sample age to 1sec. Cast to std::chrono::seconds instead of a double and do the integer division. There's no reason for the extra accuracy if the config is specified in uint32.

Sounds reasonable.

tonya11en · 2021-06-03T18:36:33Z

test/extensions/filters/http/admission_control/admission_control_filter_test.cc

+  // Validate max rejection probability
+  EXPECT_CALL(runtime_.snapshot_, getDouble("foo.aggression", 1.0)).WillRepeatedly(Return(1.0));
+  EXPECT_CALL(runtime_.snapshot_, getDouble("foo.threshold", 100.0)).WillRepeatedly(Return(100.0));
+  EXPECT_CALL(runtime_.snapshot_, getDouble("foo.max_rejection_probability", 80.0))
+      .WillRepeatedly(Return(10.0));
+
+  verifyProbabilities(100, 0.0);
+  verifyProbabilities(95, 0.05);
+  verifyProbabilities(80, 0.1);
+  verifyProbabilities(0, 0.1);


Can you make this its own test case.

tonya11en · 2021-06-03T18:45:32Z

test/extensions/filters/http/admission_control/controller_test.cc

+  // Validate historical_data_.size() < 2
+  tlc_.recordSuccess();
+  tlc_.recordFailure();
+  EXPECT_EQ(0, tlc_.averageRps());


I commented on this above, but just reiterating I'm a bit confused as to the purpose.

tonya11en · 2021-06-03T18:47:15Z

test/extensions/filters/http/admission_control/controller_test.cc

  EXPECT_EQ(RequestData(3, 3), tlc_.requestCounts());
 }

+// Test for function: averageRps


Terminate the comments with periods pls.

tonya11en · 2021-06-03T18:51:29Z

/wait

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

WeavingGao · 2021-06-04T08:32:20Z

/retest

repokitteh-read-only · 2021-06-04T08:32:23Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16742 (comment) was created by @WeavingGao.

see: more, trace.

tonya11en

Left 1 more comment. Looking good otherwise.

/wait

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

tonya11en

Sorry, you'll also need to update the release notes. See:
https://github.com/envoyproxy/envoy/blob/main/docs/root/version_history/current.rst

Mention the new features and a change in the default behavior (max rejection probability). We should be good for a maintainer to take a look once that's done.

tonya11en · 2021-06-07T18:53:44Z

source/extensions/filters/http/admission_control/thread_local_controller.cc

+  using namespace std::chrono;
+  auto count_of_seconds = duration_cast<seconds>(ageOfOldestSample()).count();
+
+  return count_of_seconds == 0 ? 0 : global_data_.requests / count_of_seconds;


I think you can just do something like below. There's just less going on that way IMO.

Suggested change

using namespace std::chrono;

auto count_of_seconds = duration_cast<seconds>(ageOfOldestSample()).count();

return count_of_seconds == 0 ? 0 : global_data_.requests / count_of_seconds;

using std::chrono::seconds;

seconds secs = std::max(seconds(1), ageOfOldestSample());

return global_data_.requests / secs.count();

tonya11en · 2021-06-07T19:00:12Z

/wait

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

tonya11en

This LGTM. I think it's ready for a maintainer pass @mattklein123

mattklein123

One small doc question/comment, thanks.

/wait

mattklein123 · 2021-06-08T20:56:14Z

docs/root/configuration/http/http_filters/admission_control_filter.rst

+* Requests will never be rejeted from this filter if the RPS is lower than 5.
+* Rejection probability will never exceed 80% even if the failure rate is 100%.


If this is true and these values are clamped, can you make this more clear in the API docs? If it's not true can you make it more clear here?

This isn't true all the time-- it's just explaining how to read the configuration above. Github cut out this line just above the additions:

The above configuration can be understood as follows:

Ah OK sorry, I missed that, thanks.

WeavingGao · 2021-06-10T01:28:45Z

/retest

repokitteh-read-only · 2021-06-10T01:28:49Z

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

🐱

Caused by: a #16742 (comment) was created by @WeavingGao.

see: more, trace.

mattklein123

Thanks!

alyssawilk · 2021-06-22T15:12:28Z

I think this may be causing CI flakes
#17067

could you take a look?

…oyproxy#16742) Signed-off-by: gaoweiwen <whereisgww@outlook.com>

add rps_threshold & max_rejection_probability

b5b8523

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

WeavingGao requested a review from mattklein123 as a code owner June 1, 2021 07:48

repokitteh-read-only bot added the api label Jun 1, 2021

repokitteh-read-only bot assigned mattklein123 Jun 1, 2021

fix format

3ee18a3

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

mattklein123 assigned tonya11en Jun 1, 2021

tonya11en requested changes Jun 1, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 1, 2021

add averageRps()

543d3ab

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

repokitteh-read-only bot removed the waiting label Jun 2, 2021

tonya11en requested changes Jun 3, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 3, 2021

some minor changes

80668e7

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

repokitteh-read-only bot removed the waiting label Jun 4, 2021

tonya11en requested changes Jun 5, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 5, 2021

Modify the calculation method of avergeRps

68e3535

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

repokitteh-read-only bot removed the waiting label Jun 7, 2021

tonya11en requested changes Jun 7, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 7, 2021

add some feature description

7c508c1

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

repokitteh-read-only bot removed the waiting label Jun 8, 2021

WeavingGao added 2 commits June 8, 2021 14:30

Merge remote-tracking branch 'upstream/main' into admission_control

0cd8d14

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

fix format

363a387

Signed-off-by: gaoweiwen <whereisgww@outlook.com>

tonya11en approved these changes Jun 8, 2021

View reviewed changes

mattklein123 requested changes Jun 8, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 8, 2021

mattklein123 approved these changes Jun 11, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jun 11, 2021

mattklein123 merged commit ce1882c into envoyproxy:main Jun 11, 2021

WeavingGao mentioned this pull request Jun 11, 2021

Admission Control: QPS threshold #16392

Closed

This was referenced Jun 23, 2021

flake on IpVersions/AdmissionControlIntegrationTest.GrpcTest/1 in admission_control_integration_test #17067

Closed

test: Fix admission control integration test flakiness #17114

Merged

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

admission control: add rps_threshold & max_rejection_probability (env…

cd337eb

…oyproxy#16742) Signed-off-by: gaoweiwen <whereisgww@outlook.com>

WeavingGao deleted the admission_control branch October 18, 2021 09:59

	In addition, there are two more configuration item:
	Note that there are additional parameters that affect the rejection probability:

		* Requests will never be rejeted from this filter if the RPS is lower than 5.
		* Rejection probability will never exceed 80% even if the failure rate is 100%.

Conversation

WeavingGao commented Jun 1, 2021

Uh oh!

repokitteh-read-only bot commented Jun 1, 2021

Uh oh!

WeavingGao commented Jun 1, 2021

Uh oh!

mattklein123 commented Jun 1, 2021

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en commented Jun 1, 2021

Uh oh!

WeavingGao commented Jun 2, 2021

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en Jun 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en commented Jun 3, 2021

Uh oh!

WeavingGao commented Jun 4, 2021

Uh oh!

repokitteh-read-only bot commented Jun 4, 2021

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en commented Jun 7, 2021

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

WeavingGao commented Jun 10, 2021

Uh oh!

repokitteh-read-only bot commented Jun 10, 2021

Uh oh!

mattklein123 left a comment

tonya11en Jun 5, 2021 •

edited

Loading

alyssawilk commented Jun 22, 2021 •

edited

Loading