[outlier detection] backport to 1.56: hack to prevent OD from working with pick_first (#33347)

Back-port #33336 to 1.56.

7 files changed, 88 insertions, 21 deletions

diff --git a/src/core/BUILD b/src/core/BUILD
index de30487abc..30f45b6ff3 100644
--- a/src/core/BUILD
+++ b/src/core/BUILD
@@ -4582,6 +4582,7 @@ grpc_cc_library(
     deps = [
         "channel_args",
         "grpc_lb_subchannel_list",
+        "grpc_outlier_detection_header",
         "json",
         "lb_policy",
         "lb_policy_factory",
@@ -4753,6 +4754,7 @@ grpc_cc_library(
         "time",
         "validation_errors",
         "//:gpr_platform",
+        "//:server_address",
     ],
 )
 
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 4226162336..7a6ec5d42e 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
@@ -68,6 +68,9 @@ 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;
@@ -368,6 +371,8 @@ class OutlierDetectionLb : public LoadBalancingPolicy {
 
   ~OutlierDetectionLb() override;
 
+  // Returns the address map key for an address, or the empty string if
+  // the address should be ignored.
   static std::string MakeKeyForAddress(const ServerAddress& address);
 
   void ShutdownLocked() override;
@@ -546,9 +551,21 @@ OutlierDetectionLb::~OutlierDetectionLb() {
 
 std::string OutlierDetectionLb::MakeKeyForAddress(
     const ServerAddress& address) {
+  // If the address has the DisableOutlierDetectionAttribute attribute,
+  // 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) {
+    return "";
+  }
   // Use only the address, not the attributes.
   auto addr_str = grpc_sockaddr_to_string(&address.address(), false);
-  return addr_str.ok() ? addr_str.value() : addr_str.status().ToString();
+  // If address couldn't be stringified, ignore it.
+  if (!addr_str.ok()) return "";
+  return std::move(*addr_str);
 }
 
 void OutlierDetectionLb::ShutdownLocked() {
@@ -621,6 +638,7 @@ absl::Status OutlierDetectionLb::UpdateLocked(UpdateArgs args) {
     std::set<std::string> current_addresses;
     for (const ServerAddress& address : *args.addresses) {
       std::string address_key = MakeKeyForAddress(address);
+      if (address_key.empty()) continue;
       auto& subchannel_state = subchannel_state_map_[address_key];
       if (subchannel_state == nullptr) {
         subchannel_state = MakeRefCounted<SubchannelState>();
@@ -722,11 +740,19 @@ OrphanablePtr<LoadBalancingPolicy> OutlierDetectionLb::CreateChildPolicyLocked(
 RefCountedPtr<SubchannelInterface> OutlierDetectionLb::Helper::CreateSubchannel(
     ServerAddress address, const ChannelArgs& args) {
   if (outlier_detection_policy_->shutting_down_) return nullptr;
-  std::string key = MakeKeyForAddress(address);
   RefCountedPtr<SubchannelState> subchannel_state;
-  auto it = outlier_detection_policy_->subchannel_state_map_.find(key);
-  if (it != outlier_detection_policy_->subchannel_state_map_.end()) {
-    subchannel_state = it->second->Ref();
+  std::string key = MakeKeyForAddress(address);
+  if (GRPC_TRACE_FLAG_ENABLED(grpc_outlier_detection_lb_trace)) {
+    gpr_log(GPR_INFO,
+            "[outlier_detection_lb %p] using key %s for subchannel address %s",
+            outlier_detection_policy_.get(), key.c_str(),
+            address.ToString().c_str());
+  }
+  if (!key.empty()) {
+    auto it = outlier_detection_policy_->subchannel_state_map_.find(key);
+    if (it != outlier_detection_policy_->subchannel_state_map_.end()) {
+      subchannel_state = it->second->Ref();
+    }
   }
   auto subchannel = MakeRefCounted<SubchannelWrapper>(
       subchannel_state,
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 4118e99555..ca1dc3a3c7 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,6 +21,9 @@
 
 #include <stdint.h>  // for uint32_t
 
+#include <memory>
+#include <string>
+
 #include "absl/types/optional.h"
 
 #include "src/core/lib/gprpp/time.h"
@@ -28,6 +31,7 @@
 #include "src/core/lib/json/json.h"
 #include "src/core/lib/json/json_args.h"
 #include "src/core/lib/json/json_object_loader.h"
+#include "src/core/lib/resolver/server_address.h"
 
 namespace grpc_core {
 
@@ -89,6 +93,23 @@ struct OutlierDetectionConfig {
                     ValidationErrors* errors);
 };
 
+// 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<AttributeInterface> Copy() const override {
+    return std::make_unique<DisableOutlierDetectionAttribute>();
+  }
+
+  int Cmp(const AttributeInterface*) const override { return true; }
+
+  std::string ToString() const override { return "true"; }
+};
+
 }  // namespace grpc_core
 
 #endif  // GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_OUTLIER_DETECTION_OUTLIER_DETECTION_H
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 05009e412f..d3c3766731 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
@@ -35,6 +35,7 @@
 #include <grpc/impl/connectivity_state.h>
 #include <grpc/support/log.h>
 
+#include "src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.h"
 #include "src/core/ext/filters/client_channel/lb_policy/subchannel_list.h"
 #include "src/core/lib/channel/channel_args.h"
 #include "src/core/lib/config/core_configuration.h"
@@ -274,6 +275,19 @@ absl::Status PickFirst::UpdateLocked(UpdateArgs args) {
   } else if (args.addresses->empty()) {
     status = absl::UnavailableError("address list must not be empty");
   }
+  // TODO(roth): This is a hack to disable outlier_detection when used
+  // with pick_first, for the reasons described in
+  // https://github.com/grpc/grpc/issues/32967.  Remove this when
+  // implementing the dualstack design.
+  if (args.addresses.ok()) {
+    ServerAddressList addresses;
+    for (const auto& address : *args.addresses) {
+      addresses.emplace_back(address.WithAttribute(
+          DisableOutlierDetectionAttribute::kName,
+          std::make_unique<DisableOutlierDetectionAttribute>()));
+    }
+    args.addresses = std::move(addresses);
+  }
   // If the update contains a resolver error and we have a previous update
   // that was not a resolver error, keep using the previous addresses.
   if (!args.addresses.ok() && latest_update_args_.config != nullptr) {
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 4de8495210..e73ec073fa 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
@@ -333,11 +333,9 @@ class LoadBalancingPolicyTest : public ::testing::Test {
     // unexpected events in the queue.
     void ExpectQueueEmpty(SourceLocation location = SourceLocation()) {
       MutexLock lock(&mu_);
-      EXPECT_TRUE(queue_.empty()) << location.file() << ":" << location.line();
-      for (const Event& event : queue_) {
-        gpr_log(GPR_ERROR, "UNEXPECTED EVENT LEFT IN QUEUE: %s",
-                EventString(event).c_str());
-      }
+      EXPECT_TRUE(queue_.empty())
+          << location.file() << ":" << location.line() << "\n"
+          << QueueString();
     }
 
     // Returns the next event in the queue if it is a state update.
@@ -394,6 +392,14 @@ class LoadBalancingPolicyTest : public ::testing::Test {
           });
     }
 
+    std::string QueueString() const ABSL_EXCLUSIVE_LOCKS_REQUIRED(&mu_) {
+      std::vector<std::string> parts = {"Queue:"};
+      for (const Event& event : queue_) {
+        parts.push_back(EventString(event));
+      }
+      return absl::StrJoin(parts, "\n  ");
+    }
+
     RefCountedPtr<SubchannelInterface> CreateSubchannel(
         ServerAddress address, const ChannelArgs& args) override {
       SubchannelKey key(address.address(), args);
diff --git a/test/core/client_channel/lb_policy/outlier_detection_test.cc b/test/core/client_channel/lb_policy/outlier_detection_test.cc
index 885285678c..907db1b4d1 100644
--- a/test/core/client_channel/lb_policy/outlier_detection_test.cc
+++ b/test/core/client_channel/lb_policy/outlier_detection_test.cc
@@ -240,7 +240,7 @@ TEST_F(OutlierDetectionTest, FailurePercentage) {
   picker = WaitForRoundRobinListChange(kAddresses, remaining_addresses);
 }
 
-TEST_F(OutlierDetectionTest, FailurePercentageWithPickFirst) {
+TEST_F(OutlierDetectionTest, DoesNotWorkWithPickFirst) {
   constexpr std::array<absl::string_view, 3> kAddresses = {
       "ipv4:127.0.0.1:440", "ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442"};
   // Send initial update.
@@ -284,13 +284,11 @@ TEST_F(OutlierDetectionTest, FailurePercentageWithPickFirst) {
   // Advance time and run the timer callback to trigger ejection.
   time_cache_.IncrementBy(Duration::Seconds(10));
   RunTimerCallback();
-  gpr_log(GPR_INFO, "### ejection complete");
-  // Expect a re-resolution request.
-  ExpectReresolutionRequest();
-  // The pick_first policy should report IDLE with a queuing picker.
-  ExpectStateAndQueuingPicker(GRPC_CHANNEL_IDLE);
-  // The queued pick should have triggered a reconnection attempt.
-  EXPECT_TRUE(subchannel->ConnectionRequested());
+  gpr_log(GPR_INFO, "### ejection timer pass complete");
+  // Subchannel should not be ejected.
+  ExpectQueueEmpty();
+  // Subchannel should not see a reconnection request.
+  EXPECT_FALSE(subchannel->ConnectionRequested());
 }
 
 }  // namespace
diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc
index 31465b82b6..0139bc28f1 100644
--- a/test/cpp/end2end/client_lb_end2end_test.cc
+++ b/test/cpp/end2end/client_lb_end2end_test.cc
@@ -2909,9 +2909,9 @@ TEST_F(ClientLbAddressTest, Basic) {
   // Make sure that the attributes wind up on the subchannels.
   std::vector<std::string> expected;
   for (const int port : GetServersPorts()) {
-    expected.emplace_back(
-        absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", port,
-                     " attributes={", kAttributeKey, "=foo}"));
+    expected.emplace_back(absl::StrCat(
+        ipv6_only_ ? "[::1]:" : "127.0.0.1:", port, " attributes={",
+        kAttributeKey, "=foo, disable_outlier_detection=true}"));
   }
   EXPECT_EQ(addresses_seen(), expected);
 }