From 2f2662c4626ef7e6bed64f2e902f5b3f451c8334 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 17 Oct 2022 08:49:10 -0700 Subject: [PATCH] outlier detection: add basic unit test (#31346) * outlier detection: add basic unit test * iwyu --- CMakeLists.txt | 36 ++++ build_autogenerated.yaml | 10 + test/core/client_channel/lb_policy/BUILD | 12 ++ .../lb_policy/lb_policy_test_lib.h | 89 +++++++-- .../lb_policy/outlier_detection_test.cc | 188 ++++++++++++++++++ tools/run_tests/generated/tests.json | 24 +++ 6 files changed, 344 insertions(+), 15 deletions(-) create mode 100644 test/core/client_channel/lb_policy/outlier_detection_test.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 9661e4592eb..87bfa48d5c8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1067,6 +1067,7 @@ if(gRPC_BUILD_TESTS) add_dependencies(buildtests_cxx orphanable_test) add_dependencies(buildtests_cxx out_of_bounds_bad_client_test) add_dependencies(buildtests_cxx outlier_detection_lb_config_parser_test) + add_dependencies(buildtests_cxx outlier_detection_test) add_dependencies(buildtests_cxx overload_test) add_dependencies(buildtests_cxx parse_address_test) if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) @@ -14079,6 +14080,41 @@ target_link_libraries(outlier_detection_lb_config_parser_test ) +endif() +if(gRPC_BUILD_TESTS) + +add_executable(outlier_detection_test + test/core/client_channel/lb_policy/outlier_detection_test.cc + third_party/googletest/googletest/src/gtest-all.cc + third_party/googletest/googlemock/src/gmock-all.cc +) + +target_include_directories(outlier_detection_test + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/include + ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + ${_gRPC_RE2_INCLUDE_DIR} + ${_gRPC_SSL_INCLUDE_DIR} + ${_gRPC_UPB_GENERATED_DIR} + ${_gRPC_UPB_GRPC_GENERATED_DIR} + ${_gRPC_UPB_INCLUDE_DIR} + ${_gRPC_XXHASH_INCLUDE_DIR} + ${_gRPC_ZLIB_INCLUDE_DIR} + third_party/googletest/googletest/include + third_party/googletest/googletest + third_party/googletest/googlemock/include + third_party/googletest/googlemock + ${_gRPC_PROTO_GENS_DIR} +) + +target_link_libraries(outlier_detection_test + ${_gRPC_PROTOBUF_LIBRARIES} + ${_gRPC_ALLTARGETS_LIBRARIES} + grpc_test_util +) + + endif() if(gRPC_BUILD_TESTS) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index cef6b0d4bd1..4836a48c5af 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -7951,6 +7951,16 @@ targets: - test/core/client_channel/outlier_detection_lb_config_parser_test.cc deps: - grpc_test_util +- name: outlier_detection_test + gtest: true + build: test + language: c++ + headers: + - test/core/client_channel/lb_policy/lb_policy_test_lib.h + src: + - test/core/client_channel/lb_policy/outlier_detection_test.cc + deps: + - grpc_test_util - name: overload_test gtest: true build: test diff --git a/test/core/client_channel/lb_policy/BUILD b/test/core/client_channel/lb_policy/BUILD index d733207f61d..8beed2cdc2a 100644 --- a/test/core/client_channel/lb_policy/BUILD +++ b/test/core/client_channel/lb_policy/BUILD @@ -48,3 +48,15 @@ grpc_cc_test( "//test/core/util:grpc_test_util", ], ) + +grpc_cc_test( + name = "outlier_detection_test", + srcs = ["outlier_detection_test.cc"], + external_deps = ["gtest"], + language = "C++", + deps = [ + ":lb_policy_test_lib", + "//:grpc_lb_policy_outlier_detection", + "//test/core/util:grpc_test_util", + ], +) diff --git a/test/core/client_channel/lb_policy/lb_policy_test_lib.h b/test/core/client_channel/lb_policy/lb_policy_test_lib.h index 794aa63c967..987a28c7ef3 100644 --- a/test/core/client_channel/lb_policy/lb_policy_test_lib.h +++ b/test/core/client_channel/lb_policy/lb_policy_test_lib.h @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -54,6 +55,7 @@ #include "src/core/lib/gprpp/work_serializer.h" #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/iomgr/resolved_address.h" +#include "src/core/lib/json/json.h" #include "src/core/lib/load_balancing/lb_policy.h" #include "src/core/lib/load_balancing/lb_policy_registry.h" #include "src/core/lib/load_balancing/subchannel_interface.h" @@ -309,6 +311,15 @@ class LoadBalancingPolicyTest : public ::testing::Test { .CreateLoadBalancingPolicy(name, std::move(args)); } + // Creates an LB policy config from json. + static RefCountedPtr MakeConfig( + const Json& json) { + return CoreConfiguration::Get() + .lb_policy_registry() + .ParseLoadBalancingConfig(json) + .value(); + } + // Converts an address URI into a grpc_resolved_address. static grpc_resolved_address MakeAddress(absl::string_view address_uri) { auto uri = URI::Parse(address_uri); @@ -333,25 +344,73 @@ class LoadBalancingPolicyTest : public ::testing::Test { return status; } + // Keeps reading state updates until continue_predicate() returns false. + // Returns false if the helper reports no events or if the event is + // not a state update; otherwise (if continue_predicate() tells us to + // stop) returns true. + bool WaitForStateUpdate( + std::function< + bool(grpc_connectivity_state, absl::Status, + std::unique_ptr)> + continue_predicate, + SourceLocation location = SourceLocation()) { + while (true) { + auto event = helper_->GetEvent(); + EXPECT_TRUE(event.has_value()) + << location.file() << ":" << location.line(); + if (!event.has_value()) return false; + auto* update = absl::get_if(&*event); + EXPECT_NE(update, nullptr) << location.file() << ":" << location.line(); + if (update == nullptr) return false; + if (!continue_predicate(update->state, std::move(update->status), + std::move(update->picker))) { + return true; + } + } + } + // Expects that the LB policy has reported the specified connectivity // state to helper_. Returns the picker from the state update. std::unique_ptr ExpectState( - grpc_connectivity_state state, absl::Status status = absl::OkStatus(), + grpc_connectivity_state expected_state, + absl::Status expected_status = absl::OkStatus(), SourceLocation location = SourceLocation()) { - auto event = helper_->GetEvent(); - EXPECT_TRUE(event.has_value()) << location.file() << ":" << location.line(); - if (!event.has_value()) return nullptr; - auto* update = absl::get_if(&*event); - EXPECT_NE(update, nullptr) << location.file() << ":" << location.line(); - if (update == nullptr) return nullptr; - EXPECT_EQ(update->state, state) - << location.file() << ":" << location.line(); - EXPECT_EQ(update->status, status) - << update->status << "\n" - << location.file() << ":" << location.line(); - EXPECT_NE(update->picker, nullptr) - << location.file() << ":" << location.line(); - return std::move(update->picker); + std::unique_ptr final_picker; + WaitForStateUpdate( + [&](grpc_connectivity_state state, absl::Status status, + std::unique_ptr picker) { + EXPECT_EQ(state, expected_state) + << "got " << ConnectivityStateName(state) << ", expected " + << ConnectivityStateName(expected_state) << "\n" + << "at " << location.file() << ":" << location.line(); + EXPECT_EQ(status, expected_status) + << status << "\n" + << location.file() << ":" << location.line(); + EXPECT_NE(picker, nullptr) + << location.file() << ":" << location.line(); + final_picker = std::move(picker); + return false; + }); + return final_picker; + } + + // Waits for the LB policy to get connected. + std::unique_ptr WaitForConnected( + SourceLocation location = SourceLocation()) { + std::unique_ptr final_picker; + WaitForStateUpdate( + [&](grpc_connectivity_state state, absl::Status status, + std::unique_ptr picker) { + if (state == GRPC_CHANNEL_CONNECTING) { + EXPECT_TRUE(status.ok()) << status; + ExpectPickQueued(picker.get(), location); + return true; + } + EXPECT_EQ(state, GRPC_CHANNEL_READY) << ConnectivityStateName(state); + final_picker = std::move(picker); + return false; + }); + return final_picker; } // Requests a pick on picker and expects a Queue result. diff --git a/test/core/client_channel/lb_policy/outlier_detection_test.cc b/test/core/client_channel/lb_policy/outlier_detection_test.cc new file mode 100644 index 00000000000..2d49106cf8c --- /dev/null +++ b/test/core/client_channel/lb_policy/outlier_detection_test.cc @@ -0,0 +1,188 @@ +// +// Copyright 2022 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 + +#include +#include +#include +#include +#include +#include + +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/strings/string_view.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/gprpp/orphanable.h" +#include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/time.h" +#include "src/core/lib/iomgr/resolved_address.h" +#include "src/core/lib/json/json.h" +#include "src/core/lib/load_balancing/lb_policy.h" +#include "src/core/lib/resolver/server_address.h" +#include "test/core/client_channel/lb_policy/lb_policy_test_lib.h" +#include "test/core/util/test_config.h" + +namespace grpc_core { +namespace testing { +namespace { + +class OutlierDetectionTest : public LoadBalancingPolicyTest { + protected: + class ConfigBuilder { + public: + ConfigBuilder() { + SetChildPolicy(Json::Object{{"round_robin", Json::Object()}}); + } + + ConfigBuilder& SetInterval(Duration duration) { + json_["interval"] = duration.ToJsonString(); + return *this; + } + ConfigBuilder& SetBaseEjectionTime(Duration duration) { + json_["baseEjectionTime"] = duration.ToJsonString(); + return *this; + } + ConfigBuilder& SetMaxEjectionTime(Duration duration) { + json_["maxEjectionTime"] = duration.ToJsonString(); + return *this; + } + ConfigBuilder& SetMaxEjectionPercent(uint32_t value) { + json_["maxEjectionPercent"] = value; + return *this; + } + ConfigBuilder& SetChildPolicy(Json::Object child_policy) { + json_["childPolicy"] = Json::Array{std::move(child_policy)}; + return *this; + } + + ConfigBuilder& SetSuccessRateStdevFactor(uint32_t value) { + GetSuccessRate()["stdevFactor"] = value; + return *this; + } + ConfigBuilder& SetSuccessRateEnforcementPercentage(uint32_t value) { + GetSuccessRate()["enforcementPercentage"] = value; + return *this; + } + ConfigBuilder& SetSuccessRateMinHosts(uint32_t value) { + GetSuccessRate()["minimumHosts"] = value; + return *this; + } + ConfigBuilder& SetSuccessRateRequestVolume(uint32_t value) { + GetSuccessRate()["requestVolume"] = value; + return *this; + } + + ConfigBuilder& SetFailurePercentageThreshold(uint32_t value) { + GetFailurePercentage()["threshold"] = value; + return *this; + } + ConfigBuilder& SetFailurePercentageEnforcementPercentage(uint32_t value) { + GetFailurePercentage()["enforcementPercentage"] = value; + return *this; + } + ConfigBuilder& SetFailurePercentageMinimumHosts(uint32_t value) { + GetFailurePercentage()["minimumHosts"] = value; + return *this; + } + ConfigBuilder& SetFailurePercentageRequestVolume(uint32_t value) { + GetFailurePercentage()["requestVolume"] = value; + return *this; + } + + RefCountedPtr Build() { + Json config = + Json::Array{Json::Object{{"outlier_detection_experimental", json_}}}; + return MakeConfig(config); + } + + private: + Json::Object& GetSuccessRate() { + auto it = json_.emplace("successRateEjection", Json::Object()).first; + return *it->second.mutable_object(); + } + + Json::Object& GetFailurePercentage() { + auto it = + json_.emplace("failurePercentageEjection", Json::Object()).first; + return *it->second.mutable_object(); + } + + Json::Object json_; + }; + + OutlierDetectionTest() + : lb_policy_(MakeLbPolicy("outlier_detection_experimental")) {} + + OrphanablePtr lb_policy_; +}; + +TEST_F(OutlierDetectionTest, Basic) { + constexpr absl::string_view kAddressUri = "ipv4:127.0.0.1:443"; + const grpc_resolved_address address = MakeAddress(kAddressUri); + // Send an update containing one address. + LoadBalancingPolicy::UpdateArgs update_args; + update_args.config = ConfigBuilder().Build(); + update_args.addresses.emplace(); + update_args.addresses->emplace_back(address, ChannelArgs()); + absl::Status status = ApplyUpdate(std::move(update_args), lb_policy_.get()); + EXPECT_TRUE(status.ok()) << status; + // LB policy should have reported CONNECTING state. + auto picker = ExpectState(GRPC_CHANNEL_CONNECTING); + ExpectPickQueued(picker.get()); + // LB policy should have created a subchannel for the address. + SubchannelKey key(address, ChannelArgs()); + auto it = subchannel_pool_.find(key); + ASSERT_NE(it, subchannel_pool_.end()); + auto& subchannel_state = it->second; + // LB policy should have requested a connection on this subchannel. + EXPECT_TRUE(subchannel_state.ConnectionRequested()); + // Tell subchannel to report CONNECTING. + subchannel_state.SetConnectivityState(GRPC_CHANNEL_CONNECTING, + absl::OkStatus()); + // LB policy should again report CONNECTING. + picker = ExpectState(GRPC_CHANNEL_CONNECTING); + ExpectPickQueued(picker.get()); + // Tell subchannel to report READY. + subchannel_state.SetConnectivityState(GRPC_CHANNEL_READY, absl::OkStatus()); + // LB policy should eventually report READY. + picker = WaitForConnected(); + ASSERT_NE(picker, nullptr); + // Picker should return the same subchannel repeatedly. + for (size_t i = 0; i < 3; ++i) { + ExpectPickComplete(picker.get(), kAddressUri); + } +} + +} // namespace +} // namespace testing +} // namespace grpc_core + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + grpc::testing::TestEnvironment env(&argc, argv); + grpc_init(); + int ret = RUN_ALL_TESTS(); + grpc_shutdown(); + return ret; +} diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index f5303039b63..ffb437b14f3 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -5001,6 +5001,30 @@ ], "uses_polling": true }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "outlier_detection_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": true + }, { "args": [], "benchmark": false,