From 98461b0fa86caa0eb342c651e331662b3a373392 Mon Sep 17 00:00:00 2001
From: Juanli Shen <juanlishen@google.com>
Date: Mon, 13 May 2019 10:56:57 -0700
Subject: [PATCH] xds enter fallback mode as long as no child policy is ready

---
 .../filters/client_channel/lb_policy/xds/xds.cc  | 13 ++++++-------
 test/cpp/end2end/xds_end2end_test.cc             | 16 ++++++++++++++++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc
index 3eb94371c71..819bad6c00d 100644
--- a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc
@@ -493,9 +493,8 @@ class XdsLb : public LoadBalancingPolicy {
   // 1. The fallback timer fires, we enter fallback mode.
   // 2. Before the fallback timer fires, the LB channel becomes
   // TRANSIENT_FAILURE or the LB call fails, we enter fallback mode.
-  // 3. Before the fallback timer fires, we receive a response from the
-  // balancer, we cancel the fallback timer and use the response to update the
-  // locality map.
+  // 3. Before the fallback timer fires, if any child policy in the locality map
+  // becomes READY, we cancel the fallback timer.
   bool fallback_at_startup_checks_pending_ = false;
   // Timeout in milliseconds for before using fallback backend addresses.
   // 0 means not using fallback.
@@ -1197,9 +1196,6 @@ void XdsLb::BalancerChannelState::BalancerCallState::
         xds_grpclb_destroy_serverlist(
             xdslb_policy->locality_serverlist_[0]->serverlist);
       } else {
-        // This is the first serverlist we've received, don't enter fallback
-        // mode.
-        xdslb_policy->MaybeCancelFallbackAtStartupChecks();
         // Initialize locality serverlist, currently the list only handles
         // one child.
         xdslb_policy->locality_serverlist_.emplace_back(
@@ -2046,7 +2042,10 @@ void XdsLb::LocalityMap::LocalityEntry::Helper::UpdateState(
     return;
   }
   // At this point, child_ must be the current child policy.
-  if (state == GRPC_CHANNEL_READY) entry_->parent_->MaybeExitFallbackMode();
+  if (state == GRPC_CHANNEL_READY) {
+    entry_->parent_->MaybeCancelFallbackAtStartupChecks();
+    entry_->parent_->MaybeExitFallbackMode();
+  }
   // If we are in fallback mode, ignore update request from the child policy.
   if (entry_->parent_->fallback_policy_ != nullptr) return;
   GPR_ASSERT(entry_->parent_->lb_chand_ != nullptr);
diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc
index b876e062426..87a231c588d 100644
--- a/test/cpp/end2end/xds_end2end_test.cc
+++ b/test/cpp/end2end/xds_end2end_test.cc
@@ -1015,6 +1015,22 @@ TEST_F(SingleBalancerTest, FallbackEarlyWhenBalancerCallFails) {
                  /* wait_for_ready */ false);
 }
 
+TEST_F(SingleBalancerTest, FallbackIfResponseReceivedButChildNotReady) {
+  const int kFallbackTimeoutMs = 500 * grpc_test_slowdown_factor();
+  ResetStub(kFallbackTimeoutMs);
+  SetNextResolution({backends_[0]->port_}, kDefaultServiceConfig_.c_str());
+  SetNextResolutionForLbChannelAllBalancers();
+  // Send a serverlist that only contains an unreachable backend before fallback
+  // timeout.
+  ScheduleResponseForBalancer(0,
+                              BalancerServiceImpl::BuildResponseForBackends(
+                                  {grpc_pick_unused_port_or_die()}, {}),
+                              0);
+  // Because no child policy is ready before fallback timeout, we enter fallback
+  // mode.
+  WaitForBackend(0);
+}
+
 TEST_F(SingleBalancerTest, FallbackModeIsExitedWhenBalancerSaysToDropAllCalls) {
   // Return an unreachable balancer and one fallback backend.
   SetNextResolution({backends_[0]->port_}, kDefaultServiceConfig_.c_str());