Minimize the stock extension set for the test server #425

oschaaf · 2020-07-31T11:18:07Z

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-31T11:25:44Z

(quick experiment to check how this affects CI build time, cuts down the number of build actions from ~ 6K to 4K)

oschaaf · 2020-07-31T12:36:31Z

cool, this seems to speed up the build CI task 2X:

https://app.circleci.com/pipelines/github/envoyproxy/nighthawk/1940/workflows/0c0da3ce-2e7b-47e0-b3e3-4b96251e4d56/jobs/15852

https://app.circleci.com/pipelines/github/envoyproxy/nighthawk/1941/workflows/0d5b191c-d6a3-4a24-a4e3-0634fd2f9f9a/jobs/15856

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-31T12:51:42Z

Requesting a high level review for this first; as this needs tidying up before this is suitable for landing.
But first I'd like to hear if there's interest in landing this, there seems to be some benefit to it:

(much) faster build times
smaller binary
"reduced surface": we explicitly list what we like to offer for the test server, instead of
having extension "creep in" through envoy updates.

It might also be possible to diverge in different build types to produce a release with all extensions
enabled, but run CI with just the minimal set we need / the use-cases we have in mind.

oschaaf · 2020-07-31T14:07:58Z

Some more CI numbers. The slowest running tests are asan and coverage.

asan goes from ~ 1.5 hours to 1 hour
coverage goes from ~ 1h05m to 50m
test goes from ~30min to ~20min
docker image goes from ~30min to ~15min

to make sure, triggered another run:
https://app.circleci.com/pipelines/github/envoyproxy/nighthawk/1943/workflows/94785027-9edc-4447-bc02-661075a157c6

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-31T20:43:48Z

Made some changes as discussed in an offline chat:

minimised the number of extensions for the stock build
documented usage

As there was consensus that there are multiple upsides to this, and this is
something we should do, this is now ready for code level review.

oschaaf · 2020-07-31T21:48:36Z

CI time taken per task with the minimised extension set as of d3c6a7b:

tsan 27m 40s
test_gcc 35m 32s
test 32m 10s
docker 22m 45s
coverage 47m 24s
clang_tidy 26m 27s
build 10m 58s
benchmark 19m 35s
asan 57m 37s

mum4k · 2020-07-31T21:55:52Z

@dubious90 please review and assign back to me once done.

dubious90 · 2020-08-04T15:32:39Z

source/server/README.md

@@ -8,6 +8,11 @@ A test-server filter which is capable of generating test responses.
 bazel test -c dbg //test/server:http_test_server_filter_integration_test
 ```

+It is possible to enable


Should this be in the build section below, rather than the testing section?

dubious90 · 2020-08-04T15:33:25Z

source/server/README.md

@@ -8,6 +8,11 @@ A test-server filter which is capable of generating test responses.
 bazel test -c dbg //test/server:http_test_server_filter_integration_test
 ```

+It is possible to enable


nit: can we say "enable additional envoy extensions"?

dubious90 · 2020-08-04T15:36:39Z

extensions_build_config.bzl

@@ -0,0 +1,9 @@
+# See https://github.com/envoyproxy/envoy/blob/master/bazel/README.md for details on how this system works.


The link here provides an avalanche of information. Do you have objections to using this link instead, which I think is to the more relevant section?
https://github.com/envoyproxy/envoy/blob/master/bazel/README.md#disabling-extensions

dubious90 · 2020-08-04T15:57:50Z

extensions_build_config.bzl

+    "envoy.filters.listener.tls_inspector": "//source/extensions/filters/listener/tls_inspector:config",
+    "envoy.filters.network.http_connection_manager": "//source/extensions/filters/network/http_connection_manager:config",
+    "envoy.tracers.zipkin": "//source/extensions/tracers/zipkin:config",
+    "envoy.transport_sockets.raw_buffer": "//source/extensions/transport_sockets/raw_buffer:config",


This is just my own ignorance of this, but why don't we need to include our test server filter here?

It's a good question, that didn't occur to me before. Tests are passing, so I think it is safe to assume that "foreign" extensions don't play along with this feature. We could look into proposing a feature over at the Envoy repository if that is desired?

(with "foreign", I meant extensions not residing in Envoy's own code base)

My area of interest with this is partially on our own documentation around it. If others want to add in their own test server equivalent building on what we already have, how would they do that? But that's only tangentially related to this. So it's probably best to move on.

…uration Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-08-04T17:04:12Z

@dubious90 review feedback applied in 06dc01e

dubious90

LGTM. Assigning to mumak for approval.

dubious90 · 2020-08-05T01:30:52Z

extensions_build_config.bzl

+    "envoy.filters.listener.tls_inspector": "//source/extensions/filters/listener/tls_inspector:config",
+    "envoy.filters.network.http_connection_manager": "//source/extensions/filters/network/http_connection_manager:config",
+    "envoy.tracers.zipkin": "//source/extensions/tracers/zipkin:config",
+    "envoy.transport_sockets.raw_buffer": "//source/extensions/transport_sockets/raw_buffer:config",


My area of interest with this is partially on our own documentation around it. If others want to add in their own test server equivalent building on what we already have, how would they do that? But that's only tangentially related to this. So it's probably best to move on.

mum4k · 2020-08-05T03:06:49Z

extensions_build_config.bzl

@@ -0,0 +1,9 @@
+# See https://github.com/envoyproxy/envoy/blob/master/bazel/README.md#disabling-extensions for details on how this system works.
+EXTENSIONS = {


Is there a complete list of extensions somewhere? I would like to take a look at what we are disabling and cross-check it with the functionality we are promising and using.

The full set is contained over at https://github.com/envoyproxy/envoy/blob/master/source/extensions/extensions_build_config.bzl

- Updates the extension build config to add ADDITIONAL_VISIBILITY - Allows fixing of envoyproxy#399: use the right include path for the fault filter. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-08-05T09:54:57Z

@mum4k I added 7cdda88 in here, which updates Envoy and fixes build breakage against the latest; what extensions_build_config.bzl should look like apparently is a fast moving target.

edit - it's easy to back that out and move it into a separate PR, let me know if you prefer that over including it here.
as a side effect, the latest revision seems to have the fault filter extension properly visible, allowing resolution of Envoy extension includes aren't available via external/envoy/... #399,
some tech-debt that is good to get rid of. Also this rather "optimizes" the clang-tidy check (it stops actually building the project, as we don't need do do that), reducing time taken from 53 minutes to ~10 minutes. Furthermore we can probably increase parallelism again, reducing the slowest tests, asan, to 30 minutes, which completes the CI run. Overall, we'd be close to speeding up complete CI cycles ~4X

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-08-05T11:13:16Z

In e863df4 there's a change that leaves in all the extensions that can be enabled as comments. That might be helpful, but it also puts a burden on us to maintain that list. @dubious90 @mum4k wdyt?

Also, optimized CI parallellism, complete CI cycles now complete close to 4X faster. clang-tidy speeds up 5X because of the update to gen_compilation_database.py: we no longer actually build the project, as we don't need that.

oschaaf · 2020-08-05T11:32:33Z

Summarising CI timings for master vs current state of this PR

CI job	new duration	old duration
tsan	24m 38s	39m 59s
test_gcc	32m 39s	48m 35s
test	18m 9s	27m 2s
format	1m 17s	1m 8s
docker	16m 17s	32m 42s
coverage	27m 20s	1h 12m 37s
clang_tidy	10m 17s	52m 13s
build	13m 35s	23m 58s
benchmark	18m 18s	36m 26s
asan	32m 25s	1h 29m 28s

oschaaf · 2020-08-05T13:15:03Z

I respun CI a few times and I think this helps stabilize coverage and asan; we should see less flakes upon merging this. Also, running clang-tidy locally is going to be a much more frictionless thing.

dubious90 · 2020-08-05T19:10:49Z

Having trouble responding directly to your comment, but re: #425 (comment)

I think that it is much more desirable to have your original behavior, rather than this, because of the lower overhead of mainenance, and it is in my opinion, harder to read for pertinent information, in my opinion.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-08-05T19:18:09Z

@dubious90 backed out the full commented list of available extensions in e198c0b

mum4k

The numbers showing the improvement of CI are awesome, thank you!

Limit the extensions we build with the test server

6a17de7

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Looks like we need the tls_inspector extension

647fec2

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf marked this pull request as ready for review July 31, 2020 12:48

oschaaf added the waiting-for-review A PR waiting for a review. label Jul 31, 2020

Minimize default extension set. Document usage.

d3c6a7b

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf changed the title ~~Limit the extensions we build with the test server~~ Minimize the stock extension set for the test server Jul 31, 2020

mum4k requested a review from dubious90 July 31, 2020 21:55

dubious90 suggested changes Aug 4, 2020

View reviewed changes

oschaaf added 2 commits August 4, 2020 18:58

Merge remote-tracking branch 'upstream/master' into extensions-config…

7a73198

…uration Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

06dc01e

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

dubious90 reviewed Aug 5, 2020

View reviewed changes

dubious90 requested a review from mum4k August 5, 2020 01:31

dubious90 assigned mum4k Aug 5, 2020

mum4k reviewed Aug 5, 2020

View reviewed changes

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Aug 5, 2020

oschaaf added 2 commits August 5, 2020 11:48

Update Envoy to c2b0d37ca5b40b71ce48984f2cf5984297b79a71

7cdda88

- Updates the extension build config to add ADDITIONAL_VISIBILITY - Allows fixing of envoyproxy#399: use the right include path for the fault filter. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Increase CI parallellism - quick test

8a71109

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added 3 commits August 5, 2020 12:00

Fix format, fix clang_tidy CI task

04b8a99

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Regen/update CLI docs

9a6ba0d

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Leave all the disabled extensions included as comments

e863df4

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fix format

cf8a47a

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Aug 5, 2020

Revert extension_build_config.bzl to the minimum

e198c0b

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k approved these changes Aug 5, 2020

View reviewed changes

mum4k merged commit f996062 into envoyproxy:master Aug 5, 2020

oschaaf mentioned this pull request Aug 5, 2020

Envoy extension includes aren't available via external/envoy/... #399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimize the stock extension set for the test server #425

Minimize the stock extension set for the test server #425

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020 •

edited

Loading

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020

mum4k commented Jul 31, 2020

dubious90 Aug 4, 2020

dubious90 Aug 4, 2020

dubious90 Aug 4, 2020

dubious90 Aug 4, 2020

oschaaf Aug 4, 2020

oschaaf Aug 4, 2020

dubious90 Aug 5, 2020

oschaaf commented Aug 4, 2020

dubious90 left a comment

dubious90 Aug 5, 2020

mum4k Aug 5, 2020

oschaaf Aug 5, 2020

oschaaf commented Aug 5, 2020 •

edited

Loading

oschaaf commented Aug 5, 2020

oschaaf commented Aug 5, 2020

oschaaf commented Aug 5, 2020

dubious90 commented Aug 5, 2020

oschaaf commented Aug 5, 2020

mum4k left a comment

		@@ -0,0 +1,9 @@
		# See https://github.com/envoyproxy/envoy/blob/master/bazel/README.md for details on how this system works.

		@@ -0,0 +1,9 @@
		# See https://github.com/envoyproxy/envoy/blob/master/bazel/README.md#disabling-extensions for details on how this system works.
		EXTENSIONS = {

Minimize the stock extension set for the test server #425

Minimize the stock extension set for the test server #425

Conversation

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020 • edited Loading

oschaaf commented Jul 31, 2020

oschaaf commented Jul 31, 2020

mum4k commented Jul 31, 2020

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

oschaaf commented Aug 4, 2020

dubious90 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

oschaaf commented Aug 5, 2020 • edited Loading

oschaaf commented Aug 5, 2020

oschaaf commented Aug 5, 2020

oschaaf commented Aug 5, 2020

dubious90 commented Aug 5, 2020

oschaaf commented Aug 5, 2020

mum4k left a comment

Choose a reason for hiding this comment

oschaaf commented Jul 31, 2020 •

edited

Loading

oschaaf commented Aug 5, 2020 •

edited

Loading