Adaptive concurrency no-op implementation by tonya11en · Pull Request #7819 · envoyproxy/envoy

tonya11en · 2019-08-03T00:14:32Z

This patch implements the adaptive concurrency controller interface and the adaptive concurrency limit filter logic #7789.

The filter's logic is quite simple. During decoding, verify if the request should bother continuing using the concurrency controller. If so, continue; otherwise, return a 503. During encoding, we will have the request's round-trip time, so this value is fed to the concurrency controller before continuing.

The concurrency controller does all of the heavy lifting in this filter. The interface can be implemented with any desired algorithm, as long as it samples request latencies, can unconditionally determine whether a request can continue on, and is thread-safe.

Assuming this patch does not dramatically change from the time the PR is opened, the next PR will be the algorithm implementation/tests.

Risk Level: No risk.
Testing: None.
Docs Changes: N/A

Initial effort towards #7789

Signed-off-by: Tony Allen <tallen@lyft.com>

Signed-off-by: Tony Allen <tony@allen.gg>

tonya11en

This was modeled after the HTTP filter example repo, so apologies if there's another way I should be doing this.

CODEOWNERS

repokitteh-read-only · 2019-08-03T00:23:13Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

Signed-off-by: Tony Allen <tony@allen.gg>

repokitteh-read-only · 2019-08-03T00:24:47Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

Signed-off-by: Tony Allen <tony@allen.gg>

repokitteh-read-only · 2019-08-03T04:32:59Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

Signed-off-by: Tony Allen <tony@allen.gg>

repokitteh-read-only · 2019-08-03T04:36:57Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

mattklein123

Thanks @tonya11en. Very excited to see this starting to land. From a quick skim looks like it's on the right track. I will review when there is 100% test coverage. Thank you!

/wait

repokitteh-read-only · 2019-08-04T22:35:50Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

Signed-off-by: Tony Allen <tony@allen.gg>

repokitteh-read-only · 2019-08-04T22:39:59Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

mattklein123 · 2019-08-05T16:38:13Z

@tonya11en friendly request to not force push a PR after it's open. It makes reviews much more difficult. Thank you!

mattklein123

Cool stuff, flushing out some comments. Note also that you are missing coverage. Please check the coverage report in CI. Thank you!

/wait

source/extensions/extensions_build_config.bzl

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc

...extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only · 2019-08-05T23:30:41Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

...extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h

source/extensions/filters/http/well_known_names.h

mattklein123

Looking good! Flushing some more comments.

/wait

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only · 2019-08-06T20:16:27Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

mattklein123

Cool, thanks. A few small comments and we can ship!

/wait

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h

...extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h

test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc

mattklein123 · 2019-08-06T23:05:17Z

source/extensions/filters/http/adaptive_concurrency/config.cc

+namespace HttpFilters {
+namespace AdaptiveConcurrency {
+
+Http::FilterFactoryCb AdaptiveConcurrencyFilterFactory::createFilterFactoryFromProtoTyped(


I don't think anything in this file or the NOP controller is actually used in this PR. Can we just remove it for now until we have the integration test?

I'm actually forced to implement createFilterFactoryFromProtoTyped or the filter factory is an abstract class. I ripped out everything else I could.

Sorry I meant I don't think you need config.h/config.cc at all in this PR? See https://github.com/envoyproxy/envoy/blob/master/source/extensions/extensions_build_config.bzl#L79 for an example of getting tests to run w/o a config file that we don't need yet.

/wait

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only · 2019-08-07T00:47:20Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only · 2019-08-07T18:29:04Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only · 2019-08-07T18:55:31Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

mattklein123

LGTM, thanks. A few small nits and a question.

/wait

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc

mattklein123 · 2019-08-07T21:48:14Z

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc

+AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() = default;
+
+Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool) {
+  if (controller_->forwardingDecision() == ConcurrencyController::RequestForwardingAction::Block) {


Q: Are we eventually going to support this on a per-cluster basis? This seems probably OK for ingress, but it seems like we should support per route/cluster limiting? What is your vision there?

I think per-cluster would make sense in the future and shouldn't be too hard to support. More thought would need to go into per-route because of runtime fractional routing.

All of the interfaces will need to change if we support per-cluster, but it's OK to sort that out later.

mattklein123 · 2019-08-07T21:48:37Z

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc

+}
+
+void AdaptiveConcurrencyFilter::encodeComplete() {
+  const auto rq_latency = config_->timeSource().monotonicTime() - rq_start_time_;


nit: just inline this into the function call on the next line.

I think this makes the intention more clear/readable. Similar to the 2-state enum comment from earlier.

The compiler will optimize it all to the same thing anyway.

test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only · 2019-08-07T22:29:47Z

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

🐱

Caused by: #7819 was synchronize by tonya11en.

see: more, trace.

mattklein123

Thanks!

Tony Allen and others added 4 commits August 2, 2019 16:14

Adaptive concurrency boilerplate

860d066

Signed-off-by: Tony Allen <tallen@lyft.com>

Remove excess deps

8884a4a

Signed-off-by: Tony Allen <tony@allen.gg>

Hook up and use noop controller.

25d3c96

Signed-off-by: Tony Allen <tony@allen.gg>

cleanup

0aa3576

Signed-off-by: Tony Allen <tony@allen.gg>

tonya11en commented Aug 3, 2019

View reviewed changes

CODEOWNERS Show resolved Hide resolved

fix format

a694091

Signed-off-by: Tony Allen <tony@allen.gg>

tonya11en force-pushed the acc_filter branch from 03f1638 to a694091 Compare August 3, 2019 00:24

tonya11en marked this pull request as ready for review August 3, 2019 00:25

tidy

b99ac62

Signed-off-by: Tony Allen <tony@allen.gg>

format

e873d81

Signed-off-by: Tony Allen <tony@allen.gg>

mattklein123 self-assigned this Aug 3, 2019

mattklein123 requested changes Aug 3, 2019

View reviewed changes

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

test passes

66bbf3e

Signed-off-by: Tony Allen <tony@allen.gg>

tonya11en force-pushed the acc_filter branch from 45a078f to 66bbf3e Compare August 4, 2019 22:39

mattklein123 requested changes Aug 5, 2019

View reviewed changes

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

Tony Allen added 3 commits August 5, 2019 16:06

Address comments

4431de4

Signed-off-by: Tony Allen <tallen@lyft.com>

fix format

2355305

Signed-off-by: Tony Allen <tallen@lyft.com>

format

879a5d4

Signed-off-by: Tony Allen <tallen@lyft.com>

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

derekargueta reviewed Aug 6, 2019

View reviewed changes

...extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h Outdated Show resolved Hide resolved

source/extensions/filters/http/well_known_names.h Outdated Show resolved Hide resolved

mattklein123 requested changes Aug 6, 2019

View reviewed changes

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc Outdated Show resolved Hide resolved

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

comments round 2

ef09c5d

Signed-off-by: Tony Allen <tallen@lyft.com>

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

mattklein123 requested changes Aug 6, 2019

View reviewed changes

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

rip out noop controller

c91c58b

Signed-off-by: Tony Allen <tallen@lyft.com>

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

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

remove config files

d3d2022

Signed-off-by: Tony Allen <tallen@lyft.com>

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

filter lib

2cfbfef

Signed-off-by: Tony Allen <tallen@lyft.com>

mattklein123 requested changes Aug 7, 2019

View reviewed changes

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

Tony Allen added 2 commits August 7, 2019 15:28

final nits

ab794b2

Signed-off-by: Tony Allen <tallen@lyft.com>

final nits

513ee6c

Signed-off-by: Tony Allen <tallen@lyft.com>

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

mattklein123 approved these changes Aug 7, 2019

View reviewed changes

mattklein123 merged commit 8e1dd33 into envoyproxy:master Aug 8, 2019

tonya11en deleted the acc_filter branch September 25, 2019 08:41

Conversation

tonya11en commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 3, 2019

Uh oh!

repokitteh-read-only bot commented Aug 3, 2019

Uh oh!

repokitteh-read-only bot commented Aug 3, 2019

Uh oh!

repokitteh-read-only bot commented Aug 3, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Aug 4, 2019

Uh oh!

repokitteh-read-only bot commented Aug 4, 2019

Uh oh!

mattklein123 commented Aug 5, 2019

Uh oh!

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

repokitteh-read-only bot commented Aug 5, 2019

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 6, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

repokitteh-read-only bot commented Aug 7, 2019

Uh oh!

repokitteh-read-only bot commented Aug 7, 2019

Uh oh!

repokitteh-read-only bot commented Aug 7, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tonya11en commented Aug 3, 2019 •

edited

Loading