Xds mux unification: introduced unifed mux implementation #17352

dmitri-d · 2021-07-14T22:09:52Z

Commit Message:
Additional Description: PR #2 out of 3. Introduces unified mux implementation
Risk Level: Low, nor enabled atm
Testing: Updated tests
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Ping @htuch

Signed-off-by: Dmitri Dolguikh <[email protected]>

dmitri-d · 2021-07-14T22:13:08Z

Supersedes #17049.

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch · 2021-07-23T00:10:06Z

@adisuissa could you take a first pass? Thanks.

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch

Sorry for the review delay, some questions to kick off.

envoy/config/grpc_mux.h

source/common/config/xds_mux/grpc_mux_impl.cc

htuch · 2021-07-25T20:43:55Z

source/common/config/xds_mux/grpc_mux_impl.cc

+}
+
+template <class S, class F, class RQ, class RS>
+void GrpcMuxImpl<S, F, RQ, RS>::trySendDiscoveryRequests() {


Is it fair to say that this new unified mux is most similar to the existing NewGrpcMuxImpl? If a reviewer were to compare the two side-by-side, what would be the key differences or things to pay attention to?

Is it fair to say that this new unified mux is most similar to the existing NewGrpcMuxImpl.

Definitely. Will add some pointers here a bit later.

The most significant source of difference between the implementations is introduction of templates; most of the time there's little to none difference in logic in methods.

There's no SubscriptionStuff in the unified mux implementation; b/c of that creating/accessing subscription state is a bit different.

Both delta and sotw muxes in the new implementation respect skip_subsequent_node flag; the flag is used insendGrpcMessage() method.

Concrete mux classes are tiny, pretty much all of the logic is in the base class.

Thanks, very helpful, CC @adisuissa

adisuissa

Thanks a lot for working on this!
Here's a first-pass comments/questions I've got.

adisuissa · 2021-07-26T03:51:54Z

source/common/config/grpc_mux_impl.cc

+void NullGrpcMuxImpl::updateWatch(const std::string&, Watch*,
+                                  const absl::flat_hash_set<std::string>&,
+                                  const SubscriptionOptions&) {
+  throw EnvoyException("ADS must be configured to support an ADS config source");
+}
+
+void NullGrpcMuxImpl::removeWatch(const std::string&, Watch*) {
+  throw EnvoyException("ADS must be configured to support an ADS config source");
+}


nit: implement these directly in the header file

We now have a check that yells if exceptions are being raised in headers, which makes me think that such an approach should be used sparingly?

Yeah, no exception throwing in headers :) @adisuissa we have an internal requirement around this.

adisuissa · 2021-07-26T03:53:33Z

source/common/config/grpc_mux_impl.h

+  bool paused(const std::string&) const override { return false; }
+  void updateWatch(const std::string&, Watch*, const absl::flat_hash_set<std::string>&,
+                   const SubscriptionOptions&) override;
+  void removeWatch(const std::string&, Watch*) override;
  void requestOnDemandUpdate(const std::string&, const absl::flat_hash_set<std::string>&) override {
    NOT_IMPLEMENTED_GCOVR_EXCL_LINE;


Why is this not implemented and the other methods throw an exception (ADS must be configured)?

Hmm, I don't feel strongly about this; my thinking was that requestOnDemandUpdate only makes sense for a delta mux, so all other implementations panic. I can throw an exception here if you think it's easier to understand/read.

We shouldn't assume that delta implies ADS though if that's what is suggested.

I do think the original message in the exception is weird (see also here: https://github.com/envoyproxy/envoy/blob/main/source/common/config/grpc_mux_impl.h#L215). Perhaps a simple panic would suffice in those places? I could a separate PR to update the message in legacy mux implementation.

source/common/config/grpc_stream.h

source/common/config/xds_mux/grpc_mux_impl.h

adisuissa · 2021-07-26T04:08:14Z

test/common/config/new_grpc_mux_impl_test.cc

@@ -88,28 +101,73 @@ class NewGrpcMuxImplTestBase : public testing::Test {
    EXPECT_CALL(async_stream_, sendMessageRaw_(Grpc::ProtoBufferEq(expected_request), false));
  }

+  void remoteClose() {
+    if (isUnifiedMuxTest()) {
+      dynamic_cast<XdsMux::GrpcMuxDelta*>(grpc_mux_.get())


It may be better to only cast in the if clause, and then apply the ->grpcStreamForTest().onRemoteClose(Grpc::Status::WellKnownGrpcStatus::Canceled, ""); outside of the if clause. (Should improve maintenance down the road).
Also applies to the functions below.

Not sure what you mean here: grpcStreamForTest() isn't in the mux interface, concrete mux types are needed to invoke it. There isn't a common base between XdsMux::GrpcMuxDelta and NewGrpcMuxImpl, I can't declare a local var to hold the result of such cast...

adisuissa · 2021-07-26T04:12:32Z

test/common/config/new_grpc_mux_impl_test.cc

@@ -126,11 +184,17 @@ TEST_F(NewGrpcMuxImplTest, DynamicContextParameters) {
  // Update to bar type should resend Node.
  expectSendMessage("bar", {}, {});
  local_info_.context_provider_.update_cb_handler_.runCallbacks("bar");
-  expectSendMessage("foo", {}, {"x", "y"});
+  if (!isUnifiedMuxTest()) {
+    // in "legacy" delta mux implementation destruction of "foo_sub"


Is this due to an issue in the legacy-delta implementation? Is there an issue# to add to this?

Nods, In NewGrpcMuxImpl addWatch call returns a wrapper a Watch, which also has a dtor (here: https://github.com/envoyproxy/envoy/blob/main/source/common/config/new_grpc_mux_impl.h#L114) that's being called when foo_sub goes out of scope, and an unsubscribe message is sent. Unified mux implementation returns a thin wrapper around a Watch that's there for compatibility with existing interface only (So I don't need to introduce a very similar addWatch method). This compatibility wrapper doesn't have perform an unsubscribe on destruction, hence the difference.

Signed-off-by: Dmitri Dolguikh <[email protected]>

dmitri-d · 2021-07-28T00:39:25Z

responded to feedback

dmitri-d · 2021-07-28T16:36:00Z

/retest

repokitteh-read-only · 2021-07-28T16:36:04Z

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

🐱

Caused by: a #17352 (comment) was created by @dmitri-d.

see: more, trace.

Signed-off-by: Dmitri Dolguikh <[email protected]>

This reverts commit 5ac0553. Signed-off-by: Dmitri Dolguikh <[email protected]>

Signed-off-by: Dmitri Dolguikh <[email protected]>

This reverts commit 2a7a5af. Signed-off-by: Dmitri Dolguikh <[email protected]>

This reverts commit 58837b7. Signed-off-by: Dmitri Dolguikh <[email protected]>

This reverts commit de40b51. Signed-off-by: Dmitri Dolguikh <[email protected]>

Signed-off-by: Dmitri Dolguikh <[email protected]>

This reverts commit cda496e. Signed-off-by: Dmitri Dolguikh <[email protected]>

Signed-off-by: Dmitri Dolguikh <[email protected]>

This reverts commit 1e58a3d. Signed-off-by: Dmitri Dolguikh <[email protected]>

Signed-off-by: Dmitri Dolguikh <[email protected]>

dmitri-d · 2021-08-19T02:34:45Z

@htuch: this should be good once #17767 has been merged. Thank you and @adisuissa for reviews.

A huge thank you to @davinci26, who helped with test failures under Windows.

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch

LGTM, thanks!

alyssawilk · 2021-09-13T15:17:40Z

test/per_file_coverage.sh

@@ -21,6 +21,7 @@ declare -a KNOWN_LOW_COVERAGE=(
 "source/common/quic:91.2"
 "source/common/tracing:96.1"
 "source/common/watchdog:42.9" # Death tests don't report LCOV
+"source/common/config/xds_mux:94.5"


Sorry, coming in late, why did this need a coverage exemption?

Dmitri Dolguikh added 7 commits July 9, 2021 13:18

Introduced a unified mux implementation

4993962

Signed-off-by: Dmitri Dolguikh <[email protected]>

Removed init fetch timeout from the mux

c43f23c

Signed-off-by: Dmitri Dolguikh <[email protected]>

delta mux tests are passing now

3308ced

Signed-off-by: Dmitri Dolguikh <[email protected]>

Moved most of mux functionality into the base class

4a66a31

Signed-off-by: Dmitri Dolguikh <[email protected]>

Added more mux tests

084a576

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge remote-tracking branch 'upstream/main' into xds-unification-part-2

b2b5729

Signed-off-by: Dmitri Dolguikh <[email protected]>

Fixed vuild failure

916d8ba

Signed-off-by: Dmitri Dolguikh <[email protected]>

dmitri-d mentioned this pull request Jul 14, 2021

WIP: xds mux unification part 2 #17049

Closed

Dmitri Dolguikh added 3 commits July 14, 2021 15:37

Fixed formatting

6bed794

Signed-off-by: Dmitri Dolguikh <[email protected]>

Fixing formatting

7289b6e

Signed-off-by: Dmitri Dolguikh <[email protected]>

Fixing formatting

968b68c

Signed-off-by: Dmitri Dolguikh <[email protected]>

adisuissa self-assigned this Jul 15, 2021

snowp assigned htuch Jul 15, 2021

Dmitri Dolguikh added 5 commits July 16, 2021 11:45

Added tests to improve test coverage

25d7134

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge branch 'main' into xds-unification-mux

6414bf7

Signed-off-by: Dmitri Dolguikh <[email protected]>

small refactoring

8aa55b8

Signed-off-by: Dmitri Dolguikh <[email protected]>

Added grpc_mux_impl.h to code coverage exceptions with 69.8 coverage

d9e3966

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge remote-tracking branch 'upstream/main' into xds-unification-mux

4947a4b

Signed-off-by: Dmitri Dolguikh <[email protected]>

Fixed per-extension coverage for xds_mux

421b56e

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch reviewed Jul 25, 2021

View reviewed changes

adisuissa reviewed Jul 26, 2021

View reviewed changes

Dmitri Dolguikh added 4 commits July 27, 2021 11:26

Merge remote-tracking branch 'upstream/main' into xds-unification-mux

93ea6f5

Signed-off-by: Dmitri Dolguikh <[email protected]>

Moved grpc_stream_ from concrete classes into the base

284cdca

Signed-off-by: Dmitri Dolguikh <[email protected]>

removed paused() method from mux interface

1fd1c36

Signed-off-by: Dmitri Dolguikh <[email protected]>

Removed BaseGrpcStream interface

a11a2dc

Signed-off-by: Dmitri Dolguikh <[email protected]>

Dmitri Dolguikh added 22 commits August 16, 2021 20:32

Merge remote-tracking branch 'upstream/main' into xds-unification-mux

335d7d6

Signed-off-by: Dmitri Dolguikh <[email protected]>

trying to fix windows build

533bb6f

Signed-off-by: Dmitri Dolguikh <[email protected]>

Fixed formatting

f8178dc

Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix tests under windows

5ac0553

Signed-off-by: Dmitri Dolguikh <[email protected]>

Revert "Trying to fix tests under windows"

03f7a7c

This reverts commit 5ac0553. Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures on Windows

de40b51

Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures on Windows

58837b7

Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures on Windows

2a7a5af

Signed-off-by: Dmitri Dolguikh <[email protected]>

Revert "Trying to fix test failures on Windows"

dfe3515

This reverts commit 2a7a5af. Signed-off-by: Dmitri Dolguikh <[email protected]>

Revert "Trying to fix test failures on Windows"

ab0cab5

This reverts commit 58837b7. Signed-off-by: Dmitri Dolguikh <[email protected]>

Revert "Trying to fix test failures on Windows"

349de21

This reverts commit de40b51. Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures under Windows

cda496e

Signed-off-by: Dmitri Dolguikh <[email protected]>

Revert "Trying to fix test failures under Windows"

3777c61

This reverts commit cda496e. Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures under Windows

1b5564c

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge remote-tracking branch 'upstream/main' into xds-unification-mux

1970420

Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures under Windows

95075f9

Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures under Windows

1e58a3d

Signed-off-by: Dmitri Dolguikh <[email protected]>

Revert "Trying to fix test failures under Windows"

ba094fb

This reverts commit 1e58a3d. Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures under Windows

e169379

Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures under Windows

893ce72

Signed-off-by: Dmitri Dolguikh <[email protected]>

Trying to fix test failures under Windows

1ab61ad

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge remote-tracking branch 'upstream/main' into xds-unification-mux

bfde33b

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge remote-tracking branch 'upstream/main' into xds-unification-mux

cd20489

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch approved these changes Aug 20, 2021

View reviewed changes

htuch merged commit fd6e675 into envoyproxy:main Aug 20, 2021

dmitri-d deleted the xds-unification-mux branch August 20, 2021 15:28

alyssawilk reviewed Sep 13, 2021

View reviewed changes

adisuissa mentioned this pull request Jul 17, 2023

Converge to unified gRPC mux #28442

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xds mux unification: introduced unifed mux implementation #17352

Xds mux unification: introduced unifed mux implementation #17352

dmitri-d commented Jul 14, 2021

dmitri-d commented Jul 14, 2021

htuch commented Jul 23, 2021

htuch left a comment

htuch Jul 25, 2021

dmitri-d Jul 26, 2021

dmitri-d Jul 28, 2021

htuch Jul 29, 2021

adisuissa left a comment

adisuissa Jul 26, 2021

dmitri-d Jul 26, 2021 •

edited

Loading

htuch Jul 29, 2021

adisuissa Jul 26, 2021

dmitri-d Jul 26, 2021

htuch Aug 1, 2021

dmitri-d Aug 3, 2021

adisuissa Jul 26, 2021

dmitri-d Jul 28, 2021

adisuissa Jul 26, 2021

dmitri-d Jul 26, 2021

dmitri-d commented Jul 28, 2021

dmitri-d commented Jul 28, 2021

repokitteh-read-only bot commented Jul 28, 2021

dmitri-d commented Aug 19, 2021

htuch left a comment

alyssawilk Sep 13, 2021

Xds mux unification: introduced unifed mux implementation #17352

Xds mux unification: introduced unifed mux implementation #17352

Conversation

dmitri-d commented Jul 14, 2021

dmitri-d commented Jul 14, 2021

htuch commented Jul 23, 2021

htuch 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

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d Jul 26, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented Jul 28, 2021

dmitri-d commented Jul 28, 2021

repokitteh-read-only bot commented Jul 28, 2021

dmitri-d commented Aug 19, 2021

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d Jul 26, 2021 •

edited

Loading