RdsConfigProvider dedup #8268
RdsConfigProvider dedup #8268
Conversation
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
| init_manager.add(subscription->init_target_); | ||
| route_config_subscriptions_.insert({manager_identifier, subscription}); | ||
| std::shared_ptr<RdsRouteConfigProviderImpl> provider{ |
There was a problem hiding this comment.
Unless you break up FactoryContext (it is tied to Listener), I don't think this deduplication works, it will cause #3953 again which is why the provider was 1:1 to the HCM.
There was a problem hiding this comment.
I see... Are you aware of what particular thing in the factory context is needed by RdsRouteConfig?
There was a problem hiding this comment.
Not sure, though in general I think it will be helpful to break FactoryContext into smaller pieces by the lifecycle.
|
@stevenzzzz Not exactly the same solution we were discussing but underneath is the same: no duplicated provider |
|
/assign @stevenzzzz |
|
I am aware of InitManager, which is kind of broken already (Need confirmation, use xds channel on slack) |
|
@lizan ads integration test is failing at the exact test case which was failing.. Thank you for surfacing it! Factory context is referred 40ish time in RouterConfig. Could be more since the indirect reference is not counted. Yet most of them are referring to server scoped resources, which has wider scope time than listener Hopefully we could save the server scope resources in provider. Any other traps I didn't mention? @lizan |
|
@stevenzzzz It's not as easy as we thought :) |
|
close for now: wip |
|
I've talked with Harvey, we may need to do quite a bit shoveling, including some API interface change, to make the RDS provider/subscription Listener independent. |
WIP still fixing tests
Description:
If N http conn managers refers the same rds config name, currently envoy
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]