From 60deb79b24855ab5ae7a3258afb555a70f816cdf Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 15 Dec 2023 13:41:20 -0800 Subject: [PATCH] [xDS] don't allocate drop config if not needed (#35326) Closes #35326 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35326 from markdroth:xds_drop_config_cleanup cad30e861eeb60e33a7bdbcc66072888d74216a6 PiperOrigin-RevId: 591348198 --- .../lb_policy/xds/xds_cluster_impl.cc | 4 ++-- .../lb_policy/xds/xds_cluster_resolver.cc | 19 ++++++++++------- src/core/ext/xds/xds_endpoint.cc | 10 ++++++--- src/core/ext/xds/xds_endpoint.h | 6 +++++- .../xds/xds_endpoint_resource_type_test.cc | 21 +++++++------------ 5 files changed, 32 insertions(+), 28 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc index 8548de565d643..123d9b7ad7c90 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc @@ -371,7 +371,7 @@ LoadBalancingPolicy::PickResult XdsClusterImplLb::Picker::Pick( LoadBalancingPolicy::PickArgs args) { // Handle EDS drops. const std::string* drop_category; - if (drop_config_->ShouldDrop(&drop_category)) { + if (drop_config_ != nullptr && drop_config_->ShouldDrop(&drop_category)) { if (drop_stats_ != nullptr) drop_stats_->AddCallDropped(*drop_category); return PickResult::Drop(absl::UnavailableError( absl::StrCat("EDS-configured drop: ", *drop_category))); @@ -705,7 +705,7 @@ void XdsClusterImplLbConfig::JsonPostLoad(const Json& json, // Parse "dropCategories" field. { auto value = LoadJsonObjectField>( - json.object(), args, "dropCategories", errors); + json.object(), args, "dropCategories", errors, /*required=*/false); if (value.has_value()) { drop_config_ = MakeRefCounted(); for (size_t i = 0; i < value->size(); ++i) { diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc index 6a58213347a1d..f6963f5d73d6d 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc @@ -889,8 +889,14 @@ XdsClusterResolverLb::CreateChildPolicyConfigLocked() { Json::FromObject(std::move(xds_override_host_lb_config))}, })}; // Wrap it in the xds_cluster_impl policy. - Json::Array drop_categories; + Json::Object xds_cluster_impl_config = { + {"clusterName", Json::FromString(discovery_config.cluster_name)}, + {"childPolicy", Json::FromArray(std::move(xds_override_host_config))}, + {"maxConcurrentRequests", + Json::FromNumber(discovery_config.max_concurrent_requests)}, + }; if (discovery_entry.latest_update->drop_config != nullptr) { + Json::Array drop_categories; for (const auto& category : discovery_entry.latest_update->drop_config->drop_category_list()) { drop_categories.push_back(Json::FromObject({ @@ -899,14 +905,11 @@ XdsClusterResolverLb::CreateChildPolicyConfigLocked() { Json::FromNumber(category.parts_per_million)}, })); } + if (!drop_categories.empty()) { + xds_cluster_impl_config["dropCategories"] = + Json::FromArray(std::move(drop_categories)); + } } - Json::Object xds_cluster_impl_config = { - {"clusterName", Json::FromString(discovery_config.cluster_name)}, - {"childPolicy", Json::FromArray(std::move(xds_override_host_config))}, - {"dropCategories", Json::FromArray(std::move(drop_categories))}, - {"maxConcurrentRequests", - Json::FromNumber(discovery_config.max_concurrent_requests)}, - }; if (!discovery_config.eds_service_name.empty()) { xds_cluster_impl_config["edsServiceName"] = Json::FromString(discovery_config.eds_service_name); diff --git a/src/core/ext/xds/xds_endpoint.cc b/src/core/ext/xds/xds_endpoint.cc index 7ac79519ea50b..f1d2da6eec6ca 100644 --- a/src/core/ext/xds/xds_endpoint.cc +++ b/src/core/ext/xds/xds_endpoint.cc @@ -142,8 +142,9 @@ std::string XdsEndpointResource::ToString() const { priority_strings.emplace_back( absl::StrCat("priority ", i, ": ", priority.ToString())); } - return absl::StrCat("priorities=[", absl::StrJoin(priority_strings, ", "), - "], drop_config=", drop_config->ToString()); + return absl::StrCat( + "priorities=[", absl::StrJoin(priority_strings, ", "), "], drop_config=", + drop_config == nullptr ? "" : drop_config->ToString()); } // @@ -447,7 +448,6 @@ absl::StatusOr> EdsResourceParse( } } // policy - eds_resource->drop_config = MakeRefCounted(); const auto* policy = envoy_config_endpoint_v3_ClusterLoadAssignment_policy( cluster_load_assignment); if (policy != nullptr) { @@ -456,6 +456,10 @@ absl::StatusOr> EdsResourceParse( const auto* const* drop_overload = envoy_config_endpoint_v3_ClusterLoadAssignment_Policy_drop_overloads( policy, &drop_size); + if (drop_size > 0) { + eds_resource->drop_config = + MakeRefCounted(); + } for (size_t i = 0; i < drop_size; ++i) { ValidationErrors::ScopedField field( &errors, absl::StrCat(".drop_overloads[", i, "]")); diff --git a/src/core/ext/xds/xds_endpoint.h b/src/core/ext/xds/xds_endpoint.h index 3c3e34550fce3..369e0921a6c86 100644 --- a/src/core/ext/xds/xds_endpoint.h +++ b/src/core/ext/xds/xds_endpoint.h @@ -62,6 +62,7 @@ struct XdsEndpointResource : public XdsResourceType::ResourceData { std::map localities; bool operator==(const Priority& other) const; + bool operator!=(const Priority& other) const { return !(*this == other); } std::string ToString() const; }; using PriorityList = std::vector; @@ -121,7 +122,10 @@ struct XdsEndpointResource : public XdsResourceType::ResourceData { RefCountedPtr drop_config; bool operator==(const XdsEndpointResource& other) const { - return priorities == other.priorities && *drop_config == *other.drop_config; + if (priorities != other.priorities) return false; + if (drop_config == nullptr) return other.drop_config == nullptr; + if (other.drop_config == nullptr) return false; + return *drop_config == *other.drop_config; } std::string ToString() const; }; diff --git a/test/core/xds/xds_endpoint_resource_type_test.cc b/test/core/xds/xds_endpoint_resource_type_test.cc index a7e668c060fe6..da9bb1cb888aa 100644 --- a/test/core/xds/xds_endpoint_resource_type_test.cc +++ b/test/core/xds/xds_endpoint_resource_type_test.cc @@ -167,8 +167,7 @@ TEST_F(XdsEndpointTest, MinimumValidConfig) { .Set(GRPC_ARG_ADDRESS_WEIGHT, 1) .Set(GRPC_ARG_XDS_HEALTH_STATUS, XdsHealthStatus::HealthStatus::kUnknown)); - ASSERT_NE(resource.drop_config, nullptr); - EXPECT_TRUE(resource.drop_config->drop_category_list().empty()); + EXPECT_EQ(resource.drop_config, nullptr); } TEST_F(XdsEndpointTest, EndpointWeight) { @@ -214,8 +213,7 @@ TEST_F(XdsEndpointTest, EndpointWeight) { .Set(GRPC_ARG_ADDRESS_WEIGHT, 3) .Set(GRPC_ARG_XDS_HEALTH_STATUS, XdsHealthStatus::HealthStatus::kUnknown)); - ASSERT_NE(resource.drop_config, nullptr); - EXPECT_TRUE(resource.drop_config->drop_category_list().empty()); + EXPECT_EQ(resource.drop_config, nullptr); } TEST_F(XdsEndpointTest, IgnoresLocalityWithNoWeight) { @@ -263,8 +261,7 @@ TEST_F(XdsEndpointTest, IgnoresLocalityWithNoWeight) { .Set(GRPC_ARG_ADDRESS_WEIGHT, 1) .Set(GRPC_ARG_XDS_HEALTH_STATUS, XdsHealthStatus::HealthStatus::kUnknown)); - ASSERT_NE(resource.drop_config, nullptr); - EXPECT_TRUE(resource.drop_config->drop_category_list().empty()); + EXPECT_EQ(resource.drop_config, nullptr); } TEST_F(XdsEndpointTest, IgnoresLocalityWithZeroWeight) { @@ -313,8 +310,7 @@ TEST_F(XdsEndpointTest, IgnoresLocalityWithZeroWeight) { .Set(GRPC_ARG_ADDRESS_WEIGHT, 1) .Set(GRPC_ARG_XDS_HEALTH_STATUS, XdsHealthStatus::HealthStatus::kUnknown)); - ASSERT_NE(resource.drop_config, nullptr); - EXPECT_TRUE(resource.drop_config->drop_category_list().empty()); + EXPECT_EQ(resource.drop_config, nullptr); } TEST_F(XdsEndpointTest, LocalityWithNoEndpoints) { @@ -346,8 +342,7 @@ TEST_F(XdsEndpointTest, LocalityWithNoEndpoints) { EXPECT_EQ(p.first->sub_zone(), "mysubzone"); EXPECT_EQ(p.second.lb_weight, 1); EXPECT_EQ(p.second.endpoints.size(), 0); - ASSERT_NE(resource.drop_config, nullptr); - EXPECT_TRUE(resource.drop_config->drop_category_list().empty()); + EXPECT_EQ(resource.drop_config, nullptr); } TEST_F(XdsEndpointTest, NoLocality) { @@ -544,8 +539,7 @@ TEST_F(XdsEndpointTest, MultipleAddressesPerEndpoint) { .Set(GRPC_ARG_ADDRESS_WEIGHT, 1) .Set(GRPC_ARG_XDS_HEALTH_STATUS, XdsHealthStatus::HealthStatus::kUnknown)); - ASSERT_NE(resource.drop_config, nullptr); - EXPECT_TRUE(resource.drop_config->drop_category_list().empty()); + EXPECT_EQ(resource.drop_config, nullptr); } TEST_F(XdsEndpointTest, AdditionalAddressesMissingAddress) { @@ -735,8 +729,7 @@ TEST_F(XdsEndpointTest, IgnoresMultipleAddressesPerEndpointWhenNotEnabled) { .Set(GRPC_ARG_ADDRESS_WEIGHT, 1) .Set(GRPC_ARG_XDS_HEALTH_STATUS, XdsHealthStatus::HealthStatus::kUnknown)); - ASSERT_NE(resource.drop_config, nullptr); - EXPECT_TRUE(resource.drop_config->drop_category_list().empty()); + EXPECT_EQ(resource.drop_config, nullptr); } TEST_F(XdsEndpointTest, MissingEndpoint) {