From 8427bacaead58fa777c6d495ae03aee726a61254 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 26 Jun 2023 10:02:06 -0700 Subject: [PATCH] [resolver API] remove address attribute interface (#33514) The address attribute interface was intended to provide a mechanism to pass attributes separately from channel args, for values that do not affect subchannel behavior and therefore do not need to be present in the subchannel key, which does include channel args. However, the mechanism as currently designed is fairly clunky and is probably not the direction we will want to go in the long term. Eventually, we will want some mechanism for registering channel args, which would provide a cleaner way to indicate that a given channel arg should not be used in the subchannel key, so that we don't need a completely different mechanism. For now, this PR is just doing an interim step, which is to establish a special channel arg key prefix to indicate that an arg is not needed in the subchannel key. --- BUILD | 1 - CMakeLists.txt | 1 - Makefile | 2 - Package.swift | 2 - build_autogenerated.yaml | 2 - config.m4 | 1 - config.w32 | 1 - gRPC-C++.podspec | 2 - gRPC-Core.podspec | 3 - grpc.gemspec | 2 - grpc.gyp | 1 - package.xml | 2 - src/core/BUILD | 23 +--- .../filters/client_channel/client_channel.cc | 4 +- .../lb_policy/address_filtering.cc | 73 ++++-------- .../lb_policy/address_filtering.h | 26 +++-- .../client_channel/lb_policy/grpclb/grpclb.cc | 63 ++++------- .../grpclb/grpclb_balancer_addresses.cc | 3 +- .../outlier_detection/outlier_detection.cc | 9 +- .../outlier_detection/outlier_detection.h | 18 +-- .../lb_policy/pick_first/pick_first.cc | 6 +- .../lb_policy/ring_hash/ring_hash.cc | 8 +- .../lb_policy/xds/xds_attributes.cc | 42 ------- .../lb_policy/xds/xds_attributes.h | 64 ----------- .../lb_policy/xds/xds_channel_args.h | 8 ++ .../lb_policy/xds/xds_cluster_impl.cc | 9 +- .../lb_policy/xds/xds_cluster_resolver.cc | 29 ++--- .../lb_policy/xds/xds_override_host.cc | 10 +- .../lb_policy/xds/xds_wrr_locality.cc | 19 ++-- src/core/ext/xds/xds_client_grpc.h | 3 +- src/core/ext/xds/xds_client_stats.h | 10 ++ src/core/ext/xds/xds_endpoint.cc | 11 +- src/core/ext/xds/xds_health_status.cc | 17 --- src/core/ext/xds/xds_health_status.h | 30 +---- src/core/lib/channel/channel_args.cc | 11 ++ src/core/lib/channel/channel_args.h | 3 + src/core/lib/resolver/server_address.cc | 104 ++---------------- src/core/lib/resolver/server_address.h | 74 ++----------- src/python/grpcio/grpc_core_dependencies.py | 1 - test/core/channel/channel_args_test.cc | 13 +++ .../client_channel/client_channel_test.cc | 18 +++ .../lb_policy/xds_override_host_test.cc | 10 +- .../xds/xds_endpoint_resource_type_test.cc | 53 ++++----- test/cpp/end2end/client_lb_end2end_test.cc | 67 +++-------- tools/doxygen/Doxyfile.c++.internal | 2 - tools/doxygen/Doxyfile.core.internal | 2 - 46 files changed, 240 insertions(+), 623 deletions(-) delete mode 100644 src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc delete mode 100644 src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h diff --git a/BUILD b/BUILD index 614aa9db96b..945acd00260 100644 --- a/BUILD +++ b/BUILD @@ -2892,7 +2892,6 @@ grpc_cc_library( "absl/status", "absl/status:statusor", "absl/strings", - "absl/strings:str_format", ], language = "c++", visibility = ["@grpc:client_channel"], diff --git a/CMakeLists.txt b/CMakeLists.txt index 79256f992b8..3f5b7a2193f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1707,7 +1707,6 @@ add_library(grpc src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc src/core/ext/filters/client_channel/lb_policy/xds/cds.cc - src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc diff --git a/Makefile b/Makefile index 4c1a32175c8..fe3213dee24 100644 --- a/Makefile +++ b/Makefile @@ -997,7 +997,6 @@ LIBGRPC_SRC = \ src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc \ src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc \ src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \ - src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc \ @@ -3050,7 +3049,6 @@ ifneq ($(OPENSSL_DEP),) # otherwise parallel compilation will fail if a source is compiled first. src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc: $(OPENSSL_DEP) src/core/ext/filters/client_channel/lb_policy/xds/cds.cc: $(OPENSSL_DEP) -src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc: $(OPENSSL_DEP) src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc: $(OPENSSL_DEP) src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc: $(OPENSSL_DEP) src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc: $(OPENSSL_DEP) diff --git a/Package.swift b/Package.swift index c3e971d3d12..19057f6fe4a 100644 --- a/Package.swift +++ b/Package.swift @@ -178,8 +178,6 @@ let package = Package( "src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc", "src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc", "src/core/ext/filters/client_channel/lb_policy/xds/cds.cc", - "src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc", - "src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h", "src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h", "src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc", "src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc", diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 23eaeed3ed8..d96032cc360 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -244,7 +244,6 @@ libs: - src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h - src/core/ext/filters/client_channel/lb_policy/subchannel_list.h - src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride_scheduler.h - - src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h - src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h - src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h - src/core/ext/filters/client_channel/local_subchannel_pool.h @@ -1052,7 +1051,6 @@ libs: - src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc - src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc - src/core/ext/filters/client_channel/lb_policy/xds/cds.cc - - src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc - src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc - src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc - src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc diff --git a/config.m4 b/config.m4 index 22bc192d4b0..f9ee388bae4 100644 --- a/config.m4 +++ b/config.m4 @@ -76,7 +76,6 @@ if test "$PHP_GRPC" != "no"; then src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc \ src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc \ src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \ - src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc \ diff --git a/config.w32 b/config.w32 index a21cb6d4237..f8756ae7c58 100644 --- a/config.w32 +++ b/config.w32 @@ -41,7 +41,6 @@ if (PHP_GRPC != "no") { "src\\core\\ext\\filters\\client_channel\\lb_policy\\weighted_round_robin\\weighted_round_robin.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\weighted_target\\weighted_target.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\xds\\cds.cc " + - "src\\core\\ext\\filters\\client_channel\\lb_policy\\xds\\xds_attributes.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\xds\\xds_cluster_impl.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\xds\\xds_cluster_manager.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\xds\\xds_cluster_resolver.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 1ba1875284a..d342da2858a 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -276,7 +276,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h', 'src/core/ext/filters/client_channel/lb_policy/subchannel_list.h', 'src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride_scheduler.h', - 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h', 'src/core/ext/filters/client_channel/local_subchannel_pool.h', @@ -1338,7 +1337,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h', 'src/core/ext/filters/client_channel/lb_policy/subchannel_list.h', 'src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride_scheduler.h', - 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h', 'src/core/ext/filters/client_channel/local_subchannel_pool.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 5fe96406928..1e24a14310c 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -279,8 +279,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc', 'src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/cds.cc', - 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc', - 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc', @@ -2088,7 +2086,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h', 'src/core/ext/filters/client_channel/lb_policy/subchannel_list.h', 'src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride_scheduler.h', - 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h', 'src/core/ext/filters/client_channel/local_subchannel_pool.h', diff --git a/grpc.gemspec b/grpc.gemspec index 1cb1cef491a..3c30d689e33 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -184,8 +184,6 @@ Gem::Specification.new do |s| s.files += %w( src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/cds.cc ) - s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc ) - s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc ) diff --git a/grpc.gyp b/grpc.gyp index d6791097df6..1a056667251 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -301,7 +301,6 @@ 'src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc', 'src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/cds.cc', - 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc', diff --git a/package.xml b/package.xml index 3cfcec2c46e..ace800d863f 100644 --- a/package.xml +++ b/package.xml @@ -166,8 +166,6 @@ - - diff --git a/src/core/BUILD b/src/core/BUILD index 530241727de..5167903f3d2 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -4349,24 +4349,9 @@ grpc_cc_library( "ext/filters/client_channel/lb_policy/xds/xds_channel_args.h", ], language = "c++", -) - -grpc_cc_library( - name = "grpc_lb_xds_attributes", - srcs = [ - "ext/filters/client_channel/lb_policy/xds/xds_attributes.cc", - ], - hdrs = [ - "ext/filters/client_channel/lb_policy/xds/xds_attributes.h", - ], - external_deps = ["absl/strings"], - language = "c++", deps = [ - "useful", "//:gpr_platform", - "//:ref_counted_ptr", "//:server_address", - "//:xds_client", ], ) @@ -4386,7 +4371,6 @@ grpc_cc_library( "channel_args", "delegating_helper", "grpc_lb_address_filtering", - "grpc_lb_xds_attributes", "grpc_lb_xds_channel_args", "grpc_xds_client", "json", @@ -4432,7 +4416,6 @@ grpc_cc_library( "channel_args", "delegating_helper", "grpc_backend_metric_data", - "grpc_lb_xds_attributes", "grpc_lb_xds_channel_args", "grpc_xds_client", "json", @@ -4507,12 +4490,13 @@ grpc_cc_library( "absl/status", "absl/status:statusor", "absl/strings", + "absl/types:optional", ], language = "c++", deps = [ "channel_args", "delegating_helper", - "grpc_lb_xds_attributes", + "grpc_lb_xds_channel_args", "json", "json_args", "json_object_loader", @@ -4548,7 +4532,10 @@ grpc_cc_library( ], language = "c++", deps = [ + "channel_args", + "ref_counted", "//:gpr_platform", + "//:ref_counted_ptr", "//:server_address", ], ) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 2e712e91187..afbc704c117 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1163,7 +1163,9 @@ ChannelArgs ClientChannel::MakeSubchannelArgs( // uniqueness. .Remove(GRPC_ARG_HEALTH_CHECK_SERVICE_NAME) .Remove(GRPC_ARG_INHIBIT_HEALTH_CHECKING) - .Remove(GRPC_ARG_CHANNELZ_CHANNEL_NODE); + .Remove(GRPC_ARG_CHANNELZ_CHANNEL_NODE) + // Remove all keys with the no-subchannel prefix. + .RemoveAllKeysWithPrefix(GRPC_ARG_NO_SUBCHANNEL_PREFIX); } void ClientChannel::ReprocessQueuedResolverCalls() { diff --git a/src/core/ext/filters/client_channel/lb_policy/address_filtering.cc b/src/core/ext/filters/client_channel/lb_policy/address_filtering.cc index e7893d6ff56..0ddaaf0f523 100644 --- a/src/core/ext/filters/client_channel/lb_policy/address_filtering.cc +++ b/src/core/ext/filters/client_channel/lb_policy/address_filtering.cc @@ -23,53 +23,24 @@ #include #include -#include "absl/strings/str_cat.h" -#include "absl/strings/str_join.h" - -#define GRPC_ARG_HIERARCHICAL_PATH "grpc.internal.address.hierarchical_path" +#include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/gprpp/ref_counted_ptr.h" namespace grpc_core { -const char* kHierarchicalPathAttributeKey = "hierarchical_path"; - -namespace { - -class HierarchicalPathAttribute : public ServerAddress::AttributeInterface { - public: - explicit HierarchicalPathAttribute(std::vector path) - : path_(std::move(path)) {} - - std::unique_ptr Copy() const override { - return std::make_unique(path_); - } - - int Cmp(const AttributeInterface* other) const override { - const std::vector& other_path = - static_cast(other)->path_; - for (size_t i = 0; i < path_.size(); ++i) { - if (other_path.size() == i) return 1; - int r = path_[i].compare(other_path[i]); - if (r != 0) return r; - } - if (other_path.size() > path_.size()) return -1; - return 0; - } +absl::string_view HierarchicalPathArg::ChannelArgName() { + return GRPC_ARG_NO_SUBCHANNEL_PREFIX "address.hierarchical_path"; +} - std::string ToString() const override { - return absl::StrCat("[", absl::StrJoin(path_, ", "), "]"); +int HierarchicalPathArg::ChannelArgsCompare(const HierarchicalPathArg* a, + const HierarchicalPathArg* b) { + for (size_t i = 0; i < a->path_.size(); ++i) { + if (b->path_.size() == i) return 1; + int r = a->path_[i].compare(b->path_[i]); + if (r != 0) return r; } - - const std::vector& path() const { return path_; } - - private: - std::vector path_; -}; - -} // namespace - -std::unique_ptr -MakeHierarchicalPathAttribute(std::vector path) { - return std::make_unique(std::move(path)); + if (b->path_.size() > a->path_.size()) return -1; + return 0; } absl::StatusOr MakeHierarchicalAddressMap( @@ -77,22 +48,20 @@ absl::StatusOr MakeHierarchicalAddressMap( if (!addresses.ok()) return addresses.status(); HierarchicalAddressMap result; for (const ServerAddress& address : *addresses) { - const HierarchicalPathAttribute* path_attribute = - static_cast( - address.GetAttribute(kHierarchicalPathAttributeKey)); - if (path_attribute == nullptr) continue; - const std::vector& path = path_attribute->path(); + const auto* path_arg = address.args().GetObject(); + if (path_arg == nullptr) continue; + const std::vector& path = path_arg->path(); auto it = path.begin(); + if (it == path.end()) continue; ServerAddressList& target_list = result[*it]; - std::unique_ptr new_attribute; + ChannelArgs args = address.args(); ++it; if (it != path.end()) { std::vector remaining_path(it, path.end()); - new_attribute = std::make_unique( - std::move(remaining_path)); + args = args.SetObject( + MakeRefCounted(std::move(remaining_path))); } - target_list.emplace_back(address.WithAttribute( - kHierarchicalPathAttributeKey, std::move(new_attribute))); + target_list.emplace_back(address.address(), args); } return result; } diff --git a/src/core/ext/filters/client_channel/lb_policy/address_filtering.h b/src/core/ext/filters/client_channel/lb_policy/address_filtering.h index 771ef6fc2f2..2e4aa2017d4 100644 --- a/src/core/ext/filters/client_channel/lb_policy/address_filtering.h +++ b/src/core/ext/filters/client_channel/lb_policy/address_filtering.h @@ -20,12 +20,14 @@ #include #include -#include #include +#include #include #include "absl/status/statusor.h" +#include "absl/strings/string_view.h" +#include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/resolver/server_address.h" // The resolver returns a flat list of addresses. When a hierarchy of @@ -82,13 +84,23 @@ namespace grpc_core { -// The attribute key to be used for hierarchical paths in ServerAddress. -extern const char* kHierarchicalPathAttributeKey; - -// Constructs an address attribute containing the hierarchical path +// An address channel arg containing the hierarchical path // to be associated with the address. -std::unique_ptr -MakeHierarchicalPathAttribute(std::vector path); +class HierarchicalPathArg : public RefCounted { + public: + explicit HierarchicalPathArg(std::vector path) + : path_(std::move(path)) {} + + // Channel arg traits methods. + static absl::string_view ChannelArgName(); + static int ChannelArgsCompare(const HierarchicalPathArg* a, + const HierarchicalPathArg* b); + + const std::vector& path() const { return path_; } + + private: + std::vector path_; +}; // A map from the next path element to the addresses that fall under // that path element. diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 3dfb92007c5..182bb9f1187 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -161,8 +161,6 @@ namespace grpc_core { TraceFlag grpc_lb_glb_trace(false, "glb"); -const char kGrpcLbAddressAttributeKey[] = "grpclb"; - namespace { using ::grpc_event_engine::experimental::EventEngine; @@ -336,30 +334,22 @@ class GrpcLb : public LoadBalancingPolicy { RefCountedPtr client_stats_; }; - class TokenAndClientStatsAttribute - : public ServerAddress::AttributeInterface { + class TokenAndClientStatsArg : public RefCounted { public: - TokenAndClientStatsAttribute(std::string lb_token, - RefCountedPtr client_stats) + TokenAndClientStatsArg(std::string lb_token, + RefCountedPtr client_stats) : lb_token_(std::move(lb_token)), client_stats_(std::move(client_stats)) {} - std::unique_ptr Copy() const override { - return std::make_unique(lb_token_, - client_stats_); + static absl::string_view ChannelArgName() { + return GRPC_ARG_NO_SUBCHANNEL_PREFIX "grpclb_token_and_client_stats"; } - int Cmp(const AttributeInterface* other_base) const override { - const TokenAndClientStatsAttribute* other = - static_cast(other_base); - int r = lb_token_.compare(other->lb_token_); + static int ChannelArgsCompare(const TokenAndClientStatsArg* a, + const TokenAndClientStatsArg* b) { + int r = a->lb_token_.compare(b->lb_token_); if (r != 0) return r; - return QsortCompare(client_stats_.get(), other->client_stats_.get()); - } - - std::string ToString() const override { - return absl::StrFormat("lb_token=\"%s\" client_stats=%p", lb_token_, - client_stats_.get()); + return QsortCompare(a->client_stats_.get(), b->client_stats_.get()); } const std::string& lb_token() const { return lb_token_; } @@ -691,14 +681,10 @@ ServerAddressList GrpcLb::Serverlist::GetServerAddressList( addr_uri.ok() ? addr_uri->c_str() : addr_uri.status().ToString().c_str()); } - // Attach attribute to address containing LB token and stats object. - std::map> - attributes; - attributes[kGrpcLbAddressAttributeKey] = - std::make_unique(std::move(lb_token), - stats); - // Add address. - addresses.emplace_back(addr, ChannelArgs(), std::move(attributes)); + // Add address with a channel arg containing LB token and stats object. + addresses.emplace_back( + addr, ChannelArgs().SetObject(MakeRefCounted( + std::move(lb_token), stats))); } return addresses; } @@ -785,16 +771,14 @@ GrpcLb::PickResult GrpcLb::Picker::Pick(PickArgs args) { RefCountedPtr GrpcLb::Helper::CreateSubchannel( ServerAddress address, const ChannelArgs& args) { if (parent()->shutting_down_) return nullptr; - const TokenAndClientStatsAttribute* attribute = - static_cast( - address.GetAttribute(kGrpcLbAddressAttributeKey)); - if (attribute == nullptr) { - Crash(absl::StrFormat( - "[grpclb %p] no TokenAndClientStatsAttribute for address %p", parent(), - address.ToString().c_str())); + const auto* arg = address.args().GetObject(); + if (arg == nullptr) { + Crash( + absl::StrFormat("[grpclb %p] no TokenAndClientStatsArg for address %p", + parent(), address.ToString().c_str())); } - std::string lb_token = attribute->lb_token(); - RefCountedPtr client_stats = attribute->client_stats(); + std::string lb_token = arg->lb_token(); + RefCountedPtr client_stats = arg->client_stats(); return MakeRefCounted( parent()->channel_control_helper()->CreateSubchannel(std::move(address), args), @@ -1534,9 +1518,10 @@ absl::Status GrpcLb::UpdateLocked(UpdateArgs args) { if (fallback_backend_addresses_.ok()) { // Add null LB token attributes. for (ServerAddress& address : *fallback_backend_addresses_) { - address = address.WithAttribute( - kGrpcLbAddressAttributeKey, - std::make_unique("", nullptr)); + address = ServerAddress( + address.address(), + address.args().SetObject( + MakeRefCounted("", nullptr))); } } resolution_note_ = std::move(args.resolution_note); diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc index eccfacc3112..c45be05a6a3 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.cc @@ -26,7 +26,8 @@ #include "src/core/lib/gpr/useful.h" // Channel arg key for the list of balancer addresses. -#define GRPC_ARG_GRPCLB_BALANCER_ADDRESSES "grpc.grpclb_balancer_addresses" +#define GRPC_ARG_GRPCLB_BALANCER_ADDRESSES \ + GRPC_ARG_NO_SUBCHANNEL_PREFIX "grpc.grpclb_balancer_addresses" namespace grpc_core { diff --git a/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc b/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc index 98755f0e07a..4ebdca2c93c 100644 --- a/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc +++ b/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc @@ -69,9 +69,6 @@ namespace grpc_core { TraceFlag grpc_outlier_detection_lb_trace(false, "outlier_detection_lb"); -const char* DisableOutlierDetectionAttribute::kName = - "disable_outlier_detection"; - namespace { using ::grpc_event_engine::experimental::EventEngine; @@ -542,14 +539,12 @@ OutlierDetectionLb::~OutlierDetectionLb() { std::string OutlierDetectionLb::MakeKeyForAddress( const ServerAddress& address) { - // If the address has the DisableOutlierDetectionAttribute attribute, - // ignore it. + // If the address has outlier detection disabled, ignore it. // TODO(roth): This is a hack to prevent outlier_detection from // working with pick_first, as per discussion in // https://github.com/grpc/grpc/issues/32967. Remove this as part of // implementing dualstack backend support. - if (address.GetAttribute(DisableOutlierDetectionAttribute::kName) != - nullptr) { + if (address.args().GetInt(GRPC_ARG_OUTLIER_DETECTION_DISABLE) == 1) { return ""; } // Use only the address, not the attributes. diff --git a/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.h b/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.h index ca1dc3a3c71..c8c7f52afd3 100644 --- a/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.h +++ b/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.h @@ -21,9 +21,6 @@ #include // for uint32_t -#include -#include - #include "absl/types/optional.h" #include "src/core/lib/gprpp/time.h" @@ -96,19 +93,8 @@ struct OutlierDetectionConfig { // TODO(roth): This is a horrible hack used to disable outlier detection // when used with the pick_first policy. Remove this as part of // implementing the dualstack backend design. -class DisableOutlierDetectionAttribute - : public ServerAddress::AttributeInterface { - public: - static const char* kName; - - std::unique_ptr Copy() const override { - return std::make_unique(); - } - - int Cmp(const AttributeInterface*) const override { return true; } - - std::string ToString() const override { return "true"; } -}; +#define GRPC_ARG_OUTLIER_DETECTION_DISABLE \ + GRPC_ARG_NO_SUBCHANNEL_PREFIX "outlier_detection_disable" } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc index 8cc2b158e73..076ae6bad6b 100644 --- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc +++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc @@ -330,9 +330,9 @@ absl::Status PickFirst::UpdateLocked(UpdateArgs args) { if (args.addresses.ok()) { ServerAddressList addresses; for (const auto& address : *args.addresses) { - addresses.emplace_back(address.WithAttribute( - DisableOutlierDetectionAttribute::kName, - std::make_unique())); + addresses.emplace_back( + address.address(), + address.args().Set(GRPC_ARG_OUTLIER_DETECTION_DISABLE, 1)); } args.addresses = std::move(addresses); } diff --git a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc index d104dbe520e..5eb92861448 100644 --- a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc +++ b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc @@ -470,16 +470,14 @@ RingHash::RingHashSubchannelList::Ring::Ring( address_weights.reserve(subchannel_list->num_subchannels()); for (size_t i = 0; i < subchannel_list->num_subchannels(); ++i) { RingHashSubchannelData* sd = subchannel_list->subchannel(i); - const ServerAddressWeightAttribute* weight_attribute = static_cast< - const ServerAddressWeightAttribute*>(sd->address().GetAttribute( - ServerAddressWeightAttribute::kServerAddressWeightAttributeKey)); + auto weight_arg = sd->address().args().GetInt(GRPC_ARG_ADDRESS_WEIGHT); AddressWeight address_weight; address_weight.address = grpc_sockaddr_to_string(&sd->address().address(), false).value(); // Weight should never be zero, but ignore it just in case, since // that value would screw up the ring-building algorithm. - if (weight_attribute != nullptr && weight_attribute->weight() > 0) { - address_weight.weight = weight_attribute->weight(); + if (weight_arg.value_or(0) > 0) { + address_weight.weight = *weight_arg; } sum += address_weight.weight; address_weights.push_back(std::move(address_weight)); diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc deleted file mode 100644 index e6918fa4d4e..00000000000 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc +++ /dev/null @@ -1,42 +0,0 @@ -// -// Copyright 2018 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -#include - -#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h" - -#include "absl/strings/str_cat.h" - -#include "src/core/lib/gpr/useful.h" - -namespace grpc_core { - -const char* kXdsLocalityNameAttributeKey = "xds_locality_name"; - -int XdsLocalityAttribute::Cmp(const AttributeInterface* other) const { - const auto* other_locality_attr = - static_cast(other); - int r = locality_name_->Compare(*other_locality_attr->locality_name_); - if (r != 0) return r; - return QsortCompare(weight_, other_locality_attr->weight_); -} - -std::string XdsLocalityAttribute::ToString() const { - return absl::StrCat("{name=", locality_name_->AsHumanReadableString(), - ", weight=", weight_, "}"); -} - -} // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h b/src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h deleted file mode 100644 index bb8d52273fe..00000000000 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h +++ /dev/null @@ -1,64 +0,0 @@ -// -// Copyright 2018 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -#ifndef GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_ATTRIBUTES_H -#define GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_ATTRIBUTES_H - -#include - -#include - -#include -#include -#include - -#include "src/core/ext/xds/xds_client_stats.h" -#include "src/core/lib/gprpp/ref_counted_ptr.h" -#include "src/core/lib/resolver/server_address.h" - -namespace grpc_core { - -extern const char* kXdsLocalityNameAttributeKey; - -class XdsLocalityAttribute : public ServerAddress::AttributeInterface { - public: - XdsLocalityAttribute(RefCountedPtr locality_name, - uint32_t weight) - : locality_name_(std::move(locality_name)), weight_(weight) {} - - RefCountedPtr locality_name() const { - return locality_name_; - } - - uint32_t weight() const { return weight_; } - - std::unique_ptr Copy() const override { - return std::make_unique(locality_name_->Ref(), - weight_); - } - - int Cmp(const AttributeInterface* other) const override; - - std::string ToString() const override; - - private: - RefCountedPtr locality_name_; - uint32_t weight_; -}; - -} // namespace grpc_core - -#endif // GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_ATTRIBUTES_H diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h b/src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h index 290e67ca116..c7bbf197da0 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h @@ -17,6 +17,10 @@ #ifndef GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_CHANNEL_ARGS_H #define GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_CHANNEL_ARGS_H +#include + +#include "src/core/lib/resolver/server_address.h" + // Channel arg indicating the xDS cluster name. // Set by xds_cluster_impl LB policy and used by GoogleDefaultCredentials. #define GRPC_ARG_XDS_CLUSTER_NAME "grpc.internal.xds_cluster_name" @@ -26,4 +30,8 @@ #define GRPC_ARG_XDS_LOGICAL_DNS_CLUSTER_FAKE_RESOLVER_RESPONSE_GENERATOR \ "grpc.TEST_ONLY.xds_logical_dns_cluster_fake_resolver_response_generator" +// Channel arg for encoding xDS locality weight. +#define GRPC_ARG_XDS_LOCALITY_WEIGHT \ + GRPC_ARG_NO_SUBCHANNEL_PREFIX "xds_locality_weight" + #endif // GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_CHANNEL_ARGS_H 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 566e53ae0e5..158428996f7 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 @@ -39,7 +39,6 @@ #include "src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h" #include "src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h" -#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h" #include "src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h" #include "src/core/ext/xds/xds_bootstrap.h" #include "src/core/ext/xds/xds_bootstrap_grpc.h" @@ -600,13 +599,7 @@ RefCountedPtr XdsClusterImplLb::Helper::CreateSubchannel( // If load reporting is enabled, wrap the subchannel such that it // includes the locality stats object, which will be used by the Picker. if (parent()->config_->lrs_load_reporting_server().has_value()) { - RefCountedPtr locality_name; - auto* attribute = address.GetAttribute(kXdsLocalityNameAttributeKey); - if (attribute != nullptr) { - const auto* locality_attr = - static_cast(attribute); - locality_name = locality_attr->locality_name(); - } + auto locality_name = address.args().GetObjectRef(); RefCountedPtr locality_stats = parent()->xds_client_->AddClusterLocalityStats( parent()->config_->lrs_load_reporting_server().value(), 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 27193add42e..b4db0dbec14 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 @@ -42,7 +42,6 @@ #include "src/core/ext/filters/client_channel/lb_policy/address_filtering.h" #include "src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h" -#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h" #include "src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h" #include "src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h" #include "src/core/ext/xds/xds_bootstrap.h" @@ -768,25 +767,17 @@ ServerAddressList XdsClusterResolverLb::CreateChildPolicyAddressesLocked() { std::vector hierarchical_path = { priority_child_name, locality_name->AsHumanReadableString()}; for (const auto& endpoint : locality.endpoints) { - const ServerAddressWeightAttribute* weight_attribute = static_cast< - const ServerAddressWeightAttribute*>(endpoint.GetAttribute( - ServerAddressWeightAttribute::kServerAddressWeightAttributeKey)); - uint32_t weight = locality.lb_weight; - if (weight_attribute != nullptr) { - weight = locality.lb_weight * weight_attribute->weight(); - } + uint32_t endpoint_weight = + locality.lb_weight * + endpoint.args().GetInt(GRPC_ARG_ADDRESS_WEIGHT).value_or(1); addresses.emplace_back( - endpoint - .WithAttribute( - kHierarchicalPathAttributeKey, - MakeHierarchicalPathAttribute(hierarchical_path)) - .WithAttribute(kXdsLocalityNameAttributeKey, - std::make_unique( - locality_name->Ref(), locality.lb_weight)) - .WithAttribute( - ServerAddressWeightAttribute:: - kServerAddressWeightAttributeKey, - std::make_unique(weight))); + endpoint.address(), + endpoint.args() + .SetObject( + MakeRefCounted(hierarchical_path)) + .Set(GRPC_ARG_ADDRESS_WEIGHT, endpoint_weight) + .SetObject(locality_name->Ref()) + .Set(GRPC_ARG_XDS_LOCALITY_WEIGHT, locality.lb_weight)); } } } diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc index 96f5fee13b3..c5445c3dcf2 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc @@ -95,12 +95,10 @@ struct PtrLessThan { }; XdsHealthStatus GetAddressHealthStatus(const ServerAddress& address) { - auto attribute = address.GetAttribute(XdsEndpointHealthStatusAttribute::kKey); - if (attribute == nullptr) { - return XdsHealthStatus(XdsHealthStatus::HealthStatus::kUnknown); - } - return static_cast(attribute) - ->status(); + return XdsHealthStatus(static_cast( + address.args() + .GetInt(GRPC_ARG_XDS_HEALTH_STATUS) + .value_or(XdsHealthStatus::HealthStatus::kUnknown))); } // diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_wrr_locality.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_wrr_locality.cc index fc709f4100d..fa4918633c9 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_wrr_locality.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_wrr_locality.cc @@ -27,12 +27,13 @@ #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" #include #include #include -#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h" +#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h" #include "src/core/ext/xds/xds_client_stats.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/config/core_configuration.h" @@ -170,17 +171,17 @@ absl::Status XdsWrrLocalityLb::UpdateLocked(UpdateArgs args) { std::map locality_weights; if (args.addresses.ok()) { for (const auto& address : *args.addresses) { - auto* attribute = static_cast( - address.GetAttribute(kXdsLocalityNameAttributeKey)); - if (attribute != nullptr) { + auto* locality_name = address.args().GetObject(); + uint32_t weight = + address.args().GetInt(GRPC_ARG_XDS_LOCALITY_WEIGHT).value_or(0); + if (locality_name != nullptr && weight > 0) { auto p = locality_weights.emplace( - attribute->locality_name()->AsHumanReadableString(), - attribute->weight()); - if (!p.second && p.first->second != attribute->weight()) { + locality_name->AsHumanReadableString(), weight); + if (!p.second && p.first->second != weight) { gpr_log(GPR_ERROR, "INTERNAL ERROR: xds_wrr_locality found different weights " - "for locality %s (%d vs %d); using first value", - p.first->first.c_str(), p.first->second, attribute->weight()); + "for locality %s (%u vs %u); using first value", + p.first->first.c_str(), p.first->second, weight); } } } diff --git a/src/core/ext/xds/xds_client_grpc.h b/src/core/ext/xds/xds_client_grpc.h index a258b2a3118..7ecb96cb52a 100644 --- a/src/core/ext/xds/xds_client_grpc.h +++ b/src/core/ext/xds/xds_client_grpc.h @@ -35,6 +35,7 @@ #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/iomgr/iomgr_fwd.h" +#include "src/core/lib/resolver/server_address.h" namespace grpc_core { @@ -63,7 +64,7 @@ class GrpcXdsClient : public XdsClient { // Helpers for encoding the XdsClient object in channel args. static absl::string_view ChannelArgName() { - return "grpc.internal.xds_client"; + return GRPC_ARG_NO_SUBCHANNEL_PREFIX "xds_client"; } static int ChannelArgsCompare(const XdsClient* a, const XdsClient* b) { return QsortCompare(a, b); diff --git a/src/core/ext/xds/xds_client_stats.h b/src/core/ext/xds/xds_client_stats.h index 3f3a776f613..0a5a293ba68 100644 --- a/src/core/ext/xds/xds_client_stats.h +++ b/src/core/ext/xds/xds_client_stats.h @@ -38,6 +38,7 @@ #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/gprpp/sync.h" +#include "src/core/lib/resolver/server_address.h" namespace grpc_core { @@ -95,6 +96,15 @@ class XdsLocalityName : public RefCounted { return human_readable_string_; } + // Channel args traits. + static absl::string_view ChannelArgName() { + return GRPC_ARG_NO_SUBCHANNEL_PREFIX "xds_locality_name"; + } + static int ChannelArgsCompare(const XdsLocalityName* a, + const XdsLocalityName* b) { + return a->Compare(*b); + } + private: std::string region_; std::string zone_; diff --git a/src/core/ext/xds/xds_endpoint.cc b/src/core/ext/xds/xds_endpoint.cc index 2638423d1c1..df6475b9740 100644 --- a/src/core/ext/xds/xds_endpoint.cc +++ b/src/core/ext/xds/xds_endpoint.cc @@ -220,13 +220,10 @@ absl::optional ServerAddressParse( } } // Convert to ServerAddress. - std::map> - attributes; - attributes[ServerAddressWeightAttribute::kServerAddressWeightAttributeKey] = - std::make_unique(weight); - attributes[XdsEndpointHealthStatusAttribute::kKey] = - std::make_unique(*status); - return ServerAddress(grpc_address, ChannelArgs(), std::move(attributes)); + return ServerAddress(grpc_address, + ChannelArgs() + .Set(GRPC_ARG_ADDRESS_WEIGHT, weight) + .Set(GRPC_ARG_XDS_HEALTH_STATUS, status->status())); } struct ParsedLocality { diff --git a/src/core/ext/xds/xds_health_status.cc b/src/core/ext/xds/xds_health_status.cc index e5c3a924802..8626a6f46f6 100644 --- a/src/core/ext/xds/xds_health_status.cc +++ b/src/core/ext/xds/xds_health_status.cc @@ -18,11 +18,8 @@ #include "src/core/ext/xds/xds_health_status.h" -#include "absl/strings/str_cat.h" #include "envoy/config/core/v3/health_check.upb.h" -#include "src/core/lib/gpr/useful.h" - namespace grpc_core { absl::optional XdsHealthStatus::FromUpb(uint32_t status) { @@ -63,18 +60,4 @@ bool operator<(const XdsHealthStatus& hs1, const XdsHealthStatus& hs2) { return hs1.status() < hs2.status(); } -const char* XdsEndpointHealthStatusAttribute::kKey = - "xds_endpoint_health_status"; - -int XdsEndpointHealthStatusAttribute::Cmp( - const AttributeInterface* other) const { - const auto* other_attr = - static_cast(other); - return QsortCompare(status_, other_attr->status_); -} - -std::string XdsEndpointHealthStatusAttribute::ToString() const { - return absl::StrCat("{status_=", status_.ToString(), "}"); -} - } // namespace grpc_core diff --git a/src/core/ext/xds/xds_health_status.h b/src/core/ext/xds/xds_health_status.h index c9c27315953..c680a03aa6a 100644 --- a/src/core/ext/xds/xds_health_status.h +++ b/src/core/ext/xds/xds_health_status.h @@ -21,15 +21,17 @@ #include -#include -#include - #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "absl/types/span.h" #include "src/core/lib/resolver/server_address.h" +// Channel arg key for xDS health status. +// Value is an XdsHealthStatus::HealthStatus enum. +#define GRPC_ARG_XDS_HEALTH_STATUS \ + GRPC_ARG_NO_SUBCHANNEL_PREFIX "xds_health_status" + namespace grpc_core { class XdsHealthStatus { @@ -82,28 +84,6 @@ class XdsHealthStatusSet { bool operator<(const XdsHealthStatus& hs1, const XdsHealthStatus& hs2); -class XdsEndpointHealthStatusAttribute - : public ServerAddress::AttributeInterface { - public: - static const char* kKey; - - explicit XdsEndpointHealthStatusAttribute(XdsHealthStatus status) - : status_(status) {} - - XdsHealthStatus status() const { return status_; } - - std::unique_ptr Copy() const override { - return std::make_unique(status_); - } - - int Cmp(const AttributeInterface* other) const override; - - std::string ToString() const override; - - private: - XdsHealthStatus status_; -}; - } // namespace grpc_core #endif // GRPC_SRC_CORE_EXT_XDS_XDS_HEALTH_STATUS_H diff --git a/src/core/lib/channel/channel_args.cc b/src/core/lib/channel/channel_args.cc index 9fced0de4a1..52bcb1d087e 100644 --- a/src/core/lib/channel/channel_args.cc +++ b/src/core/lib/channel/channel_args.cc @@ -210,6 +210,17 @@ ChannelArgs ChannelArgs::Remove(absl::string_view key) const { return ChannelArgs(args_.Remove(key)); } +ChannelArgs ChannelArgs::RemoveAllKeysWithPrefix( + absl::string_view prefix) const { + ChannelArgs result; + args_.ForEach([&](const std::string& key, const Value& value) { + if (!absl::StartsWith(key, prefix)) { + result.args_ = result.args_.Add(key, value); + } + }); + return result; +} + absl::optional ChannelArgs::GetInt(absl::string_view name) const { auto* v = Get(name); if (v == nullptr) return absl::nullopt; diff --git a/src/core/lib/channel/channel_args.h b/src/core/lib/channel/channel_args.h index 8bf1a9c84c7..57d6f35e335 100644 --- a/src/core/lib/channel/channel_args.h +++ b/src/core/lib/channel/channel_args.h @@ -395,6 +395,9 @@ class ChannelArgs { GRPC_MUST_USE_RESULT ChannelArgs Remove(absl::string_view name) const; bool Contains(absl::string_view name) const; + GRPC_MUST_USE_RESULT ChannelArgs + RemoveAllKeysWithPrefix(absl::string_view prefix) const; + template bool ContainsObject() const { return Get(ChannelArgNameTraits::ChannelArgName()) != nullptr; diff --git a/src/core/lib/resolver/server_address.cc b/src/core/lib/resolver/server_address.cc index 140d6ef7155..96a9f7ddff6 100644 --- a/src/core/lib/resolver/server_address.cc +++ b/src/core/lib/resolver/server_address.cc @@ -23,8 +23,6 @@ #include #include -#include -#include #include #include #include @@ -32,119 +30,49 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" -#include "absl/strings/str_format.h" #include "absl/strings/str_join.h" #include "src/core/lib/address_utils/sockaddr_utils.h" #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/gpr/useful.h" // IWYU pragma: no_include namespace grpc_core { -// -// ServerAddressWeightAttribute -// -const char* ServerAddressWeightAttribute::kServerAddressWeightAttributeKey = - "server_address_weight"; - // // ServerAddress // -ServerAddress::ServerAddress( - const grpc_resolved_address& address, const ChannelArgs& args, - std::map> attributes) - : address_(address), args_(args), attributes_(std::move(attributes)) {} +ServerAddress::ServerAddress(const grpc_resolved_address& address, + const ChannelArgs& args) + : address_(address), args_(args) {} ServerAddress::ServerAddress(const ServerAddress& other) - : address_(other.address_), args_(other.args_) { - for (const auto& p : other.attributes_) { - attributes_[p.first] = p.second->Copy(); - } -} + : address_(other.address_), args_(other.args_) {} + ServerAddress& ServerAddress::operator=(const ServerAddress& other) { - if (&other == this) { - return *this; - } + if (&other == this) return *this; address_ = other.address_; args_ = other.args_; - attributes_.clear(); - for (const auto& p : other.attributes_) { - attributes_[p.first] = p.second->Copy(); - } return *this; } ServerAddress::ServerAddress(ServerAddress&& other) noexcept - : address_(other.address_), - args_(std::move(other.args_)), - attributes_(std::move(other.attributes_)) {} + : address_(other.address_), args_(std::move(other.args_)) {} ServerAddress& ServerAddress::operator=(ServerAddress&& other) noexcept { address_ = other.address_; args_ = std::move(other.args_); - attributes_ = std::move(other.attributes_); return *this; } -namespace { - -int CompareAttributes( - const std::map>& - attributes1, - const std::map>& - attributes2) { - auto it2 = attributes2.begin(); - for (auto it1 = attributes1.begin(); it1 != attributes1.end(); ++it1) { - // attributes2 has fewer elements than attributes1 - if (it2 == attributes2.end()) return -1; - // compare keys - int retval = strcmp(it1->first, it2->first); - if (retval != 0) return retval; - // compare values - retval = it1->second->Cmp(it2->second.get()); - if (retval != 0) return retval; - ++it2; - } - // attributes1 has fewer elements than attributes2 - if (it2 != attributes2.end()) return 1; - // equal - return 0; -} - -} // namespace - int ServerAddress::Cmp(const ServerAddress& other) const { if (address_.len > other.address_.len) return 1; if (address_.len < other.address_.len) return -1; int retval = memcmp(address_.addr, other.address_.addr, address_.len); if (retval != 0) return retval; - retval = QsortCompare(args_, other.args_); - if (retval != 0) return retval; - return CompareAttributes(attributes_, other.attributes_); -} - -const ServerAddress::AttributeInterface* ServerAddress::GetAttribute( - const char* key) const { - auto it = attributes_.find(key); - if (it == attributes_.end()) return nullptr; - return it->second.get(); -} - -// Returns a copy of the address with a modified attribute. -// If the new value is null, the attribute is removed. -ServerAddress ServerAddress::WithAttribute( - const char* key, std::unique_ptr value) const { - ServerAddress address = *this; - if (value == nullptr) { - address.attributes_.erase(key); - } else { - address.attributes_[key] = std::move(value); - } - return address; + return QsortCompare(args_, other.args_); } std::string ServerAddress::ToString() const { @@ -155,21 +83,7 @@ std::string ServerAddress::ToString() const { if (args_ != ChannelArgs()) { parts.emplace_back(absl::StrCat("args=", args_.ToString())); } - if (!attributes_.empty()) { - std::vector attrs; - attrs.reserve(attributes_.size()); - for (const auto& p : attributes_) { - attrs.emplace_back(absl::StrCat(p.first, "=", p.second->ToString())); - } - std::sort(attrs.begin(), attrs.end()); - parts.emplace_back( - absl::StrCat("attributes={", absl::StrJoin(attrs, ", "), "}")); - } return absl::StrJoin(parts, " "); } -std::string ServerAddressWeightAttribute::ToString() const { - return absl::StrFormat("%d", weight_); -} - } // namespace grpc_core diff --git a/src/core/lib/resolver/server_address.h b/src/core/lib/resolver/server_address.h index 05d8dedc4b6..d42a70f9d74 100644 --- a/src/core/lib/resolver/server_address.h +++ b/src/core/lib/resolver/server_address.h @@ -21,17 +21,21 @@ #include -#include - -#include -#include #include #include #include "src/core/lib/channel/channel_args.h" -#include "src/core/lib/gpr/useful.h" #include "src/core/lib/iomgr/resolved_address.h" +// A channel arg key prefix used for args that are intended to be used +// only internally to resolvers and LB policies and should not be part +// of the subchannel key. The channel will automatically filter out any +// args with this prefix from the subchannel's args. +#define GRPC_ARG_NO_SUBCHANNEL_PREFIX "grpc.internal.no_subchannel." + +// A channel arg indicating the weight of an address. +#define GRPC_ARG_ADDRESS_WEIGHT GRPC_ARG_NO_SUBCHANNEL_PREFIX "address.weight" + namespace grpc_core { // @@ -43,30 +47,7 @@ namespace grpc_core { // args when a subchannel is created for this address. class ServerAddress { public: - // Base class for resolver-supplied attributes. - // Unlike channel args, these attributes don't affect subchannel - // uniqueness or behavior. They are for use by LB policies only. - // - // Attributes are keyed by a C string that is unique by address, not - // by value. All attributes added with the same key must be of the - // same type. - class AttributeInterface { - public: - virtual ~AttributeInterface() = default; - - // Creates a copy of the attribute. - virtual std::unique_ptr Copy() const = 0; - - // Compares this attribute with another. - virtual int Cmp(const AttributeInterface* other) const = 0; - - // Returns a human-readable representation of the attribute. - virtual std::string ToString() const = 0; - }; - - ServerAddress(const grpc_resolved_address& address, const ChannelArgs& args, - std::map> - attributes = {}); + ServerAddress(const grpc_resolved_address& address, const ChannelArgs& args); // Copyable. ServerAddress(const ServerAddress& other); @@ -83,13 +64,6 @@ class ServerAddress { const grpc_resolved_address& address() const { return address_; } const ChannelArgs& args() const { return args_; } - const AttributeInterface* GetAttribute(const char* key) const; - - // Returns a copy of the address with a modified attribute. - // If the new value is null, the attribute is removed. - ServerAddress WithAttribute(const char* key, - std::unique_ptr value) const; - // TODO(ctiller): Prior to making this a public API we should ensure that the // channel args are not part of the generated string, lest we make that debug // format load-bearing via Hyrum's law. @@ -98,7 +72,6 @@ class ServerAddress { private: grpc_resolved_address address_; ChannelArgs args_; - std::map> attributes_; }; // @@ -107,33 +80,6 @@ class ServerAddress { using ServerAddressList = std::vector; -// -// ServerAddressWeightAttribute -// -class ServerAddressWeightAttribute : public ServerAddress::AttributeInterface { - public: - static const char* kServerAddressWeightAttributeKey; - - explicit ServerAddressWeightAttribute(uint32_t weight) : weight_(weight) {} - - uint32_t weight() const { return weight_; } - - std::unique_ptr Copy() const override { - return std::make_unique(weight_); - } - - int Cmp(const AttributeInterface* other) const override { - const auto* other_locality_attr = - static_cast(other); - return QsortCompare(weight_, other_locality_attr->weight_); - } - - std::string ToString() const override; - - private: - uint32_t weight_; -}; - } // namespace grpc_core #endif // GRPC_SRC_CORE_LIB_RESOLVER_SERVER_ADDRESS_H diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 69ca1aac096..411e16f38cb 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -50,7 +50,6 @@ CORE_SOURCE_FILES = [ 'src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc', 'src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/cds.cc', - 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc', diff --git a/test/core/channel/channel_args_test.cc b/test/core/channel/channel_args_test.cc index ead8ecd4310..a7e55f9724a 100644 --- a/test/core/channel/channel_args_test.cc +++ b/test/core/channel/channel_args_test.cc @@ -73,6 +73,19 @@ TEST(ChannelArgsTest, SetGetRemove) { gpr_free(ptr); } +TEST(ChannelArgsTest, RemoveAllKeysWithPrefix) { + ChannelArgs args; + args = args.Set("foo", 1); + args = args.Set("foo.bar", 2); + args = args.Set("foo.baz", 3); + args = args.Set("bar", 4); + ChannelArgs modified = args.RemoveAllKeysWithPrefix("foo."); + EXPECT_EQ(modified.GetInt("foo"), 1); + EXPECT_EQ(modified.GetInt("foo.bar"), absl::nullopt); + EXPECT_EQ(modified.GetInt("foo.baz"), absl::nullopt); + EXPECT_EQ(modified.GetInt("bar"), 4); +} + TEST(ChannelArgsTest, StoreRefCountedPtr) { struct Test : public RefCounted { explicit Test(int n) : n(n) {} diff --git a/test/core/client_channel/client_channel_test.cc b/test/core/client_channel/client_channel_test.cc index 3508cfa1515..58f16e2166e 100644 --- a/test/core/client_channel/client_channel_test.cc +++ b/test/core/client_channel/client_channel_test.cc @@ -18,12 +18,14 @@ #include "src/core/ext/filters/client_channel/client_channel.h" +#include "absl/types/optional.h" #include "gtest/gtest.h" #include #include "src/core/ext/filters/client_channel/subchannel_pool_interface.h" #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/resolver/server_address.h" #include "test/core/util/test_config.h" namespace grpc_core { @@ -60,6 +62,22 @@ TEST(MakeSubchannelArgs, EXPECT_EQ(args.GetString(GRPC_ARG_DEFAULT_AUTHORITY), "bar.example.com"); } +TEST(MakeSubchannelArgs, ArgsFromChannelTrumpPerAddressArgs) { + ChannelArgs args = ClientChannel::MakeSubchannelArgs( + ChannelArgs().Set("foo", 1), ChannelArgs().Set("foo", 2), nullptr, + "foo.example.com"); + EXPECT_EQ(args.GetInt("foo"), 1); +} + +TEST(MakeSubchannelArgs, StripsOutNoSubchannelArgs) { + ChannelArgs args = ClientChannel::MakeSubchannelArgs( + ChannelArgs().Set(GRPC_ARG_NO_SUBCHANNEL_PREFIX "foo", 1), + ChannelArgs().Set(GRPC_ARG_NO_SUBCHANNEL_PREFIX "bar", 1), nullptr, + "foo.example.com"); + EXPECT_EQ(args.GetString(GRPC_ARG_NO_SUBCHANNEL_PREFIX "foo"), absl::nullopt); + EXPECT_EQ(args.GetString(GRPC_ARG_NO_SUBCHANNEL_PREFIX "bar"), absl::nullopt); +} + } // namespace } // namespace testing } // namespace grpc_core diff --git a/test/core/client_channel/lb_policy/xds_override_host_test.cc b/test/core/client_channel/lb_policy/xds_override_host_test.cc index 541682aff6d..26cfc7a60d4 100644 --- a/test/core/client_channel/lb_policy/xds_override_host_test.cc +++ b/test/core/client_channel/lb_policy/xds_override_host_test.cc @@ -16,7 +16,6 @@ #include #include -#include #include #include #include @@ -33,6 +32,7 @@ #include "src/core/ext/filters/stateful_session/stateful_session_filter.h" #include "src/core/ext/xds/xds_health_status.h" +#include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/json/json.h" @@ -78,12 +78,8 @@ class XdsOverrideHostTest : public LoadBalancingPolicyTest { ServerAddress MakeAddressWithHealthStatus( absl::string_view address, XdsHealthStatus::HealthStatus status) { - std::map> - attrs; - attrs.emplace(XdsEndpointHealthStatusAttribute::kKey, - std::make_unique( - XdsHealthStatus(status))); - return {MakeAddress(address), {}, std::move(attrs)}; + return ServerAddress(MakeAddress(address), + ChannelArgs().Set(GRPC_ARG_XDS_HEALTH_STATUS, status)); } void ApplyUpdateWithHealthStatuses( diff --git a/test/core/xds/xds_endpoint_resource_type_test.cc b/test/core/xds/xds_endpoint_resource_type_test.cc index 24294f93f98..65a4afd0d2d 100644 --- a/test/core/xds/xds_endpoint_resource_type_test.cc +++ b/test/core/xds/xds_endpoint_resource_type_test.cc @@ -161,12 +161,10 @@ TEST_F(XdsEndpointTest, MinimumValidConfig) { auto addr = grpc_sockaddr_to_string(&address.address(), /*normalize=*/false); ASSERT_TRUE(addr.ok()) << addr.status(); EXPECT_EQ(*addr, "127.0.0.1:443"); - EXPECT_EQ(address.args(), ChannelArgs()); - const auto* attribute = - static_cast(address.GetAttribute( - ServerAddressWeightAttribute::kServerAddressWeightAttributeKey)); - ASSERT_NE(attribute, nullptr); - EXPECT_EQ(attribute->weight(), 1); + EXPECT_EQ(address.args(), ChannelArgs() + .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()); } @@ -209,12 +207,10 @@ TEST_F(XdsEndpointTest, EndpointWeight) { auto addr = grpc_sockaddr_to_string(&address.address(), /*normalize=*/false); ASSERT_TRUE(addr.ok()) << addr.status(); EXPECT_EQ(*addr, "127.0.0.1:443"); - EXPECT_EQ(address.args(), ChannelArgs()); - const auto* attribute = - static_cast(address.GetAttribute( - ServerAddressWeightAttribute::kServerAddressWeightAttributeKey)); - ASSERT_NE(attribute, nullptr); - EXPECT_EQ(attribute->weight(), 3); + EXPECT_EQ(address.args(), ChannelArgs() + .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()); } @@ -259,12 +255,10 @@ TEST_F(XdsEndpointTest, IgnoresLocalityWithNoWeight) { auto addr = grpc_sockaddr_to_string(&address.address(), /*normalize=*/false); ASSERT_TRUE(addr.ok()) << addr.status(); EXPECT_EQ(*addr, "127.0.0.1:443"); - EXPECT_EQ(address.args(), ChannelArgs()); - const auto* attribute = - static_cast(address.GetAttribute( - ServerAddressWeightAttribute::kServerAddressWeightAttributeKey)); - ASSERT_NE(attribute, nullptr); - EXPECT_EQ(attribute->weight(), 1); + EXPECT_EQ(address.args(), ChannelArgs() + .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()); } @@ -310,12 +304,10 @@ TEST_F(XdsEndpointTest, IgnoresLocalityWithZeroWeight) { auto addr = grpc_sockaddr_to_string(&address.address(), /*normalize=*/false); ASSERT_TRUE(addr.ok()) << addr.status(); EXPECT_EQ(*addr, "127.0.0.1:443"); - EXPECT_EQ(address.args(), ChannelArgs()); - const auto* attribute = - static_cast(address.GetAttribute( - ServerAddressWeightAttribute::kServerAddressWeightAttributeKey)); - ASSERT_NE(attribute, nullptr); - EXPECT_EQ(attribute->weight(), 1); + EXPECT_EQ(address.args(), ChannelArgs() + .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()); } @@ -1011,19 +1003,14 @@ TEST_F(XdsEndpointTest, EndpointHealthStatus) { auto addr = grpc_sockaddr_to_string(&address->address(), /*normalize=*/false); ASSERT_TRUE(addr.ok()) << addr.status(); EXPECT_EQ(*addr, "127.0.0.1:443"); - const auto* health_attribute = - static_cast( - address->GetAttribute(XdsEndpointHealthStatusAttribute::kKey)); - ASSERT_NE(health_attribute, nullptr); - EXPECT_EQ(health_attribute->status().status(), XdsHealthStatus::kUnknown); + EXPECT_EQ(address->args().GetInt(GRPC_ARG_XDS_HEALTH_STATUS), + XdsHealthStatus::kUnknown); address = &p.second.endpoints[1]; addr = grpc_sockaddr_to_string(&address->address(), /*normalize=*/false); ASSERT_TRUE(addr.ok()) << addr.status(); EXPECT_EQ(*addr, "127.0.0.2:443"); - health_attribute = static_cast( - address->GetAttribute(XdsEndpointHealthStatusAttribute::kKey)); - ASSERT_NE(health_attribute, nullptr); - EXPECT_EQ(health_attribute->status().status(), XdsHealthStatus::kDraining); + EXPECT_EQ(address->args().GetInt(GRPC_ARG_XDS_HEALTH_STATUS), + XdsHealthStatus::kDraining); } } // namespace diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index e062cc28d80..bfda0c63588 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -212,17 +212,13 @@ class FakeResolverResponseGeneratorWrapper { response_generator_ = std::move(other.response_generator_); } - void SetNextResolution( - const std::vector& ports, const char* service_config_json = nullptr, - const char* attribute_key = nullptr, - std::unique_ptr attribute = - nullptr, - const grpc_core::ChannelArgs& per_address_args = - grpc_core::ChannelArgs()) { + void SetNextResolution(const std::vector& ports, + const char* service_config_json = nullptr, + const grpc_core::ChannelArgs& per_address_args = + grpc_core::ChannelArgs()) { grpc_core::ExecCtx exec_ctx; - response_generator_->SetResponse( - BuildFakeResults(ipv6_only_, ports, service_config_json, attribute_key, - std::move(attribute), per_address_args)); + response_generator_->SetResponse(BuildFakeResults( + ipv6_only_, ports, service_config_json, per_address_args)); } void SetNextResolutionUponError(const std::vector& ports) { @@ -249,9 +245,6 @@ class FakeResolverResponseGeneratorWrapper { static grpc_core::Resolver::Result BuildFakeResults( bool ipv6_only, const std::vector& ports, const char* service_config_json = nullptr, - const char* attribute_key = nullptr, - std::unique_ptr attribute = - nullptr, const grpc_core::ChannelArgs& per_address_args = grpc_core::ChannelArgs()) { grpc_core::Resolver::Result result; @@ -262,14 +255,7 @@ class FakeResolverResponseGeneratorWrapper { GPR_ASSERT(lb_uri.ok()); grpc_resolved_address address; GPR_ASSERT(grpc_parse_uri(*lb_uri, &address)); - std::map> - attributes; - if (attribute != nullptr) { - attributes[attribute_key] = attribute->Copy(); - } - result.addresses->emplace_back(address, per_address_args, - std::move(attributes)); + result.addresses->emplace_back(address, per_address_args); } if (result.addresses->empty()) { result.resolution_note = "fake resolver empty address list"; @@ -717,7 +703,6 @@ TEST_F(ClientLbEnd2endTest, AuthorityOverrideFromResolver) { // different value. response_generator.SetNextResolution( GetServersPorts(), /*service_config_json=*/nullptr, - /*attribute_key=*/nullptr, /*attribute=*/nullptr, grpc_core::ChannelArgs().Set(GRPC_ARG_DEFAULT_AUTHORITY, "foo.example.com")); // Send an RPC. @@ -744,7 +729,6 @@ TEST_F(ClientLbEnd2endTest, AuthorityOverridePrecedence) { // different value. response_generator.SetNextResolution( GetServersPorts(), /*service_config_json=*/nullptr, - /*attribute_key=*/nullptr, /*attribute=*/nullptr, grpc_core::ChannelArgs().Set(GRPC_ARG_DEFAULT_AUTHORITY, "bar.example.com")); // Send an RPC. @@ -2847,31 +2831,11 @@ TEST_F(ClientLbInterceptTrailingMetadataTest, BackendMetricDataMerge) { } // -// tests that address attributes from the resolver are visible to the LB policy +// tests that address args from the resolver are visible to the LB policy // class ClientLbAddressTest : public ClientLbEnd2endTest { protected: - static const char* kAttributeKey; - - class Attribute : public grpc_core::ServerAddress::AttributeInterface { - public: - explicit Attribute(const std::string& str) : str_(str) {} - - std::unique_ptr Copy() const override { - return std::make_unique(str_); - } - - int Cmp(const AttributeInterface* other) const override { - return str_.compare(static_cast(other)->str_); - } - - std::string ToString() const override { return str_; } - - private: - std::string str_; - }; - void SetUp() override { ClientLbEnd2endTest::SetUp(); current_test_instance_ = this; @@ -2909,8 +2873,6 @@ class ClientLbAddressTest : public ClientLbEnd2endTest { std::vector addresses_seen_; }; -const char* ClientLbAddressTest::kAttributeKey = "attribute_key"; - ClientLbAddressTest* ClientLbAddressTest::current_test_instance_ = nullptr; TEST_F(ClientLbAddressTest, Basic) { @@ -2919,10 +2881,10 @@ TEST_F(ClientLbAddressTest, Basic) { auto response_generator = BuildResolverResponseGenerator(); auto channel = BuildChannel("address_test_lb", response_generator); auto stub = BuildStub(channel); - // Addresses returned by the resolver will have attached attributes. - response_generator.SetNextResolution(GetServersPorts(), nullptr, - kAttributeKey, - std::make_unique("foo")); + // Addresses returned by the resolver will have attached args. + response_generator.SetNextResolution( + GetServersPorts(), nullptr, + grpc_core::ChannelArgs().Set("test_key", "test_value")); CheckRpcSendOk(DEBUG_LOCATION, stub); // Check LB policy name for the channel. EXPECT_EQ("address_test_lb", channel->GetLoadBalancingPolicyName()); @@ -2930,8 +2892,9 @@ TEST_F(ClientLbAddressTest, Basic) { std::vector expected; for (const int port : GetServersPorts()) { expected.emplace_back(absl::StrCat( - ipv6_only_ ? "[::1]:" : "127.0.0.1:", port, " attributes={", - kAttributeKey, "=foo, disable_outlier_detection=true}")); + ipv6_only_ ? "[::1]:" : "127.0.0.1:", port, + " args={grpc.internal.no_subchannel.outlier_detection_disable=1, " + "test_key=test_value}")); } EXPECT_EQ(addresses_seen(), expected); } diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 71f7e1f2ab1..8c6debdc00f 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1142,8 +1142,6 @@ src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc \ src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc \ src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \ -src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc \ -src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h \ src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 234e3f6c9e5..206860b57cb 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -948,8 +948,6 @@ src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc \ src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc \ src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \ -src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc \ -src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h \ src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc \