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

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 19bc35b373f..188d1738a5e 100644
--- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java
+++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java
@@ -282,8 +282,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 f664aad0a7a..e7bb7737345 100644
--- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java
+++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java
@@ -1517,6 +1517,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);