From 03c49f7255b4ac38a05aea7330c54a36fae32c8c Mon Sep 17 00:00:00 2001
From: Terry Wilson <terrymwilson@gmail.com>
Date: Tue, 2 Nov 2021 16:04:03 -0700
Subject: [PATCH] grpclb: fallback timer only when not already using fallback
 backends. (#8646) (#8648)

Addresses a problem where we initially only resolve addresses to the backends, but not the load balancer and then later resolve addresses to both. In this situation the fallback timer was started during the second instance even if it resulted in the timer later failing as we were already using fallback backends.

This change assures that a fallback time is only ever started if we are not already using the fallback backends.

This is a follow-up fix to #8253.
---
 .../main/java/io/grpc/grpclb/GrpclbState.java |  5 ++--
 .../grpc/grpclb/GrpclbLoadBalancerTest.java   | 27 +++++++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java
index 8607d3996a5..1eebaa63a8e 100644
--- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java
+++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java
@@ -287,8 +287,9 @@ void handleAddresses(
         cancelLbRpcRetryTimer();
         startLbRpc();
       }
-      // Start the fallback timer if it's never started
-      if (fallbackTimer == null) {
+      // Start the fallback timer if it's never started and we are not already using fallback
+      // backends.
+      if (fallbackTimer == null && !usingFallbackBackends) {
         fallbackTimer = syncContext.schedule(
             new FallbackModeTask(BALANCER_TIMEOUT_STATUS), FALLBACK_TIMEOUT_MS,
             TimeUnit.MILLISECONDS, timerService);
diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java
index cb231c6c055..293c0aa0b82 100644
--- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java
+++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java
@@ -1462,6 +1462,33 @@ public void grpclbFallback_noBalancerAddress() {
         .updateBalancingState(eq(TRANSIENT_FAILURE), any(SubchannelPicker.class));
   }
 
+  /**
+   * A test for a situation where we first only get backend addresses resolved and then in a
+   * later name resolution get both backend and load balancer addresses. The first instance
+   * will switch us to using fallback backends and it is important that in the second instance
+   * we do not start a fallback timer as it will fail when it triggers if the fallback backends
+   * are already in use.
+   */
+  @Test
+  public void grpclbFallback_noTimerWhenAlreadyInFallback() {
+    // Initially we only get backend addresses without any LB ones. This should get us to use
+    // fallback backends from the start as we won't be able to even talk to the load balancer.
+    // No fallback timer would be started as we already started to use fallback backends.
+    deliverResolvedAddresses(createResolvedBalancerAddresses(1),
+        Collections.<EquivalentAddressGroup>emptyList());
+    assertEquals(0, fakeClock.numPendingTasks(FALLBACK_MODE_TASK_FILTER));
+
+    // Later a new name resolution call happens and we get both backend and LB addresses. Since we
+    // are already operating with fallback backends a fallback timer should not be started to move
+    // us to fallback mode.
+    deliverResolvedAddresses(Collections.<EquivalentAddressGroup>emptyList(),
+        createResolvedBalancerAddresses(1));
+
+    // If a fallback timer is started it will eventually throw an exception when it tries to switch
+    // us to using fallback backends when we already are using them.
+    assertEquals(0, fakeClock.numPendingTasks(FALLBACK_MODE_TASK_FILTER));
+  }
+
   @Test
   public void grpclbFallback_balancerLost() {
     subtestGrpclbFallbackConnectionLost(true, false);