From 3c779853f7681298b40591a87e19f3a7c077ddc6 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 10 Jun 2021 16:36:17 -0700 Subject: [PATCH 1/3] Skip fallback if the LB is already in fallback mode. This can be triggered by receiving updated addresses containing backend addresses only. --- .../main/java/io/grpc/grpclb/GrpclbState.java | 5 ++-- .../grpc/grpclb/GrpclbLoadBalancerTest.java | 29 +++++++++++++++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java index 59fa67dc2dc..532494b13ea 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java +++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java @@ -573,8 +573,9 @@ private FallbackModeTask(Status reason) { @Override public void run() { - // Timer should have been cancelled if entered fallback early. - checkState(!usingFallbackBackends, "already in fallback"); + if (usingFallbackBackends) { + return; + } fallbackReason = reason; maybeUseFallbackBackends(); maybeUpdatePicker(); diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java index 7dba20be1d0..f664aad0a7a 100644 --- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java +++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java @@ -1452,8 +1452,11 @@ public void grpclbFallback_breakLbStreamBeforeFallbackTimerExpires() { public void grpclbFallback_noBalancerAddress() { InOrder inOrder = inOrder(helper, subchannelPool); - // Create just backend addresses - List backendList = createResolvedBackendAddresses(2); + // Create 5 distinct backends + List backends = createResolvedBackendAddresses(5); + + // Name resolver gives the first two backend addresses + List backendList = backends.subList(0, 2); deliverResolvedAddresses(backendList, Collections.emptyList()); assertThat(logs).containsAtLeast( @@ -1474,6 +1477,28 @@ public void grpclbFallback_noBalancerAddress() { .createOobChannel(ArgumentMatchers.anyList(), anyString()); logs.clear(); + ///////////////////////////////////////////////////////////////////////////////////////// + // Name resolver sends new resolution results with new backend addr but no balancer addr + ///////////////////////////////////////////////////////////////////////////////////////// + // Name resolver then gives the last three backends + backendList = backends.subList(2, 5); + deliverResolvedAddresses(backendList, Collections.emptyList()); + + assertThat(logs).containsAtLeast( + "INFO: [grpclb-] Using fallback backends", + "INFO: [grpclb-] " + + "Using RR list=[[[FakeSocketAddress-fake-address-2]/{}], " + + "[[FakeSocketAddress-fake-address-3]/{}], " + + "[[FakeSocketAddress-fake-address-4]/{}]], drop=[null, null, null]", + "INFO: [grpclb-] " + + "Update balancing state to CONNECTING: picks=[BUFFER_ENTRY], " + + "drops=[null, null, null]") + .inOrder(); + + // Shift to use updated backends + fallbackTestVerifyUseOfFallbackBackendLists(inOrder, backendList); + logs.clear(); + /////////////////////////////////////////////////////////////////////////////////////// // Name resolver sends new resolution results without any backend addr or balancer addr /////////////////////////////////////////////////////////////////////////////////////// From a766d51300c49fe59737a52fc184e91dde3f3dfa Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 11 Jun 2021 12:42:32 -0700 Subject: [PATCH 2/3] Inline entering fallback if no LB address is given by resolver. --- .../src/main/java/io/grpc/grpclb/GrpclbState.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java index 532494b13ea..c224540b247 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java +++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java @@ -259,11 +259,16 @@ void handleAddresses( serviceName, newLbAddressGroups, newBackendServers); + fallbackBackendList = newBackendServers; if (newLbAddressGroups.isEmpty()) { // No balancer address: close existing balancer connection and enter fallback mode // immediately. shutdownLbComm(); - syncContext.execute(new FallbackModeTask(NO_LB_ADDRESS_PROVIDED_STATUS)); + if (!usingFallbackBackends) { + fallbackReason = NO_LB_ADDRESS_PROVIDED_STATUS; + cancelFallbackTimer(); + maybeUseFallbackBackends(); + } } else { startLbComm(newLbAddressGroups); // Avoid creating a new RPC just because the addresses were updated, as it can cause a @@ -281,7 +286,6 @@ void handleAddresses( TimeUnit.MILLISECONDS, timerService); } } - fallbackBackendList = newBackendServers; if (usingFallbackBackends) { // Populate the new fallback backends to round-robin list. useFallbackBackends(); @@ -573,9 +577,8 @@ private FallbackModeTask(Status reason) { @Override public void run() { - if (usingFallbackBackends) { - return; - } + // Timer should have been cancelled if entered fallback early. + checkState(!usingFallbackBackends, "already in fallback"); fallbackReason = reason; maybeUseFallbackBackends(); maybeUpdatePicker(); From 67387e16bdf3b943e4dd484982e635e0e6b16b3c Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 11 Jun 2021 13:40:32 -0700 Subject: [PATCH 3/3] Update comment to more precisely describe the fallback behavior whene receiving no LB addresses. --- grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java | 7 +++++-- 1 file changed, 5 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 c224540b247..19bc35b373f 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java +++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java @@ -261,8 +261,11 @@ void handleAddresses( newBackendServers); fallbackBackendList = newBackendServers; if (newLbAddressGroups.isEmpty()) { - // No balancer address: close existing balancer connection and enter fallback mode - // immediately. + // No balancer address: close existing balancer connection and prepare to enter fallback + // mode. If there is no successful backend connection, it enters fallback mode immediately. + // Otherwise, fallback does not happen until backend connections are lost. This behavior + // might be different from other languages (e.g., existing balancer connection is not + // closed in C-core), but we aren't changing it at this time. shutdownLbComm(); if (!usingFallbackBackends) { fallbackReason = NO_LB_ADDRESS_PROVIDED_STATUS;