WIP: xDS mux unification #14496

dmitri-d · 2020-12-21T19:57:26Z

Fixes #11477.

reintroduces config: reinstate #8478 (unification of delta and SotW xDS), reverted by #8939 #8974
Ported "legacy" grpc mux and grpc_subscription_impl, to provide wire-level compatibility for existing server implementations
Legacy mux and subscription are used by default and controlled by the "envoy.reloadable_features.legacy_sotw_xds" runtime config feature.
Integration-level tests (ads, cds, etc) updated to test both legacy and unified muxes

TODO's

update runtime config docs
port unit-level tests for legacy mux and subscription

Signed-off-by: Fred Douglas <fredlas@google.com>

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

This reverts commit 30ce222. This restoration was to test my theory that 8350 was the cause of the hang. Now that we have established it wasn't, it will be good to have this cleaner, more sensible behavior back in. Signed-off-by: Fred Douglas <fredlas@google.com>

Signed-off-by: Fred Douglas <fredlas@google.com>

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

…ing behavior, which treats any news about the fetch as precluding a timeout, as opposed to only wanting a successful one Signed-off-by: Fred Douglas <fredlas@google.com>

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

Signed-off-by: Fred Douglas <fredlas@google.com>

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

Signed-off-by: Fred Douglas <fredlas@google.com>

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

…ementation that is wire-compatible with the legacy grpc mux used in non-delta xDS Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

repokitteh-read-only · 2020-12-21T19:57:37Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14496 was opened by dmitri-d.

see: more, trace.

dmitri-d · 2020-12-21T19:57:58Z

ping @htuch, @snowp

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

lizan

Just API review, can you check CI and then I will look into code.

lizan · 2021-01-05T20:33:42Z

api/envoy/api/v2/core/config_source.proto

@@ -51,7 +51,7 @@ message ApiConfigSource {
    // the v2 protos is used.
    REST = 1;

-    // gRPC v2 API.
+    // "State of the world" gRPC v2 API, using Discovery{Request,Response} protos.


remove v2, as we're moving to v3. or actually revert this.

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch

Thanks @dmitri-d, this is epic. A couple of high-level questions:

Did you end up root causing the original rollback? Do we know why things would be different this time around?
Is there any way to break this PR up into smaller PRs or recommend a review strategy? This is a lot of churn and code which appears new (but maybe comes from elsewhere?). The GitHub UX makes it hard to track this and reason about correctness.

htuch · 2021-01-06T00:21:16Z

source/common/config/README.md

+[Delta]DiscoveryRequest protos.
+
+What does GrpcMux even do in this diagram? Just own things and pass through function calls?
+Answer: 1) it sequences the requests and ACKs that the various type_urls send, 2) it handles the


Can you use proper markdown enumerated lists?

dmitri-d · 2021-01-07T00:45:41Z

Did you end up root causing the original rollback? Do we know why things would be different this time around?

I did not; my understanding is that @rgs1 and @fredlas couldn't find anything in the original work. @rgs offered help testing changes (here: #11477 (comment)), hopefully it's still on. I ported legacy (non-delta xDS) mux and subscription implementations into this branch, which should make the transition easier. The legacy mux is enabled by default.

Is there any way to break this PR up into smaller PRs or recommend a review strategy? This is a lot of churn and code which appears new (but maybe comes from elsewhere?).

The core of the changes is the original PR; however it had to be updated to accommodate things like subascription resumption (ScopedResume), ttl, etc. Some of the issues are due to naming: diff on source/common/config/grpc_mux_impl.cc is not going to be very useful, for example, as the new mux implementation lives there now.

I'd suggest looking at changes in source/common/config/legacy_grpc_mux_impl.* and source/common/config/legacy_grpc_subscription_impl.* and their use first: this are the original mux and subscription implementations that should guarantee compatibility with existing xDS server implementations.

A next good step, IMO, would be to look at delta xDS mux and subscription implementations and their use (here: source/common/config/delta_subscription_state.cc
and here

envoy/source/common/config/grpc_mux_impl.h

Line 144 in e8033f7

class GrpcMuxDelta

). These are, to a large extent, refactorings of current delta mux and state implementations and should have a familiar feel.

I'd leave SotW mux, subscription, and subscription state for last. I think TTL logic can be reviewed separately too, after the main functionality has been dealt with.

Hope this helps, let me know if there's anything else I can do to help with reviewing of this PR.

htuch · 2021-02-02T20:11:35Z

@adisuissa will help with a first pass, thanks!

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2021-02-05T22:48:50Z

@adisuissa: please let me know if there's anything I can do to make this review easier...

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch

Again, thanks for taking this on @dmitri-d. Following up on IM conversations, I can provide some high-level guidance here and @adisuissa will dive deeper. I think though we might want to consider splitting some of this into distinct PRs.

Some high-level suggestions to be able to land this somewhat incrementally and with better reviewability:

An initial PR that just introduces the new runtime option in the subscription factory and cluster manager. It should switch between new/old variants but keep the same name for existing SotW and for the new GrpxMuxSotw just have some dummy implementation.
Don't change names until a final PR for anything that doesn't need it. E.g. can we keep GrpxMuxImpl instead of GrpcMuxSotw for now? Can we keep NewGrpcMuxImpl instead of GrpcMuxDelta? These newer Sotw/Delta names should ideally be the names in the end state, but hopefully that is a trivial rename pass once we have the functional stuff sorted.

One thing that concerns me is that the amount of change in something like GrpcMuxImpl that isn't protected by the runtime flags is very high.

If we had as a starting point the guarantee that zero code changes in existing grpc_mux_impl.cc and new_grpc_mux_impl.cc, that these are the runtime default from the factories, and then we build up the replacements over a series of PRs, I think this would have higher safety guarantee and be easier to review.

The downside of this is that we will end up with three (or maybe four?) different gRPC muxes effectively during the transition, which is pretty horrendous but might be safest and the most incremental approach. The reason I'm making this case is that even something like grp_mux_impl.cc, which should be the legacy path, has significant delta in this PR.

This is just a suggestion, curious to hear from you and @adisuissa on this topic.

htuch · 2021-02-07T02:23:59Z

source/common/config/legacy_grpc_subscription_impl.h

+/**
+ * Adapter from typed Subscription to untyped GrpcMux. Also handles per-xDS API stats/logging.
+ */
+class LegacyGrpcSubscriptionImpl : public Subscription,


How come we need a legacy subscription? The subscription is largely independent of the gRPC mux itself, it would seem not a lot of logic should change here.

Largely due to init_fetch_timeout timer moving to watches. I think having a dedicated implementation is easier/safer/more maintainable than having conditionals in one common implementation.

dmitri-d · 2021-02-09T02:05:50Z

I agree that preserving the names of the classes would make reviewing much easier, will check how much work it is.

adisuissa · 2021-02-09T04:33:41Z

+1 for incremental changes, and for creating a new unified mux without renaming the old ones.
Also, suggest to have a runtime flag as an opt-in for the next-gen (unified) mux.
Another possibility to consider is to introduce the unified mux as another GrpcMux extended class, and try to add its functionality and introduce incremental changes.

adisuissa · 2021-02-09T14:43:14Z

test/integration/cds_integration_test.cc

@@ -160,7 +164,7 @@ TEST_P(CdsIntegrationTest, CdsClusterUpDownUp) {
  ASSERT_TRUE(codec_client_->waitForDisconnect());

  // Tell Envoy that cluster_1 is back.
-  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "42", {}, {}, {}));
+  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "42", {}, {}, {}, false));


In many of the compareDiscoveryRequest calls the PR adds the 6th arg as false (which IIUC should be false by default). Would it be better to remove it from these calls? (less changes across all integration tests)

github-actions · 2021-03-11T16:09:27Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

howardjohn · 2021-03-11T18:20:39Z

source/common/runtime/runtime_features.cc

@@ -74,6 +74,7 @@ constexpr const char* runtime_features[] = {
    "envoy.reloadable_features.http_transport_failure_reason_in_body",
    "envoy.reloadable_features.http_upstream_wait_connect_response",
    "envoy.reloadable_features.http2_skip_encoding_empty_trailers",
+    "envoy.reloadable_features.legacy_sotw_xds",


Can you give some details on the control plane facing changes that enabling/disabling this flag causes?

closed in favour of #15473

github-actions · 2021-03-20T04:03:40Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

fredlas and others added 25 commits November 25, 2019 12:09

Un-revert the revert of xDS (#9044)

c4065cb

Signed-off-by: Fred Douglas <fredlas@google.com>

remove unused var

38a29ed

Signed-off-by: Fred Douglas <fredlas@google.com>

restore the pre-WatchMap SotW behavior removed by 8350

30ce222

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into UNR_unrevert_xds_…

53a23d4

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

finish removing the vestigial resource_names_ and surrounding cruft

2342de6

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into UNR_unrevert_xds_…

d311dd7

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

swap disable init fetch timer call and onConfigUpdate, to match exist…

496c8b8

…ing behavior, which treats any news about the fetch as precluding a timeout, as opposed to only wanting a successful one Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into UNR_unrevert_xds_…

f79e8b2

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

add debug logging to new SotW xDS subscription implementation

e26c18f

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into UNR_unrevert_xds_…

baeacf9

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into UNR_unrevert_xds_…

886667d

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

more logging, tighter popping of PausableAckQueue just in case

efb453e

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into UNR_unrevert_xds_…

23b8270

…unification Signed-off-by: Fred Douglas <fredlas@google.com>

Merge branch 'xds-unification-fred' into xds-unification

50f566b

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Format fixes

d5b01e6

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge remote-tracking branch 'upstream' into xds-unification

1890777

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

All tests are passing now

da6b77a

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixed formatting

d6e6c34

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge remote-tracking branch 'upstream' into xds-unification

30093c7

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Ported GrpcMux and GrpcSubscription from master to provide a mux impl…

c786196

…ementation that is wire-compatible with the legacy grpc mux used in non-delta xDS Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Formatting fixes

9442319

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge remote-tracking branch 'upstream' into xds-unification

469c3c1

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

fixes for errors reported by codeql

f16341f

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge remote-tracking branch 'upstream' into xds-unification

d5e7a10

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

repokitteh-read-only bot added v2-freeze api labels Dec 21, 2020

repokitteh-read-only bot assigned lizan Dec 21, 2020

Dmitri Dolguikh added 5 commits January 4, 2021 12:11

Fixed formatting issues

a74d1a7

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixed formatting issues

e41dbcc

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixing formatting

abe849f

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixing formatting

d8552ac

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixing formatting

e8033f7

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

lizan reviewed Jan 5, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into xds-unification

9b699d1

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch reviewed Jan 6, 2021

View reviewed changes

htuch mentioned this pull request Jan 6, 2021

xdstp: LDS glob collection support. #14311

Merged

Base automatically changed from master to main January 15, 2021 23:02

lizan added the waiting label Jan 22, 2021

htuch assigned adisuissa Feb 2, 2021

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

3089b99

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

repokitteh-read-only bot removed the waiting label Feb 5, 2021

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

92e30c9

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Format fixes

e62f5e1

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch reviewed Feb 7, 2021

View reviewed changes

htuch mentioned this pull request Feb 7, 2021

set_node_on_first_message_only should be implemented for delta xDS #14976

Open

adisuissa reviewed Feb 9, 2021

View reviewed changes

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 11, 2021

howardjohn reviewed Mar 11, 2021

View reviewed changes

dmitri-d mentioned this pull request Mar 13, 2021

WIP: xDS mux unification #15473

Closed

github-actions bot closed this Mar 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: xDS mux unification #14496

WIP: xDS mux unification #14496

dmitri-d commented Dec 21, 2020

repokitteh-read-only bot commented Dec 21, 2020

dmitri-d commented Dec 21, 2020

lizan left a comment

lizan Jan 5, 2021 •

edited

Loading

htuch left a comment

htuch Jan 6, 2021

dmitri-d commented Jan 7, 2021

htuch commented Feb 2, 2021

dmitri-d commented Feb 5, 2021

htuch left a comment

htuch Feb 7, 2021

dmitri-d Feb 9, 2021

dmitri-d commented Feb 9, 2021

adisuissa commented Feb 9, 2021

adisuissa Feb 9, 2021

github-actions bot commented Mar 11, 2021

howardjohn Mar 11, 2021

dmitri-d Mar 13, 2021

github-actions bot commented Mar 20, 2021

WIP: xDS mux unification #14496

WIP: xDS mux unification #14496

Conversation

dmitri-d commented Dec 21, 2020

repokitteh-read-only bot commented Dec 21, 2020

dmitri-d commented Dec 21, 2020

lizan left a comment

Choose a reason for hiding this comment

lizan Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

htuch Jan 6, 2021

Choose a reason for hiding this comment

dmitri-d commented Jan 7, 2021

htuch commented Feb 2, 2021

dmitri-d commented Feb 5, 2021

htuch left a comment

Choose a reason for hiding this comment

htuch Feb 7, 2021

Choose a reason for hiding this comment

dmitri-d Feb 9, 2021

Choose a reason for hiding this comment

dmitri-d commented Feb 9, 2021

adisuissa commented Feb 9, 2021

adisuissa Feb 9, 2021

Choose a reason for hiding this comment

github-actions bot commented Mar 11, 2021

howardjohn Mar 11, 2021

Choose a reason for hiding this comment

dmitri-d Mar 13, 2021

Choose a reason for hiding this comment

github-actions bot commented Mar 20, 2021

lizan Jan 5, 2021 •

edited

Loading