diff --git a/src/core/ext/xds/xds_bootstrap.h b/src/core/ext/xds/xds_bootstrap.h index cc38cbb9b41..8a1a8214f65 100644 --- a/src/core/ext/xds/xds_bootstrap.h +++ b/src/core/ext/xds/xds_bootstrap.h @@ -50,6 +50,9 @@ class XdsBootstrap { virtual bool Equals(const XdsServer& other) const = 0; + // Returns a key to be used for uniquely identifying this XdsServer. + virtual std::string Key() const = 0; + friend bool operator==(const XdsServer& a, const XdsServer& b) { return a.Equals(b); } @@ -80,10 +83,6 @@ class XdsBootstrap { // Returns a pointer to the specified authority, or null if it does // not exist in this bootstrap config. virtual const Authority* LookupAuthority(const std::string& name) const = 0; - - // If the server exists in the bootstrap config, returns a pointer to - // the XdsServer instance in the config. Otherwise, returns null. - virtual const XdsServer* FindXdsServer(const XdsServer& server) const = 0; }; } // namespace grpc_core diff --git a/src/core/ext/xds/xds_bootstrap_grpc.cc b/src/core/ext/xds/xds_bootstrap_grpc.cc index 92b27257d3b..dd2b3da3c87 100644 --- a/src/core/ext/xds/xds_bootstrap_grpc.cc +++ b/src/core/ext/xds/xds_bootstrap_grpc.cc @@ -101,6 +101,10 @@ bool GrpcXdsBootstrap::GrpcXdsServer::Equals(const XdsServer& other) const { server_features_ == o.server_features_); } +std::string GrpcXdsBootstrap::GrpcXdsServer::Key() const { + return JsonDump(ToJson()); +} + const JsonLoaderInterface* GrpcXdsBootstrap::GrpcXdsServer::JsonLoader( const JsonArgs&) { static const auto* loader = @@ -358,19 +362,4 @@ const XdsBootstrap::Authority* GrpcXdsBootstrap::LookupAuthority( return nullptr; } -const XdsBootstrap::XdsServer* GrpcXdsBootstrap::FindXdsServer( - const XdsBootstrap::XdsServer& server) const { - if (static_cast(server) == servers_[0]) { - return &servers_[0]; - } - for (auto& p : authorities_) { - const auto* authority_server = - static_cast(p.second.server()); - if (authority_server != nullptr && *authority_server == server) { - return authority_server; - } - } - return nullptr; -} - } // namespace grpc_core diff --git a/src/core/ext/xds/xds_bootstrap_grpc.h b/src/core/ext/xds/xds_bootstrap_grpc.h index ecc9f519fd5..e3506833c65 100644 --- a/src/core/ext/xds/xds_bootstrap_grpc.h +++ b/src/core/ext/xds/xds_bootstrap_grpc.h @@ -84,6 +84,8 @@ class GrpcXdsBootstrap : public XdsBootstrap { bool Equals(const XdsServer& other) const override; + std::string Key() const override; + RefCountedPtr channel_creds_config() const { return channel_creds_config_; } @@ -132,7 +134,6 @@ class GrpcXdsBootstrap : public XdsBootstrap { return node_.has_value() ? &*node_ : nullptr; } const Authority* LookupAuthority(const std::string& name) const override; - const XdsServer* FindXdsServer(const XdsServer& server) const override; const std::string& client_default_listener_resource_name_template() const { return client_default_listener_resource_name_template_; diff --git a/src/core/ext/xds/xds_client.cc b/src/core/ext/xds/xds_client.cc index 8b45be0c831..ba3b9697351 100644 --- a/src/core/ext/xds/xds_client.cc +++ b/src/core/ext/xds/xds_client.cc @@ -488,7 +488,7 @@ void XdsClient::XdsChannel::Orphan() ABSL_NO_THREAD_SAFETY_ANALYSIS { // At this time, all strong refs are removed, remove from channel map to // prevent subsequent subscription from trying to use this XdsChannel as // it is shutting down. - xds_client_->xds_server_channel_map_.erase(&server_); + xds_client_->xds_channel_map_.erase(server_.Key()); ads_call_.reset(); lrs_call_.reset(); } @@ -510,7 +510,7 @@ void XdsClient::XdsChannel::MaybeStartLrsCall() { } void XdsClient::XdsChannel::StopLrsCallLocked() { - xds_client_->xds_load_report_server_map_.erase(&server_); + xds_client_->xds_load_report_server_map_.erase(server_.Key()); lrs_call_.reset(); } @@ -1308,8 +1308,8 @@ void XdsClient::XdsChannel::LrsCall::Orphan() { void XdsClient::XdsChannel::LrsCall::MaybeScheduleNextReportLocked() { // If there are no more registered stats to report, cancel the call. - auto it = - xds_client()->xds_load_report_server_map_.find(&xds_channel()->server_); + auto it = xds_client()->xds_load_report_server_map_.find( + xds_channel()->server_.Key()); if (it == xds_client()->xds_load_report_server_map_.end() || it->second.load_report_map.empty()) { it->second.xds_channel->StopLrsCallLocked(); @@ -1531,14 +1531,15 @@ void XdsClient::Orphan() { RefCountedPtr XdsClient::GetOrCreateXdsChannelLocked( const XdsBootstrap::XdsServer& server, const char* reason) { - auto it = xds_server_channel_map_.find(&server); - if (it != xds_server_channel_map_.end()) { + std::string key = server.Key(); + auto it = xds_channel_map_.find(key); + if (it != xds_channel_map_.end()) { return it->second->Ref(DEBUG_LOCATION, reason); } // Channel not found, so create a new one. auto xds_channel = MakeRefCounted(WeakRef(DEBUG_LOCATION, "XdsChannel"), server); - xds_server_channel_map_[&server] = xds_channel.get(); + xds_channel_map_[std::move(key)] = xds_channel.get(); return xds_channel; } @@ -1580,14 +1581,6 @@ void XdsClient::WatchResource(const XdsResourceType* type, xds_server = authority->server(); } if (xds_server == nullptr) xds_server = &bootstrap_->server(); - // Canonify the xDS server instance, so that we make sure we're using - // the same instance as will be used in AddClusterDropStats() and - // AddClusterLocalityStats(). This may yield a different result than - // the logic above if the same server is listed both in the authority - // and as the top-level server. - // TODO(roth): This is really ugly -- need to find a better way to - // index the xDS server than by address here. - xds_server = bootstrap_->FindXdsServer(*xds_server); { MutexLock lock(&mu_); MaybeRegisterResourceTypeLocked(type); @@ -1781,23 +1774,21 @@ std::string XdsClient::ConstructFullXdsResourceName( RefCountedPtr XdsClient::AddClusterDropStats( const XdsBootstrap::XdsServer& xds_server, absl::string_view cluster_name, absl::string_view eds_service_name) { - const auto* server = bootstrap_->FindXdsServer(xds_server); - if (server == nullptr) return nullptr; auto key = std::make_pair(std::string(cluster_name), std::string(eds_service_name)); RefCountedPtr cluster_drop_stats; { MutexLock lock(&mu_); - // We jump through some hoops here to make sure that the const - // XdsBootstrap::XdsServer& and absl::string_views - // stored in the XdsClusterDropStats object point to the - // XdsBootstrap::XdsServer and strings - // in the load_report_map_ key, so that they have the same lifetime. - auto server_it = - xds_load_report_server_map_.emplace(server, LoadReportServer()).first; + // We jump through some hoops here to make sure that the + // absl::string_views stored in the XdsClusterDropStats object point + // to the strings in the xds_load_report_server_map_ keys, so that + // they have the same lifetime. + auto server_it = xds_load_report_server_map_ + .emplace(xds_server.Key(), LoadReportServer()) + .first; if (server_it->second.xds_channel == nullptr) { - server_it->second.xds_channel = - GetOrCreateXdsChannelLocked(*server, "load report map (drop stats)"); + server_it->second.xds_channel = GetOrCreateXdsChannelLocked( + xds_server, "load report map (drop stats)"); } auto load_report_it = server_it->second.load_report_map .emplace(std::move(key), LoadReportState()) @@ -1812,7 +1803,7 @@ RefCountedPtr XdsClient::AddClusterDropStats( load_report_state.drop_stats->GetSnapshotAndReset(); } cluster_drop_stats = MakeRefCounted( - Ref(DEBUG_LOCATION, "DropStats"), *server, + Ref(DEBUG_LOCATION, "DropStats"), server_it->first /*xds_server*/, load_report_it->first.first /*cluster_name*/, load_report_it->first.second /*eds_service_name*/); load_report_state.drop_stats = cluster_drop_stats.get(); @@ -1824,13 +1815,11 @@ RefCountedPtr XdsClient::AddClusterDropStats( } void XdsClient::RemoveClusterDropStats( - const XdsBootstrap::XdsServer& xds_server, absl::string_view cluster_name, + absl::string_view xds_server_key, absl::string_view cluster_name, absl::string_view eds_service_name, XdsClusterDropStats* cluster_drop_stats) { - const auto* server = bootstrap_->FindXdsServer(xds_server); - if (server == nullptr) return; MutexLock lock(&mu_); - auto server_it = xds_load_report_server_map_.find(server); + auto server_it = xds_load_report_server_map_.find(xds_server_key); if (server_it == xds_load_report_server_map_.end()) return; auto load_report_it = server_it->second.load_report_map.find( std::make_pair(std::string(cluster_name), std::string(eds_service_name))); @@ -1849,23 +1838,21 @@ RefCountedPtr XdsClient::AddClusterLocalityStats( const XdsBootstrap::XdsServer& xds_server, absl::string_view cluster_name, absl::string_view eds_service_name, RefCountedPtr locality) { - const auto* server = bootstrap_->FindXdsServer(xds_server); - if (server == nullptr) return nullptr; auto key = std::make_pair(std::string(cluster_name), std::string(eds_service_name)); RefCountedPtr cluster_locality_stats; { MutexLock lock(&mu_); - // We jump through some hoops here to make sure that the const - // XdsBootstrap::XdsServer& and absl::string_views - // stored in the XdsClusterDropStats object point to the - // XdsBootstrap::XdsServer and strings - // in the load_report_map_ key, so that they have the same lifetime. - auto server_it = - xds_load_report_server_map_.emplace(server, LoadReportServer()).first; + // We jump through some hoops here to make sure that the + // absl::string_views stored in the XdsClusterDropStats object point + // to the strings in the xds_load_report_server_map_ keys, so that + // they have the same lifetime. + auto server_it = xds_load_report_server_map_ + .emplace(xds_server.Key(), LoadReportServer()) + .first; if (server_it->second.xds_channel == nullptr) { server_it->second.xds_channel = GetOrCreateXdsChannelLocked( - *server, "load report map (locality stats)"); + xds_server, "load report map (locality stats)"); } auto load_report_it = server_it->second.load_report_map .emplace(std::move(key), LoadReportState()) @@ -1882,7 +1869,7 @@ RefCountedPtr XdsClient::AddClusterLocalityStats( locality_state.locality_stats->GetSnapshotAndReset(); } cluster_locality_stats = MakeRefCounted( - Ref(DEBUG_LOCATION, "LocalityStats"), *server, + Ref(DEBUG_LOCATION, "LocalityStats"), server_it->first /*xds_server*/, load_report_it->first.first /*cluster_name*/, load_report_it->first.second /*eds_service_name*/, std::move(locality)); @@ -1895,14 +1882,12 @@ RefCountedPtr XdsClient::AddClusterLocalityStats( } void XdsClient::RemoveClusterLocalityStats( - const XdsBootstrap::XdsServer& xds_server, absl::string_view cluster_name, + absl::string_view xds_server_key, absl::string_view cluster_name, absl::string_view eds_service_name, const RefCountedPtr& locality, XdsClusterLocalityStats* cluster_locality_stats) { - const auto* server = bootstrap_->FindXdsServer(xds_server); - if (server == nullptr) return; MutexLock lock(&mu_); - auto server_it = xds_load_report_server_map_.find(server); + auto server_it = xds_load_report_server_map_.find(xds_server_key); if (server_it == xds_load_report_server_map_.end()) return; auto load_report_it = server_it->second.load_report_map.find( std::make_pair(std::string(cluster_name), std::string(eds_service_name))); @@ -1922,7 +1907,7 @@ void XdsClient::RemoveClusterLocalityStats( void XdsClient::ResetBackoff() { MutexLock lock(&mu_); - for (auto& p : xds_server_channel_map_) { + for (auto& p : xds_channel_map_) { p.second->ResetBackoff(); } } @@ -1969,7 +1954,7 @@ XdsApi::ClusterLoadReportMap XdsClient::BuildLoadReportSnapshotLocked( gpr_log(GPR_INFO, "[xds_client %p] start building load report", this); } XdsApi::ClusterLoadReportMap snapshot_map; - auto server_it = xds_load_report_server_map_.find(&xds_server); + auto server_it = xds_load_report_server_map_.find(xds_server.Key()); if (server_it == xds_load_report_server_map_.end()) return snapshot_map; auto& load_report_map = server_it->second.load_report_map; for (auto load_report_it = load_report_map.begin(); diff --git a/src/core/ext/xds/xds_client.h b/src/core/ext/xds/xds_client.h index 12c9d094b04..9edfe3ebf8c 100644 --- a/src/core/ext/xds/xds_client.h +++ b/src/core/ext/xds/xds_client.h @@ -127,7 +127,7 @@ class XdsClient : public DualRefCounted { RefCountedPtr AddClusterDropStats( const XdsBootstrap::XdsServer& xds_server, absl::string_view cluster_name, absl::string_view eds_service_name); - void RemoveClusterDropStats(const XdsBootstrap::XdsServer& xds_server, + void RemoveClusterDropStats(absl::string_view xds_server, absl::string_view cluster_name, absl::string_view eds_service_name, XdsClusterDropStats* cluster_drop_stats); @@ -139,7 +139,7 @@ class XdsClient : public DualRefCounted { absl::string_view eds_service_name, RefCountedPtr locality); void RemoveClusterLocalityStats( - const XdsBootstrap::XdsServer& xds_server, absl::string_view cluster_name, + absl::string_view xds_server, absl::string_view cluster_name, absl::string_view eds_service_name, const RefCountedPtr& locality, XdsClusterLocalityStats* cluster_locality_stats); @@ -329,15 +329,13 @@ class XdsClient : public DualRefCounted { upb::DefPool def_pool_ ABSL_GUARDED_BY(mu_); // Map of existing xDS server channels. - // Key is owned by the bootstrap config. - std::map xds_server_channel_map_ + std::map xds_channel_map_ ABSL_GUARDED_BY(mu_); std::map authority_state_map_ ABSL_GUARDED_BY(mu_); - // Key is owned by the bootstrap config. - std::map + std::map> xds_load_report_server_map_ ABSL_GUARDED_BY(mu_); // Stores started watchers whose resource name was not parsed successfully, diff --git a/src/core/ext/xds/xds_client_stats.cc b/src/core/ext/xds/xds_client_stats.cc index 512d5996b12..bb29f906f0b 100644 --- a/src/core/ext/xds/xds_client_stats.cc +++ b/src/core/ext/xds/xds_client_stats.cc @@ -40,10 +40,10 @@ uint64_t GetAndResetCounter(std::atomic* from) { // XdsClusterDropStats // -XdsClusterDropStats::XdsClusterDropStats( - RefCountedPtr xds_client, - const XdsBootstrap::XdsServer& lrs_server, absl::string_view cluster_name, - absl::string_view eds_service_name) +XdsClusterDropStats::XdsClusterDropStats(RefCountedPtr xds_client, + absl::string_view lrs_server, + absl::string_view cluster_name, + absl::string_view eds_service_name) : RefCounted(GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_refcount_trace) ? "XdsClusterDropStats" : nullptr), @@ -53,7 +53,7 @@ XdsClusterDropStats::XdsClusterDropStats( eds_service_name_(eds_service_name) { if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { gpr_log(GPR_INFO, "[xds_client %p] created drop stats %p for {%s, %s, %s}", - xds_client_.get(), this, lrs_server_.server_uri().c_str(), + xds_client_.get(), this, std::string(lrs_server_).c_str(), std::string(cluster_name_).c_str(), std::string(eds_service_name_).c_str()); } @@ -63,7 +63,7 @@ XdsClusterDropStats::~XdsClusterDropStats() { if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { gpr_log(GPR_INFO, "[xds_client %p] destroying drop stats %p for {%s, %s, %s}", - xds_client_.get(), this, lrs_server_.server_uri().c_str(), + xds_client_.get(), this, std::string(lrs_server_).c_str(), std::string(cluster_name_).c_str(), std::string(eds_service_name_).c_str()); } @@ -94,9 +94,9 @@ void XdsClusterDropStats::AddCallDropped(const std::string& category) { // XdsClusterLocalityStats::XdsClusterLocalityStats( - RefCountedPtr xds_client, - const XdsBootstrap::XdsServer& lrs_server, absl::string_view cluster_name, - absl::string_view eds_service_name, RefCountedPtr name) + RefCountedPtr xds_client, absl::string_view lrs_server, + absl::string_view cluster_name, absl::string_view eds_service_name, + RefCountedPtr name) : RefCounted(GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_refcount_trace) ? "XdsClusterLocalityStats" : nullptr), @@ -108,7 +108,7 @@ XdsClusterLocalityStats::XdsClusterLocalityStats( if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { gpr_log(GPR_INFO, "[xds_client %p] created locality stats %p for {%s, %s, %s, %s}", - xds_client_.get(), this, lrs_server_.server_uri().c_str(), + xds_client_.get(), this, std::string(lrs_server_).c_str(), std::string(cluster_name_).c_str(), std::string(eds_service_name_).c_str(), name_->AsHumanReadableString().c_str()); @@ -119,7 +119,7 @@ XdsClusterLocalityStats::~XdsClusterLocalityStats() { if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { gpr_log(GPR_INFO, "[xds_client %p] destroying locality stats %p for {%s, %s, %s, %s}", - xds_client_.get(), this, lrs_server_.server_uri().c_str(), + xds_client_.get(), this, std::string(lrs_server_).c_str(), std::string(cluster_name_).c_str(), std::string(eds_service_name_).c_str(), name_->AsHumanReadableString().c_str()); diff --git a/src/core/ext/xds/xds_client_stats.h b/src/core/ext/xds/xds_client_stats.h index 24c96f06d76..b7ffb1a3a15 100644 --- a/src/core/ext/xds/xds_client_stats.h +++ b/src/core/ext/xds/xds_client_stats.h @@ -141,7 +141,7 @@ class XdsClusterDropStats : public RefCounted { }; XdsClusterDropStats(RefCountedPtr xds_client, - const XdsBootstrap::XdsServer& lrs_server, + absl::string_view lrs_server, absl::string_view cluster_name, absl::string_view eds_service_name); ~XdsClusterDropStats() override; @@ -154,7 +154,7 @@ class XdsClusterDropStats : public RefCounted { private: RefCountedPtr xds_client_; - const XdsBootstrap::XdsServer& lrs_server_; + absl::string_view lrs_server_; absl::string_view cluster_name_; absl::string_view eds_service_name_; std::atomic uncategorized_drops_{0}; @@ -215,7 +215,7 @@ class XdsClusterLocalityStats : public RefCounted { }; XdsClusterLocalityStats(RefCountedPtr xds_client, - const XdsBootstrap::XdsServer& lrs_server_, + absl::string_view lrs_server, absl::string_view cluster_name, absl::string_view eds_service_name, RefCountedPtr name); @@ -244,7 +244,7 @@ class XdsClusterLocalityStats : public RefCounted { }; RefCountedPtr xds_client_; - const XdsBootstrap::XdsServer& lrs_server_; + absl::string_view lrs_server_; absl::string_view cluster_name_; absl::string_view eds_service_name_; RefCountedPtr name_; diff --git a/test/core/xds/xds_client_test.cc b/test/core/xds/xds_client_test.cc index 14f2ffdc9e6..3f9e2bb9615 100644 --- a/test/core/xds/xds_client_test.cc +++ b/test/core/xds/xds_client_test.cc @@ -131,6 +131,9 @@ class XdsClientTest : public ::testing::Test { return server_uri_ == o.server_uri_ && ignore_resource_deletion_ == o.ignore_resource_deletion_; } + std::string Key() const override { + return absl::StrCat(server_uri_, "#", ignore_resource_deletion_); + } void set_server_uri(std::string server_uri) { server_uri_ = std::move(server_uri); @@ -200,18 +203,6 @@ class XdsClientTest : public ::testing::Test { if (it == authorities_.end()) return nullptr; return &it->second; } - const XdsServer* FindXdsServer(const XdsServer& server) const override { - const auto& fake_server = static_cast(server); - if (fake_server == server_) return &server_; - for (const auto& p : authorities_) { - const auto* authority_server = - static_cast(p.second.server()); - if (authority_server != nullptr && *authority_server == fake_server) { - return authority_server; - } - } - return nullptr; - } private: FakeXdsServer server_; @@ -654,21 +645,16 @@ class XdsClientTest : public ::testing::Test { } RefCountedPtr WaitForAdsStream( - const XdsBootstrap::XdsServer& server, + const XdsBootstrap::XdsServer& xds_server, absl::Duration timeout = absl::Seconds(5)) { - const auto* xds_server = xds_client_->bootstrap().FindXdsServer(server); - GPR_ASSERT(xds_server != nullptr); return transport_factory_->WaitForStream( - *xds_server, FakeXdsTransportFactory::kAdsMethod, + xds_server, FakeXdsTransportFactory::kAdsMethod, timeout * grpc_test_slowdown_factor()); } - void TriggerConnectionFailure(const XdsBootstrap::XdsServer& server, + void TriggerConnectionFailure(const XdsBootstrap::XdsServer& xds_server, absl::Status status) { - const auto* xds_server = xds_client_->bootstrap().FindXdsServer(server); - GPR_ASSERT(xds_server != nullptr); - transport_factory_->TriggerConnectionFailure(*xds_server, - std::move(status)); + transport_factory_->TriggerConnectionFailure(xds_server, std::move(status)); } RefCountedPtr WaitForAdsStream( diff --git a/test/core/xds/xds_transport_fake.cc b/test/core/xds/xds_transport_fake.cc index 300e849acef..bc1134f08b6 100644 --- a/test/core/xds/xds_transport_fake.cc +++ b/test/core/xds/xds_transport_fake.cc @@ -209,7 +209,7 @@ void FakeXdsTransportFactory::FakeXdsTransport::TriggerConnectionFailure( void FakeXdsTransportFactory::FakeXdsTransport::Orphan() { { MutexLock lock(&factory_->mu_); - auto it = factory_->transport_map_.find(&server_); + auto it = factory_->transport_map_.find(server_.Key()); if (it != factory_->transport_map_.end() && it->second == this) { factory_->transport_map_.erase(it); } @@ -277,7 +277,7 @@ FakeXdsTransportFactory::Create( std::function on_connectivity_failure, absl::Status* /*status*/) { MutexLock lock(&mu_); - auto& entry = transport_map_[&server]; + auto& entry = transport_map_[server.Key()]; GPR_ASSERT(entry == nullptr); auto transport = MakeOrphanable( RefAsSubclass(), server, @@ -316,7 +316,7 @@ FakeXdsTransportFactory::WaitForStream(const XdsBootstrap::XdsServer& server, RefCountedPtr FakeXdsTransportFactory::GetTransport(const XdsBootstrap::XdsServer& server) { MutexLock lock(&mu_); - return transport_map_[&server]; + return transport_map_[server.Key()]; } } // namespace grpc_core diff --git a/test/core/xds/xds_transport_fake.h b/test/core/xds/xds_transport_fake.h index f3d51de240e..c9b24a536ba 100644 --- a/test/core/xds/xds_transport_fake.h +++ b/test/core/xds/xds_transport_fake.h @@ -251,7 +251,7 @@ class FakeXdsTransportFactory : public XdsTransportFactory { const XdsBootstrap::XdsServer& server); Mutex mu_; - std::map> + std::map> transport_map_ ABSL_GUARDED_BY(&mu_); bool auto_complete_messages_from_client_ ABSL_GUARDED_BY(&mu_) = true; bool abort_on_undrained_messages_ ABSL_GUARDED_BY(&mu_) = true;