WIP: xDS mux unification #15473

dmitri-d · 2021-03-13T00:54:29Z

Fixes #11477, reintroduces changes in #8974. and supersedes work in #14496.

Unified mux implementation (located in source/common/config/unified_mux dir) is completely in parallel to the original muxes, and leaves the latter virtually untouched
Unified muxes are disabled by default. To enable. set envoy.reloadable_features.unified_mux runtime flag set to true.
Tests that required minimal changes in order to test unified muxes (these were integration-level tests, fuzz- and performance-tests) were modified to include unified muxes.
Tests specific to unified muxes have unified prepended to their names (test/common/config/unified_grpc_mux_impl_test.cc is one of those)

TODOs:

document the runtime flag
More delta mux-specific unit tests are needed

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

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

dmitri-d · 2021-03-13T01:12:51Z

ping @htuch, @asraa.

I know you asked to break up the work into smaller chunks, but the only bits I could break out are the factory and interface changes, which are pretty trivial changes. The bulk of work is unified mux implementation + tests (new and updated). All unified mux code is now in source/common/config/unified_mux dir, and is completely independent of existing mux implementation. All tests specific to unified mux work are in test/common/config and have unified_ prefix. Additional notes on implementation:

source/common/config/subscription_factory_impl.cc and source/common/upstream/cluster_manager_impl.cc are the places where unifed muxes are intantiated (based on the value of envoy.reloadable_features.unified_mux).
I had to touch current implementation of muxes, but only to add stubs for new interfaces, which aren't being used anywhere.

dmitri-d · 2021-03-13T01:19:50Z

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

I'll try documenting the changes next week. in the meantime test/integration/ads_integration_test.cc should illustrate the main differences.

ping @howardjohn

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

howardjohn

So looks like the main changes:

if we send CDS v1, then CDS v2 while v1 is still warming, we don't get an ACK for both - just for v2 once its done warming
If we send CDS while CDS is paused, we will get 2 duplicate ACKs
?

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

htuch · 2021-03-16T17:23:06Z

@dmitri-d @adisuissa is going to take a first pass.

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

dmitri-d · 2021-04-08T00:18:17Z

ping @adisuissa, any chance you'll have time to take a look at this in the near future?

dmitri-d · 2021-04-23T23:22:02Z

Ping @adisuissa: any chance you could spend some time on this? I'm concern that changes to current muxes need to be ported here, which in turn makes the review harder...

adisuissa

Thanks for working on this!
I’ve done a few iterations over the code, trying to understand what can be done to make sure we are not missing some functionality, and not breaking current functionality.
The runtime feature is a great step to make sure we don’t break anything w/o the ability to go back to things that work.
It's still challenging to see what was added and why, because of the amount of code that was changed, and I'm trying to compare the original to the unified and to the other variant (delta to SotW).

Regarding how to make this PR more digestible, by making a series of small PRs. If we can have incremental changes, each with additional functionality, it would make it easier to understand what was changed and why. An alternative is to add things in the following order: SubscriptionState, GrpcMux, and then GrpcSubscription (these can be split into SotW and delta sub-PRs). If you’ve got other ideas on how to break this down even further this would be really helpful.

Tests: I suggest making the delta and the unified-delta use the same tests, and the sotw and unified-sotw use the same tests (as much as possible). The idea is to make sure any change in one component doesn’t negatively impact the other, and that all tests are written in a single place.

A couple of general things:

the use of void* in some place where it should be Delta/Discovery{Request/Response}. It might be possible to avoid them by using templates in the base class, and specializing it in the derived class.
Some of the code is added to allow the unified variant to work well, and will be removed when the legacy code is removed.This should be documented (add TODOs).

adisuissa · 2021-04-08T04:15:13Z

include/envoy/config/grpc_mux.h

+   * Passes through to all multiplexed SubscriptionStates. To be called when something
+   * definitive happens with the initial fetch: either an update is successfully received,
+   * or some sort of error happened.*/
+  virtual void disableInitFetchTimeoutTimer() PURE;


It seems that this and the above methods are not implemented in the non-unified versions. If that's the case, would it be possible to create an interface between GrpcMux and the unified versions that adds these methods?

adisuissa · 2021-04-08T05:07:52Z

source/common/config/unified_mux/subscription_state.h

+  // understanding of the current protocol state, and new resources that Envoy wants to request.
+  // Returns a new'd pointer, meant to be owned by the caller, who is expected to know what type the
+  // pointer actually is.
+  virtual void* getNextRequestAckless() PURE;


I wonder if the use of void* can be avoided by using templates in the base class, and specialising it in the derived class.

adisuissa · 2021-04-08T13:30:45Z

source/common/config/unified_mux/grpc_mux_impl.cc

+
+void GrpcMuxSotw::establishGrpcStream() { grpc_stream_.establishNewStream(); }
+
+void GrpcMuxSotw::sendGrpcMessage(void* msg_proto_ptr, SubscriptionState& sub_state) {


Other than the cast, this function and its GrpcMuxDelta counterpart look similar. Can this be refactored so the same logic will be in the base class?

adisuissa · 2021-04-08T14:32:05Z

source/common/config/unified_mux/grpc_mux_impl.h

+    return subscriptions_;
+  }
+
+  // legacy mux interface not implemented by unified mux.


I suggest adding comments to the places that will eventually be removed once the non-unified code is removed.

adisuissa · 2021-04-09T17:35:16Z

test/common/config/sotw_subscription_state_test.cc

+                   envoy::config::core::v3::ApiVersion::V3),
+               callbacks_, std::chrono::milliseconds(0U), dispatcher_) {
+    state_.updateSubscriptionInterest({"name1", "name2", "name3"}, {});
+    auto cur_request = getNextDiscoveryRequestAckless();


nit: const here and in simialr places

adisuissa · 2021-04-09T17:45:48Z

test/common/config/unified_delta_subscription_state_test.cc

+TEST_F(DeltaSubscriptionStateTest, SubscribeAndUnsubscribe) {
+  {
+    state_.updateSubscriptionInterest({"name4"}, {"name1"});
+    auto cur_request = getNextDeltaDiscoveryRequestAckless();


nit: const here and in similar places.

adisuissa · 2021-04-09T18:10:49Z

source/common/config/unified_mux/grpc_mux_impl.cc

+    watch_map =
+        watch_maps_.emplace(type_url, std::make_unique<WatchMap>(use_namespace_matching)).first;
+    subscriptions_.emplace(type_url, subscription_state_factory_->makeSubscriptionState(
+                                         type_url, *watch_maps_[type_url], init_fetch_timeout));


nit: instead of calling *watch_maps_[type_url] you should be able to save the result of adding to watch_maps_ above, and take the second element.

adisuissa · 2021-04-09T19:00:03Z

source/common/config/unified_mux/grpc_mux_impl.cc

+  return std::make_unique<Cleanup>([this, type_urls]() {
+    for (const auto& type_url : type_urls) {
+      pausable_ack_queue_.resume(type_url);
+      trySendDiscoveryRequests();


IIUC, previously this issued a request just for the resumed type_url, but now it will send discovery requests for all type_url that are available.
I think querying & sending to a specific type_url makes sense, but the alternative should probably be to call trySendDiscoveryRequests() after the for loop.

adisuissa · 2021-04-09T19:24:50Z

source/common/config/unified_mux/grpc_mux_impl.cc

+              type_url);
+    return;
+  }
+  pausable_ack_queue_.push(sub->second->handleResponse(response_proto_ptr));


The returned UpdateAck should probably be moved (std::move) to the queue.

adisuissa · 2021-04-09T19:55:56Z

source/common/config/unified_mux/grpc_mux_impl.h

+  std::unique_ptr<SubscriptionStateFactory> subscription_state_factory_;
+
+  // Map key is type_url.
+  // Only addWatch() should insert into these maps.


Would it make more sense to have a single map between the type_url to both the WatchMapPtr and SubscriptionStatePtr (so they will always be in sync)?

github-actions · 2021-05-26T12:03:16Z

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!

github-actions · 2021-06-02T12:20:13Z

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!

htuch · 2021-06-09T02:28:13Z

Sorry, I've been OOO, will take a look in the next couple of days.

dmitri-d · 2021-06-09T02:30:20Z

@htuch: I started breaking up this PR into smaller ones, #16486 is the one that needs reviews.

Split out subscription state out of xDS mux unification PR (#15473). Made base subscription state class a template. Updated existing delta state tests to work with both legacy and new implementations. Risk Level: low, the code is not being used atm Testing: updated existing tests, added new ones Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Split out subscription state out of xDS mux unification PR (envoyproxy#15473). Made base subscription state class a template. Updated existing delta state tests to work with both legacy and new implementations. Risk Level: low, the code is not being used atm Testing: updated existing tests, added new ones Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Port of unified mux work

e94c160

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

dmitri-d mentioned this pull request Mar 13, 2021

WIP: xDS mux unification #14496

Closed

dmitri-d marked this pull request as draft March 13, 2021 01:20

Format fixes

3804a2e

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

mattklein123 assigned adisuissa and htuch Mar 14, 2021

howardjohn reviewed Mar 15, 2021

View reviewed changes

Dmitri Dolguikh added 6 commits March 15, 2021 11:10

Fixed spelling errors

9113742

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

Fixed spelling mistakes

0a2e7e2

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

Fixed spelling

e56a976

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

Fixing spelling mistakes

839779e

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

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

de6836b

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

Fixing formatting issues

fd122cd

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

Dmitri Dolguikh added 2 commits March 16, 2021 16:37

Fixing asan test failures

95cb8d9

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

Fixing asan test failure

db5b5b1

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

yanavlasov self-assigned this Apr 13, 2021

adisuissa reviewed Apr 26, 2021

View reviewed changes

dmitri-d mentioned this pull request May 13, 2021

xDS unified mux: subscription states #16486

Merged

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

github-actions bot closed this Jun 2, 2021

htuch reopened this Jun 9, 2021

htuch closed this Jun 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: xDS mux unification #15473

WIP: xDS mux unification #15473

dmitri-d commented Mar 13, 2021

dmitri-d commented Mar 13, 2021

dmitri-d commented Mar 13, 2021 •

edited

Loading

howardjohn left a comment

htuch commented Mar 16, 2021

dmitri-d commented Apr 8, 2021

dmitri-d commented Apr 23, 2021

adisuissa left a comment

adisuissa Apr 8, 2021

adisuissa Apr 8, 2021

adisuissa Apr 8, 2021

adisuissa Apr 8, 2021

adisuissa Apr 9, 2021

adisuissa Apr 9, 2021

adisuissa Apr 9, 2021

adisuissa Apr 9, 2021

adisuissa Apr 9, 2021

adisuissa Apr 9, 2021

github-actions bot commented May 26, 2021

github-actions bot commented Jun 2, 2021

htuch commented Jun 9, 2021

dmitri-d commented Jun 9, 2021


		void GrpcMuxSotw::establishGrpcStream() { grpc_stream_.establishNewStream(); }

		void GrpcMuxSotw::sendGrpcMessage(void* msg_proto_ptr, SubscriptionState& sub_state) {

WIP: xDS mux unification #15473

WIP: xDS mux unification #15473

Conversation

dmitri-d commented Mar 13, 2021

dmitri-d commented Mar 13, 2021

dmitri-d commented Mar 13, 2021 • edited Loading

howardjohn left a comment

Choose a reason for hiding this comment

htuch commented Mar 16, 2021

dmitri-d commented Apr 8, 2021

dmitri-d commented Apr 23, 2021

adisuissa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 26, 2021

github-actions bot commented Jun 2, 2021

htuch commented Jun 9, 2021

dmitri-d commented Jun 9, 2021

dmitri-d commented Mar 13, 2021 •

edited

Loading