adaptive concurrency: Filter configuration and integration tests by tonya11en · Pull Request #8244 · envoyproxy/envoy

tonya11en · 2019-09-14T01:21:41Z

This patch brings the concurrency controller and the adaptive concurrency control (ACC) filter together to make the filter fully useable with integration tests.

The integration tests make use of the fault filter, placed after the ACC filter, to emulate request latencies and to also move time forward. Even though this is against the recommendation in the documentation to have the filter come before all other filters, it was necessary to have last. Not every request makes it to the upstream due to occasionally being blocked by the ACC filter, so it was not possible to simply call timeSystem().sleep and rely on the autonomous upstream to reply to the requests.

Patch 3/n towards #7789.

Temporal behavior

Now that all the pieces are together, it's worth comparing the filter's behavior with what we expecte from the proof-of-concept. Time sampled stats show similar behavior to the proof-of-concept filter's data in #7789. Here's a plot of latencies without the adaptive concurrency filter:

Now with the adaptive concurrency filter:

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

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

tonya11en · 2019-09-15T22:23:57Z

/wait

...e/extensions/filters/http/adaptive_concurrency/concurrency_controller/gradient_controller.cc

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

...extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_integration_test.cc

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

tonya11en · 2019-09-16T17:34:54Z

Marking as ready since the CI release test just flaked out. The thread_local_store_test works fine for me locally when tested on release:

INFO: Elapsed time: 282.725s, Critical Path: 152.57s
INFO: 1329 processes: 1329 linux-sandbox.
INFO: Build completed successfully, 1687 total actions
//test/common/stats:thread_local_store_test                              PASSED in 27.3s

Executed 1 out of 1 test: 1 test passes.
INFO: Build completed successfully, 1687 total actions

tallen@legion ~/src/envoy [acc_2]
± % git log | head -1                                                                                                                  !10376
commit 6f5684661ddcdba6cc38f6f42fbcaef8da3ea149 (HEAD -> acc_2, tony/acc_2)

tonya11en · 2019-09-16T22:06:04Z

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

@@ -27,8 +27,8 @@ AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter(
 Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool) {
  if (controller_->forwardingDecision() == ConcurrencyController::RequestForwardingAction::Block) {
    // TODO (tonya11en): Remove filler words.


Remove the TODO

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

mattklein123

Thanks, awesome to see this coming together. A few nits, but my main comment is I would rather this be done with simulated time or at a minimum have a TODO explaining why it can't be and also do a de-flaking run either way. Thank you!

/wait

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

.../extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_integration_test.h

mattklein123 · 2019-09-17T21:01:08Z

.../extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_integration_test.h

+
+  void initializeFilter() {
+    // We use the fault filter (for delays) after the adaptive concurrency filter to introduce a
+    // "service latency" to the test. This way, time is moved forward with each response.


Why can't you just use simulated time and just advance time before upstream responds?

Wherever we end up on this, please verify that you have done a de-flaking run on these new tests (--runs_per_test=1000) to verify a lack of flakes.

...extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_integration_test.cc

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

tonya11en · 2019-09-19T19:01:04Z

/wait

mattklein123

Thanks, looks good. You can fix the nits in your next change.

.../extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_integration_test.h

mattklein123 · 2019-09-27T16:07:56Z

Please merge master. (Might as well fix the nits that I pointed out at the same time since you have to push again.)

/wait

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

mattklein123

Thanks. @tonya11en please confirm that you have run the integration tests under --runs_per_test to check for flakes?

tonya11en · 2019-09-30T19:58:23Z

Not recently- let me go ahead and kick off 1000 and report back before merge.

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

tonya11en · 2019-09-30T21:35:58Z

@mattklein123 looks stable:

>> btd //test/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_integration_test --runs_per_test=1000 --test_timeout=20 --jobs=1               
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/home/tallen/src/envoy/tools/bazel.rc
INFO: Invocation ID: 457c1652-a84a-4ea4-8ef9-fd8f885fee03
INFO: Analyzed target //test/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_integration_test (0 packages loaded, 0 targets configured).
INFO: Found 1 test target...
Target //test/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_integration_test up-to-date:
  bazel-bin/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_integration_test
INFO: Elapsed time: 1428.001s, Critical Path: 1.56s
INFO: 1000 processes: 1000 linux-sandbox.
INFO: Build completed successfully, 1001 total actions
//test/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_integration_test PASSED in 1.5s
  Stats over 1000 runs: max = 1.5s, min = 1.3s, avg = 1.4s, dev = 0.0s

Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see whINFO: Build completed successfully, 1001 total actions

I needed to make sure there weren't multiple copies of this integration test running at the same time. When running it with 12 concurrency workers, there was a 2% failure rate, but from my investigations, I think this was due to the integration test framework and the fake upstreams.

mattklein123 · 2019-09-30T21:47:01Z

I needed to make sure there weren't multiple copies of this integration test running at the same time. When running it with 12 concurrency workers, there was a 2% failure rate, but from my investigations, I think this was due to the integration test framework and the fake upstreams.

This means it will definitely flake in CI. It should work 100% of the time with concurrent workers, so we will need to investigate this unfortunately.

/wait

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

tonya11en · 2019-10-01T01:25:52Z

Alright, now we're cooking:

>> btd //test/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_integration_test --runs_per_test=1000 --test_timeout=20
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/home/tallen/src/envoy/tools/bazel.rc
INFO: Invocation ID: e2f638f0-7bf8-4082-bf2e-0fa2655362c9
INFO: Build options --cache_test_results, --run_under, --runs_per_test, and 1 more have changed, discarding analysis cache.
INFO: Analyzed target //test/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_integration_test (0 packages loaded, 14062 targets configured).
INFO: Found 1 test target...
Target //test/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_integration_test up-to-date:
  bazel-bin/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_integration_test
INFO: Elapsed time: 127.585s, Critical Path: 1.79s
INFO: 1000 processes: 1000 linux-sandbox.
INFO: Build completed successfully, 1001 total actions
//test/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_integration_test PASSED in 1.8s
  Stats over 1000 runs: max = 1.8s, min = 1.3s, avg = 1.5s, dev = 0.1s

Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see whINFO: Build completed successfully, 1001 total actions

12 concurrent jobs this time.

There were actually 2 races that needed to be fixed:

The adaptive concurrency filter was responding to requests that it was rejecting before the test was able to send the request data. This caused the integration test's codec client to use a bogus stream and segfault. This was fixed by just sending header-only requests.
The header-only requests were then sent so quickly that the counters were non-deterministic. This required us to wait for the blocked counter to be incremented after each request before proceeding. Now the stats are as we'd expect.

mattklein123

Awesome, thanks for fixing the flakes Tony!

…oyproxy#8244) Signed-off-by: Tony Allen <tallen@lyft.com>

Tony Allen added 7 commits September 12, 2019 18:20

test runs, removed simulated time, behavior looks correct

614e4f4

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

Added stats and verification around them.

adf17d5

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

fix format

3f532d4

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

test: dynamic latency via headers

0dc7df4

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

move files to acc folder

b373d6e

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

more tests, should be g2g

a4169a5

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

format

6e01473

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

mattklein123 self-assigned this Sep 14, 2019

mattklein123 added the waiting label Sep 14, 2019

Kick CI

1eba068

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

repokitteh-read-only bot removed the waiting label Sep 15, 2019

repokitteh-read-only bot added the waiting label Sep 15, 2019

tonya11en commented Sep 15, 2019

View reviewed changes

...e/extensions/filters/http/adaptive_concurrency/concurrency_controller/gradient_controller.cc Outdated Show resolved Hide resolved

Make test less flaky.

81270c2

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

repokitteh-read-only bot removed the waiting label Sep 16, 2019

tonya11en commented Sep 16, 2019

View reviewed changes

...extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_integration_test.cc Outdated Show resolved Hide resolved

tonya11en added 3 commits September 15, 2019 18:15

format

132ddcc

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

Removing flaky test.

5a52865

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

removing redundant max()

6f56846

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

tonya11en marked this pull request as ready for review September 16, 2019 17:32

tonya11en requested a review from mattklein123 as a code owner September 16, 2019 17:32

tonya11en commented Sep 16, 2019

View reviewed changes

Kick CI

d55a10d

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

mattklein123 requested changes Sep 17, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 17, 2019

address nits

3bbba11

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

repokitteh-read-only bot removed the waiting label Sep 19, 2019

repokitteh-read-only bot removed the waiting label Sep 26, 2019

mattklein123 previously approved these changes Sep 26, 2019

View reviewed changes

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

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

repokitteh-read-only bot added the waiting label Sep 27, 2019

Tony Allen added 3 commits September 30, 2019 10:57

nits

e4ae58e

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

Merge remote-tracking branch 'upstream/master' into acc_3

0f3f7bc

format

f6bca56

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

tonya11en dismissed mattklein123’s stale review via f6bca56 September 30, 2019 18:20

repokitteh-read-only bot removed the waiting label Sep 30, 2019

mattklein123 added the waiting label Sep 30, 2019

tidy

1dba65e

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

repokitteh-read-only bot removed the waiting label Sep 30, 2019

mattklein123 previously approved these changes Sep 30, 2019

View reviewed changes

mattklein123 added the waiting label Sep 30, 2019

tidy

51973aa

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

tonya11en dismissed mattklein123’s stale review via 51973aa September 30, 2019 21:32

repokitteh-read-only bot removed the waiting label Sep 30, 2019

Merge remote-tracking branch 'upstream/master' into acc_3

bb156bf

repokitteh-read-only bot added the waiting label Sep 30, 2019

Fix test flakiness

ba46fa5

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

repokitteh-read-only bot removed the waiting label Oct 1, 2019

mattklein123 approved these changes Oct 1, 2019

View reviewed changes

mattklein123 merged commit a119a07 into envoyproxy:master Oct 1, 2019

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

adaptive concurrency: Filter configuration and integration tests (env…

0ff1d94

…oyproxy#8244) Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en deleted the acc_2 branch November 13, 2019 04:35

Conversation

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

Uh oh!

Temporal behavior

Uh oh!

tonya11en commented Sep 15, 2019

Uh oh!

Uh oh!

Uh oh!

tonya11en commented Sep 16, 2019

Uh oh!

tonya11en Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tonya11en commented Sep 19, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented Sep 27, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

tonya11en commented Sep 30, 2019

Uh oh!

tonya11en commented Sep 30, 2019

Uh oh!

mattklein123 commented Sep 30, 2019

Uh oh!

tonya11en commented Oct 1, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tonya11en commented Sep 14, 2019 •

edited

Loading