Fixes #2943 - implements shared grpc connection per multiple EDS subscriptions. #5026

dmitri-d · 2018-11-13T21:13:10Z

Was originally started in #4823.

Description: Fixes #2943 by implementing shared grpc connection per multiple EDS subscriptions.
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

@htuch: I removed ClusterManager dependency on GrpcMuxFactory, as there was a circular dependency after some recent changed. The extacted code now lives in EdsSubscriptionFactory, and all changes I made are constrained within EDS.

htuch

The basic approach still looks good.. I'm curious why this is constrained to EDS so concretely? I think the same concerns also exist for RDS and possibly SDS. Would it make sense to templatize or something like that?

dmitri-d · 2018-11-14T02:15:36Z

fixed broken tests

htuch · 2018-11-15T15:57:04Z

source/common/upstream/eds_subscription_factory.cc

+
+    if (muxes_.find(mux_key) == muxes_.end()) {
+      muxes_.insert({mux_key,
+                     std::unique_ptr<Config::GrpcMuxImpl>(new Config::GrpcMuxImpl(local_info, std::move(async_client), dispatcher,


Nit; std::make_unique is preferred in C++14. Do you need to std::move?

repokitteh-read-only · 2018-11-15T15:57:06Z

🙀 Error while processing event:

evaluation error
error: finished: error from server: {module load error GET https://api.github.com/repos/repokitteh/modules/contents/assign.star: 502 Server Error [] map[]}

🐱

Caused by: a #5026 (review) was submitted by @htuch.

see: more, trace.

htuch · 2018-11-15T21:32:00Z

source/common/upstream/eds_subscription_factory.cc

@@ -12,8 +12,8 @@ namespace Upstream {

    if (muxes_.find(mux_key) == muxes_.end()) {
      muxes_.insert({mux_key,
-                     std::make_unique<Config::GrpcMuxImpl>(local_info, std::move(async_client), dispatcher,
-                                                           service_method, random, scope, rate_limit_settings)});
+                     std::move(std::make_unique<Config::GrpcMuxImpl>(local_info, std::move(async_client), dispatcher,


Maybe use emplace?

dmitri-d · 2018-11-15T22:33:11Z

@htuch: looks like using emplace instead of insert did the trick.

htuch · 2018-11-19T18:12:15Z

source/common/upstream/eds_subscription_factory.h

+  public:
+    EdsSubscriptionFactory() : muxes_() {}
+
+    Config::GrpcMux& getOrCreateMux(const LocalInfo::LocalInfo& local_info, Grpc::AsyncClientPtr async_client,


Should this be private? I don't think this is part of the interface.

It's not part of the interface, but I have tests for it. The only thing I can think of in order to keep tests (which I would like to do) is to subclass EdsSubscriptionFactory in tests and add a proxy method for getOrCreateMux. Would that work?

Yeah, you can either use a testing peer pattern or provide a public getOrCreateMuxForTests that makes clear this is only to be used in tests (and make the existing implementation private and the target).

htuch · 2018-11-19T18:12:44Z

source/common/upstream/eds_subscription_factory.cc

+      return result;
+    }
+
+    auto to_return = Config::SubscriptionFactory::subscriptionFromConfigSource<


Just return directly?

htuch · 2018-11-19T18:19:34Z

source/common/config/eds_grpc_subscription_impl.h

+namespace Envoy {
+namespace Config {
+
+class EdsGrpcSubscriptionImpl : public Config::Subscription<envoy::api::v2::ClusterLoadAssignment> {


I'm still a bit confused what this is providing over GrpcSubscriptionImpl? They seem to have the same implementation.

Meant to ask you about this and then forgot. My understanding is that the instance of GrpcMux in GrpcSubscriptionImpl will be deallocated together with the main class. I need to pass a reference to GrpcMux to EdsGrpcSubscriptionImpl, as the mux instances are managed by EdsSubscriptionFactory. Subscription classes are very much the same, but I wasn't sure how to reconcile mux ownership...

Sure, I think we can refactor this a bit, have a base GrpcSubscriptionImpl that takes a GrpcMux& and then an OwnedGrpcSubscriptionImpl subclass that creates and owns the GrpcMux for those use cases.

htuch · 2018-11-19T18:21:02Z

test/common/upstream/eds_subscription_factory_test.cc

+  ASSERT_TRUE(&first_mux == &second_mux);
+}
+
+TEST_F(EdsSubscriptionFactoryTest, ShouldReturnDiffrentMuxes) {


Nit: s/Diffrent/Different/

htuch · 2018-11-20T17:15:27Z

source/common/config/eds_grpc_subscription_impl.h

+namespace Envoy {
+namespace Config {
+
+class EdsGrpcSubscriptionImpl : public Config::Subscription<envoy::api::v2::ClusterLoadAssignment> {


Sure, I think we can refactor this a bit, have a base GrpcSubscriptionImpl that takes a GrpcMux& and then an OwnedGrpcSubscriptionImpl subclass that creates and owns the GrpcMux for those use cases.

htuch · 2018-11-20T17:16:10Z

test/common/upstream/BUILD

+        "//test/mocks/local_info:local_info_mocks",
+        "//test/mocks/config:config_mocks",
+        "//test/mocks/runtime:runtime_mocks",
+	"//test/mocks/stats:stats_mocks",


This probably needs buildifier run on it (fix_format).

htuch · 2018-11-20T17:17:11Z

test/common/upstream/eds_subscription_factory_test.cc

+  ASSERT_TRUE(&first_mux != &second_mux);
+}
+
+TEST_F(EdsSubscriptionFactoryTest, ShouldUseGetOrCreateMuxWhenApiConfigSourceIsUsed) {


Can you add a single line // above each test explaining what it does in natural language?

htuch · 2018-11-20T17:20:57Z

test/common/upstream/eds_subscription_factory_test.cc

+                                                     random_, config.api_config_source(), stats_store_, rate_limit_settings_);
+
+  Config::Subscription<envoy::api::v2::ClusterLoadAssignment>& eds_subscription = *subscription;
+  ASSERT_TRUE(&expected_mux == &(dynamic_cast<Config::EdsGrpcSubscriptionImpl&>(eds_subscription).grpcMux()));


I think you are after EXPECT in all these final test statements, and specifically EXPECT_EQ here.

dmitri-d · 2018-11-23T00:42:55Z

rebased
renamed EdsGrpcSubscriptionImpl to GrpcManagedMuxSubscriptionImpl
Refactored GrpcSubscriptionImpl to become a wrapper around GrpcManagedMuxSubscptionImpl
Reduced visibility of EdsSubscriptionFactory::getOrCreateMux to protected

Still need to fix formatting.

dmitri-d · 2018-11-23T20:22:35Z

fixed formatting of BUILD files

htuch

Looks good!

htuch · 2018-11-26T16:09:31Z

source/common/upstream/eds_subscription_factory.cc

+    Runtime::RandomGenerator& random, const ::envoy::api::v2::core::ApiConfigSource& config_source,
+    Stats::Scope& scope, const Config::RateLimitSettings& rate_limit_settings) {
+
+  const uint64_t mux_key = MessageUtil::hash(config_source.grpc_services(0));


Nit: remove blank lines above/beyond, Google C++ style encourages dense packing of lines except where needed to logically divide up sections.

htuch · 2018-11-26T16:09:38Z

source/common/upstream/eds_subscription_factory.cc

+    std::function<Config::Subscription<envoy::api::v2::ClusterLoadAssignment>*()>
+        rest_legacy_constructor,
+    const std::string& rest_method, const std::string& grpc_method) {
+


Nit: same here.

htuch · 2018-11-26T16:13:26Z

source/common/config/grpc_managed_mux_subscription_impl.h

+namespace Config {
+
+template <class ResourceType>
+class GrpcManagedMuxSubscriptionImpl : public Config::Subscription<ResourceType> {


We should make https://github.com/envoyproxy/envoy/blob/master/source/common/config/grpc_subscription_impl.h a subclass of this as discussed on Slack.

htuch · 2018-11-26T16:14:15Z

source/common/upstream/eds_subscription_factory.h

+
+namespace Envoy {
+namespace Upstream {
+class EdsSubscriptionFactory {


A comment explaining what this does would be helpful, motivation, link back to original issue. Also a TODO explaining that it would make sense to generalize this for RDS in the future.

dmitri-d · 2018-11-26T19:35:16Z

removed empty spaces as pointed above
added a reference to the original issue + a TODO note

dmitri-d · 2018-11-26T20:17:48Z

signed-off and rebased

htuch · 2018-11-26T21:26:40Z

@dmitri-d still some format and tidy changes needed.

dmitri-d · 2018-11-27T02:13:45Z

fixed formatting and clang_tidy issues

I'll squash and sign-off the commit once the latest changes will have been ok'd.

dmitri-d · 2018-11-28T20:20:40Z

/retest

repokitteh-read-only · 2018-11-28T20:20:42Z

🔨 rebuilding ci/circleci: mac (failed build)

🐱

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

see: more, trace.

Signed-off-by: hello-ming <hello.mingyu@gmail.com>

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

dmitri-d · 2018-11-29T02:34:22Z

rebased

dmitri-d · 2018-11-29T03:52:34Z

/restest

dmitri-d · 2018-11-29T17:39:49Z

Thanks for the reviews and the merge, @htuch.

… multiple EDS subscriptions. (envoyproxy#5026)" This reverts commit a085974. Signed-off-by: Adil Hafeez <ahafeez@lyft.com>

…EDS subscriptions. (#5026)" (#5272) This reverts commit a085974. Signed-off-by: Adil Hafeez <ahafeez@lyft.com>

…e EDS subscriptions. (envoyproxy#5026) Fixes envoyproxy#2943 by implementing shared grpc connection per multiple EDS subscriptions. Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com> Signed-off-by: Fred Douglas <fredlas@google.com>

… multiple EDS subscriptions. (envoyproxy#5026)" (envoyproxy#5272) This reverts commit a085974. Signed-off-by: Adil Hafeez <ahafeez@lyft.com> Signed-off-by: Fred Douglas <fredlas@google.com>

htuch reviewed Nov 13, 2018

View reviewed changes

mattklein123 assigned htuch Nov 14, 2018

htuch reviewed Nov 15, 2018

View reviewed changes

htuch reviewed Nov 19, 2018

View reviewed changes

htuch requested changes Nov 20, 2018

View reviewed changes

htuch requested changes Nov 26, 2018

View reviewed changes

htuch changed the title ~~[WIP] Fixes #2943 - implements shared grpc connection per multiple EDS subscriptions.~~ Fixes #2943 - implements shared grpc connection per multiple EDS subscriptions. Nov 28, 2018

hello-ming and others added 2 commits November 28, 2018 18:32

fix inconsistent between document and code (#5020)

c4458e1

Signed-off-by: hello-ming <hello.mingyu@gmail.com>

Fixes #2943 - grpc connection is reused betweem EDS subscriptions

f609671

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

htuch approved these changes Nov 29, 2018

View reviewed changes

htuch merged commit a085974 into envoyproxy:master Nov 29, 2018

adilhafeez mentioned this pull request Dec 11, 2018

Revert "Fixes #2943 - implements shared grpc connection per multiple … #5272

Merged

mattklein123 pushed a commit that referenced this pull request Dec 14, 2018

Revert "Fixes #2943 - implements shared grpc connection per multiple …

298dbf9

…EDS subscriptions. (#5026)" (#5272) This reverts commit a085974. Signed-off-by: Adil Hafeez <ahafeez@lyft.com>

htuch mentioned this pull request Aug 19, 2019

eds: during startup Envoy is sending too many ClusterLoadAssignment request to Istio pilot #7955

Closed

dmitri-d deleted the 2943-optimize-eds branch May 20, 2020 19:44

nezdolik mentioned this pull request Jul 26, 2022

Optimize EDS stream allocation #22419

Merged

Fixes #2943 - implements shared grpc connection per multiple EDS subscriptions. #5026

Fixes #2943 - implements shared grpc connection per multiple EDS subscriptions. #5026

Conversation

dmitri-d commented Nov 13, 2018

htuch left a comment

Choose a reason for hiding this comment

dmitri-d commented Nov 14, 2018

Choose a reason for hiding this comment

repokitteh-read-only bot commented Nov 15, 2018

Choose a reason for hiding this comment

dmitri-d commented Nov 15, 2018

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

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 Nov 23, 2018

dmitri-d commented Nov 23, 2018

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

dmitri-d commented Nov 26, 2018

dmitri-d commented Nov 26, 2018

htuch commented Nov 26, 2018

dmitri-d commented Nov 27, 2018

dmitri-d commented Nov 28, 2018

repokitteh-read-only bot commented Nov 28, 2018

dmitri-d commented Nov 29, 2018

dmitri-d commented Nov 29, 2018

dmitri-d commented Nov 29, 2018