xds: share rds configuration among envoy #8523

lambdai · 2019-10-07T20:50:51Z

WIP since this PR is on top of unlanded PR #8485 and I will rebase.

Context: Currently if a dynamic rds config can be referenced by multiple HCM and multiple scoped rds config. Envoy is maintaining many copies.

Description:
In this PR envoy will maintain only one copy of rds config for each resource name. Notes the rule that listener is ready only if the dependent RDS is ready is respected.

~~BTW there is a bug so the rule is not fully respected. Will fix in future PRs.~~
Fixed the bug where a init manager might see rds config ready when it's not. That's why SharedTargetImpl is introduced.

Fix #8303

stale · 2019-10-16T17:03:44Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

lambdai · 2019-10-17T06:31:40Z

Update: dependency is almost done. Will rebase now and then

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-10-23T07:24:21Z

/assign @htuch
CC @stevenzzzz

stevenzzzz

can you use the new(ish) config-provider framework, I think they are doing the same thing. With the ServerFactoryContext PR merged, I think there is no blocker to migrate this into the config-provider framework.

source/common/init/target_impl.cc

source/common/init/target_impl.h

source/common/router/rds_impl.cc

source/common/router/scoped_rds.cc

test/common/init/target_impl_test.cc

test/common/router/scoped_rds_test.cc

lambdai · 2019-10-23T17:22:42Z

Thanks @stevenzzzz for the quick review! Will update soon

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai

Hi @stevenzzzz I'd like to use the new provider in the follow up PR(s), making the future PR a pure migration. WDYT?

lambdai · 2019-10-23T20:46:31Z

source/common/router/rds_impl.cc

-    subscription = it->second.lock();
+    auto existing_provider = it->second.lock();
+    RELEASE_ASSERT(existing_provider != nullptr, "");
+    init_manager.add(existing_provider->subscription_->init_target_);


shared target is used here

How do you know that existing_provider->subscription_->init_target_ hasn't already initialized?

InitManager doesn't need to know. Manager will be notified after Manager::initialize(), no matter target is initialized or not at this line.

The Target impl (which is SharedTargetImpl) maintains the internal states.

If the target need to be initialized only after all managers call initialze(): This behavior should be achieved by another instance of InitManager IMHO :)

test/common/router/rds_impl_test.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

htuch · 2019-10-24T15:10:22Z

@lambdai if your PR and the shared config provider do the same thing, then why do this PR? I'd prefer we don't review and merge code that is going to be throw away.

lambdai · 2019-10-24T18:01:23Z

@htuch I am not sure what config provider you are referring to.
Assuming you are referring to ScopedRdsConfigProvider where srds is used:

There is only very few lines tackling how a route config is shared. It's a trivial change that RouteConfigProvider user should hold shared ptr.
Instead, the majority of the PR is fixing the misused assumption: owners of rds config will be signaled when the config is ready. SRDS is also the victim and that provider framework depends on the correct assumption.

htuch · 2019-10-25T09:01:51Z

@lambdai I'm talking about the base classes of ScopedRdsConfigProvider. They are basically designed to do what this PR sets out to achieve; make it safe and efficient to share immutable objects related to route config across workers and referees. I think the clean thing to do is have all the route config use this pattern.

lambdai · 2019-10-25T18:44:04Z

ScopedRdsConfigProvider derives from MutableConfigProviderCommonBase
My understanding is that ScopedRdsConfigProvider and ScopedRdsConfigProviderManager delegate how the underlying RouteConfig creation to RouteConfigProviderManager
This PR is fixing the underlying RouteConfigProviderManager

I don't see how MutableConfigProviderCommonBase could magically resolve the issues that notifying multiple Init::Manager

If you are referring that rds config user should rely on a MutableConfigProviderCommonBase instead of "RouteConfigProvider`. I agree and I think it could be the follow up since it's a purely refactor.

htuch · 2019-10-27T11:59:59Z

@lambdai you are correct that ScopedRdsConfigProvider delegates to RouteConfigProviderManager.

The point made above is that RouteConfigProviderManager would benefit from the same design patterns as ScopedRdsConfigProvider, to solve basically the same problem you set out to achieve in this PR. @stevenzzzz can probably elaborate in much more detail, but a number of the recent PRs around the config provider have been about solving (I think) a somewhat similar problem you are tackling in this PR, namely how to share configs and subscriptions across multiple referees and worker threads safely.

It might be simple to "just use a shared pointer", but there are subtleties around this that we explored in depth in those PRs, specifically, what happens when a shared pointer gets destructed on a worker thread vs. main thread, when it happens to be the last thread standing.

Also, I'd want to understand why the same init concerns that exist here don't also exist for SRDS. Can you sync with @stevenzzzz and come up with a common pattern/framework that works for both SRDS and RDS? RDS can be referenced by multiple HCMs and SRDS, similarly SRDS might be referenced by multiple HCMs.

lambdai · 2019-10-27T22:07:27Z

It might be simple to "just use a shared pointer", but there are subtleties around this that we explored in depth in those PRs, specifically, what happens when a shared pointer gets destructed on a worker thread vs. main thread, when it happens to be the last thread standing.

I admit that I didn't take full care of destruction path. My understanding is that my change affect only the lifetime of RDSProvider in master thread and the new TLS magic should work with the new provider.

Also, I'd want to understand why the same init concerns that exist here don't also exist for SRDS.

The bug must exist in SRDS. My observation is that since SRDS delegate the RdsConfigProvider creation to RouteConfigProviderManager. SRDS is built on top of the assumption that RouteConfigProviderManager would notify the init manager owned by SRDS. But it's not the truth. See the failing test case I added in #8781 It's a little hard for me to manipulate the SRDS test cases and fail it. I hope my test in rds ring the bell that why SRDS would fail as well

stevenzzzz · 2019-10-29T13:14:57Z

It might be simple to "just use a shared pointer", but there are subtleties around this that we explored in depth in those PRs, specifically, what happens when a shared pointer gets destructed on a worker thread vs. main thread, when it happens to be the last thread standing.

I admit that I didn't take full care of destruction path. My understanding is that my change affect only the lifetime of RDSProvider in master thread and the new TLS magic should work with the new provider.

There was a previous hard-failure assertion in TLS slot destruction, which limits the destruction of SlotImpl can only happen in mainThread, it's relaxed now I added the "Bookeeper" into the thread_local_impl.h to solve the life-cycle problem of RDS Provider may gets removed by LDS while there are active lambdas in workers dispatcher queue pointing to the provider (which cause undefined behaviors). If we are talking about that, I think it's resolved now. (for RDS and SRDS only).

Also, I'd want to understand why the same init concerns that exist here don't also exist for SRDS.

The bug must exist in SRDS. My observation is that since SRDS delegate the RdsConfigProvider creation to RouteConfigProviderManager. SRDS is built on top of the assumption that RouteConfigProviderManager would notify the init manager owned by SRDS. But it's not the truth. See the failing test case I added in #8781 It's a little hard for me to manipulate the SRDS test cases and fail it. I hope my test in rds ring the bell that why SRDS would fail as well
If lambdai@ hasn't changed SRDS to also depends on ServerFactoryContext, the init-manager bug is still alive in SRDS, i.e., SRDS still depends on the per-Listener factoryContext.

Once that's done. I think we can ignore this PR and move to the config-provider framework, which instead of sharing a config provider(which contains a configImpl) across listeners, shares a subscription (just like the current RDS, but contains a config impl now) across RDS providers.
IMHO, it'd be much less change to make and consistency across all xDS is the most appealing selling point.

lambdai · 2019-10-29T18:55:10Z

Talked to @stevenzzzz offline
Agreements:

Confirmed that listener might be early initialized before rds config is ready.
We might need to understand the new stats: since rds config is shared by multiple listeners but not owned by listener. It's not reasonable to build counter("listenerA.route.hit").

Steps:

Let's land this PR because it fix the multi-notification bug which existing in the RDS and SRDS code
Moving RDS to use ConfigProvider is on my short road map. I will be working on it in follow up.
Sharing SRDS config among listeners is in the long road map.

htuch

@lambdai this seems like a reasonable plan. Can you update the PR description to make it clear that the main contribution here is the shared init manager?

Re: shared init manager. I'm still trying to persuade myself this is the right thing. It's simpler than adding a pub-sub or message bus to Envoy, but it also just generally makes reasoning about init watches and handlers that much more complicated. CC @mergeconflict who is the world premier expert on our init manager :)

htuch · 2019-10-31T23:20:29Z

source/common/router/rds_impl.cc

  } else {
    // Because the RouteConfigProviderManager's weak_ptrs only get cleaned up
    // in the RdsRouteConfigSubscription destructor, and the single threaded nature
    // of this code, locking the weak_ptr will not fail.
-    subscription = it->second.lock();
+    auto existing_provider = it->second.lock();
+    RELEASE_ASSERT(existing_provider != nullptr, "");


This should be a regular ASSERT I think.

1 line below the existing_provider is the deref. Instead of a nullptr deref I prefer to throw a message here. I added a detailed error message in the follow commit.
There is no penalty if the condition is true.

htuch · 2019-10-31T23:22:48Z

source/common/router/rds_impl.cc

-    subscription = it->second.lock();
+    auto existing_provider = it->second.lock();
+    RELEASE_ASSERT(existing_provider != nullptr, "");
+    init_manager.add(existing_provider->subscription_->init_target_);


How do you know that existing_provider->subscription_->init_target_ hasn't already initialized?

mergeconflict

I've just reviewed the SharedTarget portion of this PR. I guess there's a real use case for registering an init target with multiple init managers, so I think I'm ok with this, but I should point out that there are two choices you could have made here and neither is obviously "more correct" than the other:

What was done here: the target initializes as soon as any of the managers tell it to.
The other option: the target waits to initialize until all of the managers tell it to.

Maybe there are more options I didn't think of... My point is that there's sort of a rabbit hole here, where you could keep adding new kinds of targets for different use cases. Maybe that's a good thing, I don't know.

Anyway, a few minor nits, but this otherwise looks fine to me.

mergeconflict · 2019-11-01T00:54:30Z

source/common/init/BUILD

+    external_deps = [
+        "abseil_memory",
+    ],


I'd prefer not to add a dependency just so you can use absl::WrapUnique instead of std::unique_ptr ctor (sorry @stevenzzzz, I know you suggested it). Is this style more common in the Envoy codebase?

6 occurrences. I like the simplicity of WrapUnique. I can seek to mark abseil_memory as a common deps in targets so that I don't have to introduce this deps.
And eventually we can replace WrapUnique along with better c++ stdlib

source/common/init/target_impl.h

mergeconflict · 2019-11-01T01:02:00Z

source/common/init/target_impl.h

@@ -85,5 +87,49 @@ class TargetImpl : public Target, Logger::Loggable<Logger::Id::init> {
  const std::shared_ptr<InternalInitalizeFn> fn_;
 };

+/**
+ * A specialized Target which can be added by multiple Manager.


source/common/init/target_impl.h

mergeconflict · 2019-11-01T01:06:25Z

source/common/init/target_impl.h

+  // Human-readable name for logging
+  const std::string name_;
+
+  // Handle to the ManagerImpl's internal watcher, to call when this target is initialized


Update comment for multiple managers/multiple watchers.

stale · 2019-11-08T01:57:33Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

lambdai · 2019-11-08T22:27:46Z

Back to this PR. Sorry for the late response

lambdai · 2019-11-08T22:38:36Z

As an analogy to when_all(futures) and when_any(futures), initialize() might need a when-all-init-manager-init semantic.
But I also have the hunch that the when_all is done by the existing Init::Watcher. Let's discuss when there is a requirement.

lambdai · 2019-11-08T22:39:20Z

Addressing comments...

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

htuch · 2019-11-11T02:00:42Z

@lambdai can you update the GH top comment (and maybe even title)? It seems this PR is mostly about introducing a shared init manager. In fact, if you want to split out the shared init manager to a separate dedicated PR, that could be even better, as we could review that and this PR becomes much smaller and self-contained.

lambdai · 2019-11-12T17:41:20Z

@htuch will do. Now that we reach the consensus such a shared_target is required I can create a new PR referring to this scenario.

mergeconflict · 2019-11-12T19:33:49Z

@htuch will do. Now that we reach the consensus such a shared_target is required I can create a new PR referring to this scenario.

Just to be clear, I don't believe it's required. I think you can build what you need without it, by having multiple targets — one per init manager — and running the initialization logic when the first target is initialized.

stale · 2019-11-19T21:32:40Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

@mergeconflict

Extracted from #8523 @mergeconflict mentioned we can also maintain a collection of TargetImpl(and a initialize_ flag, std::once_flag) at the target owner. I agree it works but a SharedTarget provides stronger cohesion. Follow up: * update #8523 on top of this PR * complete fuzz test #8714 Signed-off-by: Yuchen Dai <silentdai@gmail.com>

stale · 2019-11-26T22:15:18Z

This pull request has been automatically closed because it has not had activity in the last 14 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!

jmarantz changed the title ~~xds: share rds configuration among envoy~~ WiP xds: share rds configuration among envoy Oct 9, 2019

lambdai mentioned this pull request Oct 12, 2019

Proof of Concept: Introduce ServerFactoryContext #8454

Closed

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 16, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 17, 2019

lambdai mentioned this pull request Oct 19, 2019

rds: make RouteConfigProvider shared among ListenerImpls, or move factory_context_ outside of Listener. #8303

Closed

shared rds provider on top of server factory context

9130a00

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai force-pushed the sharerds branch from 7f14606 to 9130a00 Compare October 19, 2019 05:18

lambdai added 2 commits October 23, 2019 06:24

fix early declared route ready

df24862

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

fix srds test

28b8634

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai changed the title ~~WiP xds: share rds configuration among envoy~~ xds: share rds configuration among envoy Oct 23, 2019

lambdai marked this pull request as ready for review October 23, 2019 07:00

lambdai requested review from alyssawilk and mattklein123 as code owners October 23, 2019 07:00

merge master and fix

6b02fb2

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot assigned htuch Oct 23, 2019

stevenzzzz reviewed Oct 23, 2019

View reviewed changes

address comment

153792b

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai commented Oct 23, 2019

View reviewed changes

fix spelling

43dfb52

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai mentioned this pull request Oct 24, 2019

Wip: fuzz: init manager #8714

Closed

htuch reviewed Oct 31, 2019

View reviewed changes

mergeconflict previously approved these changes Nov 1, 2019

View reviewed changes

htuch added the waiting label Nov 4, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 8, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 8, 2019

lambdai requested a review from htuch November 9, 2019 00:30

address comment

e873573

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai dismissed mergeconflict’s stale review via e873573 November 9, 2019 00:34

repokitteh-read-only bot removed the waiting label Nov 9, 2019

lambdai mentioned this pull request Nov 13, 2019

init: add shared target #9008

Merged

htuch added the waiting label Nov 15, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 19, 2019

stale bot closed this Nov 26, 2019

lambdai mentioned this pull request Dec 3, 2019

xds: share rds configuration among envoy 2/2 #9209

Merged

xds: share rds configuration among envoy #8523

xds: share rds configuration among envoy #8523

Conversation

lambdai commented Oct 7, 2019 • edited Loading

stale bot commented Oct 16, 2019

lambdai commented Oct 17, 2019

lambdai commented Oct 23, 2019

stevenzzzz left a comment

Choose a reason for hiding this comment

lambdai commented Oct 23, 2019

lambdai 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

htuch commented Oct 24, 2019

lambdai commented Oct 24, 2019

htuch commented Oct 25, 2019 • edited Loading

lambdai commented Oct 25, 2019

htuch commented Oct 27, 2019

lambdai commented Oct 27, 2019

stevenzzzz commented Oct 29, 2019

lambdai commented Oct 29, 2019

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

mergeconflict 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

stale bot commented Nov 8, 2019

lambdai commented Nov 8, 2019

lambdai commented Nov 8, 2019

lambdai commented Nov 8, 2019

htuch commented Nov 11, 2019

lambdai commented Nov 12, 2019

mergeconflict commented Nov 12, 2019

stale bot commented Nov 19, 2019

stale bot commented Nov 26, 2019

lambdai commented Oct 7, 2019 •

edited

Loading

htuch commented Oct 25, 2019 •

edited

Loading