Skip to content

Commit

Permalink
[xDS] don't allocate drop config if not needed (grpc#35326)
Browse files Browse the repository at this point in the history 
Closes grpc#35326

COPYBARA_INTEGRATE_REVIEW=grpc#35326 from markdroth:xds_drop_config_cleanup cad30e8
PiperOrigin-RevId: 591348198
  • Loading branch information
@markdroth @copybara-github
markdroth authored and copybara-github committed Dec 15, 2023
1 parent 4f7ead2 commit 60deb79
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 28 deletions.
4 changes: 2 additions & 2 deletions src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Expand Down Expand Up @@ -705,7 +705,7 @@ void XdsClusterImplLbConfig::JsonPostLoad(const Json& json,
// Parse "dropCategories" field.
{
auto value = LoadJsonObjectField<std::vector<DropCategory>>(
json.object(), args, "dropCategories", errors);
json.object(), args, "dropCategories", errors, /*required=*/false);
if (value.has_value()) {
drop_config_ = MakeRefCounted<XdsEndpointResource::DropConfig>();
for (size_t i = 0; i < value->size(); ++i) {
Expand Down
19 changes: 11 additions & 8 deletions src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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);
Expand Down
10 changes: 7 additions & 3 deletions src/core/ext/xds/xds_endpoint.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ? "<null>" : drop_config->ToString());
}

//
Expand Down Expand Up @@ -447,7 +448,6 @@ absl::StatusOr<std::shared_ptr<const XdsEndpointResource>> EdsResourceParse(
}
}
// policy
eds_resource->drop_config = MakeRefCounted<XdsEndpointResource::DropConfig>();
const auto* policy = envoy_config_endpoint_v3_ClusterLoadAssignment_policy(
cluster_load_assignment);
if (policy != nullptr) {
Expand All @@ -456,6 +456,10 @@ absl::StatusOr<std::shared_ptr<const XdsEndpointResource>> 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<XdsEndpointResource::DropConfig>();
}
for (size_t i = 0; i < drop_size; ++i) {
ValidationErrors::ScopedField field(
&errors, absl::StrCat(".drop_overloads[", i, "]"));
Expand Down
6 changes: 5 additions & 1 deletion src/core/ext/xds/xds_endpoint.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ struct XdsEndpointResource : public XdsResourceType::ResourceData {
std::map<XdsLocalityName*, Locality, XdsLocalityName::Less> localities;

bool operator==(const Priority& other) const;
bool operator!=(const Priority& other) const { return !(*this == other); }
std::string ToString() const;
};
using PriorityList = std::vector<Priority>;
Expand Down Expand Up @@ -121,7 +122,10 @@ struct XdsEndpointResource : public XdsResourceType::ResourceData {
RefCountedPtr<DropConfig> 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;
};
Expand Down
21 changes: 7 additions & 14 deletions test/core/xds/xds_endpoint_resource_type_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 60deb79

Please sign in to comment.