diff --git a/include/envoy/runtime/runtime.h b/include/envoy/runtime/runtime.h index 52abc0e506163..2df95731398b4 100644 --- a/include/envoy/runtime/runtime.h +++ b/include/envoy/runtime/runtime.h @@ -259,8 +259,6 @@ class Loader { public: virtual ~Loader() = default; - using ReadyCallback = std::function; - /** * Post-construction initialization. Runtime will be generally available after * the constructor is finished, with the exception of dynamic RTDS layers, @@ -288,12 +286,6 @@ class Loader { * @param values the values to merge */ virtual void mergeValues(const std::unordered_map& values) PURE; - - /** - * Initiate all RTDS subscriptions. The `on_done` callback is invoked when all RTDS requests - * have either received and applied their responses or timed out. - */ - virtual void startRtdsSubscriptions(ReadyCallback on_done) PURE; }; using LoaderPtr = std::unique_ptr; diff --git a/include/envoy/upstream/cluster_manager.h b/include/envoy/upstream/cluster_manager.h index 4bfe98beee6b6..047bf2aafd481 100644 --- a/include/envoy/upstream/cluster_manager.h +++ b/include/envoy/upstream/cluster_manager.h @@ -73,13 +73,6 @@ class ClusterManagerFactory; /** * Manages connection pools and load balancing for upstream clusters. The cluster manager is * persistent and shared among multiple ongoing requests/connections. - * Cluster manager is initialized in two phases. In the first phase which begins at the construction - * all primary clusters (i.e. with endpoint assignments provisioned statically in bootstrap, - * discovered through DNS or file based CDS) are initialized. - * After the first phase has completed the server instance initializes services (i.e. RTDS) needed - * to successfully deploy the rest of dynamic configuration. - * In the second phase all secondary clusters (with endpoint assignments provisioned by xDS servers) - * are initialized and then the rest of the configuration provisioned through xDS. */ class ClusterManager { public: @@ -103,14 +96,6 @@ class ClusterManager { */ virtual void setInitializedCb(std::function callback) PURE; - /** - * Start initialization of secondary clusters and then dynamically configured clusters. - * The "initialized callback" set in the method above is invoked when secondary and - * dynamically provisioned clusters have finished initializing. - */ - virtual void - initializeSecondaryClusters(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) PURE; - using ClusterInfoMap = std::unordered_map>; /** diff --git a/source/common/runtime/BUILD b/source/common/runtime/BUILD index ddeb069e3e5ae..dbab335cf0a76 100644 --- a/source/common/runtime/BUILD +++ b/source/common/runtime/BUILD @@ -67,9 +67,7 @@ envoy_cc_library( "//source/common/config:subscription_base_interface", "//source/common/filesystem:directory_lib", "//source/common/grpc:common_lib", - "//source/common/init:manager_lib", "//source/common/init:target_lib", - "//source/common/init:watcher_lib", "//source/common/protobuf:message_validator_lib", "//source/common/protobuf:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 4ca76e9172e29..1ed9faf2c4322 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -465,12 +465,11 @@ void ProtoLayer::walkProtoValue(const ProtobufWkt::Value& v, const std::string& LoaderImpl::LoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, const envoy::config::bootstrap::v3::LayeredRuntime& config, - const LocalInfo::LocalInfo& local_info, Stats::Store& store, - RandomGenerator& generator, + const LocalInfo::LocalInfo& local_info, Init::Manager& init_manager, + Stats::Store& store, RandomGenerator& generator, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) : generator_(generator), stats_(generateStats(store)), tls_(tls.allocateSlot()), - config_(config), service_cluster_(local_info.clusterName()), api_(api), - init_watcher_("RDTS", [this]() { onRdtsReady(); }) { + config_(config), service_cluster_(local_info.clusterName()), api_(api) { std::unordered_set layer_names; for (const auto& layer : config_.layers()) { auto ret = layer_names.insert(layer.name()); @@ -498,7 +497,7 @@ LoaderImpl::LoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator case envoy::config::bootstrap::v3::RuntimeLayer::LayerSpecifierCase::kRtdsLayer: subscriptions_.emplace_back( std::make_unique(*this, layer.rtds_layer(), store, validation_visitor)); - init_manager_.add(subscriptions_.back()->init_target_); + init_manager.add(subscriptions_.back()->init_target_); break; default: NOT_REACHED_GCOVR_EXCL_LINE; @@ -510,16 +509,6 @@ LoaderImpl::LoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator void LoaderImpl::initialize(Upstream::ClusterManager& cm) { cm_ = &cm; } -void LoaderImpl::startRtdsSubscriptions(ReadyCallback on_done) { - on_rtds_initialized_ = on_done; - init_manager_.initialize(init_watcher_); -} - -void LoaderImpl::onRdtsReady() { - ENVOY_LOG(info, "RTDS has finished initialization"); - on_rtds_initialized_(); -} - RtdsSubscription::RtdsSubscription( LoaderImpl& parent, const envoy::config::bootstrap::v3::RuntimeLayer::RtdsLayer& rtds_layer, Stats::Store& store, ProtobufMessage::ValidationVisitor& validation_visitor) diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index e59afd9c7361c..8fa838e88cd89 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -24,7 +24,6 @@ #include "common/common/logger.h" #include "common/common/thread.h" #include "common/config/subscription_base.h" -#include "common/init/manager_impl.h" #include "common/init/target_impl.h" #include "common/singleton/threadsafe_singleton.h" @@ -243,16 +242,15 @@ class LoaderImpl : public Loader, Logger::Loggable { public: LoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, const envoy::config::bootstrap::v3::LayeredRuntime& config, - const LocalInfo::LocalInfo& local_info, Stats::Store& store, - RandomGenerator& generator, ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api& api); + const LocalInfo::LocalInfo& local_info, Init::Manager& init_manager, + Stats::Store& store, RandomGenerator& generator, + ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api); // Runtime::Loader void initialize(Upstream::ClusterManager& cm) override; const Snapshot& snapshot() override; std::shared_ptr threadsafeSnapshot() override; void mergeValues(const std::unordered_map& values) override; - void startRtdsSubscriptions(ReadyCallback on_done) override; private: friend RtdsSubscription; @@ -262,7 +260,6 @@ class LoaderImpl : public Loader, Logger::Loggable { // Load a new Snapshot into TLS void loadNewSnapshot(); RuntimeStats generateStats(Stats::Store& store); - void onRdtsReady(); RandomGenerator& generator_; RuntimeStats stats_; @@ -272,9 +269,6 @@ class LoaderImpl : public Loader, Logger::Loggable { const std::string service_cluster_; Filesystem::WatcherPtr watcher_; Api::Api& api_; - ReadyCallback on_rtds_initialized_; - Init::WatcherImpl init_watcher_; - Init::ManagerImpl init_manager_{"RTDS"}; std::vector subscriptions_; Upstream::ClusterManager* cm_{}; diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 40f1cb07d33e9..8e4d6f9975c3f 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -123,12 +123,12 @@ void ClusterManagerInitHelper::maybeFinishInitialize() { // Do not do anything if we are still doing the initial static load or if we are waiting for // CDS initialize. ENVOY_LOG(debug, "maybe finish initialize state: {}", enumToInt(state_)); - if (state_ == State::Loading || state_ == State::WaitingToStartCdsInitialization) { + if (state_ == State::Loading || state_ == State::WaitingForCdsInitialize) { return; } // If we are still waiting for primary clusters to initialize, do nothing. - ASSERT(state_ == State::WaitingToStartSecondaryInitialization || state_ == State::CdsInitialized); + ASSERT(state_ == State::WaitingForStaticInitialize || state_ == State::CdsInitialized); ENVOY_LOG(debug, "maybe finish initialize primary init clusters empty: {}", primary_init_clusters_.empty()); if (!primary_init_clusters_.empty()) { @@ -162,9 +162,9 @@ void ClusterManagerInitHelper::maybeFinishInitialize() { // directly to initialized. started_secondary_initialize_ = false; ENVOY_LOG(debug, "maybe finish initialize cds api ready: {}", cds_ != nullptr); - if (state_ == State::WaitingToStartSecondaryInitialization && cds_) { + if (state_ == State::WaitingForStaticInitialize && cds_) { ENVOY_LOG(info, "cm init: initializing cds"); - state_ = State::WaitingToStartCdsInitialization; + state_ = State::WaitingForCdsInitialize; cds_->initialize(); } else { ENVOY_LOG(info, "cm init: all clusters initialized"); @@ -177,14 +177,7 @@ void ClusterManagerInitHelper::maybeFinishInitialize() { void ClusterManagerInitHelper::onStaticLoadComplete() { ASSERT(state_ == State::Loading); - // After initialization of primary clusters has completed, transition to - // waiting for signal to initialize secondary clusters and then CDS. - state_ = State::WaitingToStartSecondaryInitialization; -} - -void ClusterManagerInitHelper::startInitializingSecondaryClusters() { - ASSERT(state_ == State::WaitingToStartSecondaryInitialization); - ENVOY_LOG(debug, "continue initializing secondary clusters"); + state_ = State::WaitingForStaticInitialize; maybeFinishInitialize(); } @@ -193,7 +186,7 @@ void ClusterManagerInitHelper::setCds(CdsApi* cds) { cds_ = cds; if (cds_) { cds_->setInitializedCb([this]() -> void { - ASSERT(state_ == State::WaitingToStartCdsInitialization); + ASSERT(state_ == State::WaitingForCdsInitialize); state_ = State::CdsInitialized; maybeFinishInitialize(); }); @@ -353,22 +346,15 @@ ClusterManagerImpl::ClusterManagerImpl( init_helper_.onStaticLoadComplete(); ads_mux_->start(); -} -void ClusterManagerImpl::initializeSecondaryClusters( - const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - init_helper_.startInitializingSecondaryClusters(); - - const auto& cm_config = bootstrap.cluster_manager(); if (cm_config.has_load_stats_config()) { const auto& load_stats_config = cm_config.load_stats_config(); - load_stats_reporter_ = std::make_unique( - local_info_, *this, stats_, + local_info, *this, stats, Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, load_stats_config, - stats_, false) + stats, false) ->create(), - load_stats_config.transport_api_version(), dispatcher_); + load_stats_config.transport_api_version(), main_thread_dispatcher); } } diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 4f89e443acee0..707cc1ca476e0 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -110,18 +110,15 @@ class ClusterManagerInitHelper : Logger::Loggable { : cm_(cm), per_cluster_init_callback_(per_cluster_init_callback) {} enum class State { - // Initial state. During this state all static clusters are loaded. Any primary clusters + // Initial state. During this state all static clusters are loaded. Any phase 1 clusters // are immediately initialized. Loading, - // During this state cluster manager waits to start initializing secondary clusters. In this - // state all - // primary clusters have completed initialization. Initialization of the secondary clusters - // is started by the `initializeSecondaryClusters` method. - WaitingToStartSecondaryInitialization, - // In this state cluster manager waits for all secondary clusters (if configured) to finish - // initialization. Then, if CDS is configured, this state tracks waiting for the first CDS - // response to populate dynamically configured clusters. - WaitingToStartCdsInitialization, + // During this state we wait for all static clusters to fully initialize. This requires + // completing phase 1 clusters, initializing phase 2 clusters, and then waiting for them. + WaitingForStaticInitialize, + // If CDS is configured, this state tracks waiting for the first CDS response to populate + // clusters. + WaitingForCdsInitialize, // During this state, all CDS populated clusters are undergoing either phase 1 or phase 2 // initialization. CdsInitialized, @@ -136,8 +133,6 @@ class ClusterManagerInitHelper : Logger::Loggable { void setInitializedCb(std::function callback); State state() const { return state_; } - void startInitializingSecondaryClusters(); - private: // To enable invariant assertions on the cluster lists. friend ClusterManagerImpl; @@ -247,9 +242,6 @@ class ClusterManagerImpl : public ClusterManager, Logger::LoggableinitializeStats(stats_store_, "server."); } - // The broad order of initialization from this point on is the following: - // 1. Statically provisioned configuration (bootstrap) are loaded. - // 2. Cluster manager is created and all primary clusters (i.e. with endpoint assignments - // provisioned statically in bootstrap, discovered through DNS or file based CDS) are - // initialized. - // 3. Various services are initialized and configured using the bootstrap config. - // 4. RTDS is initialized using primary clusters. This allows runtime overrides to be fully - // configured before the rest of xDS configuration is provisioned. - // 5. Secondary clusters (with endpoint assignments provisioned by xDS servers) are initialized. - // 6. The rest of the dynamic configuration is provisioned. - // - // Please note: this order requires that RTDS is provisioned using a primary cluster. If RTDS is - // provisioned through ADS then ADS must use primary cluster as well. This invariant is enforced - // during RTDS initialization and invalid configuration will be rejected. - // Runtime gets initialized before the main configuration since during main configuration // load things may grab a reference to the loader for later use. runtime_singleton_ = std::make_unique( @@ -427,27 +412,6 @@ void InstanceImpl::initialize(const Options& options, // instantiated (which in turn relies on runtime...). Runtime::LoaderSingleton::get().initialize(clusterManager()); - // If RTDS was not configured the `onRuntimeReady` callback is immediately invoked. - Runtime::LoaderSingleton::get().startRtdsSubscriptions([this]() { onRuntimeReady(); }); - - for (Stats::SinkPtr& sink : config_.statsSinks()) { - stats_store_.addSink(*sink); - } - - // Some of the stat sinks may need dispatcher support so don't flush until the main loop starts. - // Just setup the timer. - stat_flush_timer_ = dispatcher_->createTimer([this]() -> void { flushStats(); }); - stat_flush_timer_->enableTimer(config_.statsFlushInterval()); - - // GuardDog (deadlock detection) object and thread setup before workers are - // started and before our own run() loop runs. - guard_dog_ = std::make_unique(stats_store_, config_, *api_); -} - -void InstanceImpl::onRuntimeReady() { - // Begin initializing secondary clusters after RTDS configuration has been applied. - clusterManager().initializeSecondaryClusters(bootstrap_); - if (bootstrap_.has_hds_config()) { const auto& hds_config = bootstrap_.hds_config(); async_client_manager_ = std::make_unique( @@ -462,6 +426,19 @@ void InstanceImpl::onRuntimeReady() { *config_.clusterManager(), *local_info_, *admin_, *singleton_manager_, thread_local_, messageValidationContext().dynamicValidationVisitor(), *api_); } + + for (Stats::SinkPtr& sink : config_.statsSinks()) { + stats_store_.addSink(*sink); + } + + // Some of the stat sinks may need dispatcher support so don't flush until the main loop starts. + // Just setup the timer. + stat_flush_timer_ = dispatcher_->createTimer([this]() -> void { flushStats(); }); + stat_flush_timer_->enableTimer(config_.statsFlushInterval()); + + // GuardDog (deadlock detection) object and thread setup before workers are + // started and before our own run() loop runs. + guard_dog_ = std::make_unique(stats_store_, config_, *api_); } void InstanceImpl::startWorkers() { @@ -481,8 +458,8 @@ Runtime::LoaderPtr InstanceUtil::createRuntime(Instance& server, ENVOY_LOG(info, "runtime: {}", MessageUtil::getYamlStringFromMessage(config.runtime())); return std::make_unique( server.dispatcher(), server.threadLocal(), config.runtime(), server.localInfo(), - server.stats(), server.random(), server.messageValidationContext().dynamicValidationVisitor(), - server.api()); + server.initManager(), server.stats(), server.random(), + server.messageValidationContext().dynamicValidationVisitor(), server.api()); } void InstanceImpl::loadServerFlags(const absl::optional& flags_path) { @@ -703,4 +680,4 @@ ProtobufTypes::MessagePtr InstanceImpl::dumpBootstrapConfig() { } } // namespace Server -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/source/server/server.h b/source/server/server.h index 7670ff08ab7d6..c5016887700af 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -285,7 +285,6 @@ class InstanceImpl final : Logger::Loggable, void terminate(); void notifyCallbacksForStage( Stage stage, Event::PostCb completion_cb = [] {}); - void onRuntimeReady(); using LifecycleNotifierCallbacks = std::list; using LifecycleNotifierCompletionCallbacks = std::list; @@ -306,9 +305,6 @@ class InstanceImpl final : Logger::Loggable, const Options& options_; ProtobufMessage::ProdValidationContextImpl validation_context_; TimeSource& time_source_; - // Delete local_info_ as late as possible as some members below may reference it during their - // destruction. - LocalInfo::LocalInfoPtr local_info_; HotRestart& restarter_; const time_t start_time_; time_t original_start_time_; @@ -332,6 +328,7 @@ class InstanceImpl final : Logger::Loggable, Configuration::MainImpl config_; Network::DnsResolverSharedPtr dns_resolver_; Event::TimerPtr stat_flush_timer_; + LocalInfo::LocalInfoPtr local_info_; DrainManagerPtr drain_manager_; AccessLog::AccessLogManagerImpl access_log_manager_; std::unique_ptr cluster_manager_factory_; diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 6ba14af4d27d6..13b4a40e09b33 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1373,9 +1373,9 @@ class DeprecatedFieldsTest : public testing::TestWithParam { runtime_deprecated_feature_use_(store_.counter("runtime.deprecated_feature_use")) { envoy::config::bootstrap::v3::LayeredRuntime config; config.add_layers()->mutable_admin_layer(); - loader_ = std::make_unique( - Runtime::LoaderPtr{new Runtime::LoaderImpl(dispatcher_, tls_, config, local_info_, store_, - generator_, validation_visitor_, *api_)}); + loader_ = std::make_unique(Runtime::LoaderPtr{ + new Runtime::LoaderImpl(dispatcher_, tls_, config, local_info_, init_manager_, store_, + generator_, validation_visitor_, *api_)}); } void checkForDeprecation(const Protobuf::Message& message) { @@ -1399,6 +1399,7 @@ class DeprecatedFieldsTest : public testing::TestWithParam { std::unique_ptr loader_; Stats::Counter& runtime_deprecated_feature_use_; NiceMock local_info_; + Init::MockManager init_manager_; NiceMock validation_visitor_; }; diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 4701f54128345..1d9d7076a372f 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -28,7 +28,6 @@ using testing::_; using testing::Invoke; using testing::InvokeWithoutArgs; -using testing::MockFunction; using testing::NiceMock; using testing::Return; @@ -119,6 +118,7 @@ class LoaderImplTest : public testing::Test { Api::ApiPtr api_; Upstream::MockClusterManager cm_; NiceMock local_info_; + Init::MockManager init_manager_; std::vector on_changed_cbs_; NiceMock validation_visitor_; std::string expected_watch_root_; @@ -145,8 +145,9 @@ class DiskLoaderImplTest : public LoaderImplTest { envoy::config::bootstrap::v3::LayeredRuntime layered_runtime; Config::translateRuntime(runtime, layered_runtime); - loader_ = std::make_unique(dispatcher_, tls_, layered_runtime, local_info_, store_, - generator_, validation_visitor_, *api_); + loader_ = + std::make_unique(dispatcher_, tls_, layered_runtime, local_info_, init_manager_, + store_, generator_, validation_visitor_, *api_); } void write(const std::string& path, const std::string& value) { @@ -556,8 +557,8 @@ TEST_F(DiskLoaderImplTest, MultipleAdminLayersFail) { layer->mutable_admin_layer(); } EXPECT_THROW_WITH_MESSAGE( - std::make_unique(dispatcher_, tls_, layered_runtime, local_info_, store_, - generator_, validation_visitor_, *api_), + std::make_unique(dispatcher_, tls_, layered_runtime, local_info_, init_manager_, + store_, generator_, validation_visitor_, *api_), EnvoyException, "Too many admin layers specified in LayeredRuntime, at most one may be specified"); } @@ -577,8 +578,9 @@ class StaticLoaderImplTest : public LoaderImplTest { layer->set_name("admin"); layer->mutable_admin_layer(); } - loader_ = std::make_unique(dispatcher_, tls_, layered_runtime, local_info_, store_, - generator_, validation_visitor_, *api_); + loader_ = + std::make_unique(dispatcher_, tls_, layered_runtime, local_info_, init_manager_, + store_, generator_, validation_visitor_, *api_); } ProtobufWkt::Struct base_; @@ -863,6 +865,9 @@ class RtdsLoaderImplTest : public LoaderImplTest { rtds_layer->mutable_rtds_config(); } EXPECT_CALL(cm_, subscriptionFactory()).Times(layers_.size()); + EXPECT_CALL(init_manager_, add(_)).WillRepeatedly(Invoke([this](const Init::Target& target) { + init_target_handles_.emplace_back(target.createHandle("test")); + })); ON_CALL(cm_.subscription_factory_, subscriptionFromConfigSource(_, _, _, _)) .WillByDefault(testing::Invoke( [this](const envoy::config::core::v3::ConfigSource&, absl::string_view, Stats::Scope&, @@ -872,14 +877,15 @@ class RtdsLoaderImplTest : public LoaderImplTest { rtds_callbacks_.push_back(&callbacks); return ret; })); - loader_ = std::make_unique(dispatcher_, tls_, config, local_info_, store_, - generator_, validation_visitor_, *api_); + loader_ = std::make_unique(dispatcher_, tls_, config, local_info_, init_manager_, + store_, generator_, validation_visitor_, *api_); loader_->initialize(cm_); for (auto* sub : rtds_subscriptions_) { EXPECT_CALL(*sub, start(_)); } - - loader_->startRtdsSubscriptions(rtds_init_callback_.AsStdFunction()); + for (auto& handle : init_target_handles_) { + handle->initialize(init_watcher_); + } // Validate that the layer name is set properly for dynamic layers. EXPECT_EQ(layers_[0], loader_->snapshot().getLayers()[1]->name()); @@ -915,7 +921,8 @@ class RtdsLoaderImplTest : public LoaderImplTest { std::vector layers_{"some_resource"}; std::vector rtds_callbacks_; std::vector rtds_subscriptions_; - MockFunction rtds_init_callback_; + Init::ExpectableWatcherImpl init_watcher_; + std::vector init_target_handles_; }; // Empty resource lists are rejected. @@ -924,7 +931,7 @@ TEST_F(RtdsLoaderImplTest, UnexpectedSizeEmpty) { Protobuf::RepeatedPtrField runtimes; - EXPECT_CALL(rtds_init_callback_, Call()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_THROW_WITH_MESSAGE(rtds_callbacks_[0]->onConfigUpdate(runtimes, ""), EnvoyException, "Unexpected RTDS resource length: 0"); @@ -942,7 +949,7 @@ TEST_F(RtdsLoaderImplTest, UnexpectedSizeTooMany) { runtimes.Add(); runtimes.Add(); - EXPECT_CALL(rtds_init_callback_, Call()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_THROW_WITH_MESSAGE(rtds_callbacks_[0]->onConfigUpdate(runtimes, ""), EnvoyException, "Unexpected RTDS resource length: 2"); @@ -956,7 +963,7 @@ TEST_F(RtdsLoaderImplTest, UnexpectedSizeTooMany) { TEST_F(RtdsLoaderImplTest, FailureSubscription) { setup(); - EXPECT_CALL(rtds_init_callback_, Call()); + EXPECT_CALL(init_watcher_, ready()); // onConfigUpdateFailed() should not be called for gRPC stream connection failure rtds_callbacks_[0]->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, {}); @@ -1002,7 +1009,7 @@ TEST_F(RtdsLoaderImplTest, OnConfigUpdateSuccess) { foo: bar baz: meh )EOF"); - EXPECT_CALL(rtds_init_callback_, Call()); + EXPECT_CALL(init_watcher_, ready()); doOnConfigUpdateVerifyNoThrow(runtime); EXPECT_EQ("bar", loader_->snapshot().get("foo").value().get()); @@ -1041,7 +1048,7 @@ TEST_F(RtdsLoaderImplTest, DeltaOnConfigUpdateSuccess) { foo: bar baz: meh )EOF"); - EXPECT_CALL(rtds_init_callback_, Call()); + EXPECT_CALL(init_watcher_, ready()); doDeltaOnConfigUpdateVerifyNoThrow(runtime); EXPECT_EQ("bar", loader_->snapshot().get("foo").value().get()); @@ -1085,7 +1092,7 @@ TEST_F(RtdsLoaderImplTest, MultipleRtdsLayers) { foo: bar baz: meh )EOF"); - EXPECT_CALL(rtds_init_callback_, Call()).Times(1); + EXPECT_CALL(init_watcher_, ready()).Times(2); doOnConfigUpdateVerifyNoThrow(runtime, 0); EXPECT_EQ("bar", loader_->snapshot().get("foo").value().get()); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 95d0766b78504..2e300b5b2844f 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -41,7 +41,6 @@ class ClusterManagerImplTest : public testing::Test { bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, *api_, http_context_, grpc_context_); - cluster_manager_->initializeSecondaryClusters(bootstrap); } void createWithLocalClusterUpdate(const bool enable_merge_window = true) { @@ -2830,7 +2829,6 @@ TEST_F(ClusterManagerInitHelperTest, ImmediateInitialize) { cluster1.initialize_callback_(); init_helper_.onStaticLoadComplete(); - init_helper_.startInitializingSecondaryClusters(); ReadyWatcher cm_initialized; EXPECT_CALL(cm_initialized, ready()); @@ -2851,10 +2849,8 @@ TEST_F(ClusterManagerInitHelperTest, StaticSdsInitialize) { ON_CALL(cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); init_helper_.addCluster(cluster1); - init_helper_.onStaticLoadComplete(); - EXPECT_CALL(cluster1, initialize(_)); - init_helper_.startInitializingSecondaryClusters(); + init_helper_.onStaticLoadComplete(); ReadyWatcher cm_initialized; init_helper_.setInitializedCb([&]() -> void { cm_initialized.ready(); }); @@ -2905,9 +2901,8 @@ TEST_F(ClusterManagerInitHelperTest, InitSecondaryWithoutEdsPaused) { ON_CALL(cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); init_helper_.addCluster(cluster1); - init_helper_.onStaticLoadComplete(); EXPECT_CALL(cluster1, initialize(_)); - init_helper_.startInitializingSecondaryClusters(); + init_helper_.onStaticLoadComplete(); EXPECT_CALL(*this, onClusterInit(Ref(cluster1))); EXPECT_CALL(cm_initialized, ready()); @@ -2928,10 +2923,8 @@ TEST_F(ClusterManagerInitHelperTest, InitSecondaryWithEdsPaused) { ON_CALL(cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); init_helper_.addCluster(cluster1); - init_helper_.onStaticLoadComplete(); - EXPECT_CALL(cluster1, initialize(_)); - init_helper_.startInitializingSecondaryClusters(); + init_helper_.onStaticLoadComplete(); EXPECT_CALL(*this, onClusterInit(Ref(cluster1))); EXPECT_CALL(cm_initialized, ready()); @@ -2979,9 +2972,6 @@ TEST_F(ClusterManagerInitHelperTest, RemoveClusterWithinInitLoop) { ON_CALL(cluster, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); init_helper_.addCluster(cluster); - // onStaticLoadComplete() must not initialize secondary clusters - init_helper_.onStaticLoadComplete(); - // Set up the scenario seen in Issue 903 where initialize() ultimately results // in the removeCluster() call. In the real bug this was a long and complex call // chain. @@ -2989,9 +2979,9 @@ TEST_F(ClusterManagerInitHelperTest, RemoveClusterWithinInitLoop) { init_helper_.removeCluster(cluster); })); - // Now call initializeSecondaryClusters which will exercise maybeFinishInitialize() + // Now call onStaticLoadComplete which will exercise maybeFinishInitialize() // which calls initialize() on the members of the secondary init list. - init_helper_.startInitializingSecondaryClusters(); + init_helper_.onStaticLoadComplete(); } // Validate that when options are set in the ClusterManager and/or Cluster, we see the socket option diff --git a/test/extensions/clusters/aggregate/cluster_update_test.cc b/test/extensions/clusters/aggregate/cluster_update_test.cc index e7cbbcb4311de..f040c6b88c5d8 100644 --- a/test/extensions/clusters/aggregate/cluster_update_test.cc +++ b/test/extensions/clusters/aggregate/cluster_update_test.cc @@ -35,12 +35,10 @@ class AggregateClusterUpdateTest : public testing::Test { : http_context_(stats_store_.symbolTable()), grpc_context_(stats_store_.symbolTable()) {} void initialize(const std::string& yaml_config) { - auto bootstrap = parseBootstrapFromV2Yaml(yaml_config); cluster_manager_ = std::make_unique( - bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, - factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, - *api_, http_context_, grpc_context_); - cluster_manager_->initializeSecondaryClusters(bootstrap); + parseBootstrapFromV2Yaml(yaml_config), factory_, factory_.stats_, factory_.tls_, + factory_.runtime_, factory_.random_, factory_.local_info_, log_manager_, + factory_.dispatcher_, admin_, validation_context_, *api_, http_context_, grpc_context_); EXPECT_EQ(cluster_manager_->activeClusters().size(), 1); cluster_ = cluster_manager_->get("aggregate_cluster"); } @@ -259,12 +257,10 @@ TEST_F(AggregateClusterUpdateTest, InitializeAggregateClusterAfterOtherClusters) - secondary )EOF"; - auto bootstrap = parseBootstrapFromV2Yaml(config); cluster_manager_ = std::make_unique( - bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, - factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, *api_, - http_context_, grpc_context_); - cluster_manager_->initializeSecondaryClusters(bootstrap); + parseBootstrapFromV2Yaml(config), factory_, factory_.stats_, factory_.tls_, factory_.runtime_, + factory_.random_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, + validation_context_, *api_, http_context_, grpc_context_); EXPECT_EQ(cluster_manager_->activeClusters().size(), 2); cluster_ = cluster_manager_->get("aggregate_cluster"); auto primary = cluster_manager_->get("primary"); diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 9153bce772a05..42de78c81fae6 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -889,107 +889,4 @@ TEST_P(AdsClusterFromFileIntegrationTest, BasicTestWidsAdsEndpointLoadedFromFile {"ads_eds_cluster"}, {}, {})); } -class AdsIntegrationTestWithRtds : public AdsIntegrationTest { -public: - AdsIntegrationTestWithRtds() = default; - - void initialize() override { - config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - auto* layered_runtime = bootstrap.mutable_layered_runtime(); - auto* layer = layered_runtime->add_layers(); - layer->set_name("foobar"); - auto* rtds_layer = layer->mutable_rtds_layer(); - rtds_layer->set_name("ads_rtds_layer"); - auto* rtds_config = rtds_layer->mutable_rtds_config(); - rtds_config->mutable_ads(); - - auto* ads_config = bootstrap.mutable_dynamic_resources()->mutable_ads_config(); - ads_config->set_set_node_on_first_message_only(true); - }); - AdsIntegrationTest::initialize(); - } - - void testBasicFlow() { - // Test that runtime discovery request comes first and cluster discovery request comes after - // runtime was loaded. - EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"ads_rtds_layer"}, - {"ads_rtds_layer"}, {}, true)); - auto some_rtds_layer = TestUtility::parseYaml(R"EOF( - name: ads_rtds_layer - layer: - foo: bar - baz: meh - )EOF"); - sendDiscoveryResponse( - Config::TypeUrl::get().Runtime, {some_rtds_layer}, {some_rtds_layer}, {}, "1"); - - test_server_->waitForCounterGe("runtime.load_success", 1); - EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, false)); - EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "1", {"ads_rtds_layer"}, {}, - {}, false)); - } -}; - -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsIntegrationTestWithRtds, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); - -TEST_P(AdsIntegrationTestWithRtds, Basic) { - initialize(); - testBasicFlow(); -} - -class AdsIntegrationTestWithRtdsAndSecondaryClusters : public AdsIntegrationTestWithRtds { -public: - AdsIntegrationTestWithRtdsAndSecondaryClusters() = default; - - void initialize() override { - config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - // Add secondary cluster to the list of static resources. - auto* eds_cluster = bootstrap.mutable_static_resources()->add_clusters(); - eds_cluster->set_name("eds_cluster"); - eds_cluster->set_type(envoy::config::cluster::v3::Cluster::EDS); - auto* eds_cluster_config = eds_cluster->mutable_eds_cluster_config(); - eds_cluster_config->mutable_eds_config()->mutable_ads(); - }); - AdsIntegrationTestWithRtds::initialize(); - } - - void testBasicFlow() { - // Test that runtime discovery request comes first followed by the cluster load assignment - // discovery request for secondary cluster and then CDS discovery request. - EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"ads_rtds_layer"}, - {"ads_rtds_layer"}, {}, true)); - auto some_rtds_layer = TestUtility::parseYaml(R"EOF( - name: ads_rtds_layer - layer: - foo: bar - baz: meh - )EOF"); - sendDiscoveryResponse( - Config::TypeUrl::get().Runtime, {some_rtds_layer}, {some_rtds_layer}, {}, "1"); - - test_server_->waitForCounterGe("runtime.load_success", 1); - EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "", - {"eds_cluster"}, {"eds_cluster"}, {}, false)); - sendDiscoveryResponse( - Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("eds_cluster")}, - {buildClusterLoadAssignment("eds_cluster")}, {}, "1"); - - EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "1", {"ads_rtds_layer"}, {}, - {}, false)); - EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, false)); - sendDiscoveryResponse( - Config::TypeUrl::get().Cluster, {buildCluster("cluster_0")}, {buildCluster("cluster_0")}, - {}, "1"); - } -}; - -INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsIntegrationTestWithRtdsAndSecondaryClusters, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); - -TEST_P(AdsIntegrationTestWithRtdsAndSecondaryClusters, Basic) { - initialize(); - testBasicFlow(); -} - } // namespace Envoy diff --git a/test/mocks/runtime/mocks.h b/test/mocks/runtime/mocks.h index d73bb3eb5317b..532c5650e3a11 100644 --- a/test/mocks/runtime/mocks.h +++ b/test/mocks/runtime/mocks.h @@ -74,7 +74,6 @@ class MockLoader : public Loader { MOCK_METHOD(const Snapshot&, snapshot, ()); MOCK_METHOD(std::shared_ptr, threadsafeSnapshot, ()); MOCK_METHOD(void, mergeValues, ((const std::unordered_map&))); - MOCK_METHOD(void, startRtdsSubscriptions, (ReadyCallback)); testing::NiceMock snapshot_; }; diff --git a/test/mocks/upstream/mocks.h b/test/mocks/upstream/mocks.h index 2e659cd59464d..9a8ca01b00f8e 100644 --- a/test/mocks/upstream/mocks.h +++ b/test/mocks/upstream/mocks.h @@ -309,8 +309,6 @@ class MockClusterManager : public ClusterManager { (const envoy::config::cluster::v3::Cluster& cluster, const std::string& version_info)); MOCK_METHOD(void, setInitializedCb, (std::function)); - MOCK_METHOD(void, initializeSecondaryClusters, - (const envoy::config::bootstrap::v3::Bootstrap& bootstrap)); MOCK_METHOD(ClusterInfoMap, clusters, ()); MOCK_METHOD(ThreadLocalCluster*, get, (absl::string_view cluster)); MOCK_METHOD(Http::ConnectionPool::Instance*, httpConnPoolForCluster, diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 36ea86675dc36..6c8a9798c8b99 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -681,6 +681,7 @@ TEST_P(ServerInstanceImplTest, BootstrapRuntime) { EXPECT_EQ("bar", server_->runtime().snapshot().get("foo").value().get()); // This should access via the override/some_service overlay. EXPECT_EQ("fozz", server_->runtime().snapshot().get("fizz").value().get()); + EXPECT_EQ("foobar", server_->runtime().snapshot().getLayers()[3]->name()); } // Validate that a runtime absent an admin layer will fail mutating operations @@ -699,22 +700,6 @@ TEST_P(ServerInstanceImplTest, RuntimeNoAdminLayer) { EXPECT_EQ("No admin layer specified", response_body); } -// Verify that bootstrap fails if RTDS is configured through an EDS cluster -TEST_P(ServerInstanceImplTest, BootstrapRtdsThroughEdsFails) { - options_.service_cluster_name_ = "some_service"; - options_.service_node_name_ = "some_node_name"; - EXPECT_THROW_WITH_REGEX(initialize("test/server/test_data/server/runtime_bootstrap_eds.yaml"), - EnvoyException, "must have a statically defined non-EDS cluster"); -} - -// Verify that bootstrap fails if RTDS is configured through an ADS using EDS cluster -TEST_P(ServerInstanceImplTest, BootstrapRtdsThroughAdsViaEdsFails) { - options_.service_cluster_name_ = "some_service"; - options_.service_node_name_ = "some_node_name"; - EXPECT_THROW_WITH_REGEX(initialize("test/server/test_data/server/runtime_bootstrap_ads_eds.yaml"), - EnvoyException, "Unknown gRPC client cluster"); -} - TEST_P(ServerInstanceImplTest, DEPRECATED_FEATURE_TEST(InvalidLegacyBootstrapRuntime)) { EXPECT_THROW_WITH_MESSAGE( initialize("test/server/test_data/server/invalid_runtime_bootstrap.yaml"), EnvoyException, diff --git a/test/server/test_data/server/runtime_bootstrap.yaml b/test/server/test_data/server/runtime_bootstrap.yaml index e92c3fd5a9039..ab26028ef1839 100644 --- a/test/server/test_data/server/runtime_bootstrap.yaml +++ b/test/server/test_data/server/runtime_bootstrap.yaml @@ -7,3 +7,12 @@ layered_runtime: disk_layer: { symlink_root: {{ test_rundir }}/test/server/test_data/runtime/primary } - name: overlay_disk_layer disk_layer: { symlink_root: {{ test_rundir }}/test/server/test_data/runtime/override, append_service_cluster: true } + - name: foobar + rtds_layer: + name: foobar + rtds_config: + api_config_source: + api_type: GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster diff --git a/test/server/test_data/server/runtime_bootstrap_ads_eds.yaml b/test/server/test_data/server/runtime_bootstrap_ads_eds.yaml deleted file mode 100644 index 9bd1730bf927b..0000000000000 --- a/test/server/test_data/server/runtime_bootstrap_ads_eds.yaml +++ /dev/null @@ -1,38 +0,0 @@ -static_resources: - clusters: - - name: dummy_cluster - connect_timeout: 1s - load_assignment: - cluster_name: dummy_cluster - endpoints: - - lb_endpoints: - - endpoint: - address: - socket_address: - address: {{ ntop_ip_loopback_address }} - port_value: 0 - - name: ads_cluster - connect_timeout: 1s - type: EDS - eds_cluster_config: - eds_config: - api_config_source: - api_type: GRPC - grpc_services: - envoy_grpc: - cluster_name: "dummy_cluster" -dynamic_resources: - ads_config: - api_type: GRPC - grpc_services: - envoy_grpc: - cluster_name: ads_cluster - set_node_on_first_message_only: true -layered_runtime: - layers: - - name: foobar - rtds_layer: - name: foobar - rtds_config: - ads: {} - diff --git a/test/server/test_data/server/runtime_bootstrap_eds.yaml b/test/server/test_data/server/runtime_bootstrap_eds.yaml deleted file mode 100644 index c74b692288e18..0000000000000 --- a/test/server/test_data/server/runtime_bootstrap_eds.yaml +++ /dev/null @@ -1,35 +0,0 @@ -static_resources: - clusters: - - name: dummy_cluster - connect_timeout: 1s - load_assignment: - cluster_name: dummy_cluster - endpoints: - - lb_endpoints: - - endpoint: - address: - socket_address: - address: {{ ntop_ip_loopback_address }} - port_value: 0 - - name: rtds_cluster - connect_timeout: 1s - type: EDS - eds_cluster_config: - eds_config: - api_config_source: - api_type: GRPC - grpc_services: - envoy_grpc: - cluster_name: "dummy_cluster" -layered_runtime: - layers: - - name: foobar - rtds_layer: - name: foobar - rtds_config: - api_config_source: - api_type: GRPC - grpc_services: - envoy_grpc: - cluster_name: rtds_cluster - diff --git a/test/test_common/test_runtime.h b/test/test_common/test_runtime.h index 0532b5529f9f2..93bc51876ddea 100644 --- a/test/test_common/test_runtime.h +++ b/test/test_common/test_runtime.h @@ -35,8 +35,8 @@ class TestScopedRuntime { config.add_layers()->mutable_admin_layer(); loader_ = std::make_unique( - std::make_unique(dispatcher_, tls_, config, local_info_, store_, - generator_, validation_visitor_, *api_)); + std::make_unique(dispatcher_, tls_, config, local_info_, init_manager_, + store_, generator_, validation_visitor_, *api_)); } private: @@ -46,6 +46,7 @@ class TestScopedRuntime { Runtime::MockRandomGenerator generator_; Api::ApiPtr api_; testing::NiceMock local_info_; + Init::MockManager init_manager_; testing::NiceMock validation_visitor_; std::unique_ptr loader_; };