listener, srds, rds, vhds: replace FactoryContext with ServerFactoryContext in router/... #9779
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,9 @@ envoy_cc_library( | |
| "//include/envoy/server:config_tracker_interface", | ||
| "//include/envoy/singleton:instance_interface", | ||
| "//include/envoy/thread_local:thread_local_interface", | ||
| "//source/common/init:manager_lib", | ||
| "//source/common/init:target_lib", | ||
| "//source/common/init:watcher_lib", | ||
| "//source/common/protobuf", | ||
| ], | ||
| ) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ namespace Envoy { | |
| namespace Config { | ||
|
|
||
| ImmutableConfigProviderBase::ImmutableConfigProviderBase( | ||
| Server::Configuration::ServerFactoryContext& factory_context, | ||
| ConfigProviderManagerImplBase& config_provider_manager, | ||
| ConfigProviderInstanceType instance_type, ApiType api_type) | ||
| : last_updated_(factory_context.timeSource().systemTime()), | ||
|
|
@@ -20,7 +20,7 @@ ImmutableConfigProviderBase::~ImmutableConfigProviderBase() { | |
| } | ||
|
|
||
| ConfigSubscriptionCommonBase::~ConfigSubscriptionCommonBase() { | ||
| local_init_target_.ready(); | ||
| config_provider_manager_.unbindSubscription(manager_identifier_); | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -11,7 +11,9 @@ | |||||
| #include "common/common/thread.h" | ||||||
| #include "common/common/utility.h" | ||||||
| #include "common/config/utility.h" | ||||||
| #include "common/init/manager_impl.h" | ||||||
| #include "common/init/target_impl.h" | ||||||
| #include "common/init/watcher_impl.h" | ||||||
| #include "common/protobuf/protobuf.h" | ||||||
|
|
||||||
| namespace Envoy { | ||||||
|
|
@@ -115,7 +117,7 @@ class ImmutableConfigProviderBase : public ConfigProvider { | |||||
| ConfigProviderInstanceType instanceType() const { return instance_type_; } | ||||||
|
|
||||||
| protected: | ||||||
| ImmutableConfigProviderBase(Server::Configuration::ServerFactoryContext& factory_context, | ||||||
| ConfigProviderManagerImplBase& config_provider_manager, | ||||||
| ConfigProviderInstanceType instance_type, ApiType api_type); | ||||||
|
|
||||||
|
|
@@ -137,12 +139,6 @@ class MutableConfigProviderCommonBase; | |||||
| * A subscription is intended to be co-owned by config providers with the same config source, it's | ||||||
| * designed to be created/destructed on admin thread only. | ||||||
| * | ||||||
| */ | ||||||
| class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::config> { | ||||||
| public: | ||||||
|
|
@@ -180,7 +176,7 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf | |||||
| */ | ||||||
| void onConfigUpdate() { | ||||||
| setLastUpdated(); | ||||||
| local_init_target_.ready(); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -189,7 +185,7 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf | |||||
| */ | ||||||
| void onConfigUpdateFailed() { | ||||||
| setLastUpdated(); | ||||||
| local_init_target_.ready(); | ||||||
| } | ||||||
|
|
||||||
| protected: | ||||||
|
|
@@ -202,13 +198,22 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf | |||||
|
|
||||||
| ConfigSubscriptionCommonBase(const std::string& name, const uint64_t manager_identifier, | ||||||
| ConfigProviderManagerImplBase& config_provider_manager, | ||||||
| Server::Configuration::ServerFactoryContext& factory_context) | ||||||
| : name_(name), tls_(factory_context.threadLocal().allocateSlot()), | ||||||
| local_init_target_( | ||||||
| fmt::format("ConfigSubscriptionCommonBase local init target '{}'", name_), | ||||||
| [this]() { start(); }), | ||||||
| parent_init_target_(fmt::format("ConfigSubscriptionCommonBase init target '{}'", name_), | ||||||
| [this]() { local_init_manager_.initialize(local_init_watcher_); }), | ||||||
| local_init_watcher_(fmt::format("ConfigSubscriptionCommonBase local watcher '{}'", name_), | ||||||
| [this]() { parent_init_target_.ready(); }), | ||||||
| local_init_manager_( | ||||||
| fmt::format("ConfigSubscriptionCommonBase local init manager '{}'", name_)), | ||||||
| manager_identifier_(manager_identifier), config_provider_manager_(config_provider_manager), | ||||||
| time_source_(factory_context.timeSource()), | ||||||
| last_updated_(factory_context.timeSource().systemTime()) { | ||||||
| Envoy::Config::Utility::checkLocalInfo(name, factory_context.localInfo()); | ||||||
| local_init_manager_.add(local_init_target_); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -220,7 +225,7 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf | |||||
| void applyConfigUpdate(const ConfigUpdateCb& update_fn); | ||||||
|
|
||||||
| void setLastUpdated() { last_updated_ = time_source_.systemTime(); } | ||||||
| Init::Manager& localInitManager() { return local_init_manager_; } | ||||||
| void setLastConfigInfo(absl::optional<LastConfigInfo>&& config_info) { | ||||||
| config_info_ = std::move(config_info); | ||||||
| } | ||||||
|
|
@@ -232,7 +237,15 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf | |||||
| ThreadLocal::SlotPtr tls_; | ||||||
|
|
||||||
| private: | ||||||
| // Local init target which signals first RPC interaction with management server. | ||||||
| Init::TargetImpl local_init_target_; | ||||||
| // Target added to factory context's initManager. | ||||||
| Init::TargetImpl parent_init_target_; | ||||||
| // Watcher that marks parent_init_target_ ready when the local init manager is ready. | ||||||
| Init::WatcherImpl local_init_watcher_; | ||||||
| // Local manager that tracks the subscription's sub-resources(RDS subscriptions) initialization. | ||||||
|
Member
There was a problem hiding this comment.
Suggested change
Because RDS is just one example, right? |
||||||
| Init::ManagerImpl local_init_manager_; | ||||||
|
|
||||||
| const uint64_t manager_identifier_; | ||||||
| ConfigProviderManagerImplBase& config_provider_manager_; | ||||||
| TimeSource& time_source_; | ||||||
|
|
@@ -259,7 +272,7 @@ class ConfigSubscriptionInstance : public ConfigSubscriptionCommonBase { | |||||
| public: | ||||||
| ConfigSubscriptionInstance(const std::string& name, const uint64_t manager_identifier, | ||||||
| ConfigProviderManagerImplBase& config_provider_manager, | ||||||
| Server::Configuration::ServerFactoryContext& factory_context) | ||||||
| : ConfigSubscriptionCommonBase(name, manager_identifier, config_provider_manager, | ||||||
| factory_context) {} | ||||||
|
|
||||||
|
|
@@ -419,7 +432,7 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl | |||||
| // around it. However, since this is not a performance critical path we err on the side | ||||||
| // of simplicity. | ||||||
| subscription = subscription_factory_fn(manager_identifier, *this); | ||||||
| init_manager.add(subscription->parent_init_target_); | ||||||
|
|
||||||
| bindSubscription(manager_identifier, subscription); | ||||||
| } else { | ||||||
|
|
||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to read the rest of the PR, but given the change below which add messageValidationVisitor() to the CommonFactoryContext, why is this function also taking a validator as a parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenzzzz I am still wondering about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, missed this comment. I think you mean "add a validation visitor context"?
It's because we can't tell from the current ServerFactoryContext if the static validator or the dynamic validator should be used.
For example, a static Route config provider could be put into a dynamic(LDS) listener's config, in which case we should use the dynamic validator instead (per Harvey).
For that, we will have to pass validator alongside the Serverfatorycontext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so only the code that is building the config provider knows if it should be using the static or dynamic visitor, but the constructor itself does not. Hence why the caller of createStaticRouteConfigProvider needs to pass the intended visitor.
Not to bike shed, but it seems that we have gone in one direction here, and the opposite in the ListernerImpl constructor where we used to pass the validator reference but now we decide inside the constructor. Should we pass an added_via_api boolean here and have the constructor decide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, I'd like the validator choice made as close to the consumer entity(in this case, the listenerImpl) as possible, which provides better modularity.
The listener could make the decision by itself as there is a somewhat hacky paramter add_via_api in its constructor, the added_via_api information could be deducted by any RDS-config-provider or any subscription-initiated config-provider,
but not in static config provider: its context doesn't provide enough information for making the choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that is why I was suggesting if we should hint the static config provider as well (albeit I have not looked if we have access to the same hint when we create this config provider, but my intuition tells me we do). I am not sure what the right answer here is. My point is that we seem to have gone in opposite directions where we could have the same setup.
Perhaps the next reviewer can state their preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed this w/ Harvey before, passing the correct validator is preferred over hinting (passing the added_via_api signal). Agreed it's a preference thing as to if we want to keep all the places have the same interface.