outlier detection: add basic unit test (#31346)

* outlier detection: add basic unit test

* iwyu

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)
 

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

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",
+    ],
+)

test/core/client_channel/lb_policy/lb_policy_test_lib.h

@@ -22,6 +22,7 @@
 #include <stddef.h>
 
 #include <deque>
+#include <functional>
 #include <map>
 #include <memory>
 #include <string>
@@ -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<LoadBalancingPolicy::Config> 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<LoadBalancingPolicy::SubchannelPicker>)>
+          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<FakeHelper::StateUpdate>(&*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<LoadBalancingPolicy::SubchannelPicker> 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();
+    std::unique_ptr<LoadBalancingPolicy::SubchannelPicker> final_picker;
-    EXPECT_TRUE(event.has_value()) << location.file() << ":" << location.line();
+    WaitForStateUpdate(
-    if (!event.has_value()) return nullptr;
+        [&](grpc_connectivity_state state, absl::Status status,
-    auto* update = absl::get_if<FakeHelper::StateUpdate>(&*event);
+            std::unique_ptr<LoadBalancingPolicy::SubchannelPicker> picker) {
-    EXPECT_NE(update, nullptr) << location.file() << ":" << location.line();
+          EXPECT_EQ(state, expected_state)
-    if (update == nullptr) return nullptr;
+              << "got " << ConnectivityStateName(state) << ", expected "
-    EXPECT_EQ(update->state, state)
+              << ConnectivityStateName(expected_state) << "\n"
-        << location.file() << ":" << location.line();
+              << "at " << location.file() << ":" << location.line();
-    EXPECT_EQ(update->status, status)
+          EXPECT_EQ(status, expected_status)
-        << update->status << "\n"
+              << status << "\n"
-        << location.file() << ":" << location.line();
+              << location.file() << ":" << location.line();
-    EXPECT_NE(update->picker, nullptr)
+          EXPECT_NE(picker, nullptr)
-        << location.file() << ":" << location.line();
+              << location.file() << ":" << location.line();
-    return std::move(update->picker);
+          final_picker = std::move(picker);
+          return false;
+        });
+    return final_picker;
+  }
+
+  // Waits for the LB policy to get connected.
+  std::unique_ptr<LoadBalancingPolicy::SubchannelPicker> WaitForConnected(
+      SourceLocation location = SourceLocation()) {
+    std::unique_ptr<LoadBalancingPolicy::SubchannelPicker> final_picker;
+    WaitForStateUpdate(
+        [&](grpc_connectivity_state state, absl::Status status,
+            std::unique_ptr<LoadBalancingPolicy::SubchannelPicker> 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.

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 <stddef.h>
+#include <stdint.h>
+
+#include <algorithm>
+#include <map>
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "absl/status/status.h"
+#include "absl/status/statusor.h"
+#include "absl/strings/string_view.h"
+#include "gtest/gtest.h"
+
+#include <grpc/grpc.h>
+
+#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<LoadBalancingPolicy::Config> 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<LoadBalancingPolicy> 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;
+}

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,