rds: split subscription out from RdsRouteConfigProviderImpl by lizan · Pull Request #3960 · envoyproxy/envoy

lizan · 2018-07-26T07:11:23Z

Signed-off-by: Lizan Zhou zlizan@google.com

Description:
RdsRouteConfigProviderImpl holds reference to FactoryContext (implemented by ListenerImpl), which could be freed after LDS update (#3953), this PR split subscription part out and make RdsRouteConfigProviderImpl owned by each Listener. The subscription class holds references to all live providers to provide RDS update.

Risk Level: Low
Testing: unit test, integration test
Docs Changes:
Release Notes:

Fixes #3953

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan · 2018-07-26T07:16:47Z

Thanks to integration test added by @qiwzhang (#3955) and @PiotrSikora (#3957).

This PR is still WIP: need cleanup interface between RdsRouteConfig*, add comment for them, and tests in rds_impl_test. At least ads_integration_test passes with ASAN for now.

Let me know if you think this is a right approach. @htuch @mattklein123 @PiotrSikora @qiwzhang @mandarjog

rshriram · 2018-07-26T15:49:42Z

source/common/router/rds_impl.cc

-
-Router::ConfigConstSharedPtr RdsRouteConfigProviderImpl::config() {
-  return tls_->getTyped<ThreadLocalConfig>().config_;
+  route_config_provider_manager_.route_config_subscriptions_.erase(manager_identifier_);


how do you avoid the race here?
an updated lister's rdsrouteconfig object is being deleted and tries to tell the routeconfig manager that it should be removed..
At the same time, an RDS update arrives and the subscription manager is updating all the routeconfig objects.

There is not race. ProviderImpl and RdsSubscription are separated. ProviderImpl is tightly associated with ListenerImpl, one per one. But RdsSubscription is shared, across ListenerImpl, the ones with different names, or old one with the new one with config change. A freed ProviderImpl will not free RdsSubscription if it is shared by others.
BTW, here provider_manager should be called subscripton_manager.

BTW, here provider_manager should be called subscripton_manager.

provider_manager still manage providers (both static and rds), it doesn't own each rds provider but manages them through subscriptions.

qiwzhang · 2018-07-26T17:25:47Z

@lizan I think this approach should work. This is the smallest change I can think of to fix this crash. BTW, provider should be changed to use unique_ptr instead of shared_ptr.

lizan · 2018-07-26T18:38:41Z

I think eventually we will need to break FactoryContext into small pieces and locking them down, but for now this should be good.

shared_ptr is used for a couple reasons (weak_ptr, config_dump), I will see if it can be unique_ptr as follow up.

Signed-off-by: Lizan Zhou <zlizan@google.com>

qiwzhang · 2018-07-26T21:29:51Z

source/common/router/rds_impl.cc

+    : subscription_(std::move(subscription)), factory_context_(factory_context),
+      tls_(factory_context.threadLocal().allocateSlot()) {
+  ConfigConstSharedPtr initial_config;
+  if (subscription_->config_info_.has_value()) {


can we define a getter function? not to use subscription member directly.

qiwzhang · 2018-07-26T21:31:56Z

source/common/router/rds_impl.cc

+  tls_->set([initial_config](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr {
+    return std::make_shared<ThreadLocalConfig>(initial_config);
+  });
+  subscription_->route_config_providers_.insert(this);


Subscription defines a OnUpdate interface, it only stores the interface pointers.

It should be
subscription_->add_update_callback(this);

No it won't work, you will need a pointer to RdsRouteConfigProviderImpl to implement std::vector<RouteConfigProviderSharedPtr> getRdsRouteConfigProviders() in RouteConfigProviderManagerImpl

All the onUpdate is only used in rds_impl and should not be used elsewhere, would rather keep this in private.

qiwzhang · 2018-07-26T21:34:44Z

source/common/router/rds_impl.cc

-  // of this code, locking the weak_ptr will not fail.
-  Router::RouteConfigProviderSharedPtr new_provider = it->second.lock();
-  ASSERT(new_provider);
+  Router::RouteConfigProviderSharedPtr new_provider{


Can this be unique_ptr?

It will involving interface changes and make them consistent with StaticRouteConfigProviders, and update HttpConnectionManager too.

IMHO, it is important to use unique_ptr to show Provider object is not shared, only HttpConnectionManager owns it.

qiwzhang · 2018-07-26T21:35:26Z

source/common/router/rds_impl.h

+  std::unordered_set<RdsRouteConfigProviderImpl*> route_config_providers_;
+
+  friend class RouteConfigProviderManagerImpl;
+  friend class RdsRouteConfigProviderImpl;


can we not use friend class? define some getter functions.

What is the point of define more getter functions? No other classes should access them via getter than friend classes listed here.

qiwzhang · 2018-07-26T21:36:06Z

source/common/router/rds_impl.h

  SystemTime last_updated_;
+  absl::optional<LastConfigInfo> config_info_;
+  envoy::api::v2::RouteConfiguration route_config_proto_;
+  std::unordered_set<RdsRouteConfigProviderImpl*> route_config_providers_;


Not to store the whole provider object, define OnUpdate interface, only store the interface pointers.

ditto above, you need pointer to ProviderImpl to get shared_ptr of it.

This is what I mean:

class RdsSubscriptionCallback { public: virtual onConfigUpdate() PURE; };

Here RdsConfigSubscription uses it as:

std::unordered_set<RdsSubscriptionCallback*> update_callbacks_;

ProviderImpl will also implement such Callback

Yes I know what you meant, then you won't be able to do https://github.com/lizan/envoy/blob/6d433fef72eebc37b63acef63fd492cbbb9ff659/source/common/router/rds_impl.cc#L204 without dynamic_cast.

It doesn't make sense to make an interface which is completely implementation detail.

qiwzhang · 2018-07-26T21:37:26Z

source/common/router/rds_impl.h

+  const std::string route_config_name_;

  friend class RouteConfigProviderManagerImpl;
+  friend class RdsRouteConfigSubscription;


Why does Subscription need to access ProviderImpl private members?

mostly for for onConfigUpdate since no class other than Subscription should call it. will make it public and remove friend here.

If we use SubscriptionCallback interface, this friend class will not be needed.

mattklein123 · 2018-07-26T21:45:43Z

Talked to @lizan offline. This approach/fix LGTM. Please let me know when the rest of the comments are finished and I can take a look. Thank you for fixing!

qiwzhang · 2018-07-26T21:47:07Z

test/integration/ads_integration_test.cc

+  sendDiscoveryResponse<envoy::api::v2::RouteConfiguration>(
+      Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")},
+      "2");
+}


can we make "makeSingleRequest()" call here

qiwzhang · 2018-07-26T21:48:12Z

test/integration/ads_integration_test.cc

+  sendDiscoveryResponse<envoy::api::v2::RouteConfiguration>(
+      Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_1")},
+      "2");
+}


can we add "makeSingleRequest()" call here to make sure it works?
May have to wait for rds update success.

Signed-off-by: Lizan Zhou <zlizan@google.com>

qiwzhang · 2018-07-26T23:30:15Z

LGTM for the quick fix.

Maybe a follow up PR to do the follow clean up:

use unique_ptr for Provider
Try to cleanly separate Subscription class with ProviderImpl class.

lizan · 2018-07-26T23:38:07Z

@mattklein123 and/or @htuch PTAL.

@qiwzhang I'm looking at how large it will be to do unique_ptr, within or not in this PR. Though not convinced that we need to do 2.

htuch

LGTM; nice find and fix to all involved.

mattklein123

LGTM, thanks for fixing this.

rds: split subscription out from RdsRouteConfigProviderImpl

813116e

Signed-off-by: Lizan Zhou <zlizan@google.com>

rshriram reviewed Jul 26, 2018

View reviewed changes

lizan added 4 commits July 26, 2018 11:46

use typedef shared ptr

4b2680d

Signed-off-by: Lizan Zhou <zlizan@google.com>

fix unit test

842d08d

Signed-off-by: Lizan Zhou <zlizan@google.com>

fix format

4fa2562

Signed-off-by: Lizan Zhou <zlizan@google.com>

add test from @qiwzhang and @PiotrSikora

6d433fe

Signed-off-by: Lizan Zhou <zlizan@google.com>

qiwzhang reviewed Jul 26, 2018

View reviewed changes

mattklein123 self-assigned this Jul 26, 2018

qiwzhang reviewed Jul 26, 2018

View reviewed changes

address comments

3c2ea5d

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan changed the title ~~[WIP] rds: split subscription out from RdsRouteConfigProviderImpl~~ rds: split subscription out from RdsRouteConfigProviderImpl Jul 26, 2018

lizan requested review from htuch and mattklein123 July 26, 2018 23:41

htuch approved these changes Jul 27, 2018

View reviewed changes

mattklein123 approved these changes Jul 27, 2018

View reviewed changes

lizan merged commit 280baee into envoyproxy:master Jul 27, 2018

lizan deleted the rds_crash branch July 27, 2018 04:37

lizan mentioned this pull request Jul 27, 2018

rds: make RouteConfigProvider unique_ptr #3967

Merged

AndresGuedez mentioned this pull request Dec 14, 2018

config: Scoped RDS (PR 1/4): Introduce {static,dynamic} config provider framework #5243

Merged

lizan mentioned this pull request Aug 14, 2019

N^2 RDS memory for SNI #7923

Closed

htuch mentioned this pull request Aug 15, 2019

[config] /config_dump shows rejected RDS config vs the last valid RDS config envoy is running #7939

Closed

Conversation

lizan commented Jul 26, 2018

Uh oh!

lizan commented Jul 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang commented Jul 26, 2018

Uh oh!

lizan commented Jul 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang commented Jul 26, 2018

Uh oh!

lizan commented Jul 26, 2018

Uh oh!

htuch left a comment

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!

5 participants

qiwzhang Jul 26, 2018 •

edited

Loading

qiwzhang Jul 26, 2018 •

edited

Loading