Optimize EDS stream allocation by nezdolik · Pull Request #22419 · envoyproxy/envoy

nezdolik · 2022-07-26T22:42:41Z

Signed-off-by: Kateryna Nezdolii [email protected]

Commit Message: Optimize streams allocation for Eds Grpc
Additional Description: With this change Eds Grpc subscriptions towards the same management server reuse a shared grpc connection, which greatly reduces the number of concurrent H2 streams towards a single management server. The work is built on top of 5026, with bugs fixed and an addition of integration test for Eds over Grpc. I could not port 3 existing Eds integration tests for warming clusters, as they were blocked by non graceful shutdown of GoogleAsyncClient.

The change has also been tested in production Spotify environment, we use Eds Grpc. Envoy successfully resolved cluster load assignments via optimized Eds.

Risk Level: High
Testing: Unit tests + integration tests
Docs Changes: TBD
Release Notes: TBD
Platform Specific Features:
Fixes #2943

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik · 2022-07-26T22:43:16Z

cc @htuch we have chatted about this change long time ago prior to my parental leave.

htuch · 2022-07-27T04:20:45Z

Thanks so much @nezdolik, this is a definitely long asked for feature from the community and will be a welcome contribution. Assigning @adisuissa for review, since Adi is now leading work around xDS architecture in Envoy.

Signed-off-by: Kateryna Nezdolii <[email protected]>

alyssawilk · 2022-08-01T14:04:45Z

/wait on CI fixes

adisuissa · 2022-08-03T12:06:46Z

Apologies for the late reply.
Thanks for working on this, and thanks for trying this in prod and giving us the feedback!
Overall I think we would like to have this support for any xDS service, not only for EDS.

I'll start by giving some background on where I think we should be heading, and then answer your questions.

In the future, as we move towards the new xDS-TP, the subscriptions to the same authority will essentially be multiplexed using this feature. Your optimized EDS stream allocation will work for each ADS subscription to a specific authority.
For now, we have a single gRPC stream when ADS is used (which doesn't need this multiplexing), and for non-ADS we have per-subscription stream.

IIUC the current design is: The cluster manager has a single EdsSubscriptionFactory which maps between config and a gRPC stream.
In the future, we will probably have a ConfigSubscriptionsManager in the server, that will own all the subscriptions, and a map between a config-source (and authority) to its actual GrpcMux. The GrpcMux can be configured to either SotW or Delta, and will abstract over it (just as is being done today).

For this PR I've got a few high-level comments to start with:

Should the EdsSubscriptionFactory extend SubscriptionFactoryImpl, and just have a method to return the correct GrpcMux (overriding the current create methods, and calling them if the subscription isn't there already)?
Can we use a different name for this factory, maybe MultiplexedSubscriptionFactory? (other ideas to make it more generic are welcome :) )
Can we add some integration tests?
For now I do agree that this factory should co-exist with the current (non-multiplexed) subscription factory, but we should gradually move away from using the original one. I think that introducing (and guarding) it for each xDS-type is the right way forward.

Addressing your questions:

Do we need support for Delta Grpc APi type?
Yes, we should support both SotW and Delta, but I don't think it will add too much to this implementation.

Do we need support for unified mux?
The unified-mux code essentially uses the same code-base for both SotW and Delta implementations, so yes we would like to support it. In the near future we will move to that code-base and abandon the old one. The unified mux code should implement the GrpcMux interface, so I don't think it will require much work.

Implement collectionSubscriptionFromUrl method? (part of udpa/resource federation)
Yes, it should be implemented, but I think it is very straight-forward.

Hide change behind feature flag in api? (as it is high risk)
I think this is highly desired, at least temporarily, as we want to allow the use of the current subscription code if we break anyone.

LMK if there's something unclear, and if I can help with anything.

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik · 2022-08-13T22:16:51Z

@adisuissa thanks for a very detailed feedback and apologies for late reply, I have been stuck in figuring out mocks fix for the remaining Eds tests.

I agree that MultiplexedSubscriptionFactory should be a general feature that could be enabled for any XDS type (since RDS and SDS currently expose the same perf problem for a large number of resources).We could start by implementing a generic MultiplexedSubscriptionFactory and exposing it as a config option in EDS api as a start, does that sound good?

I will start addressing your feedback in a series of commits and will ping you when all parts are done.

May need some help later with the integration test, I was not completely sure on which signal/stats to rely to verify that grpc channel is being multiplexed in the test(may figure it out later in the process). The original idea was to look at Envoy stats for successfully completed requests for an H2 Eds cluster, since in H2 each stream maps to a single logical request/response.

adisuissa · 2022-08-15T09:15:13Z

I agree that MultiplexedSubscriptionFactory should be a general feature that could be enabled for any XDS type (since RDS and SDS currently expose the same perf problem for a large number of resources).We could start by implementing a generic MultiplexedSubscriptionFactory and exposing it as a config option in EDS api as a start, does that sound good?

Sounds good to me, thanks for all the efforts!

May need some help later with the integration test, I was not completely sure on which signal/stats to rely to verify that grpc channel is being multiplexed in the test(may figure it out later in the process). The original idea was to look at Envoy stats for successfully completed requests for an H2 Eds cluster, since in H2 each stream maps to a single logical request/response.

I wonder if the upstream statistics can be used to count the number of connections.
If you point me to the specific test, I can try to take a look.

adisuissa · 2022-08-17T13:22:32Z

/wait-any

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik · 2022-08-19T22:56:03Z

Applied portion of review comments:

EdsSubscriptionFactory renamed to MultiplexedSubscriptionFactory
MultiplexedSubscriptionFactory extends SubscriptionFactoryImpl and overrides method for creating mux
MultiplexedSubscriptionFactory shares mux for subscriptions per management server per xds type url.
Updated MultiplexedSubscriptionFactory test

I will now work on integration test, add tests for new feature and make it parametrised for xds type (sotw, delta) and grpc flavour (envoy, google).
@adisuissa I am not sure if upstream stats for number of connections is good enough signal to rely on. Since mux manages a single grpc stream (according to docs) and grpc channel may have many streams.

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik · 2022-09-04T22:46:52Z

test/integration/eds_integration_test.cc

+  EXPECT_EQ(0, test_server_->gauge("cluster.cluster_1.membership_total")->value());
+  EXPECT_EQ(0, test_server_->gauge("cluster.cluster_2.membership_total")->value());
+  switch (clientType()) {
+  case Grpc::ClientType::EnvoyGrpc:


@adisuissa figured out the approach to verify the number of streams for both Grpc client types. For Envoy Grpc number of total streams towards specific cluster equals to total number of H2 requests. Google Grpc actually exposes stream metrics per client instance, which is nice. Please let me know wdyt.

nezdolik · 2022-09-04T22:47:16Z

test/integration/eds_integration_test.cc

@@ -1,3 +1,5 @@
+#include <iostream>


to be cleaned up

nezdolik · 2022-09-04T22:51:39Z

Added integration test to verify that discovery requests for multiple EDS clusters reuse one stream/mux towards same management server.

Looks like there is merge conflict that needs to be fixed. Will work now on adding api feature switch in EDS config and extending tests for the config property.

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik · 2022-12-12T12:57:35Z

The patch has been successfully tested in Spotify prod environment, @lizan

KBaichoo · 2023-01-10T18:33:51Z

friendly ping @lizan

I think this is waiting for your input.

@nezdolik this needs a main merge

/wait

Signed-off-by: Kateryna Nezdolii <[email protected]>

repokitteh-read-only · 2023-01-13T12:10:47Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #22419 was synchronize by nezdolik.

see: more, trace.

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik · 2023-01-16T12:22:44Z

@lizan @adisuissa @mattklein123 any chance this PR could get a final pass? I may not be able to keep it up to date in the near future due to other priorities.

adisuissa

LGTM, thanks!

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2023-01-17T14:18:54Z

@envoyproxy/senior-maintainers assignee is @lizan

🐱

Caused by: a #22419 (review) was submitted by @adisuissa.

see: more, trace.

mattklein123 · 2023-01-17T16:39:26Z

changelogs/current.yaml

+- area: eds
+  change: |
+    added ``envoy.reloadable_features.multiplex_eds`` to enable eds multiplexing by default.


Can you say more about what this does? Also, can you flip the text about turning it off via runtime flag because I think this is on by default now?

This will need another rebase as we will land this after the release cut today due to risk. Thank you!

/wait

Signed-off-by: Kateryna Nezdolii <[email protected]>

mattklein123

Thanks!

This reverts commit 97a7f00. There was a crash reported by go-control-plane attributed to this change: envoyproxy/go-control-plane#625 Signed-off-by: Greg Greenway <[email protected]>

Signed-off-by: Kateryna Nezdolii <[email protected]>

…y#25157) This reverts commit 97a7f00. There was a crash reported by go-control-plane attributed to this change: envoyproxy/go-control-plane#625 Signed-off-by: Greg Greenway <[email protected]>

Optimize EDS stream allocation

10e9220

Signed-off-by: Kateryna Nezdolii <[email protected]>

htuch assigned adisuissa Jul 27, 2022

Fix format

f99c8d2

Signed-off-by: Kateryna Nezdolii <[email protected]>

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

Fix remaining eds tests

bb6dd41

Signed-off-by: Kateryna Nezdolii <[email protected]>

repokitteh-read-only bot removed the waiting label Aug 13, 2022

Cleanup

32609ca

Signed-off-by: Kateryna Nezdolii <[email protected]>

repokitteh-read-only bot added the waiting:any label Aug 17, 2022

Apply review comments

5fd046d

Signed-off-by: Kateryna Nezdolii <[email protected]>

repokitteh-read-only bot removed the waiting:any label Aug 19, 2022

lizan added the waiting label Aug 25, 2022

Kateryna Nezdolii added 2 commits September 4, 2022 21:56

Add integration test for shared mux/stream

c9e6b25

Signed-off-by: Kateryna Nezdolii <[email protected]>

Merge remote-tracking branch 'origin/main'

a8a205f

Signed-off-by: Kateryna Nezdolii <[email protected]>

repokitteh-read-only bot removed the waiting label Sep 4, 2022

nezdolik commented Sep 4, 2022

View reviewed changes

test/integration/eds_integration_test.cc Outdated

@@ -1,3 +1,5 @@

#include <iostream>

Copy link
Copy Markdown

Member Author

nezdolik Sep 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be cleaned up

Kateryna Nezdolii added 4 commits September 5, 2022 19:01

Fix merge conflict

566332a

Signed-off-by: Kateryna Nezdolii <[email protected]>

Revert broken formatting

22e1f07

Signed-off-by: Kateryna Nezdolii <[email protected]>

Fix clang tidy

94875a2

Signed-off-by: Kateryna Nezdolii <[email protected]>

Add config setting to api

fbe682c

Signed-off-by: Kateryna Nezdolii <[email protected]>

repokitteh-read-only bot added the v2-freeze label Sep 7, 2022

Merge remote-tracking branch 'origin/main'

1acdccb

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik dismissed adisuissa’s stale review via 1acdccb December 11, 2022 19:11

repokitteh-read-only bot added the waiting label Jan 10, 2023

Merge remote-tracking branch 'origin/main'

4011bfb

Signed-off-by: Kateryna Nezdolii <[email protected]>

repokitteh-read-only bot removed the waiting label Jan 13, 2023

Kateryna Nezdolii added 2 commits January 13, 2023 12:37

Fix format

641bb24

Signed-off-by: Kateryna Nezdolii <[email protected]>

Increase coverage for subscription_factory_impl

52efbeb

Signed-off-by: Kateryna Nezdolii <[email protected]>

adisuissa previously approved these changes Jan 17, 2023

View reviewed changes

mattklein123 self-assigned this Jan 17, 2023

mattklein123 reviewed Jan 17, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 17, 2023

Kateryna Nezdolii added 2 commits January 18, 2023 18:34

Apply review comments

bc5f441

Signed-off-by: Kateryna Nezdolii <[email protected]>

Merge remote-tracking branch 'origin/main'

0d0651d

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik dismissed adisuissa’s stale review via 0d0651d January 18, 2023 18:41

repokitteh-read-only bot removed the waiting label Jan 18, 2023

Fix changelog

9b1ae06

Signed-off-by: Kateryna Nezdolii <[email protected]>

mattklein123 approved these changes Jan 18, 2023

View reviewed changes

mattklein123 enabled auto-merge (squash) January 18, 2023 18:59

mattklein123 merged commit 97a7f00 into envoyproxy:main Jan 18, 2023

xjtian mentioned this pull request Jan 26, 2023

EDS multiplexing causes EDS initial fetch failure #25195

Open

VishalDamgude pushed a commit to freshworks-oss/envoy that referenced this pull request Feb 2, 2023

Optimize EDS stream allocation (envoyproxy#22419)

64547c2

Signed-off-by: Kateryna Nezdolii <[email protected]>

Conversation

nezdolik commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nezdolik commented Jul 26, 2022

Uh oh!

htuch commented Jul 27, 2022

Uh oh!

alyssawilk commented Aug 1, 2022

Uh oh!

adisuissa commented Aug 3, 2022

Uh oh!

nezdolik commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adisuissa commented Aug 15, 2022

Uh oh!

adisuissa commented Aug 17, 2022

Uh oh!

nezdolik commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nezdolik Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

nezdolik Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

nezdolik commented Sep 4, 2022

Uh oh!

nezdolik commented Dec 12, 2022

Uh oh!

KBaichoo commented Jan 10, 2023

Uh oh!

repokitteh-read-only bot commented Jan 13, 2023

Uh oh!

nezdolik commented Jan 16, 2023

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jan 17, 2023

Uh oh!

mattklein123 Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

nezdolik Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

nezdolik commented Jul 26, 2022 •

edited

Loading

nezdolik commented Aug 13, 2022 •

edited

Loading

nezdolik commented Aug 19, 2022 •

edited

Loading