test: adding a multi-envoy test #20016

alyssawilk · 2022-02-17T14:46:29Z

Functionally this

handles the multi-envoy signal handler crash
skips instantiating a runtime singleton (off by default, must stay off until remove global runtime: rebase runtime features on ABSL_flags #19847 is closed)

Multi-envoy does not correctly support runtime flags or deprecation stats due to #19847 being incomplete. It can still handle proxy traffic client - L1 - L2 - upstream as shown in test.

Risk Level: low
Testing: yes
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#2003

Signed-off-by: Alyssa Wilk <[email protected]>

RyanTheOptimist

LGTM. I think it might be a good idea to also get a review from someone more familiar with Runtime in order to assess the implications of making it no longer a proper Singleton. But it looks good from my perspective.

RyanTheOptimist · 2022-02-17T15:00:36Z

source/server/config_validation/server.h

@@ -91,7 +91,7 @@ class ValidationInstance final : Logger::Loggable<Logger::Id::main>,
  ServerLifecycleNotifier& lifecycleNotifier() override { return *this; }
  ListenerManager& listenerManager() override { return *listener_manager_; }
  Secret::SecretManager& secretManager() override { return *secret_manager_; }
-  Runtime::Loader& runtime() override { return Runtime::LoaderSingleton::get(); }
+  Runtime::Loader& runtime() override { return runtime_singleton_->instance(); }


I would have expected this to delegate to InstanceImpl::runtime(). But does that not work because ValidationInstance does not extend InstanceImpl?

exactly. arguably I should do the same restart flag checks here, but I'lll get there in the PR I flip the runtime guard true by default.

RyanTheOptimist · 2022-02-17T15:02:35Z

test/integration/multi_envoy_test.cc

+public:
+  // Create an envoy in front of the original Envoy.
+  void createL1Envoy();
+  ~MultiEnvoyTest() { test_server_.reset(); }


nit: destructors usually come before other methods. I'm mildly surprised this doesn't need to be marked override, but C++ is weird.

it probably does and clang tidy will catch me :-P Will fix.

RyanTheOptimist · 2022-02-17T15:05:28Z

test/integration/multi_envoy_test.cc

+// A simple test to make sure that traffic can flow between client - L1 - L2 - upstream.
+// This test does not currently support mixed protocol hops, or much of the other envoy test
+// framework knobs.
+TEST_P(MultiEnvoyTest, SimpleRequestAndResponse) {


Nice!

Would it also make sense (perhaps as a follow up?) to have a test in which we create 2 distinct Envoy instances, both in front of the same (or different, I suppose) upstreams. Then we could verify that requests flow through both, close one, and verify that the other still works?

yeah I also plan on having tests where I tear down the L2 and verify the L1 500s correctly, or tear down L1 and make sure I can talk directly to L2 -> client, to ensure lifetime issues are really sorted and regression tested against.

Having some tests that verify different shutdown ordering would be useful I think, just to make sure that they can exist independently of each other

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk

yeah for sure this gets second pass review, even with minimal code.
The impact of not having a singleton runtime should be documented in the PR description, and should be moot once the other PRs I have in flight land, at which point there is no runtime singleton access in the code any more and I can fix up the final tests and flip the flag true.

alyssawilk · 2022-02-17T15:28:53Z

test/integration/multi_envoy_test.cc

+// A simple test to make sure that traffic can flow between client - L1 - L2 - upstream.
+// This test does not currently support mixed protocol hops, or much of the other envoy test
+// framework knobs.
+TEST_P(MultiEnvoyTest, SimpleRequestAndResponse) {


yeah I also plan on having tests where I tear down the L2 and verify the L1 500s correctly, or tear down L1 and make sure I can talk directly to L2 -> client, to ensure lifetime issues are really sorted and regression tested against.

alyssawilk · 2022-02-17T15:29:09Z

test/integration/multi_envoy_test.cc

+public:
+  // Create an envoy in front of the original Envoy.
+  void createL1Envoy();
+  ~MultiEnvoyTest() { test_server_.reset(); }


it probably does and clang tidy will catch me :-P Will fix.

alyssawilk · 2022-02-17T15:31:15Z

source/server/config_validation/server.h

@@ -91,7 +91,7 @@ class ValidationInstance final : Logger::Loggable<Logger::Id::main>,
  ServerLifecycleNotifier& lifecycleNotifier() override { return *this; }
  ListenerManager& listenerManager() override { return *listener_manager_; }
  Secret::SecretManager& secretManager() override { return *secret_manager_; }
-  Runtime::Loader& runtime() override { return Runtime::LoaderSingleton::get(); }
+  Runtime::Loader& runtime() override { return runtime_singleton_->instance(); }


exactly. arguably I should do the same restart flag checks here, but I'lll get there in the PR I flip the runtime guard true by default.

Signed-off-by: Alyssa Wilk <[email protected]>

snowp

Seems reasonable to me

What is the behavior if the two instances are run with different options? e.g. if the log output path is different between the two

snowp · 2022-02-17T19:07:34Z

test/integration/multi_envoy_test.cc

+// A simple test to make sure that traffic can flow between client - L1 - L2 - upstream.
+// This test does not currently support mixed protocol hops, or much of the other envoy test
+// framework knobs.
+TEST_P(MultiEnvoyTest, SimpleRequestAndResponse) {


Having some tests that verify different shutdown ordering would be useful I think, just to make sure that they can exist independently of each other

alyssawilk · 2022-02-17T19:46:27Z

re logging: unsure. Added to my list of things on envoyproxy/envoy-mobile#2003 to do in follow up PRs as I don't think I can get it done today :-)

rojkov · 2022-02-28T13:57:21Z

/wait
on CI

Signed-off-by: Alyssa Wilk <[email protected]>

rojkov · 2022-03-01T07:16:18Z

/wait
on merge conflict

Signed-off-by: Alyssa Wilk <[email protected]>

Functionally this handles the multi-envoy signal handler crash skips instantiating a runtime singleton (off by default, must stay off until remove global runtime: rebase runtime features on ABSL_flags envoyproxy#19847 is closed) Multi-envoy does not correctly support runtime flags or deprecation stats due to envoyproxy#19847 being incomplete. It can still handle proxy traffic client - L1 - L2 - upstream as shown in test. Risk Level: low Testing: yes Docs Changes: n/a Release Notes: n/a Part of envoyproxy/envoy-mobile#2003 Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Rex Chang <[email protected]>

* test: adding a multi-envoy test (#20016) Functionally this handles the multi-envoy signal handler crash skips instantiating a runtime singleton (off by default, must stay off until remove global runtime: rebase runtime features on ABSL_flags #19847 is closed) Multi-envoy does not correctly support runtime flags or deprecation stats due to #19847 being incomplete. It can still handle proxy traffic client - L1 - L2 - upstream as shown in test. Risk Level: low Testing: yes Docs Changes: n/a Release Notes: n/a Part of envoyproxy/envoy-mobile#2003 Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Rex Chang <[email protected]> * Add a congestionWindowInBytes method to Envoy::Network::Connection (#20105) Signed-off-by: Bin Wu <[email protected]> Signed-off-by: Rex Chang <[email protected]> * Update QUICHE from 50f15e7a5 to cf1588207 (#20154) https://github.com/google/quiche/compare/50f15e7a5..cf1588207 $ git log 50f15e7a5..cf1588207 --date=short --no-merges --format="%ad %al %s" 2022-02-28 wub Deprecate --gfe2_reloadable_flag_quic_crypto_noop_if_disconnected_after_process_chlo. 2022-02-27 vasilvv Remove QuicheMemSlice(QuicUniqueBufferPtr, size_t) constructor. 2022-02-26 fayang Use std::string instead of absl::string_view in CryptoBufferMap. 2022-02-25 bnc Ignore incoming HTTP/3 MAX_PUSH_ID frames. 2022-02-25 bnc Remove Http3DebugVisitor::OnMaxPushIdFrameSent(). 2022-02-25 bnc Remove QuicSpdySession::CanCreatePushStreamWithId(). 2022-02-25 fayang Deprecate gfe2_reloadable_flag_quic_single_ack_in_packet2. Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Rex Chang <[email protected]> * build(deps): bump actions/stale from 4.1.0 to 5 (#20159) Bumps [actions/stale](https://github.com/actions/stale) from 4.1.0 to 5. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](actions/stale@v4.1.0...v5) --- updated-dependencies: - dependency-name: actions/stale dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Rex Chang <[email protected]> * admin: improve test coverage and increase the coverage-percent threshold (#20025) Adds a missing test for recent lookups now that there are no more fake symbol tables. Adds tests for a variety of override methods defined in admin.h that were previously hard to hit. Adds a benchmark test to establish a baseline for the speedups in #19693 Signed-off-by: Joshua Marantz <[email protected]> Signed-off-by: Rex Chang <[email protected]> * test: removing a bunch of direct runtime singleton access (#19993) Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Rex Chang <[email protected]> * build(deps): bump grpcio-tools in /examples/grpc-bridge/client (#20040) Bumps [grpcio-tools](https://github.com/grpc/grpc) from 1.43.0 to 1.44.0. - [Release notes](https://github.com/grpc/grpc/releases) - [Changelog](https://github.com/grpc/grpc/blob/master/doc/grpc_release_schedule.md) - [Commits](grpc/grpc@v1.43.0...v1.44.0) --- updated-dependencies: - dependency-name: grpcio-tools dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Rex Chang <[email protected]> * adds to spellcheck Signed-off-by: Rex Chang <[email protected]> * xray tracer: set subsegment type for child spans (#2) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * Xray subsegment (#3) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * updates xray subsegment name to use operation name (instead of parent's span name) Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * Xray subsegment (#4) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * updates xray subsegment name to use operation name (instead of parent's span name) Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * adds to spell check dictionary Signed-off-by: Rex Chang <[email protected]> * fixes spellcheck Signed-off-by: Rex Chang <[email protected]> * adds to spellcheck Signed-off-by: Rex Chang <[email protected]> xray tracer: set subsegment type for child spans (#2) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> Xray subsegment (#3) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * updates xray subsegment name to use operation name (instead of parent's span name) Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> Xray subsegment (#4) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * updates xray subsegment name to use operation name (instead of parent's span name) Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * adds to spell check dictionary Signed-off-by: Rex Chang <[email protected]> fixes spellcheck Signed-off-by: Rex Chang <[email protected]> * fixes spell check Signed-off-by: Rex Chang <[email protected]> Co-authored-by: alyssawilk <[email protected]> Co-authored-by: Bin Wu <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Joshua Marantz <[email protected]>

…yproxy#20170) * test: adding a multi-envoy test (envoyproxy#20016) Functionally this handles the multi-envoy signal handler crash skips instantiating a runtime singleton (off by default, must stay off until remove global runtime: rebase runtime features on ABSL_flags envoyproxy#19847 is closed) Multi-envoy does not correctly support runtime flags or deprecation stats due to envoyproxy#19847 being incomplete. It can still handle proxy traffic client - L1 - L2 - upstream as shown in test. Risk Level: low Testing: yes Docs Changes: n/a Release Notes: n/a Part of envoyproxy/envoy-mobile#2003 Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Rex Chang <[email protected]> * Add a congestionWindowInBytes method to Envoy::Network::Connection (envoyproxy#20105) Signed-off-by: Bin Wu <[email protected]> Signed-off-by: Rex Chang <[email protected]> * Update QUICHE from 50f15e7a5 to cf1588207 (envoyproxy#20154) https://github.com/google/quiche/compare/50f15e7a5..cf1588207 $ git log 50f15e7a5..cf1588207 --date=short --no-merges --format="%ad %al %s" 2022-02-28 wub Deprecate --gfe2_reloadable_flag_quic_crypto_noop_if_disconnected_after_process_chlo. 2022-02-27 vasilvv Remove QuicheMemSlice(QuicUniqueBufferPtr, size_t) constructor. 2022-02-26 fayang Use std::string instead of absl::string_view in CryptoBufferMap. 2022-02-25 bnc Ignore incoming HTTP/3 MAX_PUSH_ID frames. 2022-02-25 bnc Remove Http3DebugVisitor::OnMaxPushIdFrameSent(). 2022-02-25 bnc Remove QuicSpdySession::CanCreatePushStreamWithId(). 2022-02-25 fayang Deprecate gfe2_reloadable_flag_quic_single_ack_in_packet2. Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Rex Chang <[email protected]> * build(deps): bump actions/stale from 4.1.0 to 5 (envoyproxy#20159) Bumps [actions/stale](https://github.com/actions/stale) from 4.1.0 to 5. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](actions/stale@v4.1.0...v5) --- updated-dependencies: - dependency-name: actions/stale dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Rex Chang <[email protected]> * admin: improve test coverage and increase the coverage-percent threshold (envoyproxy#20025) Adds a missing test for recent lookups now that there are no more fake symbol tables. Adds tests for a variety of override methods defined in admin.h that were previously hard to hit. Adds a benchmark test to establish a baseline for the speedups in envoyproxy#19693 Signed-off-by: Joshua Marantz <[email protected]> Signed-off-by: Rex Chang <[email protected]> * test: removing a bunch of direct runtime singleton access (envoyproxy#19993) Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Rex Chang <[email protected]> * build(deps): bump grpcio-tools in /examples/grpc-bridge/client (envoyproxy#20040) Bumps [grpcio-tools](https://github.com/grpc/grpc) from 1.43.0 to 1.44.0. - [Release notes](https://github.com/grpc/grpc/releases) - [Changelog](https://github.com/grpc/grpc/blob/master/doc/grpc_release_schedule.md) - [Commits](grpc/grpc@v1.43.0...v1.44.0) --- updated-dependencies: - dependency-name: grpcio-tools dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Rex Chang <[email protected]> * adds to spellcheck Signed-off-by: Rex Chang <[email protected]> * xray tracer: set subsegment type for child spans (#2) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * Xray subsegment (#3) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * updates xray subsegment name to use operation name (instead of parent's span name) Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * Xray subsegment (#4) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * updates xray subsegment name to use operation name (instead of parent's span name) Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * adds to spell check dictionary Signed-off-by: Rex Chang <[email protected]> * fixes spellcheck Signed-off-by: Rex Chang <[email protected]> * adds to spellcheck Signed-off-by: Rex Chang <[email protected]> xray tracer: set subsegment type for child spans (#2) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> Xray subsegment (#3) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * updates xray subsegment name to use operation name (instead of parent's span name) Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> Xray subsegment (#4) * xray tracer: set subsegment type for child spans Signed-off-by: Rex Chang <[email protected]> * adds test coverage Signed-off-by: Rex Chang <[email protected]> * updates xray subsegment name to use operation name (instead of parent's span name) Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * updates doc Signed-off-by: Rex Chang <[email protected]> * adds to spell check dictionary Signed-off-by: Rex Chang <[email protected]> fixes spellcheck Signed-off-by: Rex Chang <[email protected]> * fixes spell check Signed-off-by: Rex Chang <[email protected]> Co-authored-by: alyssawilk <[email protected]> Co-authored-by: Bin Wu <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Joshua Marantz <[email protected]>

test: adding a multi-envoy test

1a608f7

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk assigned RyanTheOptimist Feb 17, 2022

RyanTheOptimist previously approved these changes Feb 17, 2022

View reviewed changes

comment

951803c

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk commented Feb 17, 2022

View reviewed changes

fix

2486a11

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk dismissed RyanTheOptimist’s stale review via 2486a11 February 17, 2022 16:12

alyssawilk assigned snowp Feb 17, 2022

alyssawilk added 2 commits February 17, 2022 11:26

Merge branch 'main' into multi_envoy

8c1e2cc

Signed-off-by: Alyssa Wilk <[email protected]>

test fix

fa6cb9e

Signed-off-by: Alyssa Wilk <[email protected]>

snowp reviewed Feb 17, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 28, 2022

alyssawilk added 2 commits February 28, 2022 12:01

Merge branch 'main' into multi_envoy

6eedd48

Signed-off-by: Alyssa Wilk <[email protected]>

tidy

3dfd2f3

Signed-off-by: Alyssa Wilk <[email protected]>

repokitteh-read-only bot removed the waiting label Feb 28, 2022

alyssawilk added the waiting label Feb 28, 2022

maybe fix test

ec3d21b

Signed-off-by: Alyssa Wilk <[email protected]>

repokitteh-read-only bot removed the waiting label Feb 28, 2022

repokitteh-read-only bot added the waiting label Mar 1, 2022

Merge branch 'main' into multi_envoy

a9a478d

Signed-off-by: Alyssa Wilk <[email protected]>

repokitteh-read-only bot removed the waiting label Mar 1, 2022

alyssawilk enabled auto-merge (squash) March 1, 2022 16:56

RyanTheOptimist approved these changes Mar 1, 2022

View reviewed changes

alyssawilk merged commit c62e064 into envoyproxy:main Mar 1, 2022

alyssawilk deleted the multi_envoy branch March 20, 2023 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: adding a multi-envoy test #20016

test: adding a multi-envoy test #20016

alyssawilk commented Feb 17, 2022

RyanTheOptimist left a comment

RyanTheOptimist Feb 17, 2022

alyssawilk Feb 17, 2022

RyanTheOptimist Feb 17, 2022

alyssawilk Feb 17, 2022

RyanTheOptimist Feb 17, 2022

alyssawilk Feb 17, 2022

snowp Feb 17, 2022

alyssawilk left a comment

alyssawilk Feb 17, 2022

alyssawilk Feb 17, 2022

alyssawilk Feb 17, 2022

snowp left a comment

snowp Feb 17, 2022

alyssawilk commented Feb 17, 2022

rojkov commented Feb 28, 2022

rojkov commented Mar 1, 2022

test: adding a multi-envoy test #20016

test: adding a multi-envoy test #20016

Conversation

alyssawilk commented Feb 17, 2022

RyanTheOptimist 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

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

alyssawilk 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

snowp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented Feb 17, 2022

rojkov commented Feb 28, 2022

rojkov commented Mar 1, 2022